From b987bea37a593201107f207dc065e973e3ec39e8 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 2 Aug 2022 16:56:56 +0100 Subject: [PATCH] Added OIDC group sync functionality Is generally aligned with out SAML2 group sync functionality, but for OIDC based upon feedback in #3004. Neeeded the tangental addition of being able to define custom scopes on the initial auth request as some systems use this to provide additional id token claims such as groups. Includes tests to cover. Tested live using Okta. --- .env.example.complete | 4 ++ app/Auth/Access/Oidc/OidcOAuthProvider.php | 16 ++++- app/Auth/Access/Oidc/OidcService.php | 73 +++++++++++++++++++++- app/Config/oidc.php | 12 ++++ tests/Auth/OidcTest.php | 69 ++++++++++++++++++++ 5 files changed, 170 insertions(+), 4 deletions(-) diff --git a/.env.example.complete b/.env.example.complete index 45b1c7a86..c097af4f6 100644 --- a/.env.example.complete +++ b/.env.example.complete @@ -263,7 +263,11 @@ OIDC_ISSUER_DISCOVER=false OIDC_PUBLIC_KEY=null OIDC_AUTH_ENDPOINT=null OIDC_TOKEN_ENDPOINT=null +OIDC_ADDITIONAL_SCOPES=null OIDC_DUMP_USER_DETAILS=false +OIDC_USER_TO_GROUPS=false +OIDC_GROUP_ATTRIBUTE=groups +OIDC_REMOVE_FROM_GROUPS=false # Disable default third-party services such as Gravatar and Draw.IO # Service-specific options will override this option diff --git a/app/Auth/Access/Oidc/OidcOAuthProvider.php b/app/Auth/Access/Oidc/OidcOAuthProvider.php index 9b9d0524c..07bd980a3 100644 --- a/app/Auth/Access/Oidc/OidcOAuthProvider.php +++ b/app/Auth/Access/Oidc/OidcOAuthProvider.php @@ -30,6 +30,11 @@ class OidcOAuthProvider extends AbstractProvider */ protected $tokenEndpoint; + /** + * Scopes to use for the OIDC authorization call + */ + protected array $scopes = ['openid', 'profile', 'email']; + /** * Returns the base URL for authorizing a client. */ @@ -54,6 +59,15 @@ class OidcOAuthProvider extends AbstractProvider return ''; } + /** + * Add an additional scope to this provider upon the default. + */ + public function addScope(string $scope): void + { + $this->scopes[] = $scope; + $this->scopes = array_unique($this->scopes); + } + /** * Returns the default scopes used by this provider. * @@ -62,7 +76,7 @@ class OidcOAuthProvider extends AbstractProvider */ protected function getDefaultScopes(): array { - return ['openid', 'profile', 'email']; + return $this->scopes; } /** diff --git a/app/Auth/Access/Oidc/OidcService.php b/app/Auth/Access/Oidc/OidcService.php index eeacdb732..3443baaf6 100644 --- a/app/Auth/Access/Oidc/OidcService.php +++ b/app/Auth/Access/Oidc/OidcService.php @@ -2,6 +2,8 @@ namespace BookStack\Auth\Access\Oidc; +use BookStack\Auth\Access\GroupSyncService; +use Illuminate\Support\Arr; use function auth; use BookStack\Auth\Access\LoginService; use BookStack\Auth\Access\RegistrationService; @@ -26,15 +28,22 @@ class OidcService protected RegistrationService $registrationService; protected LoginService $loginService; protected HttpClient $httpClient; + protected GroupSyncService $groupService; /** * OpenIdService constructor. */ - public function __construct(RegistrationService $registrationService, LoginService $loginService, HttpClient $httpClient) + public function __construct( + RegistrationService $registrationService, + LoginService $loginService, + HttpClient $httpClient, + GroupSyncService $groupService + ) { $this->registrationService = $registrationService; $this->loginService = $loginService; $this->httpClient = $httpClient; + $this->groupService = $groupService; } /** @@ -117,10 +126,31 @@ class OidcService */ protected function getProvider(OidcProviderSettings $settings): OidcOAuthProvider { - return new OidcOAuthProvider($settings->arrayForProvider(), [ + $provider = new OidcOAuthProvider($settings->arrayForProvider(), [ 'httpClient' => $this->httpClient, 'optionProvider' => new HttpBasicAuthOptionProvider(), ]); + + foreach ($this->getAdditionalScopes() as $scope) { + $provider->addScope($scope); + } + + return $provider; + } + + /** + * Get any user-defined addition/custom scopes to apply to the authentication request. + * + * @return string[] + */ + protected function getAdditionalScopes(): array + { + $scopeConfig = $this->config()['additional_scopes'] ?: ''; + + $scopeArr = explode(',', $scopeConfig); + $scopeArr = array_map(fn(string $scope) => trim($scope), $scopeArr); + + return array_filter($scopeArr); } /** @@ -145,10 +175,32 @@ class OidcService return implode(' ', $displayName); } + /** + * Extract the assigned groups from the id token. + * + * @return string[] + */ + protected function getUserGroups(OidcIdToken $token): array + { + $groupsAttr = $this->config()['group_attribute']; + if (empty($groupsAttr)) { + return []; + } + + $groupsList = Arr::get($token->getAllClaims(), $groupsAttr); + if (!is_array($groupsList)) { + return []; + } + + return array_values(array_filter($groupsList, function($val) { + return is_string($val); + })); + } + /** * Extract the details of a user from an ID token. * - * @return array{name: string, email: string, external_id: string} + * @return array{name: string, email: string, external_id: string, groups: string[]} */ protected function getUserDetails(OidcIdToken $token): array { @@ -158,6 +210,7 @@ class OidcService 'external_id' => $id, 'email' => $token->getClaim('email'), 'name' => $this->getUserDisplayName($token, $id), + 'groups' => $this->getUserGroups($token), ]; } @@ -209,6 +262,12 @@ class OidcService throw new OidcException($exception->getMessage()); } + if ($this->shouldSyncGroups()) { + $groups = $userDetails['groups']; + $detachExisting = $this->config()['remove_from_groups']; + $this->groupService->syncUserWithFoundGroups($user, $groups, $detachExisting); + } + $this->loginService->login($user, 'oidc'); return $user; @@ -221,4 +280,12 @@ class OidcService { return config('oidc'); } + + /** + * Check if groups should be synced. + */ + protected function shouldSyncGroups(): bool + { + return $this->config()['user_to_groups'] !== false; + } } diff --git a/app/Config/oidc.php b/app/Config/oidc.php index 842ac8af6..8a9dd3a87 100644 --- a/app/Config/oidc.php +++ b/app/Config/oidc.php @@ -32,4 +32,16 @@ return [ // OAuth2 endpoints. 'authorization_endpoint' => env('OIDC_AUTH_ENDPOINT', null), 'token_endpoint' => env('OIDC_TOKEN_ENDPOINT', null), + + // Add extra scopes, upon those required, to the OIDC authentication request + // Multiple values can be provided comma seperated. + 'additional_scopes' => env('OIDC_ADDITIONAL_SCOPES', null), + + // Group sync options + // Enable syncing, upon login, of OIDC groups to BookStack roles + 'user_to_groups' => env('OIDC_USER_TO_GROUPS', false), + // Attribute, within a OIDC ID token, to find group names within + 'group_attribute' => env('OIDC_GROUP_ATTRIBUTE', 'groups'), + // When syncing groups, remove any groups that no longer match. Otherwise sync only adds new groups. + 'remove_from_groups' => env('OIDC_REMOVE_FROM_GROUPS', false), ]; diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index aa2c99a36..4215f6a54 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -3,6 +3,7 @@ namespace Tests\Auth; use BookStack\Actions\ActivityType; +use BookStack\Auth\Role; use BookStack\Auth\User; use GuzzleHttp\Psr7\Request; use GuzzleHttp\Psr7\Response; @@ -37,6 +38,10 @@ class OidcTest extends TestCase 'oidc.token_endpoint' => 'https://oidc.local/token', 'oidc.discover' => false, 'oidc.dump_user_details' => false, + 'oidc.additional_scopes' => '', + 'oidc.user_to_groups' => false, + 'oidc.group_attribute' => 'group', + 'oidc.remove_from_groups' => false, ]); } @@ -159,6 +164,17 @@ class OidcTest extends TestCase $this->assertActivityExists(ActivityType::AUTH_LOGIN, null, "oidc; ({$user->id}) Barry Scott"); } + public function test_login_uses_custom_additional_scopes_if_defined() + { + config()->set([ + 'oidc.additional_scopes' => 'groups, badgers', + ]); + + $redirect = $this->post('/oidc/login')->headers->get('location'); + + $this->assertStringContainsString('scope=openid%20profile%20email%20groups%20badgers', $redirect); + } + public function test_callback_fails_if_no_state_present_or_matching() { $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=abc124'); @@ -344,6 +360,59 @@ class OidcTest extends TestCase $this->assertTrue(auth()->check()); } + public function test_login_group_sync() + { + config()->set([ + 'oidc.user_to_groups' => true, + 'oidc.group_attribute' => 'groups', + 'oidc.remove_from_groups' => false, + ]); + $roleA = Role::factory()->create(['display_name' => 'Wizards']); + $roleB = Role::factory()->create(['display_name' => 'ZooFolks', 'external_auth_id' => 'zookeepers']); + $roleC = Role::factory()->create(['display_name' => 'Another Role']); + + $resp = $this->runLogin([ + 'email' => 'benny@example.com', + 'sub' => 'benny1010101', + 'groups' => ['Wizards', 'Zookeepers'] + ]); + $resp->assertRedirect('/'); + + /** @var User $user */ + $user = User::query()->where('email', '=', 'benny@example.com')->first(); + + $this->assertTrue($user->hasRole($roleA->id)); + $this->assertTrue($user->hasRole($roleB->id)); + $this->assertFalse($user->hasRole($roleC->id)); + } + + public function test_login_group_sync_with_nested_groups_in_token() + { + config()->set([ + 'oidc.user_to_groups' => true, + 'oidc.group_attribute' => 'my.custom.groups.attr', + 'oidc.remove_from_groups' => false, + ]); + $roleA = Role::factory()->create(['display_name' => 'Wizards']); + + $resp = $this->runLogin([ + 'email' => 'benny@example.com', + 'sub' => 'benny1010101', + 'my' => [ + 'custom' => [ + 'groups' => [ + 'attr' => ['Wizards'] + ] + ] + ] + ]); + $resp->assertRedirect('/'); + + /** @var User $user */ + $user = User::query()->where('email', '=', 'benny@example.com')->first(); + $this->assertTrue($user->hasRole($roleA->id)); + } + protected function withAutodiscovery() { config()->set([