From d329176aff8e397e66460aab4dbe46a5a99b78ae Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Sun, 26 Nov 2023 15:51:59 -0600 Subject: [PATCH 1/3] Add web_ide_oauth feature flag - Adds behavior to ide controller to auto create the OAuthApp if needed. - See [original impl][1]. [1]: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132496 --- .../javascripts/ide/init_gitlab_web_ide.js | 25 ++++-- .../lib/gitlab_web_ide/get_oauth_config.js | 12 +++ .../ide/lib/gitlab_web_ide/index.js | 2 + .../javascripts/ide/mount_oauth_callback.js | 12 +++ .../pages/ide/{ => index}/index.js | 0 .../pages/ide/oauth_redirect/index.js | 3 + app/controllers/ide_controller.rb | 19 +++- app/helpers/ide_helper.rb | 15 +++- app/models/application_setting.rb | 1 + app/views/ide/oauth_redirect.html.haml | 3 + app/views/shared/_ide_root.html.haml | 2 +- .../development/web_ide_oauth.yml | 8 ++ config/routes.rb | 1 + .../web_ide/default_oauth_application.rb | 51 +++++++++++ locale/gitlab.pot | 3 + spec/frontend/ide/init_gitlab_web_ide_spec.js | 28 ++++++ .../gitlab_web_ide/get_oauth_config_spec.js | 16 ++++ .../frontend/ide/mount_oauth_callback_spec.js | 53 ++++++++++++ .../web_ide/default_oauth_application_spec.rb | 86 +++++++++++++++++++ spec/requests/ide_controller_spec.rb | 86 +++++++++++++++++++ 20 files changed, 415 insertions(+), 11 deletions(-) create mode 100644 app/assets/javascripts/ide/lib/gitlab_web_ide/get_oauth_config.js create mode 100644 app/assets/javascripts/ide/mount_oauth_callback.js rename app/assets/javascripts/pages/ide/{ => index}/index.js (100%) create mode 100644 app/assets/javascripts/pages/ide/oauth_redirect/index.js create mode 100644 app/views/ide/oauth_redirect.html.haml create mode 100644 config/feature_flags/development/web_ide_oauth.yml create mode 100644 lib/gitlab/web_ide/default_oauth_application.rb create mode 100644 spec/frontend/ide/lib/gitlab_web_ide/get_oauth_config_spec.js create mode 100644 spec/frontend/ide/mount_oauth_callback_spec.js create mode 100644 spec/lib/gitlab/web_ide/default_oauth_application_spec.rb diff --git a/app/assets/javascripts/ide/init_gitlab_web_ide.js b/app/assets/javascripts/ide/init_gitlab_web_ide.js index 868830c953a5e3..f5fb4c8be2fee9 100644 --- a/app/assets/javascripts/ide/init_gitlab_web_ide.js +++ b/app/assets/javascripts/ide/init_gitlab_web_ide.js @@ -6,10 +6,13 @@ import { confirmAction } from '~/lib/utils/confirm_via_gl_modal/confirm_action'; import { createAndSubmitForm } from '~/lib/utils/create_and_submit_form'; import csrf from '~/lib/utils/csrf'; import Tracking from '~/tracking'; -import { getBaseConfig } from './lib/gitlab_web_ide/get_base_config'; -import { setupRootElement } from './lib/gitlab_web_ide/setup_root_element'; +import { + getBaseConfig, + getOAuthConfig, + setupRootElement, + handleTracking, +} from './lib/gitlab_web_ide'; import { GITLAB_WEB_IDE_FEEDBACK_ISSUE } from './constants'; -import { handleTracking } from './lib/gitlab_web_ide/handle_tracking_event'; const buildRemoteIdeURL = (ideRemotePath, remoteHost, remotePathArg) => { const remotePath = cleanLeadingSeparator(remotePathArg); @@ -51,15 +54,21 @@ export const initGitlabWebIDE = async (el) => { : null; const forkInfo = forkInfoJSON ? JSON.parse(forkInfoJSON) : null; + const oauthConfig = getOAuthConfig(el.dataset); + const httpHeaders = oauthConfig + ? undefined + : // Use same headers as defined in axios_utils (not needed in oauth) + { + [csrf.headerKey]: csrf.token, + 'X-Requested-With': 'XMLHttpRequest', + }; + // See ClientOnlyConfig https://gitlab.com/gitlab-org/gitlab-web-ide/-/blob/main/packages/web-ide-types/src/config.ts#L17 start(rootEl, { ...getBaseConfig(), nonce, - // Use same headers as defined in axios_utils - httpHeaders: { - [csrf.headerKey]: csrf.token, - 'X-Requested-With': 'XMLHttpRequest', - }, + httpHeaders, + auth: oauthConfig, projectPath, ref, filePath, diff --git a/app/assets/javascripts/ide/lib/gitlab_web_ide/get_oauth_config.js b/app/assets/javascripts/ide/lib/gitlab_web_ide/get_oauth_config.js new file mode 100644 index 00000000000000..5493a9ba7c79bd --- /dev/null +++ b/app/assets/javascripts/ide/lib/gitlab_web_ide/get_oauth_config.js @@ -0,0 +1,12 @@ +export const getOAuthConfig = ({ clientId, callbackUrl }) => { + if (!clientId) { + return undefined; + } + + return { + type: 'oauth', + clientId, + callbackUrl, + protectRefreshToken: true, + }; +}; diff --git a/app/assets/javascripts/ide/lib/gitlab_web_ide/index.js b/app/assets/javascripts/ide/lib/gitlab_web_ide/index.js index 8311e11672efbf..87e0002c8c8e89 100644 --- a/app/assets/javascripts/ide/lib/gitlab_web_ide/index.js +++ b/app/assets/javascripts/ide/lib/gitlab_web_ide/index.js @@ -1,2 +1,4 @@ export * from './get_base_config'; +export * from './get_oauth_config'; +export * from './handle_tracking_event'; export * from './setup_root_element'; diff --git a/app/assets/javascripts/ide/mount_oauth_callback.js b/app/assets/javascripts/ide/mount_oauth_callback.js new file mode 100644 index 00000000000000..79fffb24f8eff2 --- /dev/null +++ b/app/assets/javascripts/ide/mount_oauth_callback.js @@ -0,0 +1,12 @@ +import { oauthCallback } from '@gitlab/web-ide'; +import { getBaseConfig, getOAuthConfig } from './lib/gitlab_web_ide'; + +export const mountOAuthCallback = () => { + const el = document.getElementById('ide'); + + return oauthCallback({ + ...getBaseConfig(), + username: gon.current_username, + auth: getOAuthConfig(el.dataset), + }); +}; diff --git a/app/assets/javascripts/pages/ide/index.js b/app/assets/javascripts/pages/ide/index/index.js similarity index 100% rename from app/assets/javascripts/pages/ide/index.js rename to app/assets/javascripts/pages/ide/index/index.js diff --git a/app/assets/javascripts/pages/ide/oauth_redirect/index.js b/app/assets/javascripts/pages/ide/oauth_redirect/index.js new file mode 100644 index 00000000000000..ee9233fab38cae --- /dev/null +++ b/app/assets/javascripts/pages/ide/oauth_redirect/index.js @@ -0,0 +1,3 @@ +import { mountOAuthCallback } from '~/ide/mount_oauth_callback'; + +mountOAuthCallback(); diff --git a/app/controllers/ide_controller.rb b/app/controllers/ide_controller.rb index d9566121dcd72a..d00610b2b03e97 100644 --- a/app/controllers/ide_controller.rb +++ b/app/controllers/ide_controller.rb @@ -5,7 +5,8 @@ class IdeController < ApplicationController include StaticObjectExternalStorageCSP include Gitlab::Utils::StrongMemoize - before_action :authorize_read_project! + before_action :authorize_read_project!, only: [:index] + before_action :ensure_web_ide_oauth_application!, only: [:index] before_action do push_frontend_feature_flag(:build_service_proxy) @@ -27,12 +28,28 @@ def index render layout: helpers.use_new_web_ide? ? 'fullscreen' : 'application' end + def oauth_redirect + return render_404 unless ::Gitlab::WebIde::DefaultOauthApplication.feature_enabled?(current_user) + # TODO - It's **possible** we end up here and no oauth application has been set up. + # We need to have better handling of these edge cases. Here's a # follow-up issue: + # https://gitlab.com/gitlab-org/gitlab/-/issues/433322 + return render_404 unless ::Gitlab::WebIde::DefaultOauthApplication.oauth_application + + render layout: 'fullscreen', locals: { minimal: true } + end + private def authorize_read_project! render_404 unless can?(current_user, :read_project, project) end + def ensure_web_ide_oauth_application! + return unless ::Gitlab::WebIde::DefaultOauthApplication.feature_enabled?(current_user) + + ::Gitlab::WebIde::DefaultOauthApplication.ensure_oauth_application! + end + def fork_info(project, branch) return if can?(current_user, :push_code, project) diff --git a/app/helpers/ide_helper.rb b/app/helpers/ide_helper.rb index f2d393f1f77d21..2ec11b8a9ed9e5 100644 --- a/app/helpers/ide_helper.rb +++ b/app/helpers/ide_helper.rb @@ -52,6 +52,19 @@ def new_ide_code_suggestions_data {} end + def new_ide_oauth_data + return {} unless ::Gitlab::WebIde::DefaultOauthApplication.feature_enabled?(current_user) + return {} unless ::Gitlab::WebIde::DefaultOauthApplication.oauth_application + + client_id = ::Gitlab::WebIde::DefaultOauthApplication.oauth_application.uid + callback_url = ::Gitlab::WebIde::DefaultOauthApplication.oauth_callback_url + + { + 'client-id' => client_id, + 'callback-url' => callback_url + } + end + def new_ide_data(project:) { 'project-path' => project&.path_with_namespace, @@ -59,7 +72,7 @@ def new_ide_data(project:) # We will replace these placeholders in the FE 'ide-remote-path' => ide_remote_path(remote_host: ':remote_host', remote_path: ':remote_path'), 'editor-font' => new_ide_fonts.to_json - }.merge(new_ide_code_suggestions_data) + }.merge(new_ide_code_suggestions_data).merge(new_ide_oauth_data) end def legacy_ide_data(project:) diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index a33d7e33769ec0..0b816517deac9e 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -42,6 +42,7 @@ class ApplicationSetting < MainClusterwide::ApplicationRecord add_authentication_token_field :error_tracking_access_token, encrypted: :required belongs_to :push_rule + belongs_to :web_ide_oauth_application, class_name: 'Doorkeeper::Application' alias_attribute :housekeeping_optimize_repository_period, :housekeeping_incremental_repack_period diff --git a/app/views/ide/oauth_redirect.html.haml b/app/views/ide/oauth_redirect.html.haml new file mode 100644 index 00000000000000..e0ce9f9768c216 --- /dev/null +++ b/app/views/ide/oauth_redirect.html.haml @@ -0,0 +1,3 @@ +- page_title _("IDE") + += render partial: 'shared/ide_root', locals: { data: new_ide_oauth_data, loading_text: _('Authenticating...') } diff --git a/app/views/shared/_ide_root.html.haml b/app/views/shared/_ide_root.html.haml index db3e76e188c4df..173b081d69328e 100644 --- a/app/views/shared/_ide_root.html.haml +++ b/app/views/shared/_ide_root.html.haml @@ -5,6 +5,6 @@ -# 100vh because of the presence of the bottom bar #ide.gl-h-full{ data: data } - .web-ide-loader.gl-display-flex.gl-justify-content-center.gl-align-items-center.gl-flex-direction-column.gl-h-full.gl-mr-auto.gl-ml-auto + .web-ide-loader.gl-display-flex.gl-justify-content-center.gl-align-items-center.gl-flex-direction-column.gl-h-full.gl-mx-auto = brand_header_logo %h3.clblack.gl-mt-6= loading_text diff --git a/config/feature_flags/development/web_ide_oauth.yml b/config/feature_flags/development/web_ide_oauth.yml new file mode 100644 index 00000000000000..fc3132d01b4001 --- /dev/null +++ b/config/feature_flags/development/web_ide_oauth.yml @@ -0,0 +1,8 @@ +--- +name: web_ide_oauth +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/138015 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/433324 +milestone: '16.7' +type: development +group: group::ide +default_enabled: false diff --git a/config/routes.rb b/config/routes.rb index fc6a7a377151b9..018c9633d94fbf 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -130,6 +130,7 @@ scope :ide, as: :ide, format: false do get '/', to: 'ide#index' get '/project', to: 'ide#index' + get '/oauth_redirect', to: 'ide#oauth_redirect' scope path: 'project/:project_id', as: :project, constraints: { project_id: Gitlab::PathRegex.full_namespace_route_regex } do %w[edit tree blob].each do |action| diff --git a/lib/gitlab/web_ide/default_oauth_application.rb b/lib/gitlab/web_ide/default_oauth_application.rb new file mode 100644 index 00000000000000..e6751fc5e34188 --- /dev/null +++ b/lib/gitlab/web_ide/default_oauth_application.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module Gitlab + module WebIde + module DefaultOauthApplication + class << self + def feature_enabled?(current_user) + Feature.enabled?(:vscode_web_ide, current_user) && Feature.enabled?(:web_ide_oauth, current_user) + end + + def oauth_application + application_settings.web_ide_oauth_application + end + + def oauth_callback_url + Gitlab::Routing.url_helpers.ide_oauth_redirect_url + end + + def ensure_oauth_application! + return if oauth_application + + application_settings.transaction do + # note: This should run very rarely and should be safe for us to do a lock + # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132496#note_1587293087 + application_settings.lock! + + # We need to double check here so that requests previously waiting on the lock can now just skip + next if oauth_application + + application = Doorkeeper::Application.new( + name: 'GitLab Web IDE', + redirect_uri: oauth_callback_url, + scopes: ['api'], + trusted: true, + confidential: false) + application.save! + application_settings.update!(web_ide_oauth_application: application) + + ::Gitlab::CurrentSettings.expire_current_application_settings + end + end + + private + + def application_settings + ::Gitlab::CurrentSettings.current_application_settings + end + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index af4930a341b824..acaf1ced983869 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -7127,6 +7127,9 @@ msgstr "" msgid "Authenticated web requests" msgstr "" +msgid "Authenticating..." +msgstr "" + msgid "Authentication" msgstr "" diff --git a/spec/frontend/ide/init_gitlab_web_ide_spec.js b/spec/frontend/ide/init_gitlab_web_ide_spec.js index 6a5bedb0bbb332..d7a16bec1c3ebb 100644 --- a/spec/frontend/ide/init_gitlab_web_ide_spec.js +++ b/spec/frontend/ide/init_gitlab_web_ide_spec.js @@ -40,6 +40,9 @@ const TEST_EDITOR_FONT_SRC_URL = 'http://gitlab.test/assets/gitlab-mono/GitLabMo const TEST_EDITOR_FONT_FORMAT = 'woff2'; const TEST_EDITOR_FONT_FAMILY = 'GitLab Mono'; +const TEST_OAUTH_CLIENT_ID = 'oauth-client-id-123abc'; +const TEST_OAUTH_CALLBACK_URL = 'https://example.com/oauth_callback'; + describe('ide/init_gitlab_web_ide', () => { let resolveConfirm; @@ -231,4 +234,29 @@ describe('ide/init_gitlab_web_ide', () => { ); }); }); + + describe('when oauth info is in dataset', () => { + beforeEach(() => { + findRootElement().dataset.clientId = TEST_OAUTH_CLIENT_ID; + findRootElement().dataset.callbackUrl = TEST_OAUTH_CALLBACK_URL; + + createSubject(); + }); + + it('calls start with element', () => { + expect(start).toHaveBeenCalledTimes(1); + expect(start).toHaveBeenCalledWith( + findRootElement(), + expect.objectContaining({ + auth: { + type: 'oauth', + clientId: TEST_OAUTH_CLIENT_ID, + callbackUrl: TEST_OAUTH_CALLBACK_URL, + protectRefreshToken: true, + }, + httpHeaders: undefined, + }), + ); + }); + }); }); diff --git a/spec/frontend/ide/lib/gitlab_web_ide/get_oauth_config_spec.js b/spec/frontend/ide/lib/gitlab_web_ide/get_oauth_config_spec.js new file mode 100644 index 00000000000000..3431068937f1eb --- /dev/null +++ b/spec/frontend/ide/lib/gitlab_web_ide/get_oauth_config_spec.js @@ -0,0 +1,16 @@ +import { getOAuthConfig } from '~/ide/lib/gitlab_web_ide/get_oauth_config'; + +describe('~/ide/lib/gitlab_web_ide/get_oauth_config', () => { + it('returns undefined if no clientId found', () => { + expect(getOAuthConfig({})).toBeUndefined(); + }); + + it('returns auth config from dataset', () => { + expect(getOAuthConfig({ clientId: 'test-clientId', callbackUrl: 'test-callbackUrl' })).toEqual({ + type: 'oauth', + clientId: 'test-clientId', + callbackUrl: 'test-callbackUrl', + protectRefreshToken: true, + }); + }); +}); diff --git a/spec/frontend/ide/mount_oauth_callback_spec.js b/spec/frontend/ide/mount_oauth_callback_spec.js new file mode 100644 index 00000000000000..6ac0b4e4615237 --- /dev/null +++ b/spec/frontend/ide/mount_oauth_callback_spec.js @@ -0,0 +1,53 @@ +import { oauthCallback } from '@gitlab/web-ide'; +import { TEST_HOST } from 'helpers/test_constants'; +import { mountOAuthCallback } from '~/ide/mount_oauth_callback'; + +jest.mock('@gitlab/web-ide'); + +const TEST_USERNAME = 'gandalf.the.grey'; +const TEST_GITLAB_WEB_IDE_PUBLIC_PATH = 'test/webpack/assets/gitlab-web-ide/public/path'; + +const TEST_OAUTH_CLIENT_ID = 'oauth-client-id-123abc'; +const TEST_OAUTH_CALLBACK_URL = 'https://example.com/oauth_callback'; + +describe('~/ide/mount_oauth_callback', () => { + const createRootElement = () => { + const el = document.createElement('div'); + + el.id = 'ide'; + el.dataset.clientId = TEST_OAUTH_CLIENT_ID; + el.dataset.callbackUrl = TEST_OAUTH_CALLBACK_URL; + + document.body.append(el); + }; + + beforeEach(() => { + gon.current_username = TEST_USERNAME; + process.env.GITLAB_WEB_IDE_PUBLIC_PATH = TEST_GITLAB_WEB_IDE_PUBLIC_PATH; + + createRootElement(); + }); + + afterEach(() => { + document.body.innerHTML = ''; + }); + + it('calls oauthCallback', () => { + expect(oauthCallback).not.toHaveBeenCalled(); + + mountOAuthCallback(); + + expect(oauthCallback).toHaveBeenCalledTimes(1); + expect(oauthCallback).toHaveBeenCalledWith({ + auth: { + type: 'oauth', + callbackUrl: TEST_OAUTH_CALLBACK_URL, + clientId: TEST_OAUTH_CLIENT_ID, + protectRefreshToken: true, + }, + gitlabUrl: TEST_HOST, + baseUrl: `${TEST_HOST}/${TEST_GITLAB_WEB_IDE_PUBLIC_PATH}`, + username: TEST_USERNAME, + }); + }); +}); diff --git a/spec/lib/gitlab/web_ide/default_oauth_application_spec.rb b/spec/lib/gitlab/web_ide/default_oauth_application_spec.rb new file mode 100644 index 00000000000000..cdb1d0181572b0 --- /dev/null +++ b/spec/lib/gitlab/web_ide/default_oauth_application_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::WebIde::DefaultOauthApplication, feature_category: :web_ide do + let_it_be(:current_user) { create(:user) } + let_it_be(:oauth_application) { create(:oauth_application, owner: nil) } + + describe '#feature_enabled?' do + where(:vscode_web_ide, :web_ide_oauth, :expectation) do + [ + [ref(:current_user), false, false], + [false, current_user, false], + [ref(:current_user), ref(:current_user), true] + ] + end + + with_them do + it 'returns the expected value' do + stub_feature_flags(vscode_web_ide: vscode_web_ide, web_ide_oauth: web_ide_oauth) + + expect(described_class.feature_enabled?(current_user)).to be(expectation) + end + end + end + + describe '#oauth_application' do + it 'returns web_ide_oauth_application from application_settings' do + expect(described_class.oauth_application).to be_nil + + stub_application_setting({ web_ide_oauth_application: oauth_application }) + + expect(described_class.oauth_application).to be(oauth_application) + end + end + + describe '#oauth_callback_url' do + it 'returns route URL for oauth callback' do + expect(described_class.oauth_callback_url).to eq(Gitlab::Routing.url_helpers.ide_oauth_redirect_url) + end + end + + describe '#ensure_oauth_application!' do + it 'if web_ide_oauth_application already exists, does nothing' do + expect(application_settings).not_to receive(:lock!) + expect(::Doorkeeper::Application).not_to receive(:new) + + stub_application_setting({ web_ide_oauth_application: oauth_application }) + + described_class.ensure_oauth_application! + end + + it 'if web_ide_oauth_application created while locked, does nothing' do + expect(application_settings).to receive(:lock!) do + stub_application_setting({ web_ide_oauth_application: oauth_application }) + end + expect(::Doorkeeper::Application).not_to receive(:new) + + described_class.ensure_oauth_application! + end + + it 'creates web_ide_oauth_application' do + expect(application_settings).to receive(:transaction).and_call_original + expect(::Doorkeeper::Application).to receive(:new).and_call_original + expect(::Gitlab::CurrentSettings).to receive(:expire_current_application_settings).and_call_original + + expect(application_settings.web_ide_oauth_application).to be_nil + + described_class.ensure_oauth_application! + + result = application_settings.web_ide_oauth_application + expect(result).not_to be_nil + expect(result).to have_attributes( + name: 'GitLab Web IDE', + redirect_uri: described_class.oauth_callback_url, + scopes: ['api'], + trusted: true, + confidential: false + ) + end + end + + def application_settings + ::Gitlab::CurrentSettings.current_application_settings + end +end diff --git a/spec/requests/ide_controller_spec.rb b/spec/requests/ide_controller_spec.rb index fae3d9533c5054..c132620aaf50ab 100644 --- a/spec/requests/ide_controller_spec.rb +++ b/spec/requests/ide_controller_spec.rb @@ -164,6 +164,14 @@ expect(response).to render_template('layouts/application') end + + it 'does not create oauth application' do + expect(Doorkeeper::Application).not_to receive(:new) + + subject + + expect(web_ide_oauth_application).to be_nil + end end describe 'vscode IDE' do @@ -177,6 +185,40 @@ expect(response).to render_template('layouts/fullscreen') end end + + describe 'with web_ide_oauth flag off' do + before do + stub_feature_flags(web_ide_oauth: false) + end + + it 'does not create oauth application' do + expect(Doorkeeper::Application).not_to receive(:new) + + subject + + expect(web_ide_oauth_application).to be_nil + end + end + + it 'ensures web_ide_oauth_application' do + expect(Doorkeeper::Application).to receive(:new).and_call_original + + subject + + expect(web_ide_oauth_application).not_to be_nil + expect(web_ide_oauth_application[:name]).to eq('GitLab Web IDE') + end + + it 'when web_ide_oauth_application already exists, does not create new one' do + existing_app = create(:oauth_application, owner_id: nil, owner_type: nil) + + stub_application_setting({ web_ide_oauth_application: existing_app }) + expect(Doorkeeper::Application).not_to receive(:new) + + subject + + expect(web_ide_oauth_application).to eq(existing_app) + end end describe 'content security policy' do @@ -199,4 +241,48 @@ end end end + + describe '#oauth_redirect', :aggregate_failures do + subject { get '/-/ide/oauth_redirect' } + + it 'with no web_ide_oauth_application, returns not_found' do + subject + + expect(response).to have_gitlab_http_status(:not_found) + end + + context 'with web_ide_oauth_application set' do + before do + stub_application_setting({ + web_ide_oauth_application: create(:oauth_application, owner_id: nil, owner_type: nil) + }) + end + + it 'returns ok and renders view' do + subject + + expect(response).to have_gitlab_http_status(:ok) + end + + it 'with vscode_web_ide flag off, returns not_found' do + stub_feature_flags(vscode_web_ide: false) + + subject + + expect(response).to have_gitlab_http_status(:not_found) + end + + it 'with web_ide_oauth flag off, returns not_found' do + stub_feature_flags(web_ide_oauth: false) + + subject + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + + def web_ide_oauth_application + ::Gitlab::CurrentSettings.current_application_settings.web_ide_oauth_application + end end -- GitLab From 65ca76b092b906feb45c78cb1af1976908cd4590 Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Sun, 3 Dec 2023 17:32:50 -0600 Subject: [PATCH 2/3] Move expire_current_application_settings out transaction --- lib/gitlab/web_ide/default_oauth_application.rb | 12 +++++++++--- .../gitlab/web_ide/default_oauth_application_spec.rb | 1 + 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/web_ide/default_oauth_application.rb b/lib/gitlab/web_ide/default_oauth_application.rb index e6751fc5e34188..01b7637c1c081d 100644 --- a/lib/gitlab/web_ide/default_oauth_application.rb +++ b/lib/gitlab/web_ide/default_oauth_application.rb @@ -19,12 +19,16 @@ def oauth_callback_url def ensure_oauth_application! return if oauth_application + should_expire_cache = false + application_settings.transaction do # note: This should run very rarely and should be safe for us to do a lock # https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132496#note_1587293087 application_settings.lock! - # We need to double check here so that requests previously waiting on the lock can now just skip + # note: `lock!`` breaks applicaiton_settings cache and will trigger another query. + # We need to double check here so that requests previously waiting on the lock can + # now just skip. next if oauth_application application = Doorkeeper::Application.new( @@ -35,9 +39,11 @@ def ensure_oauth_application! confidential: false) application.save! application_settings.update!(web_ide_oauth_application: application) - - ::Gitlab::CurrentSettings.expire_current_application_settings + should_expire_cache = true end + + # note: This needs to happen outside the transaction, but only if we actually changed something + ::Gitlab::CurrentSettings.expire_current_application_settings if should_expire_cache end private diff --git a/spec/lib/gitlab/web_ide/default_oauth_application_spec.rb b/spec/lib/gitlab/web_ide/default_oauth_application_spec.rb index cdb1d0181572b0..2a979cdb55c4f9 100644 --- a/spec/lib/gitlab/web_ide/default_oauth_application_spec.rb +++ b/spec/lib/gitlab/web_ide/default_oauth_application_spec.rb @@ -55,6 +55,7 @@ stub_application_setting({ web_ide_oauth_application: oauth_application }) end expect(::Doorkeeper::Application).not_to receive(:new) + expect(::Gitlab::CurrentSettings).not_to receive(:expire_current_application_settings) described_class.ensure_oauth_application! end -- GitLab From 402ab9f7bc34278a7969cb3970035fb3b9ad2cff Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Wed, 6 Dec 2023 16:02:16 -0600 Subject: [PATCH 3/3] Polish specs from maintainer feedback --- app/controllers/ide_controller.rb | 2 +- .../gitlab/web_ide/default_oauth_application_spec.rb | 2 +- spec/requests/ide_controller_spec.rb | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/controllers/ide_controller.rb b/app/controllers/ide_controller.rb index d00610b2b03e97..4a4d41f3e6fe73 100644 --- a/app/controllers/ide_controller.rb +++ b/app/controllers/ide_controller.rb @@ -31,7 +31,7 @@ def index def oauth_redirect return render_404 unless ::Gitlab::WebIde::DefaultOauthApplication.feature_enabled?(current_user) # TODO - It's **possible** we end up here and no oauth application has been set up. - # We need to have better handling of these edge cases. Here's a # follow-up issue: + # We need to have better handling of these edge cases. Here's a follow-up issue: # https://gitlab.com/gitlab-org/gitlab/-/issues/433322 return render_404 unless ::Gitlab::WebIde::DefaultOauthApplication.oauth_application diff --git a/spec/lib/gitlab/web_ide/default_oauth_application_spec.rb b/spec/lib/gitlab/web_ide/default_oauth_application_spec.rb index 2a979cdb55c4f9..9bfdc799aec2d4 100644 --- a/spec/lib/gitlab/web_ide/default_oauth_application_spec.rb +++ b/spec/lib/gitlab/web_ide/default_oauth_application_spec.rb @@ -10,7 +10,7 @@ where(:vscode_web_ide, :web_ide_oauth, :expectation) do [ [ref(:current_user), false, false], - [false, current_user, false], + [false, ref(:current_user), false], [ref(:current_user), ref(:current_user), true] ] end diff --git a/spec/requests/ide_controller_spec.rb b/spec/requests/ide_controller_spec.rb index c132620aaf50ab..20d890fadbf12a 100644 --- a/spec/requests/ide_controller_spec.rb +++ b/spec/requests/ide_controller_spec.rb @@ -243,10 +243,10 @@ end describe '#oauth_redirect', :aggregate_failures do - subject { get '/-/ide/oauth_redirect' } + subject(:oauth_redirect) { get '/-/ide/oauth_redirect' } it 'with no web_ide_oauth_application, returns not_found' do - subject + oauth_redirect expect(response).to have_gitlab_http_status(:not_found) end @@ -259,7 +259,7 @@ end it 'returns ok and renders view' do - subject + oauth_redirect expect(response).to have_gitlab_http_status(:ok) end @@ -267,7 +267,7 @@ it 'with vscode_web_ide flag off, returns not_found' do stub_feature_flags(vscode_web_ide: false) - subject + oauth_redirect expect(response).to have_gitlab_http_status(:not_found) end @@ -275,7 +275,7 @@ it 'with web_ide_oauth flag off, returns not_found' do stub_feature_flags(web_ide_oauth: false) - subject + oauth_redirect expect(response).to have_gitlab_http_status(:not_found) end -- GitLab