Skip to content

Commit f16104d

Browse files
blizzzblizzz
authored
Merge pull request #518 from nextcloud/backport/508/stable-3.2
[stable-3.2] sanitize and test user id received from IdP, if original does not match
2 parents 5e7dca1 + b4400b7 commit f16104d

File tree

9 files changed

+627
-503
lines changed

9 files changed

+627
-503
lines changed

‎appinfo/app.php

+8-1
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,12 @@
4545
$session
4646
);
4747

48+
$userData = new \OCA\User_SAML\UserData(
49+
new \OCA\User_SAML\UserResolver(\OC::$server->getUserManager()),
50+
$samlSettings,
51+
$config
52+
);
53+
4854
$userBackend = new \OCA\User_SAML\UserBackend(
4955
$config,
5056
$urlGenerator,
@@ -53,7 +59,8 @@
5359
\OC::$server->getUserManager(),
5460
\OC::$server->getGroupManager(),
5561
$samlSettings,
56-
\OC::$server->getLogger()
62+
\OC::$server->getLogger(),
63+
$userData
5764
);
5865
$userBackend->registerBackends(\OC::$server->getUserManager()->getBackends());
5966
OC_User::useBackend($userBackend);

‎lib/Controller/SAMLController.php

+66-77
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@
2828
use OCA\User_SAML\Exceptions\NoUserFoundException;
2929
use OCA\User_SAML\SAMLSettings;
3030
use OCA\User_SAML\UserBackend;
31+
use OCA\User_SAML\UserData;
32+
use OCA\User_SAML\UserResolver;
3133
use OCP\AppFramework\Controller;
3234
use OCP\AppFramework\Http;
3335
use OCP\IConfig;
@@ -58,12 +60,14 @@ class SAMLController extends Controller {
5860
private $config;
5961
/** @var IURLGenerator */
6062
private $urlGenerator;
61-
/** @var IUserManager */
62-
private $userManager;
6363
/** @var ILogger */
6464
private $logger;
6565
/** @var IL10N */
6666
private $l;
67+
/** @var UserResolver */
68+
private $userResolver;
69+
/** @var UserData */
70+
private $userData;
6771
/**
6872
* @var ICrypto
6973
*/
@@ -78,94 +82,81 @@ class SAMLController extends Controller {
7882
* @param UserBackend $userBackend
7983
* @param IConfig $config
8084
* @param IURLGenerator $urlGenerator
81-
* @param IUserManager $userManager
8285
* @param ILogger $logger
8386
* @param IL10N $l
8487
*/
85-
public function __construct($appName,
86-
IRequest $request,
87-
ISession $session,
88-
IUserSession $userSession,
89-
SAMLSettings $SAMLSettings,
90-
UserBackend $userBackend,
91-
IConfig $config,
92-
IURLGenerator $urlGenerator,
93-
IUserManager $userManager,
94-
ILogger $logger,
95-
IL10N $l,
96-
ICrypto $crypto) {
88+
public function __construct(
89+
$appName,
90+
IRequest $request,
91+
ISession $session,
92+
IUserSession $userSession,
93+
SAMLSettings $SAMLSettings,
94+
UserBackend $userBackend,
95+
IConfig $config,
96+
IURLGenerator $urlGenerator,
97+
ILogger $logger,
98+
IL10N $l,
99+
UserResolver $userResolver,
100+
UserData $userData,
101+
ICrypto $crypto
102+
) {
97103
parent::__construct($appName, $request);
98104
$this->session = $session;
99105
$this->userSession = $userSession;
100106
$this->SAMLSettings = $SAMLSettings;
101107
$this->userBackend = $userBackend;
102108
$this->config = $config;
103109
$this->urlGenerator = $urlGenerator;
104-
$this->userManager = $userManager;
105110
$this->logger = $logger;
106111
$this->l = $l;
112+
$this->userResolver = $userResolver;
113+
$this->userData = $userData;
107114
$this->crypto = $crypto;
108115
}
109116

110117
/**
111-
* @param array $auth
112118
* @throws NoUserFoundException
113119
*/
114-
private function autoprovisionIfPossible(array $auth) {
120+
private function autoprovisionIfPossible() {
121+
$auth = $this->userData->getAttributes();
115122

116-
$prefix = $this->SAMLSettings->getPrefix();
117-
$uidMapping = $this->config->getAppValue('user_saml', $prefix . 'general-uid_mapping');
118-
if(isset($auth[$uidMapping])) {
119-
if(is_array($auth[$uidMapping])) {
120-
$uid = $auth[$uidMapping][0];
121-
} else {
122-
$uid = $auth[$uidMapping];
123-
}
124-
125-
// make sure that a valid UID is given
126-
if (empty($uid)) {
127-
$this->logger->error('Uid "' . $uid . '" is not a valid uid please check your attribute mapping', ['app' => $this->appName]);
128-
throw new \InvalidArgumentException('No valid uid given, please check your attribute mapping. Given uid: ' . $uid);
129-
}
130-
131-
$uid = $this->userBackend->testEncodedObjectGUID($uid);
132-
133-
// if this server acts as a global scale master and the user is not
134-
// a local admin of the server we just create the user and continue
135-
// no need to update additional attributes
136-
$isGsEnabled = $this->config->getSystemValue('gs.enabled', false);
137-
$isGsMaster = $this->config->getSystemValue('gss.mode', 'slave') === 'master';
138-
$isGsMasterAdmin = in_array($uid, $this->config->getSystemValue('gss.master.admin', []));
139-
if ($isGsEnabled && $isGsMaster && !$isGsMasterAdmin) {
140-
$this->userBackend->createUserIfNotExists($uid);
141-
return;
142-
}
143-
$userExists = $this->userManager->userExists($uid);
144-
$autoProvisioningAllowed = $this->userBackend->autoprovisionAllowed();
145-
if($userExists === true) {
146-
if($autoProvisioningAllowed) {
147-
$this->userBackend->updateAttributes($uid, $auth);
148-
}
149-
return;
150-
}
123+
if(!$this->userData->hasUidMappingAttribute()) {
124+
throw new NoUserFoundException('IDP parameter for the UID not found. Possible parameters are: ' . json_encode(array_keys($auth)));
125+
}
151126

152-
if(!$userExists && !$autoProvisioningAllowed) {
153-
// it is possible that the user was not logged in before and
154-
// thus is not known to the original backend. A search can
155-
// help with it and make the user known
156-
$this->userManager->search($uid);
157-
if($this->userManager->userExists($uid)) {
158-
return;
159-
}
160-
throw new NoUserFoundException('Auto provisioning not allowed and user ' . $uid . ' does not exist');
161-
} elseif(!$userExists && $autoProvisioningAllowed) {
162-
$this->userBackend->createUserIfNotExists($uid, $auth);
127+
if ($this->userData->getOriginalUid() === '') {
128+
$this->logger->error('Uid is not a valid uid please check your attribute mapping', ['app' => $this->appName]);
129+
throw new \InvalidArgumentException('No valid uid given, please check your attribute mapping.');
130+
}
131+
$uid = $this->userData->getEffectiveUid();
132+
$userExists = $uid !== '';
133+
134+
// if this server acts as a global scale master and the user is not
135+
// a local admin of the server we just create the user and continue
136+
// no need to update additional attributes
137+
$isGsEnabled = $this->config->getSystemValue('gs.enabled', false);
138+
$isGsMaster = $this->config->getSystemValue('gss.mode', 'slave') === 'master';
139+
$isGsMasterAdmin = in_array($uid, $this->config->getSystemValue('gss.master.admin', []));
140+
if ($isGsEnabled && $isGsMaster && !$isGsMasterAdmin) {
141+
$this->userBackend->createUserIfNotExists($this->userData->getOriginalUid());
142+
return;
143+
}
144+
$autoProvisioningAllowed = $this->userBackend->autoprovisionAllowed();
145+
if($userExists) {
146+
if($autoProvisioningAllowed) {
163147
$this->userBackend->updateAttributes($uid, $auth);
164-
return;
165148
}
149+
return;
166150
}
167151

168-
throw new NoUserFoundException('IDP parameter for the UID (' . $uidMapping . ') not found. Possible parameters are: ' . json_encode(array_keys($auth)));
152+
$uid = $this->userData->getOriginalUid();
153+
if(!$userExists && !$autoProvisioningAllowed) {
154+
throw new NoUserFoundException('Auto provisioning not allowed and user ' . $uid . ' does not exist');
155+
} elseif(!$userExists && $autoProvisioningAllowed) {
156+
$this->userBackend->createUserIfNotExists($uid, $auth);
157+
$this->userBackend->updateAttributes($uid, $auth);
158+
return;
159+
}
169160
}
170161

171162
/**
@@ -221,11 +212,9 @@ public function login($idp) {
221212
}
222213
$this->session->set('user_saml.samlUserData', $_SERVER);
223214
try {
224-
$this->autoprovisionIfPossible($this->session->get('user_saml.samlUserData'));
225-
$user = $this->userManager->get($this->userBackend->getCurrentUserId());
226-
if(!($user instanceof IUser)) {
227-
throw new NoUserFoundException('User' . $this->userBackend->getCurrentUserId() . ' not valid or not found');
228-
}
215+
$this->userData->setAttributes($this->session->get('user_saml.samlUserData'));
216+
$this->autoprovisionIfPossible();
217+
$user = $this->userResolver->findExistingUser($this->userBackend->getCurrentUserId());
229218
$user->updateLastLoginTimestamp();
230219
} catch (NoUserFoundException $e) {
231220
if ($e->getMessage()) {
@@ -342,7 +331,8 @@ public function assertionConsumerService(): Http\RedirectResponse {
342331
// Check whether the user actually exists, if not redirect to an error page
343332
// explaining the issue.
344333
try {
345-
$this->autoprovisionIfPossible($auth->getAttributes());
334+
$this->userData->setAttributes($auth->getAttributes());
335+
$this->autoprovisionIfPossible();
346336
} catch (NoUserFoundException $e) {
347337
$this->logger->error($e->getMessage(), ['app' => $this->appName]);
348338
$response = new Http\RedirectResponse($this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.notProvisioned'));
@@ -358,14 +348,13 @@ public function assertionConsumerService(): Http\RedirectResponse {
358348
$this->session->set('user_saml.samlSessionIndex', $auth->getSessionIndex());
359349
$this->session->set('user_saml.samlSessionExpiration', $auth->getSessionExpiration());
360350
try {
361-
$user = $this->userManager->get($this->userBackend->getCurrentUserId());
362-
if (!($user instanceof IUser)) {
363-
throw new \InvalidArgumentException('User is not valid');
364-
}
351+
$user = $this->userResolver->findExistingUser($this->userBackend->getCurrentUserId());
365352
$firstLogin = $user->updateLastLoginTimestamp();
366-
if($firstLogin) {
353+
if ($firstLogin) {
367354
$this->userBackend->initializeHomeDir($user->getUID());
368355
}
356+
} catch (NoUserFoundException $e) {
357+
throw new \InvalidArgumentException('User "' . $this->userBackend->getCurrentUserId() . '" is not valid');
369358
} catch (\Exception $e) {
370359
$this->logger->logException($e, ['app' => $this->appName]);
371360
$response = new Http\RedirectResponse($this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.notProvisioned'));

0 commit comments

Comments
 (0)