From fb877a19b207f41b3c131a29de97da6b28138aeb Mon Sep 17 00:00:00 2001 From: nrosandich Date: Fri, 31 Jan 2025 15:08:09 +1300 Subject: [PATCH 1/5] Audit authorizing an OAuth application Changelog: added --- .../oauth/authorizations_controller.rb | 25 ++++++ .../user_authorized_oauth_application.yml | 10 +++ doc/user/compliance/audit_event_types.md | 1 + lib/gitlab/audit/auditor.rb | 3 +- .../oauth/authorizations_controller_spec.rb | 76 +++++++++++++++++++ 5 files changed, 114 insertions(+), 1 deletion(-) create mode 100644 config/audit_events/types/user_authorized_oauth_application.yml diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index 08e38811841f54..67a2438c3e4c0f 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -12,6 +12,7 @@ class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController before_action :add_gon_variables before_action :verify_confirmed_email!, :verify_admin_allowed! + after_action :audit_oauth_authorization, only: [:create] layout 'minimal' @@ -35,8 +36,32 @@ def new end end + def create + redirect_or_render(authorize_response) + end + private + def audit_oauth_authorization + return unless performed? && (response.successful? || response.redirect?) + + application = pre_auth.client.application + + Gitlab::Audit::Auditor.audit( + name: 'user_authorized_oauth_application', + author: current_user, + scope: current_user, + target: current_user, + message: 'User authorized an OAuth application.', + additional_details: { + application_name: application.name, + application_id: application.id, + scopes: application.scopes.to_a + }, + ip_address: request.remote_ip + ) + end + # Chrome blocks redirections if the form-action CSP directive is present # and the redirect location's scheme isn't allow-listed # https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/form-action diff --git a/config/audit_events/types/user_authorized_oauth_application.yml b/config/audit_events/types/user_authorized_oauth_application.yml new file mode 100644 index 00000000000000..63289211e4b06d --- /dev/null +++ b/config/audit_events/types/user_authorized_oauth_application.yml @@ -0,0 +1,10 @@ +--- +name: user_authorized_oauth_application +description: User authorized an OAuth application +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/514152 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179187 +feature_category: authorization +milestone: '17.9' +saved_to_database: true +streamed: true +scope: [User] diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index bd17586b288770..696d2448a46967 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -98,6 +98,7 @@ Audit event types belong to the following product categories. | Type name | Event triggered when | Saved to database | Introduced in | Scope | |:----------|:---------------------|:------------------|:--------------|:------| | [`secure_ci_job_token_policies_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/170930) | Permissions are updated for a CI_JOB_TOKEN scope | **{check-circle}** Yes | GitLab [17.6](https://gitlab.com/gitlab-org/gitlab/-/issues/495144) | Project | +| [`user_authorized_oauth_application`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/179187) | User authorized an OAuth application | **{check-circle}** Yes | GitLab [17.9](https://gitlab.com/gitlab-org/gitlab/-/issues/514152) | User | ### Build artifacts diff --git a/lib/gitlab/audit/auditor.rb b/lib/gitlab/audit/auditor.rb index d455f8864ca29d..7fe348dc2bd2d3 100644 --- a/lib/gitlab/audit/auditor.rb +++ b/lib/gitlab/audit/auditor.rb @@ -8,7 +8,8 @@ class Auditor include ::Gitlab::Audit::Logging PERMITTED_TARGET_CLASSES = [ - ::Operations::FeatureFlag + ::Operations::FeatureFlag, + ::Doorkeeper::Application ].freeze # Record audit events diff --git a/spec/controllers/oauth/authorizations_controller_spec.rb b/spec/controllers/oauth/authorizations_controller_spec.rb index 4c16d26ea6c898..455aeb5ed6c4d6 100644 --- a/spec/controllers/oauth/authorizations_controller_spec.rb +++ b/spec/controllers/oauth/authorizations_controller_spec.rb @@ -383,4 +383,80 @@ expect(controller).to be_a(Gitlab::GonHelper) end end + + describe '#audit_oauth_authorization' do + let(:pre_auth) { instance_double(Doorkeeper::OAuth::PreAuthorization) } + let(:client) { instance_double(Doorkeeper::OAuth::Client) } + + before do + allow(controller).to receive(:pre_auth).and_return(pre_auth) + allow(pre_auth).to receive(:client).and_return(client) + allow(client).to receive(:application).and_return(application) + end + + context 'when response is successful' do + before do + allow(controller).to receive(:performed?).and_return(true) + allow(controller).to receive_message_chain(:response, :successful?).and_return(true) + end + + it 'creates an audit event' do + expect(Gitlab::Audit::Auditor).to receive(:audit).with( + name: 'user_authorized_oauth_application', + author: user, + scope: user, + target: user, + message: 'User authorized an OAuth application.', + additional_details: { + application_name: application.name, + application_id: application.id, + scopes: application.scopes.to_a + }, + ip_address: request.remote_ip + ) + + controller.send(:audit_oauth_authorization) + end + end + + context 'when response is a redirect' do + before do + allow(controller).to receive(:performed?).and_return(true) + allow(controller).to receive_message_chain(:response, :successful?).and_return(false) + allow(controller).to receive_message_chain(:response, :redirect?).and_return(true) + end + + it 'creates an audit event' do + expect(Gitlab::Audit::Auditor).to receive(:audit) + + controller.send(:audit_oauth_authorization) + end + end + + context 'when response is not performed' do + before do + allow(controller).to receive(:performed?).and_return(false) + end + + it 'does not create an audit event' do + expect(Gitlab::Audit::Auditor).not_to receive(:audit) + + controller.send(:audit_oauth_authorization) + end + end + + context 'when response is neither successful nor redirect' do + before do + allow(controller).to receive(:performed?).and_return(true) + allow(controller).to receive_message_chain(:response, :successful?).and_return(false) + allow(controller).to receive_message_chain(:response, :redirect?).and_return(false) + end + + it 'does not create an audit event' do + expect(Gitlab::Audit::Auditor).not_to receive(:audit) + + controller.send(:audit_oauth_authorization) + end + end + end end -- GitLab From a2531f5ac28dac59616b6240b5a20df602ae67a0 Mon Sep 17 00:00:00 2001 From: Nate Rosandich Date: Tue, 4 Feb 2025 02:37:48 +0000 Subject: [PATCH 2/5] Apply 4 suggestion(s) to 3 file(s) Co-authored-by: Huzaifa Iftikhar --- app/controllers/oauth/authorizations_controller.rb | 2 +- lib/gitlab/audit/auditor.rb | 3 +-- spec/controllers/oauth/authorizations_controller_spec.rb | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index 67a2438c3e4c0f..9013fbbd2fda86 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -51,7 +51,7 @@ def audit_oauth_authorization name: 'user_authorized_oauth_application', author: current_user, scope: current_user, - target: current_user, + target: application, message: 'User authorized an OAuth application.', additional_details: { application_name: application.name, diff --git a/lib/gitlab/audit/auditor.rb b/lib/gitlab/audit/auditor.rb index 7fe348dc2bd2d3..d455f8864ca29d 100644 --- a/lib/gitlab/audit/auditor.rb +++ b/lib/gitlab/audit/auditor.rb @@ -8,8 +8,7 @@ class Auditor include ::Gitlab::Audit::Logging PERMITTED_TARGET_CLASSES = [ - ::Operations::FeatureFlag, - ::Doorkeeper::Application + ::Operations::FeatureFlag ].freeze # Record audit events diff --git a/spec/controllers/oauth/authorizations_controller_spec.rb b/spec/controllers/oauth/authorizations_controller_spec.rb index 455aeb5ed6c4d6..80bbdabe71a58e 100644 --- a/spec/controllers/oauth/authorizations_controller_spec.rb +++ b/spec/controllers/oauth/authorizations_controller_spec.rb @@ -405,7 +405,7 @@ name: 'user_authorized_oauth_application', author: user, scope: user, - target: user, + target: application, message: 'User authorized an OAuth application.', additional_details: { application_name: application.name, -- GitLab From 054f7c7e294d54ce7ccab886a049f62c07856aa0 Mon Sep 17 00:00:00 2001 From: nrosandich Date: Wed, 5 Feb 2025 15:10:51 +1300 Subject: [PATCH 3/5] Remove method overwrite --- app/controllers/oauth/authorizations_controller.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index 9013fbbd2fda86..096f459f6548b7 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -12,7 +12,10 @@ class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController before_action :add_gon_variables before_action :verify_confirmed_email!, :verify_admin_allowed! + # rubocop: disable Rails/LexicallyScopedActionFilter + # :create is defined in Doorkeeper::AuthorizationsController after_action :audit_oauth_authorization, only: [:create] + # rubocop: enable Rails/LexicallyScopedActionFilter layout 'minimal' @@ -36,10 +39,6 @@ def new end end - def create - redirect_or_render(authorize_response) - end - private def audit_oauth_authorization -- GitLab From f9d2b4df98d751c956b3598af6b9d6a03b1b4ee4 Mon Sep 17 00:00:00 2001 From: Nate Rosandich Date: Wed, 5 Feb 2025 02:46:06 +0000 Subject: [PATCH 4/5] Apply 2 suggestion(s) to 1 file(s) --- app/controllers/oauth/authorizations_controller.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index 096f459f6548b7..53a5b7a5a1cfcb 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -12,8 +12,7 @@ class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController before_action :add_gon_variables before_action :verify_confirmed_email!, :verify_admin_allowed! - # rubocop: disable Rails/LexicallyScopedActionFilter - # :create is defined in Doorkeeper::AuthorizationsController + # rubocop: disable Rails/LexicallyScopedActionFilter -- :create is defined in Doorkeeper::AuthorizationsController after_action :audit_oauth_authorization, only: [:create] # rubocop: enable Rails/LexicallyScopedActionFilter -- GitLab From 3f7a3b4a0789229222b13a2e6bd62c95cbe52cd2 Mon Sep 17 00:00:00 2001 From: Nate Rosandich Date: Wed, 5 Feb 2025 02:53:29 +0000 Subject: [PATCH 5/5] Apply 1 suggestion(s) to 1 file(s) Co-authored-by: GitLab Duo --- app/controllers/oauth/authorizations_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index 53a5b7a5a1cfcb..96b2f399cf2104 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -41,7 +41,7 @@ def new private def audit_oauth_authorization - return unless performed? && (response.successful? || response.redirect?) + return unless performed? && (response.successful? || response.redirect?) && pre_auth&.client application = pre_auth.client.application -- GitLab