Merge branch 'feature/oidc-subject-claim' into 'master'

Don't hash user ID in OIDC subject claim

Closes #47791

See merge request gitlab-org/gitlab-ce!19784

1 files changed, 61 insertions, 49 deletions

diff --git a/spec/requests/openid_connect_spec.rb b/spec/requests/openid_connect_spec.rb
index bcb8d6c2bfc..b14d4b8fb6e 100644
--- a/spec/requests/openid_connect_spec.rb
+++ b/spec/requests/openid_connect_spec.rb
@@ -1,11 +1,49 @@
 require 'spec_helper'
 
 describe 'OpenID Connect requests' do
-  let(:user) { create :user }
+  let(:user) do
+    create(
+      :user,
+      name: 'Alice',
+      username: 'alice',
+      email: 'private@example.com',
+      emails: [public_email],
+      public_email: public_email.email,
+      website_url: 'https://example.com',
+      avatar: fixture_file_upload('spec/fixtures/dk.png')
+    )
+  end
+
+  let(:public_email) { build :email, email: 'public@example.com' }
+
   let(:access_grant) { create :oauth_access_grant, application: application, resource_owner_id: user.id }
   let(:access_token) { create :oauth_access_token, application: application, resource_owner_id: user.id }
 
-  def request_access_token
+  let(:hashed_subject) do
+    Digest::SHA256.hexdigest("#{user.id}-#{Rails.application.secrets.secret_key_base}")
+  end
+
+  let(:id_token_claims) do
+    {
+      'sub'        => user.id.to_s,
+      'sub_legacy' => hashed_subject
+    }
+  end
+
+  let(:user_info_claims) do
+    {
+      'name'           => 'Alice',
+      'nickname'       => 'alice',
+      'email'          => 'public@example.com',
+      'email_verified' => true,
+      'website'        => 'https://example.com',
+      'profile'        => 'http://localhost/alice',
+      'picture'        => "http://localhost/uploads/-/system/user/avatar/#{user.id}/dk.png",
+      'groups'         => kind_of(Array)
+    }
+  end
+
+  def request_access_token!
     login_as user
 
     post '/oauth/token',
@@ -16,26 +54,22 @@ describe 'OpenID Connect requests' do
       client_secret: application.secret
   end
 
-  def request_user_info
+  def request_user_info!
     get '/oauth/userinfo', nil, 'Authorization' => "Bearer #{access_token.token}"
   end
 
-  def hashed_subject
-    Digest::SHA256.hexdigest("#{user.id}-#{Rails.application.secrets.secret_key_base}")
-  end
-
   context 'Application without OpenID scope' do
     let(:application) { create :oauth_application, scopes: 'api' }
 
     it 'token response does not include an ID token' do
-      request_access_token
+      request_access_token!
 
       expect(json_response).to include 'access_token'
       expect(json_response).not_to include 'id_token'
     end
 
     it 'userinfo response is unauthorized' do
-      request_user_info
+      request_user_info!
 
       expect(response).to have_gitlab_http_status 403
       expect(response.body).to be_blank
@@ -46,28 +80,12 @@ describe 'OpenID Connect requests' do
     let(:application) { create :oauth_application, scopes: 'openid' }
 
     it 'token response includes an ID token' do
-      request_access_token
+      request_access_token!
 
       expect(json_response).to include 'id_token'
     end
 
     context 'UserInfo payload' do
-      let(:user) do
-        create(
-          :user,
-          name: 'Alice',
-          username: 'alice',
-          emails: [private_email, public_email],
-          email: private_email.email,
-          public_email: public_email.email,
-          website_url: 'https://example.com',
-          avatar: fixture_file_upload('spec/fixtures/dk.png')
-        )
-      end
-
-      let!(:public_email) { build :email, email: 'public@example.com' }
-      let!(:private_email) { build :email, email: 'private@example.com' }
-
       let!(:group1) { create :group }
       let!(:group2) { create :group }
       let!(:group3) { create :group, parent: group2 }
@@ -76,41 +94,35 @@ describe 'OpenID Connect requests' do
       before do
         group1.add_user(user, GroupMember::OWNER)
         group3.add_user(user, Gitlab::Access::DEVELOPER)
+
+        request_user_info!
       end
 
       it 'includes all user information and group memberships' do
-        request_user_info
-
-        expect(json_response).to match(a_hash_including({
-          'sub'            => hashed_subject,
-          'name'           => 'Alice',
-          'nickname'       => 'alice',
-          'email'          => 'public@example.com',
-          'email_verified' => true,
-          'website'        => 'https://example.com',
-          'profile'        => 'http://localhost/alice',
-          'picture'        => "http://localhost/uploads/-/system/user/avatar/#{user.id}/dk.png",
-          'groups'         => anything
-        }))
+        expect(json_response).to match(id_token_claims.merge(user_info_claims))
 
         expected_groups = [group1.full_path, group3.full_path]
         expected_groups << group4.full_path if Group.supports_nested_groups?
         expect(json_response['groups']).to match_array(expected_groups)
       end
+
+      it 'does not include any unknown claims' do
+        expect(json_response.keys).to eq %w[sub sub_legacy] + user_info_claims.keys
+      end
     end
 
     context 'ID token payload' do
       before do
-        request_access_token
+        request_access_token!
         @payload = JSON::JWT.decode(json_response['id_token'], :skip_verification)
       end
 
-      it 'includes the Gitlab root URL' do
-        expect(@payload['iss']).to eq Gitlab.config.gitlab.url
+      it 'includes the subject claims' do
+        expect(@payload).to match(a_hash_including(id_token_claims))
       end
 
-      it 'includes the hashed user ID' do
-        expect(@payload['sub']).to eq hashed_subject
+      it 'includes the Gitlab root URL' do
+        expect(@payload['iss']).to eq Gitlab.config.gitlab.url
       end
 
       it 'includes the time of the last authentication', :clean_gitlab_redis_shared_state do
@@ -118,7 +130,7 @@ describe 'OpenID Connect requests' do
       end
 
       it 'does not include any unknown properties' do
-        expect(@payload.keys).to eq %w[iss sub aud exp iat auth_time]
+        expect(@payload.keys).to eq %w[iss sub aud exp iat auth_time sub_legacy]
       end
     end
 
@@ -134,10 +146,10 @@ describe 'OpenID Connect requests' do
     context 'when user is blocked' do
       it 'returns authentication error' do
         access_grant
-        user.block
+        user.block!
 
         expect do
-          request_access_token
+          request_access_token!
         end.to raise_error UncaughtThrowError
       end
     end
@@ -145,10 +157,10 @@ describe 'OpenID Connect requests' do
     context 'when user is ldap_blocked' do
       it 'returns authentication error' do
         access_grant
-        user.ldap_block
+        user.ldap_block!
 
         expect do
-          request_access_token
+          request_access_token!
         end.to raise_error UncaughtThrowError
       end
     end