From e8b15f753f7e2b26f670688665ffca3b0ee1cbbc Mon Sep 17 00:00:00 2001 From: Chad Woolley Date: Mon, 25 Aug 2025 03:20:47 -0700 Subject: [PATCH 1/2] Add support for Workspaces OAuth app - See https://gitlab.com/gitlab-org/gitlab/-/issues/545064 Changelog: changed EE: true --- .../workspaces_oauth_application_id.yml | 12 ++ ..._application_id_to_application_settings.rb | 10 ++ ...002_add_fk_workspaces_oauth_application.rb | 24 ++++ db/schema_migrations/20250825000001 | 1 + db/schema_migrations/20250825000002 | 1 + db/structure.sql | 6 + ee/app/models/ee/application_setting.rb | 1 + ee/lib/api/remote_development/api.rb | 9 ++ .../internal/agent/agentw.rb | 38 ++++++ ee/lib/ee/api/api.rb | 1 + ee/lib/remote_development/README.md | 6 +- ee/lib/remote_development/messages.rb | 3 + .../settings/default_settings.rb | 1 + .../settings/gitlab_config_reader.rb | 51 +++++++ ee/lib/remote_development/settings/main.rb | 3 + .../remote_development/settings/messages.rb | 1 + .../server_config/main.rb | 50 +++++++ .../oauth_application_attributes_generator.rb | 45 +++++++ .../oauth_application_ensurer.rb | 76 +++++++++++ .../server_config/values_extractor.rb | 29 ++++ .../workspaces_oauth_applications.rb | 28 ++++ .../settings/gitlab_config_reader_spec.rb | 63 +++++++++ .../remote_development/settings/main_spec.rb | 13 ++ .../settings/settings_initializer_spec.rb | 3 + .../settings/settings_integration_spec.rb | 43 +++++- .../server_config/main_integration_spec.rb | 127 ++++++++++++++++++ .../server_config/main_spec.rb | 49 +++++++ ...h_application_attributes_generator_spec.rb | 42 ++++++ .../oauth_application_ensurer_spec.rb | 75 +++++++++++ .../server_config/values_extractor_spec.rb | 36 +++++ ee/spec/models/application_setting_spec.rb | 1 + .../internal/agent/agentw_spec.rb | 77 +++++++++++ .../remote_development/integration_spec.rb | 77 ++++++++--- 33 files changed, 979 insertions(+), 23 deletions(-) create mode 100644 config/application_setting_columns/workspaces_oauth_application_id.yml create mode 100644 db/migrate/20250825000001_add_workspaces_oauth_application_id_to_application_settings.rb create mode 100644 db/migrate/20250825000002_add_fk_workspaces_oauth_application.rb create mode 100644 db/schema_migrations/20250825000001 create mode 100644 db/schema_migrations/20250825000002 create mode 100644 ee/lib/api/remote_development/api.rb create mode 100644 ee/lib/api/remote_development/internal/agent/agentw.rb create mode 100644 ee/lib/remote_development/settings/gitlab_config_reader.rb create mode 100644 ee/lib/remote_development/workspaces_server_operations/server_config/main.rb create mode 100644 ee/lib/remote_development/workspaces_server_operations/server_config/oauth_application_attributes_generator.rb create mode 100644 ee/lib/remote_development/workspaces_server_operations/server_config/oauth_application_ensurer.rb create mode 100644 ee/lib/remote_development/workspaces_server_operations/server_config/values_extractor.rb create mode 100644 ee/spec/factories/remote_development/workspaces_oauth_applications.rb create mode 100644 ee/spec/lib/remote_development/settings/gitlab_config_reader_spec.rb create mode 100644 ee/spec/lib/remote_development/workspaces_server_operations/server_config/main_integration_spec.rb create mode 100644 ee/spec/lib/remote_development/workspaces_server_operations/server_config/main_spec.rb create mode 100644 ee/spec/lib/remote_development/workspaces_server_operations/server_config/oauth_application_attributes_generator_spec.rb create mode 100644 ee/spec/lib/remote_development/workspaces_server_operations/server_config/oauth_application_ensurer_spec.rb create mode 100644 ee/spec/lib/remote_development/workspaces_server_operations/server_config/values_extractor_spec.rb create mode 100644 ee/spec/requests/api/remote_development/internal/agent/agentw_spec.rb diff --git a/config/application_setting_columns/workspaces_oauth_application_id.yml b/config/application_setting_columns/workspaces_oauth_application_id.yml new file mode 100644 index 00000000000000..ad34104c0034f6 --- /dev/null +++ b/config/application_setting_columns/workspaces_oauth_application_id.yml @@ -0,0 +1,12 @@ +--- +api_type: +attr: workspaces_oauth_application_id +clusterwide: false +column: workspaces_oauth_application_id +db_type: bigint +default: +description: ID of the OAuth application used by the Workspaces HTTP Server in GitLab Agent Server(KAS) +encrypted: false +gitlab_com_different_than_default: true +jihu: false +not_null: false diff --git a/db/migrate/20250825000001_add_workspaces_oauth_application_id_to_application_settings.rb b/db/migrate/20250825000001_add_workspaces_oauth_application_id_to_application_settings.rb new file mode 100644 index 00000000000000..9966ba836e6ea4 --- /dev/null +++ b/db/migrate/20250825000001_add_workspaces_oauth_application_id_to_application_settings.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class AddWorkspacesOauthApplicationIdToApplicationSettings < Gitlab::Database::Migration[2.3] + milestone "18.3" + + # @return [void] + def change + add_column :application_settings, :workspaces_oauth_application_id, :bigint, if_not_exists: true + end +end diff --git a/db/migrate/20250825000002_add_fk_workspaces_oauth_application.rb b/db/migrate/20250825000002_add_fk_workspaces_oauth_application.rb new file mode 100644 index 00000000000000..e8ecfdef9c1a17 --- /dev/null +++ b/db/migrate/20250825000002_add_fk_workspaces_oauth_application.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class AddFkWorkspacesOauthApplication < Gitlab::Database::Migration[2.3] + milestone "18.3" + disable_ddl_transaction! + + INDEX_NAME = 'index_application_settings_workspaces_oauth_application_id' + + # @return [void] + def up + add_concurrent_index :application_settings, :workspaces_oauth_application_id, name: INDEX_NAME + add_concurrent_foreign_key :application_settings, :oauth_applications, + column: :workspaces_oauth_application_id, + on_delete: :nullify + end + + # @return [void] + def down + with_lock_retries do + remove_foreign_key :application_settings, column: :workspaces_oauth_application_id + end + remove_concurrent_index_by_name :application_settings, INDEX_NAME + end +end diff --git a/db/schema_migrations/20250825000001 b/db/schema_migrations/20250825000001 new file mode 100644 index 00000000000000..8293010a8fba3a --- /dev/null +++ b/db/schema_migrations/20250825000001 @@ -0,0 +1 @@ +3281260662c58a8bf3c46c129753a44bf9fc74f459508ac95c04c382efb06696 \ No newline at end of file diff --git a/db/schema_migrations/20250825000002 b/db/schema_migrations/20250825000002 new file mode 100644 index 00000000000000..541f4ea245d1ac --- /dev/null +++ b/db/schema_migrations/20250825000002 @@ -0,0 +1 @@ +b86d8b9dd82bc0a71fd0a7a6c8b46caa75b1ed803c95a94baf9b0db4c12bd386 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index be4cf22d3aac58..005c3bdc217d8d 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -10572,6 +10572,7 @@ CREATE TABLE application_settings ( resource_access_tokens_settings jsonb DEFAULT '{}'::jsonb NOT NULL, auto_duo_code_review_enabled boolean DEFAULT false NOT NULL, lock_auto_duo_code_review_enabled boolean DEFAULT false NOT NULL, + workspaces_oauth_application_id bigint, CONSTRAINT app_settings_container_reg_cleanup_tags_max_list_size_positive CHECK ((container_registry_cleanup_tags_service_max_list_size >= 0)), CONSTRAINT app_settings_dep_proxy_ttl_policies_worker_capacity_positive CHECK ((dependency_proxy_ttl_group_policy_worker_capacity >= 0)), CONSTRAINT app_settings_ext_pipeline_validation_service_url_text_limit CHECK ((char_length(external_pipeline_validation_service_url) <= 255)), @@ -37090,6 +37091,8 @@ CREATE INDEX index_application_settings_on_usage_stats_set_by_user_id ON applica CREATE INDEX index_application_settings_web_ide_oauth_application_id ON application_settings USING btree (web_ide_oauth_application_id); +CREATE INDEX index_application_settings_workspaces_oauth_application_id ON application_settings USING btree (workspaces_oauth_application_id); + CREATE INDEX index_approval_group_rules_groups_on_group_id ON approval_group_rules_groups USING btree (group_id); CREATE INDEX index_approval_group_rules_on_approval_policy_rule_id ON approval_group_rules USING btree (approval_policy_rule_id); @@ -46611,6 +46614,9 @@ ALTER TABLE ONLY dast_scanner_profiles_builds ALTER TABLE ONLY protected_environment_deploy_access_levels ADD CONSTRAINT fk_5d9b05a7e9 FOREIGN KEY (protected_environment_project_id) REFERENCES projects(id) ON DELETE CASCADE; +ALTER TABLE ONLY application_settings + ADD CONSTRAINT fk_5d9b886930 FOREIGN KEY (workspaces_oauth_application_id) REFERENCES oauth_applications(id) ON DELETE SET NULL; + ALTER TABLE ONLY merge_requests_approval_rules_merge_requests ADD CONSTRAINT fk_5ddc4a2f7b FOREIGN KEY (approval_rule_id) REFERENCES merge_requests_approval_rules(id) ON DELETE CASCADE; diff --git a/ee/app/models/ee/application_setting.rb b/ee/app/models/ee/application_setting.rb index 41d42995be928f..98b9aff4a905a8 100644 --- a/ee/app/models/ee/application_setting.rb +++ b/ee/app/models/ee/application_setting.rb @@ -20,6 +20,7 @@ module ApplicationSetting ERROR_NO_SEATS_AVAILABLE = 'NO_SEATS_AVAILABLE' belongs_to :file_template_project, class_name: "Project" + belongs_to :workspaces_oauth_application, class_name: 'Doorkeeper::Application', optional: true jsonb_accessor :search, global_search_code_enabled: [:boolean, { default: true }], diff --git a/ee/lib/api/remote_development/api.rb b/ee/lib/api/remote_development/api.rb new file mode 100644 index 00000000000000..c07aae2bfc3290 --- /dev/null +++ b/ee/lib/api/remote_development/api.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +module API + module RemoteDevelopment + class API < ::API::Base + mount ::API::RemoteDevelopment::Internal::Agent::Agentw + end + end +end diff --git a/ee/lib/api/remote_development/internal/agent/agentw.rb b/ee/lib/api/remote_development/internal/agent/agentw.rb new file mode 100644 index 00000000000000..54d668c6d3d630 --- /dev/null +++ b/ee/lib/api/remote_development/internal/agent/agentw.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module API + module RemoteDevelopment + module Internal + module Agent + class Agentw < ::API::Base + before do + authenticate_gitlab_kas_request! + end + + helpers ::API::Helpers::KasHelpers + + namespace "internal" do + namespace "agents" do + namespace "agentw" do + desc "server_config" do + detail "Returns configuration for Workspaces HTTP Server." + end + get "/server_config", feature_category: :workspaces, urgency: :low do + response = ::RemoteDevelopment::CommonService.execute( + domain_main_class: ::RemoteDevelopment::WorkspacesServerOperations::ServerConfig::Main, + domain_main_class_args: { request: request } + ) + + # NOTE: There's currently no way an error can be returned other than an unexpected raised + # exception, so we assume success. + + response.payload + end + end + end + end + end + end + end + end +end diff --git a/ee/lib/ee/api/api.rb b/ee/lib/ee/api/api.rb index a675fee6270856..e5c9872d407c1f 100644 --- a/ee/lib/ee/api/api.rb +++ b/ee/lib/ee/api/api.rb @@ -38,6 +38,7 @@ module API mount ::API::License mount ::API::ProjectMirror mount ::API::ProjectPushRule + mount ::API::RemoteDevelopment::API mount ::API::GroupPushRule mount ::API::MergeTrains mount ::API::MemberRoles diff --git a/ee/lib/remote_development/README.md b/ee/lib/remote_development/README.md index 791b86196f589e..754046788ff0b4 100644 --- a/ee/lib/remote_development/README.md +++ b/ee/lib/remote_development/README.md @@ -1209,9 +1209,9 @@ order of the steps in the 1. First, the default value is used, via the first `SettingsInitializer` step in the ROP chain. 1. Next, the default values may be overridden by any of the next steps in the ROP chain. For example: - 1. `CurrentSettingsReader` - 1. `GitlabSettingsReader` (not yet implemented) - 1. `CascadingSettingsReader` (not yet implemented) + 1. `CurrentSettingsReader` - reads relevant values from `Gitlab::CurrentSettings`, AKA the `application_settings` table in the DB. + 1. `GitlabConfigReader` - reads relevant values from `Gitlab.config`, AKA [the`gitlab.yml` file](https://docs.gitlab.com/omnibus/settings/gitlab.yml/). + 1. `CascadingSettingsReader` - not yet implemented. 1. Next, the `Gitlab::Fp::Settings::EnvVarOverrideProcessor` allows ENV values to be used if they are defined (i.e. not `nil`). The `EnvVarOverrideProcessor` is intentionally placed as the last step and highest precedence (but before any validation steps), so it can always diff --git a/ee/lib/remote_development/messages.rb b/ee/lib/remote_development/messages.rb index d0f4ab37ba3680..aaed84872295f4 100644 --- a/ee/lib/remote_development/messages.rb +++ b/ee/lib/remote_development/messages.rb @@ -75,5 +75,8 @@ module Messages # Devfile domain events DevfileValidateSuccessful = Class.new(Gitlab::Fp::Message) + + # Workspaces Server Operations domain events + WorkspacesServerConfigSuccessful = Class.new(Gitlab::Fp::Message) end end diff --git a/ee/lib/remote_development/settings/default_settings.rb b/ee/lib/remote_development/settings/default_settings.rb index 44cb5a86b7f46a..b0139eee28afcc 100644 --- a/ee/lib/remote_development/settings/default_settings.rb +++ b/ee/lib/remote_development/settings/default_settings.rb @@ -25,6 +25,7 @@ def self.default_settings default_resources_per_workspace_container: [{}, Hash], default_runtime_class: ["", String], full_reconciliation_interval_seconds: [3600, Integer], + gitlab_kas_external_url: ["", String], gitlab_workspaces_proxy_namespace: ["gitlab-workspaces", String], image_pull_secrets: [[], Array], labels: [{}, Hash], diff --git a/ee/lib/remote_development/settings/gitlab_config_reader.rb b/ee/lib/remote_development/settings/gitlab_config_reader.rb new file mode 100644 index 00000000000000..513839eb5d9e25 --- /dev/null +++ b/ee/lib/remote_development/settings/gitlab_config_reader.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module RemoteDevelopment + module Settings + class GitlabConfigReader + include Messages + + RELEVANT_SETTING_NAMES = %i[ + gitlab_kas_external_url + ].freeze + + # @param [Hash] context + # @return [Gitlab::Fp::Result] + def self.read(context) + err_result = nil + + context[:settings].each_key do |setting_name| + next unless RELEVANT_SETTING_NAMES.include?(setting_name) + + gitlab_config_value = + case setting_name + in :gitlab_kas_external_url + # noinspection RubyResolve -- RubyMine can't find the dynamic Gitlab.config methods + Gitlab.config.gitlab_kas&.external_url + end + + next if gitlab_config_value.nil? + + setting_type = context[:setting_types][setting_name] + + unless gitlab_config_value.is_a?(setting_type) + # err_result will be set to a non-nil Gitlab::Fp::Result.err if type check fails + err_result = ::Gitlab::Fp::Result.err(SettingsGitlabConfigReadFailed.new( + details: "Gitlab.config.#{setting_name} type of '#{gitlab_config_value.class}' " \ + "did not match initialized Remote Development Settings type of '#{setting_type}'." + )) + + break + end + + # CurrentSettings entry of correct type found for declared default setting, use its value as override + context[:settings][setting_name] = gitlab_config_value + end + + return err_result if err_result + + ::Gitlab::Fp::Result.ok(context) + end + end + end +end diff --git a/ee/lib/remote_development/settings/main.rb b/ee/lib/remote_development/settings/main.rb index 51e01f121e2586..be980dc6b9918c 100644 --- a/ee/lib/remote_development/settings/main.rb +++ b/ee/lib/remote_development/settings/main.rb @@ -18,6 +18,7 @@ def self.get_settings(context) initial_result .map(SettingsInitializer.method(:init)) .and_then(CurrentSettingsReader.method(:read)) + .and_then(GitlabConfigReader.method(:read)) # NOTE: EnvVarOverrideProcessor is kept as last settings processing step, so it can always be used # to easily overrideany settings for local or temporary testing, but still before all validators. .and_then(Gitlab::Fp::Settings::EnvVarOverrideProcessor.method(:process)) @@ -35,6 +36,8 @@ def self.get_settings(context) generate_error_response_from_message(message: message, reason: :internal_server_error) in { err: SettingsCurrentSettingsReadFailed => message } generate_error_response_from_message(message: message, reason: :internal_server_error) + in { err: SettingsGitlabConfigReadFailed => message } + generate_error_response_from_message(message: message, reason: :internal_server_error) in { err: SettingsFullReconciliationIntervalSecondsValidationFailed => message } generate_error_response_from_message(message: message, reason: :internal_server_error) in { err: SettingsPartialReconciliationIntervalSecondsValidationFailed => message } diff --git a/ee/lib/remote_development/settings/messages.rb b/ee/lib/remote_development/settings/messages.rb index 4ce46158de79f9..dbff3aff1661fd 100644 --- a/ee/lib/remote_development/settings/messages.rb +++ b/ee/lib/remote_development/settings/messages.rb @@ -13,6 +13,7 @@ module Messages SettingsCurrentSettingsReadFailed = Class.new(Gitlab::Fp::Message) SettingsEnvironmentVariableOverrideFailed = Class.new(Gitlab::Fp::Message) SettingsFullReconciliationIntervalSecondsValidationFailed = Class.new(Gitlab::Fp::Message) + SettingsGitlabConfigReadFailed = Class.new(Gitlab::Fp::Message) SettingsPartialReconciliationIntervalSecondsValidationFailed = Class.new(Gitlab::Fp::Message) SettingsNetworkPolicyEgressValidationFailed = Class.new(Gitlab::Fp::Message) #--------------------------------------------------------- diff --git a/ee/lib/remote_development/workspaces_server_operations/server_config/main.rb b/ee/lib/remote_development/workspaces_server_operations/server_config/main.rb new file mode 100644 index 00000000000000..9dfda417ec6d2c --- /dev/null +++ b/ee/lib/remote_development/workspaces_server_operations/server_config/main.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +module RemoteDevelopment + module WorkspacesServerOperations + module ServerConfig + class Main + include Messages + extend Gitlab::Fp::MessageSupport + + # @param [Hash] context + # @return [Hash] + def self.main(context) + initial_result = Gitlab::Fp::Result.ok(context) + + result = + initial_result + .map(OauthApplicationAttributesGenerator.method(:generate)) + .map(OauthApplicationEnsurer.method(:ensure)) + .map(ValuesExtractor.method(:extract)) + .map( + # As the final step, return the response_payload content in a WorkspacesServerConfigSuccessful message + ->(context) do + WorkspacesServerConfigSuccessful.new(context.fetch(:response_payload)) + end + ) + + # noinspection RubyMismatchedReturnType -- RubyMine not properly detecting return type of Hash + case result + in { ok: WorkspacesServerConfigSuccessful => message } + # Type-check the payload before returning it + message.content => { + api_external_url: String, + oauth_client_id: String, + oauth_redirect_url: String + } + { status: :success, payload: message.content } + + # NOTE: This ROP chain currently consists of only `map` steps, there are no `and_then` steps. Therefore it + # is not possible for anything other than the AgentPrerequisitesSuccessful message from last lambda + # step to be returned. If we ever add an `and_then` step, we should uncomment the else case below, and + # add an appropriate spec example named: "when an unmatched error is returned, an exception is raised" + # + # else + # raise Gitlab::Fp::UnmatchedResultError.new(result: result) + end + end + end + end + end +end diff --git a/ee/lib/remote_development/workspaces_server_operations/server_config/oauth_application_attributes_generator.rb b/ee/lib/remote_development/workspaces_server_operations/server_config/oauth_application_attributes_generator.rb new file mode 100644 index 00000000000000..a49212bffb106e --- /dev/null +++ b/ee/lib/remote_development/workspaces_server_operations/server_config/oauth_application_attributes_generator.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +module RemoteDevelopment + module WorkspacesServerOperations + module ServerConfig + class OauthApplicationAttributesGenerator + OAUTH_NAME = "GitLab Workspaces" + API_EXTERNAL_URL_PATH_SEGMENT = "workspaces" + OAUTH_REDIRECT_URI_PATH_SEGMENT = "oauth/redirect" + TRUSTED = true + CONFIDENTIAL = false + + # @param [Hash] context + # @return [Hash] + def self.generate(context) + context => { + settings: { + gitlab_kas_external_url: gitlab_kas_external_url + } + } + + api_external_url = URI.parse(gitlab_kas_external_url) + api_external_url.scheme = api_external_url.scheme.in?(%w[grpcs wss]) ? "https" : "http" + api_external_url.path = "#{api_external_url.path}/#{API_EXTERNAL_URL_PATH_SEGMENT}" + + redirect_uri = api_external_url.dup + redirect_uri.path = "#{redirect_uri.path}/#{OAUTH_REDIRECT_URI_PATH_SEGMENT}" + + attributes = { + name: OAUTH_NAME, + redirect_uri: redirect_uri.to_s, + scopes: "openid", + trusted: TRUSTED, + confidential: CONFIDENTIAL + } + + context.merge( + api_external_url: api_external_url.to_s, + oauth_application_attributes: attributes + ) + end + end + end + end +end diff --git a/ee/lib/remote_development/workspaces_server_operations/server_config/oauth_application_ensurer.rb b/ee/lib/remote_development/workspaces_server_operations/server_config/oauth_application_ensurer.rb new file mode 100644 index 00000000000000..473f65195007e1 --- /dev/null +++ b/ee/lib/remote_development/workspaces_server_operations/server_config/oauth_application_ensurer.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +module RemoteDevelopment + module WorkspacesServerOperations + module ServerConfig + class OauthApplicationEnsurer + # @param [Hash] context + # @return [Hash] + def self.ensure(context) + # NOTE 1: The logic in this class is based on the existing logic in the WebIde::DefaultOauthApplication class. + # See that class and https://gitlab.com/gitlab-org/gitlab/-/merge_requests/132496 for more context. + # The main addition here is the automatic update if the attributes have been modified to be incorrect. + # + # NOTE 2: Even if there happens to be any unexpected errors due to a race condition due to + # multiple KAS clients calling this code path concurrently, and any of them get an exception, + # this is still fine, because the KAS clients have backoff-retry logic, and will successfully retrieve the + # (now-created) record on the re-attempt. + + context => { + oauth_application_attributes: Hash => oauth_application_attributes, + request: (Grape::Request | ActionController::TestRequest) => request, + } + + # If there is no existing link to the app in application_settings, create the app and link it. + unless 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! + + # note: `lock!`` breaks application_settings cache and will trigger another query. + # We need to double check here by re-attempting to fetch the app from the settings, + # so that requests previously waiting on the lock can now just skip. + next if oauth_application + + service = ::Applications::CreateService.new(nil, request, oauth_application_attributes) + created_oauth_app = service.execute + + # We have to manually check whether it got saved, because ::Applications::CreateService doesn't return + # a ServiceRespone, it just returns the (possibly invalid and un-saved) record. + raise ActiveRecord::RecordInvalid.new(created_oauth_app) unless created_oauth_app.persisted? # rubocop:disable Style/RaiseArgs -- We want to directly use ActiveRecord::RecordInvalid.new(created_oauth_app) so it preserves the record attribute of the exception + + application_settings.update!(workspaces_oauth_application: created_oauth_app) + 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 + + # In case the app already existed, use the <= operator to ensure that the attributes are a subset of the + # existing application attributes to ensure that no admin user has manually changed the application attributes + unless oauth_application_attributes.stringify_keys <= oauth_application.attributes + oauth_application.update!(oauth_application_attributes) + end + + context.merge(workspaces_oauth_application: oauth_application) + end + + # @return [Authn::OauthApplication] + def self.oauth_application + application_settings.workspaces_oauth_application + end + + # @return [ApplicationSetting] + def self.application_settings + ::Gitlab::CurrentSettings.current_application_settings + end + + private_class_method :oauth_application, :application_settings + end + end + end +end diff --git a/ee/lib/remote_development/workspaces_server_operations/server_config/values_extractor.rb b/ee/lib/remote_development/workspaces_server_operations/server_config/values_extractor.rb new file mode 100644 index 00000000000000..919524fdf6d55f --- /dev/null +++ b/ee/lib/remote_development/workspaces_server_operations/server_config/values_extractor.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module RemoteDevelopment + module WorkspacesServerOperations + module ServerConfig + class ValuesExtractor + # @param [Hash] context + # @return [Hash] + def self.extract(context) + context => { + api_external_url: String => api_external_url, + workspaces_oauth_application: workspaces_oauth_application, + } + + { + response_payload: { + api_external_url: api_external_url, + oauth_client_id: workspaces_oauth_application.uid, + # NOTE: We are changing the terminology here from `redirect_uri` to `redirect_url` for the external API. + # The Doorkeeper API calls it "uri", so we use that internally in the code, + # but all the other configuration for KAS and related docs use the term URL. + oauth_redirect_url: workspaces_oauth_application.redirect_uri + } + } + end + end + end + end +end diff --git a/ee/spec/factories/remote_development/workspaces_oauth_applications.rb b/ee/spec/factories/remote_development/workspaces_oauth_applications.rb new file mode 100644 index 00000000000000..7c66a4ac9291b7 --- /dev/null +++ b/ee/spec/factories/remote_development/workspaces_oauth_applications.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +FactoryBot.define do + # Factory for Workspaces OAuth application (renamed from workspaces_doorkeeper_application) + factory :workspaces_oauth_application, class: 'Authn::OauthApplication', parent: :oauth_application do + without_owner + + transient do + app_attributes do + RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationAttributesGenerator + .generate({ settings: { gitlab_kas_external_url: Gitlab.config.gitlab_kas.external_url } }) + .fetch(:oauth_application_attributes) + end + end + + name { app_attributes[:name] } + redirect_uri { app_attributes[:redirect_uri] } + scopes { app_attributes[:scopes] } + trusted { app_attributes[:trusted] } + confidential { app_attributes[:confidential] } + + after(:create) do |oauth_app| + settings = ::Gitlab::CurrentSettings.current_application_settings + settings.update!(workspaces_oauth_application_id: oauth_app.id) + ::Gitlab::CurrentSettings.expire_current_application_settings + end + end +end diff --git a/ee/spec/lib/remote_development/settings/gitlab_config_reader_spec.rb b/ee/spec/lib/remote_development/settings/gitlab_config_reader_spec.rb new file mode 100644 index 00000000000000..13b7494f564d7a --- /dev/null +++ b/ee/spec/lib/remote_development/settings/gitlab_config_reader_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' + +RSpec.describe ::RemoteDevelopment::Settings::GitlabConfigReader, feature_category: :workspaces do + include ResultMatchers + + let(:gitlab_kas_external_url_type) { String } + + let(:context) do + { + settings: { + non_relevant_setting: "non_relevant", + gitlab_kas_external_url: "default value" + }, + setting_types: { + non_relevant_setting: String, + gitlab_kas_external_url: gitlab_kas_external_url_type + } + } + end + + subject(:result) do + described_class.read(context) + end + + describe "gitlab_kas_external_url setting" do + context "when config has a value set" do + before do + allow(Gitlab).to receive_message_chain(:config, :gitlab_kas, :external_url) { "value from file" } + end + + it "returns the value" do + expect(result).to be_ok_result(context) + expect(result.unwrap[:settings][:gitlab_kas_external_url]).to eq("value from file") + end + end + + context "when config does not have a value set" do + before do + allow(Gitlab).to receive_message_chain(:config, :gitlab_kas).and_return(nil) + end + + it "returns the value" do + expect(result).to be_ok_result(context) + expect(result.unwrap[:settings][:gitlab_kas_external_url]).to eq("default value") + end + end + end + + context "when the type from GitLab.config does not match the declared remote development setting type" do + let(:gitlab_kas_external_url_type) { Integer } + + it "returns an err Result containing a read failed message with details" do + expect(result).to be_err_result( + RemoteDevelopment::Settings::Messages::SettingsGitlabConfigReadFailed.new( + details: "Gitlab.config.gitlab_kas_external_url type of 'String' " \ + "did not match initialized Remote Development Settings type of 'Integer'." + ) + ) + end + end +end diff --git a/ee/spec/lib/remote_development/settings/main_spec.rb b/ee/spec/lib/remote_development/settings/main_spec.rb index 3fffaeb8bd6e46..79822fc9184854 100644 --- a/ee/spec/lib/remote_development/settings/main_spec.rb +++ b/ee/spec/lib/remote_development/settings/main_spec.rb @@ -10,6 +10,7 @@ [ [RemoteDevelopment::Settings::SettingsInitializer, :map], [RemoteDevelopment::Settings::CurrentSettingsReader, :and_then], + [RemoteDevelopment::Settings::GitlabConfigReader, :and_then], [Gitlab::Fp::Settings::EnvVarOverrideProcessor, :and_then], [RemoteDevelopment::Settings::ReconciliationIntervalSecondsValidator, :and_then], [RemoteDevelopment::Settings::NetworkPolicyEgressValidator, :and_then] @@ -70,6 +71,18 @@ reason: :internal_server_error }, ], + [ + "when GitlabConfigReader returns SettingsGitlabConfigReadFailed", + { + step_class: RemoteDevelopment::Settings::GitlabConfigReader, + returned_message: lazy { RemoteDevelopment::Settings::Messages::SettingsGitlabConfigReadFailed.new(err_message_content) } + }, + { + status: :error, + message: lazy { "Settings gitlab config read failed: #{error_details}" }, + reason: :internal_server_error + }, + ], [ "when EnvVarOverrideProcessor returns SettingsEnvironmentVariableOverrideFailed", { diff --git a/ee/spec/lib/remote_development/settings/settings_initializer_spec.rb b/ee/spec/lib/remote_development/settings/settings_initializer_spec.rb index 81ca59bb8e1b56..ca5230f9fec6e3 100644 --- a/ee/spec/lib/remote_development/settings/settings_initializer_spec.rb +++ b/ee/spec/lib/remote_development/settings/settings_initializer_spec.rb @@ -34,6 +34,7 @@ :default_resources_per_workspace_container, :default_runtime_class, :full_reconciliation_interval_seconds, + :gitlab_kas_external_url, :gitlab_workspaces_proxy_namespace, :image_pull_secrets, :labels, @@ -58,6 +59,7 @@ default_resources_per_workspace_container: {}, default_runtime_class: "", full_reconciliation_interval_seconds: 3600, + gitlab_kas_external_url: "", gitlab_workspaces_proxy_namespace: "gitlab-workspaces", image_pull_secrets: [], labels: {}, @@ -86,6 +88,7 @@ default_resources_per_workspace_container: Hash, default_runtime_class: String, full_reconciliation_interval_seconds: Integer, + gitlab_kas_external_url: String, gitlab_workspaces_proxy_namespace: String, image_pull_secrets: Array, labels: Hash, diff --git a/ee/spec/lib/remote_development/settings/settings_integration_spec.rb b/ee/spec/lib/remote_development/settings/settings_integration_spec.rb index 2b5be758744d26..f6d40aa0095d0d 100644 --- a/ee/spec/lib/remote_development/settings/settings_integration_spec.rb +++ b/ee/spec/lib/remote_development/settings/settings_integration_spec.rb @@ -32,7 +32,48 @@ end end - context "when there is and ENV var override and also a ::Gitlab::CurrentSettings override" do + context "when there is a ::Gitlab::CurrentSettings override" do + let(:overridden_setting_value_from_current_settings) { "value_from_current_settings" } + let(:current_settings_class) do + # NOTE: We intentionally do not attempt to make this test use the real `ApplicationSetting` class with + # `stub_application_setting`, because this resulted in occasional cache-related test failures in + # `CurrentSettings.respond_to?` for `ApplicationSetting` fields which are in the process of being deprecated. + double( # rubocop:disable RSpec/VerifiedDoubles -- We don't care about using a verified double here, this is a mix of class and dynamic stubbed methods. + "Gitlab::CurrentSettings", + respond_to?: true, + default_branch_name: overridden_setting_value_from_current_settings + ) + end + + before do + stub_const("Gitlab::CurrentSettings", current_settings_class) + end + + it "uses the CurrentSettings value" do + # fixture sanity check + expect(Gitlab::CurrentSettings.default_branch_name).to eq(overridden_setting_value_from_current_settings) + + expect(settings_module.get_single_setting(:default_branch_name)) + .to eq(overridden_setting_value_from_current_settings) + end + end + + context "when there is a ::Gitlab.config override" do + let(:overridden_setting_value_from_gitlab_config) { "value_from_gitlab config" } + + before do + allow(Gitlab).to receive_message_chain(:config, :gitlab_kas, :external_url) do + overridden_setting_value_from_gitlab_config + end + end + + it "uses the ENV var value and not the Gitlab.config value" do + expect(settings_module.get_single_setting(:gitlab_kas_external_url)) + .to eq(overridden_setting_value_from_gitlab_config) + end + end + + context "when there is an ENV var override and also a ::Gitlab::CurrentSettings override" do let(:override_value_from_env) { "value_from_env" } let(:overridden_setting_value_from_current_settings) { "value_from_current_settings" } let(:current_settings_class) do diff --git a/ee/spec/lib/remote_development/workspaces_server_operations/server_config/main_integration_spec.rb b/ee/spec/lib/remote_development/workspaces_server_operations/server_config/main_integration_spec.rb new file mode 100644 index 00000000000000..9dc0c91d4d54b6 --- /dev/null +++ b/ee/spec/lib/remote_development/workspaces_server_operations/server_config/main_integration_spec.rb @@ -0,0 +1,127 @@ +# frozen_string_literal: true + +require "spec_helper" + +# noinspection RubyArgCount -- Rubymine detecting wrong types, it thinks some #create are from Minitest, not FactoryBot +RSpec.describe ::RemoteDevelopment::WorkspacesServerOperations::ServerConfig::Main, feature_category: :workspaces do + include TestRequestHelpers + + include_context "with remote development shared fixtures" + + let_it_be(:attributes_generator_class) do + ::RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationAttributesGenerator + end + + let_it_be(:gitlab_kas_external_scheme) { "ws" } + # noinspection RubyResolve - Rubymine can't resolve gitlab_kas + let_it_be(:gitlab_kas_external_url_without_scheme) { Gitlab.config.gitlab_kas.external_url.sub(%r{^\w+://}, '') } + let_it_be(:gitlab_kas_external_url) do + "#{gitlab_kas_external_scheme}://#{gitlab_kas_external_url_without_scheme}" + end + + let_it_be(:expected_api_external_url) do + uri_scheme = "http" # this is "http" because the original gitlab_kas_external_url scheme is "ws" + "#{uri_scheme}://#{gitlab_kas_external_url_without_scheme}" \ + "/#{attributes_generator_class::API_EXTERNAL_URL_PATH_SEGMENT}" + end + + let_it_be(:expected_oauth_redirect_url) do + "#{expected_api_external_url}/#{attributes_generator_class::OAUTH_REDIRECT_URI_PATH_SEGMENT}" + end + + # noinspection HttpUrlsUsage,RubyResolve + let_it_be(:expected_oauth_application_attributes) do + { + name: attributes_generator_class::OAUTH_NAME, + redirect_uri: expected_oauth_redirect_url, + scopes: "openid", + trusted: + RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationAttributesGenerator::TRUSTED, + confidential: + RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationAttributesGenerator::CONFIDENTIAL + } + end + + let(:settings) { { gitlab_kas_external_url: gitlab_kas_external_url } } + + let(:context) do + { + settings: settings, + request: test_request + } + end + + subject(:response) do + described_class.main(context) + end + + shared_examples "expected server config response" do + it "returns the expected server config response" do + response + + oauth_application = ::Gitlab::CurrentSettings.current_application_settings.workspaces_oauth_application || raise + + expect(response).to eq( + { + status: :success, + payload: { + api_external_url: expected_api_external_url, + oauth_client_id: oauth_application.uid, + oauth_redirect_url: oauth_application.redirect_uri + } + } + ) + end + end + + context "when the oauth application does not yet exist" do + it "creates the oauth application" do + expect { response }.to change { Authn::OauthApplication.count }.by(1) + end + + it_behaves_like("expected server config response") + end + + context "when the oauth application already exists" do + let!(:workspaces_oauth_application) { create(:workspaces_oauth_application) } + + shared_examples "oauth application already exists" do + it "does not create the oauth application" do + expect { response }.not_to change { Authn::OauthApplication.count } + end + + it "has the expected attributes for the oauth application" do + response + + actual_oauth_application_attributes = workspaces_oauth_application.reload.attributes + + expect(actual_oauth_application_attributes).to include expected_oauth_application_attributes.stringify_keys + end + end + + context "when oauth attributes are already all correct" do + it_behaves_like("oauth application already exists") + + it_behaves_like("expected server config response") + end + + context "when oauth attributes have been changed" do + before do + confidential = + RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationAttributesGenerator::CONFIDENTIAL + workspaces_oauth_application.update!( + name: "not the right name", + redirect_uri: "https://not-the-right-uri", + scopes: "api", + trusted: + !RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationAttributesGenerator::TRUSTED, + confidential: confidential + ) + end + + it_behaves_like("oauth application already exists") + + it_behaves_like("expected server config response") + end + end +end diff --git a/ee/spec/lib/remote_development/workspaces_server_operations/server_config/main_spec.rb b/ee/spec/lib/remote_development/workspaces_server_operations/server_config/main_spec.rb new file mode 100644 index 00000000000000..853d506f0d58dc --- /dev/null +++ b/ee/spec/lib/remote_development/workspaces_server_operations/server_config/main_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require "fast_spec_helper" + +RSpec.describe ::RemoteDevelopment::WorkspacesServerOperations::ServerConfig::Main, feature_category: :workspaces do + let(:context_passed_along_steps) { {} } + let(:response_payload) do + { + api_external_url: "external url", + oauth_client_id: "client id", + oauth_redirect_url: "redirect url" + } + end + + let(:rop_steps) do + [ + [::RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationAttributesGenerator, :map], + [::RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationEnsurer, :map], + [::RemoteDevelopment::WorkspacesServerOperations::ServerConfig::ValuesExtractor, :map] + ] + end + + describe "happy path" do + let(:context_passed_along_steps) do + { + ok_details: "Everything is OK!", + response_payload: response_payload + } + end + + let(:expected_response) do + { + status: :success, + payload: response_payload + } + end + + it "returns expected response" do + # noinspection RubyResolve - https://handbook.gitlab.com/handbook/tools-and-tips/editors-and-ides/jetbrains-ides/tracked-jetbrains-issues/#ruby-31542 + expect do + described_class.main(context_passed_along_steps) + end + .to invoke_rop_steps(rop_steps) + .from_main_class(described_class) + .with_context_passed_along_steps(context_passed_along_steps) + .and_return_expected_value(expected_response) + end + end +end diff --git a/ee/spec/lib/remote_development/workspaces_server_operations/server_config/oauth_application_attributes_generator_spec.rb b/ee/spec/lib/remote_development/workspaces_server_operations/server_config/oauth_application_attributes_generator_spec.rb new file mode 100644 index 00000000000000..198b2760a7aee3 --- /dev/null +++ b/ee/spec/lib/remote_development/workspaces_server_operations/server_config/oauth_application_attributes_generator_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require "fast_spec_helper" + +RSpec.describe ::RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationAttributesGenerator, feature_category: :workspaces do + let(:expected_redirect_uri_scheme) { "http" } + + let(:expected_oauth_application_attributes) do + { + name: "GitLab Workspaces", + redirect_uri: "#{expected_redirect_uri_scheme}://host:3000/-/kubernetes-agent/workspaces/oauth/redirect", + scopes: "openid", + trusted: + RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationAttributesGenerator::TRUSTED, + confidential: + RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationAttributesGenerator::CONFIDENTIAL + } + end + + let(:external_url_scheme) { "ws" } + let(:gitlab_kas_external_url) { "#{external_url_scheme}://host:3000/-/kubernetes-agent" } + let(:settings) { { gitlab_kas_external_url: gitlab_kas_external_url } } + + let(:context) { { settings: settings } } + + subject(:oauth_application_attributes) do + described_class.generate(context)[:oauth_application_attributes] + end + + it "returns the expected oauth application attributes" do + expect(oauth_application_attributes).to eq(expected_oauth_application_attributes) + end + + context "when the external URL scheme is grpcs or wss" do + let(:external_url_scheme) { "wss" } + let(:expected_redirect_uri_scheme) { "https" } + + it "uses https as the redirect URI scheme" do + expect(oauth_application_attributes).to eq(expected_oauth_application_attributes) + end + end +end diff --git a/ee/spec/lib/remote_development/workspaces_server_operations/server_config/oauth_application_ensurer_spec.rb b/ee/spec/lib/remote_development/workspaces_server_operations/server_config/oauth_application_ensurer_spec.rb new file mode 100644 index 00000000000000..17aadd21f89b07 --- /dev/null +++ b/ee/spec/lib/remote_development/workspaces_server_operations/server_config/oauth_application_ensurer_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe ::RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationEnsurer, feature_category: :workspaces do + include TestRequestHelpers + + let(:oauth_application_attributes) do + { + name: "App Name", + redirect_uri: "https://example.com/redirect/uri", + scopes: "openid", + trusted: + RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationAttributesGenerator::TRUSTED, + confidential: + RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationAttributesGenerator::CONFIDENTIAL + } + end + + let(:request) { test_request } + let(:context) do + { + oauth_application_attributes: oauth_application_attributes, + request: request + } + end + + subject(:returned_value) { described_class.ensure(context) } + + context "when application does not already exist" do + it "creates the application" do + expect { returned_value }.to change { Doorkeeper::Application.count }.by(1) + + id = returned_value[:workspaces_oauth_application].id + expect(Doorkeeper::Application.find(id).attributes).to include(oauth_application_attributes.stringify_keys) + end + end + + context "when application already exists" do + let!(:existing_application) { create(:workspaces_oauth_application) } + + it "returns the existing application and does not create a new one" do + expect { returned_value }.not_to change { Doorkeeper::Application.count } + + expect(returned_value[:workspaces_oauth_application].id).to eq(existing_application.id) + end + end + + context "when application already exists but has been updated with incorrect values" do + let!(:incorrect_workspaces_oauth_application) do + confidential = + !RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationAttributesGenerator::CONFIDENTIAL + create( + :workspaces_oauth_application, + name: "not the right name", + redirect_uri: "https://not-the-right-uri", + scopes: "api", + trusted: + !RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationAttributesGenerator::TRUSTED, + confidential: + confidential + ) + end + + let(:workspaces_oauth_application_id) { incorrect_workspaces_oauth_application.id } + + it "returns the existing application and does not create a new one" do + expect { returned_value }.not_to change { Doorkeeper::Application.count } + + id = returned_value[:workspaces_oauth_application].id + expect(id).to eq(incorrect_workspaces_oauth_application.id) + expect(Doorkeeper::Application.find(id).attributes).to include(oauth_application_attributes.stringify_keys) + end + end +end diff --git a/ee/spec/lib/remote_development/workspaces_server_operations/server_config/values_extractor_spec.rb b/ee/spec/lib/remote_development/workspaces_server_operations/server_config/values_extractor_spec.rb new file mode 100644 index 00000000000000..656d8244c6ed5c --- /dev/null +++ b/ee/spec/lib/remote_development/workspaces_server_operations/server_config/values_extractor_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require "fast_spec_helper" + +RSpec.describe ::RemoteDevelopment::WorkspacesServerOperations::ServerConfig::ValuesExtractor, feature_category: :workspaces do + let(:workspaces_oauth_application) do + instance_double( + "Doorkeeper::Application", # rubocop:disable RSpec/VerifiedDoubleReference -- We're using the quoted version so we can use fast_spec_helper + uid: "test_client_id", + redirect_uri: "https://host:3000/-/kubernetes-agent/workspaces/oauth/redirect" + ) + end + + let(:context) do + { + api_external_url: "https://host:3000/-/kubernetes-agent/workspaces", + workspaces_oauth_application: workspaces_oauth_application + } + end + + let(:expected_result) do + { + response_payload: { + api_external_url: "https://host:3000/-/kubernetes-agent/workspaces", + oauth_client_id: "test_client_id", + oauth_redirect_url: "https://host:3000/-/kubernetes-agent/workspaces/oauth/redirect" + } + } + end + + subject(:result) { described_class.extract(context) } + + it "returns expected result" do + expect(result).to eq(expected_result) + end +end diff --git a/ee/spec/models/application_setting_spec.rb b/ee/spec/models/application_setting_spec.rb index 444f47f063e84e..768d67094bbcab 100644 --- a/ee/spec/models/application_setting_spec.rb +++ b/ee/spec/models/application_setting_spec.rb @@ -167,6 +167,7 @@ describe 'associations' do it { is_expected.to belong_to(:file_template_project).class_name('Project') } + it { is_expected.to belong_to(:workspaces_oauth_application).class_name('Doorkeeper::Application') } end describe 'validations' do diff --git a/ee/spec/requests/api/remote_development/internal/agent/agentw_spec.rb b/ee/spec/requests/api/remote_development/internal/agent/agentw_spec.rb new file mode 100644 index 00000000000000..31eceaea317301 --- /dev/null +++ b/ee/spec/requests/api/remote_development/internal/agent/agentw_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true + +require "spec_helper" + +RSpec.describe API::RemoteDevelopment::Internal::Agent::Agentw, feature_category: :workspaces do + let(:jwt_auth_headers) do + jwt_token = JWT.encode( + { "iss" => Gitlab::Kas::JWT_ISSUER, "aud" => Gitlab::Kas::JWT_AUDIENCE }, + Gitlab::Kas.secret, + "HS256" + ) + + { Gitlab::Kas::INTERNAL_API_KAS_REQUEST_HEADER => jwt_token } + end + + let(:jwt_secret) { SecureRandom.random_bytes(Gitlab::Kas::SECRET_LENGTH) } + + let(:stub_service_payload) do + { + api_external_url: "external url", + oauth_redirect_url: "redirect url", + some_key: "some value" + } + end + + let(:expected_service_args) do + { + domain_main_class: ::RemoteDevelopment::WorkspacesServerOperations::ServerConfig::Main, + domain_main_class_args: { + request: instance_of(Grape::Request) + } + } + end + + let(:stub_service_response) { ServiceResponse.success(payload: stub_service_payload) } + + before do + allow(Gitlab::Kas).to receive(:secret).and_return(jwt_secret) + end + + # @param [Hash] headers + # @return [Integer] response status code + def send_request(headers: {}) + get api("/internal/agents/agentw/server_config"), headers: headers.reverse_merge(jwt_auth_headers) + end + + shared_examples "authorization" do + context "when not authenticated" do + it "returns 401" do + send_request(headers: { Gitlab::Kas::INTERNAL_API_KAS_REQUEST_HEADER => "" }) + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end + end + + describe "GET /internal/agents/agentw/server_config" do + include_examples "authorization" + + context "with valid auth" do + it "returns expected response structure with correct status" do + expect(RemoteDevelopment::CommonService).to receive(:execute).with(expected_service_args) do + stub_service_response + end + + send_request + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to eq({ + "api_external_url" => "external url", + "oauth_redirect_url" => "redirect url", + "some_key" => "some value" + }) + end + end + end +end diff --git a/ee/spec/requests/remote_development/integration_spec.rb b/ee/spec/requests/remote_development/integration_spec.rb index fcd6837cdb3308..974717c344c4eb 100644 --- a/ee/spec/requests/remote_development/integration_spec.rb +++ b/ee/spec/requests/remote_development/integration_spec.rb @@ -140,7 +140,7 @@ end # @param [String] random_string - # @param [User] user + # @param [User, QA::Resource::User] user # @return [Array] def expected_internal_variables(random_string:, user:) # rubocop:disable Layout/LineLength -- keep them on one line for easier readability and editability @@ -166,6 +166,52 @@ def expected_internal_variables(random_string:, user:) # rubocop:enable Layout/LineLength end + # @return [String] + def kas_jwt_token + JWT.encode( + { "iss" => Gitlab::Kas::JWT_ISSUER, 'aud' => Gitlab::Kas::JWT_AUDIENCE }, + Gitlab::Kas.secret, + "HS256" + ) + end + + # @return [void] + def do_get_internal_agents_agentw_server_config + # Perform an internal agents agentw server_config GET which will cause a workspaces oauth application to be created + + params = {} # No params currently for this endpoint + + agent_token_headers = { + Gitlab::Kas::INTERNAL_API_KAS_REQUEST_HEADER => kas_jwt_token + } + server_config_url = api("/internal/agents/agentw/server_config") + + allow_next_instance_of(Grape::Request) do |request| + allow(request).to receive(:remote_ip).and_return("1.1.1.1") + end + + get server_config_url, params: params, headers: agent_token_headers, as: :json + + expect(response).to have_gitlab_http_status(:ok) + + expect(::Gitlab::CurrentSettings.current_application_settings.workspaces_oauth_application).not_to be_nil + + workspaces_oauth_application = + ::Gitlab::CurrentSettings.current_application_settings.workspaces_oauth_application || raise + + generator_module = RemoteDevelopment::WorkspacesServerOperations::ServerConfig::OauthApplicationAttributesGenerator + expect(json_response.symbolize_keys).to eq( + { + api_external_url: + workspaces_oauth_application.redirect_uri.gsub("/#{generator_module::OAUTH_REDIRECT_URI_PATH_SEGMENT}", ""), + oauth_client_id: workspaces_oauth_application.uid, + oauth_redirect_url: workspaces_oauth_application.redirect_uri + } + ) + + nil + end + # @return [void] def do_create_workspaces_agent_config # Perform an agent update post which will cause a workspaces_agent_config record to be created @@ -176,14 +222,9 @@ def do_create_workspaces_agent_config agent_config: agent_config_file_hash } - jwt_token = JWT.encode( - { "iss" => Gitlab::Kas::JWT_ISSUER, 'aud' => Gitlab::Kas::JWT_AUDIENCE }, - Gitlab::Kas.secret, - "HS256" - ) agent_token_headers = { Gitlab::Kas::INTERNAL_API_AGENT_REQUEST_HEADER => agent_token.token, - Gitlab::Kas::INTERNAL_API_KAS_REQUEST_HEADER => jwt_token + Gitlab::Kas::INTERNAL_API_KAS_REQUEST_HEADER => kas_jwt_token } agent_configuration_url = api("/internal/kubernetes/agent_configuration", personal_access_token: agent_token) @@ -193,7 +234,7 @@ def do_create_workspaces_agent_config # it only returns 204 No Content. expect(response).to have_gitlab_http_status(:no_content) - workspaces_agent_config = RemoteDevelopment::WorkspacesAgentConfig.find_by_cluster_agent_id(agent.id) + workspaces_agent_config = RemoteDevelopment::WorkspacesAgentConfig.find_by_cluster_agent_id!(agent.id) # Get the agent config via the GraphQL API to verify it was created correctly get_graphql(namespace_agents_config_query, current_user: agent_admin_user) @@ -266,7 +307,7 @@ def do_create_org_mapping # rubocop:disable Metrics/AbcSize -- We want this to stay a single method # @param [Array] random_string_array - # @param [User] user + # @param [User, QA::Resource::User] user # @return [RemoteDevelopment::Workspace] def do_create_workspace(random_string_array:, user:) allow(FFaker::Food).to receive(:fruit).and_return(random_string_array[0]) @@ -285,7 +326,7 @@ def do_create_workspace(random_string_array:, user:) ) workspace_gid = create_mutation_response.fetch("workspace").fetch("id") - workspace_id = GitlabSchema.parse_gid(workspace_gid, expected_type: ::RemoteDevelopment::Workspace).model_id + workspace_id = GitlabSchema.parse_gid(workspace_gid, expected_type: ::RemoteDevelopment::Workspace)&.model_id workspace = ::RemoteDevelopment::Workspace.find(workspace_id) # NOTE: Where possible, avoid explicit assertions here and replace them with assertions on the @@ -408,7 +449,7 @@ def do_change_workspace_state(workspace:, desired_state:) # @param [Symbol] name # @param [Hash] input - # @param [User] user + # @param [User, QA::Resource::User] user # @return [Hash] def do_graphql_mutation_post(name:, input:, user:) mutation = graphql_mutation(name, input) @@ -425,14 +466,9 @@ def do_graphql_mutation_post(name:, input:, user:) def do_reconcile_post(params:, agent_token:) # Custom logic to perform the reconcile post for a _REQUEST_ spec - jwt_token = JWT.encode( - { "iss" => Gitlab::Kas::JWT_ISSUER, 'aud' => Gitlab::Kas::JWT_AUDIENCE }, - Gitlab::Kas.secret, - "HS256" - ) agent_token_headers = { Gitlab::Kas::INTERNAL_API_AGENT_REQUEST_HEADER => agent_token.token, - Gitlab::Kas::INTERNAL_API_KAS_REQUEST_HEADER => jwt_token + Gitlab::Kas::INTERNAL_API_KAS_REQUEST_HEADER => kas_jwt_token } reconcile_url = api("/internal/kubernetes/modules/remote_development/reconcile", personal_access_token: agent_token) @@ -452,10 +488,13 @@ def do_reconcile_post(params:, agent_token:) end it "successfully exercises the full lifecycle of a workspace", :unlimited_max_formatted_output_length do # rubocop:disable RSpec/NoExpectationExample -- the expectations are in the called methods - # CREATE THE MAPPING VIA GRAPHQL API, SO WE HAVE PROPER AUTHORIZATION + # CREATE the OAuth app and GET THE agentw server config VIA REST API + do_get_internal_agents_agentw_server_config + + # CREATE THE Namespace Mapping VIA GRAPHQL API do_create_namespace_mapping - # CREATE THE WorkspacesAgentConfig VIA GRAPHQL API + # CREATE THE WorkspacesAgentConfig VIA REST and GRAPHQL APIs do_create_workspaces_agent_config # CREATE WORKSPACE VIA GRAPHQL API -- GitLab From 60ea96f2288a83f6bd745d0980f8f8103739ddbf Mon Sep 17 00:00:00 2001 From: Chad Woolley Date: Tue, 26 Aug 2025 10:19:53 -0700 Subject: [PATCH 2/2] Make Applications::CreateService robust for internal api usage --- db/structure.sql | 38 +++++------ .../ee/applications/create_service.rb | 20 ++++-- .../applications/create_service_spec.rb | 63 ++++++++++++++++--- 3 files changed, 90 insertions(+), 31 deletions(-) diff --git a/db/structure.sql b/db/structure.sql index 005c3bdc217d8d..2211bb475c42bd 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -5026,25 +5026,6 @@ CREATE TABLE merge_request_commits_metadata ( ) PARTITION BY RANGE (project_id); -CREATE TABLE merge_requests_merge_data ( - merge_request_id bigint NOT NULL, - project_id bigint NOT NULL, - merge_user_id bigint, - merge_params text, - merge_error text, - merge_jid text, - merge_commit_sha bytea, - merged_commit_sha bytea, - merge_ref_sha bytea, - squash_commit_sha bytea, - in_progress_merge_commit_sha bytea, - merge_status smallint DEFAULT 0 NOT NULL, - auto_merge_enabled boolean DEFAULT false NOT NULL, - squash boolean DEFAULT false NOT NULL, - CONSTRAINT check_d25e93fc19 CHECK ((char_length(merge_jid) <= 255)) -) -PARTITION BY RANGE (merge_request_id); - CREATE TABLE p_ai_active_context_code_enabled_namespaces ( id bigint NOT NULL, namespace_id bigint NOT NULL, @@ -18976,6 +18957,25 @@ CREATE SEQUENCE merge_requests_id_seq ALTER SEQUENCE merge_requests_id_seq OWNED BY merge_requests.id; +CREATE TABLE merge_requests_merge_data ( + merge_request_id bigint NOT NULL, + project_id bigint NOT NULL, + merge_user_id bigint, + merge_params text, + merge_error text, + merge_jid text, + merge_commit_sha bytea, + merged_commit_sha bytea, + merge_ref_sha bytea, + squash_commit_sha bytea, + in_progress_merge_commit_sha bytea, + merge_status smallint DEFAULT 0 NOT NULL, + auto_merge_enabled boolean DEFAULT false NOT NULL, + squash boolean DEFAULT false NOT NULL, + CONSTRAINT check_d25e93fc19 CHECK ((char_length(merge_jid) <= 255)) +) +PARTITION BY RANGE (merge_request_id); + CREATE TABLE merge_trains ( id bigint NOT NULL, merge_request_id bigint NOT NULL, diff --git a/ee/app/services/ee/applications/create_service.rb b/ee/app/services/ee/applications/create_service.rb index 8fbdcb413d9f54..a35e38484c30ba 100644 --- a/ee/app/services/ee/applications/create_service.rb +++ b/ee/app/services/ee/applications/create_service.rb @@ -24,15 +24,27 @@ def disable_ropc_available? override :execute def execute super.tap do |application| - audit_oauth_application_creation(application, request.remote_ip) + # NOTE: These guard clauses exist to avoid errors when this service is invoked from an internal + # API, such as KAS. In this case, the request may not have a `remote_ip` method, and the + # application.owner and/or current_user may not be present. We also don't want to pass nil for these, + # value because this may break the contract/behavior of the audit logic, which may expect them in + # to be present and fail if they are not. + + next unless request.present? + + next unless request.respond_to?(:remote_ip) + + entity = application.owner || current_user + + next unless entity + + audit_oauth_application_creation(application, request.remote_ip, entity) end end private - def audit_oauth_application_creation(application, ip_address) - entity = application.owner || current_user - + def audit_oauth_application_creation(application, ip_address, entity) ::Gitlab::Audit::Auditor.audit( name: 'oauth_application_created', author: current_user, diff --git a/ee/spec/services/applications/create_service_spec.rb b/ee/spec/services/applications/create_service_spec.rb index 1b6e89c4fa34c1..525b0c99d9ba3b 100644 --- a/ee/spec/services/applications/create_service_spec.rb +++ b/ee/spec/services/applications/create_service_spec.rb @@ -8,16 +8,17 @@ let_it_be(:user) { create(:user) } + let(:request) { test_request } let(:group) { create(:group) } let(:params) { attributes_for(:application, scopes: %w[read_user]) } - subject(:service) { described_class.new(user, test_request, params) } + subject(:service) { described_class.new(user, request, params) } describe '#audit_oauth_application_creation' do where(:case_name, :owner, :entity_type) do - 'instance application' | nil | 'User' - 'group application' | ref(:group) | 'Group' - 'user application' | ref(:user) | 'User' + 'instance application' | nil | 'User' + 'group application' | ref(:group) | 'Group' + 'user application' | ref(:user) | 'User' end with_them do @@ -38,7 +39,7 @@ application_id: anything, scopes: %w[read_user] ), - ip_address: test_request.remote_ip + ip_address: request.remote_ip ) service.execute @@ -67,12 +68,58 @@ end end + context 'when executed from an internal api call' do + before do + stub_licensed_features(extended_audit_events: true) + end + + context 'when request is nil' do + let(:request) { nil } + + it 'creates the app and does not fail' do + expect { service.execute }.to change { Authn::OauthApplication.count }.by(1) + end + + it 'skips audit event' do + expect { service.execute }.not_to change { AuditEvent.count } + end + end + + context 'when request does not respond to #remote_ip' do + before do + allow(request).to receive(:respond_to?).and_call_original + allow(request).to receive(:respond_to?).with(:remote_ip).and_return(false) + end + + it 'creates the app and does not fail' do + expect { service.execute }.to change { Authn::OauthApplication.count }.by(1) + end + + it 'skips audit event' do + expect { service.execute }.not_to change { AuditEvent.count } + end + end + + context 'when application owner and user are both nil' do + let(:user) { nil } + let(:params) { attributes_for(:application, :without_owner, scopes: %w[read_user]) } + + it 'creates the app and does not fail' do + expect { service.execute }.to change { Authn::OauthApplication.count }.by(1) + end + + it 'skips audit event' do + expect { service.execute }.not_to change { AuditEvent.count } + end + end + end + context 'for ROPC' do where(:saas_feature_available, :feature_enabled, :ropc_enabled) do false | false | true - false | true | true - true | false | true - true | true | false + false | true | true + true | false | true + true | true | false end with_them do -- GitLab