From 583ef9458c5e5c32a14629f5754bc53ed0ad8a33 Mon Sep 17 00:00:00 2001 From: Hassan Zamani Date: Tue, 30 May 2017 10:36:00 +0430 Subject: [PATCH] Add groups to OpenID Connect claims --- app/models/user.rb | 7 +++++- .../unreleased/feature-oidc-groups-claim.yml | 4 +++ .../initializers/doorkeeper_openid_connect.rb | 1 + config/locales/doorkeeper.en.yml | 2 +- doc/integration/openid_connect_provider.md | 1 + spec/models/user_spec.rb | 25 ++++++++++++++++++- spec/requests/openid_connect_spec.rb | 24 +++++++++++++++--- 7 files changed, 57 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/feature-oidc-groups-claim.yml diff --git a/app/models/user.rb b/app/models/user.rb index a57ba3740c9..d4e720dc173 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -564,7 +564,7 @@ class User < ActiveRecord::Base gpg_keys.each(&:update_invalid_gpg_signatures) end - # Returns the groups a user has access to + # Returns the groups a user has access to, either through a membership or a project authorization def authorized_groups union = Gitlab::SQL::Union .new([groups.select(:id), authorized_projects.select(:namespace_id)]) @@ -572,6 +572,11 @@ class User < ActiveRecord::Base Group.where("namespaces.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection end + # Returns the groups a user is a member of, either directly or through a parent group + def membership_groups + Gitlab::GroupHierarchy.new(groups).base_and_descendants + end + # Returns a relation of groups the user has access to, including their parent # and child groups (recursively). def all_expanded_groups diff --git a/changelogs/unreleased/feature-oidc-groups-claim.yml b/changelogs/unreleased/feature-oidc-groups-claim.yml new file mode 100644 index 00000000000..bde19130114 --- /dev/null +++ b/changelogs/unreleased/feature-oidc-groups-claim.yml @@ -0,0 +1,4 @@ +--- +title: Add groups to OpenID Connect claims +merge_request: 16929 +author: Hassan Zamani diff --git a/config/initializers/doorkeeper_openid_connect.rb b/config/initializers/doorkeeper_openid_connect.rb index af174def047..98e1f6e830f 100644 --- a/config/initializers/doorkeeper_openid_connect.rb +++ b/config/initializers/doorkeeper_openid_connect.rb @@ -31,6 +31,7 @@ Doorkeeper::OpenidConnect.configure do o.claim(:website) { |user| user.full_website_url if user.website_url? } o.claim(:profile) { |user| Gitlab::Routing.url_helpers.user_url user } o.claim(:picture) { |user| user.avatar_url(only_path: false) } + o.claim(:groups) { |user| user.membership_groups.map(&:full_path) } end end end diff --git a/config/locales/doorkeeper.en.yml b/config/locales/doorkeeper.en.yml index b1c71095d4f..889111282ef 100644 --- a/config/locales/doorkeeper.en.yml +++ b/config/locales/doorkeeper.en.yml @@ -68,7 +68,7 @@ en: read_user: Read-only access to the user's profile information, like username, public email and full name openid: - The ability to authenticate using GitLab, and read-only access to the user's profile information + The ability to authenticate using GitLab, and read-only access to the user's profile information and group memberships sudo: Access to the Sudo feature, to perform API actions as any user in the system (only available for admins) flash: diff --git a/doc/integration/openid_connect_provider.md b/doc/integration/openid_connect_provider.md index 56f367d841e..ad41be52045 100644 --- a/doc/integration/openid_connect_provider.md +++ b/doc/integration/openid_connect_provider.md @@ -39,6 +39,7 @@ Currently the following user information is shared with clients: | `website` | `string` | URL for the user's website | `profile` | `string` | URL for the user's GitLab profile | `picture` | `string` | URL for the user's GitLab avatar +| `groups` | `array` | Names of the groups the user is a member of [OpenID Connect]: http://openid.net/connect/ "OpenID Connect website" [doorkeeper-openid_connect]: https://github.com/doorkeeper-gem/doorkeeper-openid_connect "Doorkeeper::OpenidConnect website" diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 18c91d4cffd..264a2b948c9 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1557,14 +1557,37 @@ describe User do describe '#authorized_groups' do let!(:user) { create(:user) } let!(:private_group) { create(:group) } + let!(:child_group) { create(:group, parent: private_group) } + + let!(:project_group) { create(:group) } + let!(:project) { create(:project, group: project_group) } before do private_group.add_user(user, Gitlab::Access::MASTER) + project.add_master(user) end subject { user.authorized_groups } - it { is_expected.to eq([private_group]) } + it { is_expected.to contain_exactly private_group, project_group } + end + + describe '#membership_groups' do + let!(:user) { create(:user) } + let!(:parent_group) { create(:group) } + let!(:child_group) { create(:group, parent: parent_group) } + + before do + parent_group.add_user(user, Gitlab::Access::MASTER) + end + + subject { user.membership_groups } + + if Group.supports_nested_groups? + it { is_expected.to contain_exactly parent_group, child_group } + else + it { is_expected.to contain_exactly parent_group } + end end describe '#authorized_projects', :delete do diff --git a/spec/requests/openid_connect_spec.rb b/spec/requests/openid_connect_spec.rb index 1a5ad9b04e4..5d349f45a33 100644 --- a/spec/requests/openid_connect_spec.rb +++ b/spec/requests/openid_connect_spec.rb @@ -65,10 +65,20 @@ describe 'OpenID Connect requests' do ) end - let(:public_email) { build :email, email: 'public@example.com' } - let(:private_email) { build :email, email: 'private@example.com' } + let!(:public_email) { build :email, email: 'public@example.com' } + let!(:private_email) { build :email, email: 'private@example.com' } - it 'includes all user information' do + let!(:group1) { create :group, path: 'group1' } + let!(:group2) { create :group, path: 'group2' } + let!(:group3) { create :group, path: 'group3', parent: group2 } + let!(:group4) { create :group, path: 'group4', parent: group3 } + + before do + group1.add_user(user, GroupMember::OWNER) + group3.add_user(user, Gitlab::Access::DEVELOPER) + end + + it 'includes all user information and group memberships' do request_user_info expect(json_response).to eq({ @@ -79,7 +89,13 @@ describe 'OpenID Connect requests' do 'email_verified' => true, 'website' => 'https://example.com', 'profile' => 'http://localhost/alice', - 'picture' => "http://localhost/uploads/-/system/user/avatar/#{user.id}/dk.png" + 'picture' => "http://localhost/uploads/-/system/user/avatar/#{user.id}/dk.png", + 'groups' => + if Group.supports_nested_groups? + ['group1', 'group2/group3', 'group2/group3/group4'] + else + ['group1', 'group2/group3'] + end }) end end