From fb664948664d1ba64a8414f769d55d446ee45faf Mon Sep 17 00:00:00 2001 From: James Lopez Date: Fri, 1 Mar 2019 13:37:46 +0100 Subject: [PATCH 01/11] Add model and migration --- ...1095211_create_scim_oauth_access_tokens.rb | 12 +++++++++++ .../groups/scim_oauth_controller.rb | 20 +++++++++++++++++++ ee/app/models/scim_oauth_access_token.rb | 12 +++++++++++ .../models/scim_oauth_access_token_spec.rb | 19 ++++++++++++++++++ 4 files changed, 63 insertions(+) create mode 100644 db/migrate/20190301095211_create_scim_oauth_access_tokens.rb create mode 100644 ee/app/controllers/groups/scim_oauth_controller.rb create mode 100644 ee/app/models/scim_oauth_access_token.rb create mode 100644 ee/spec/models/scim_oauth_access_token_spec.rb diff --git a/db/migrate/20190301095211_create_scim_oauth_access_tokens.rb b/db/migrate/20190301095211_create_scim_oauth_access_tokens.rb new file mode 100644 index 00000000000000..13523b6c901324 --- /dev/null +++ b/db/migrate/20190301095211_create_scim_oauth_access_tokens.rb @@ -0,0 +1,12 @@ +class CreateScimOauthAccessTokens < ActiveRecord::Migration[5.0] + def change + create_table :scim_oauth_access_tokens do |t| + t.timestamps_with_timezone null: false + t.references :group, null: false, index: false + t.string "token", null: false + + t.index [:group_id, :token], unique: true + t.foreign_key :namespaces, column: :group_id, on_delete: :cascade + end + end +end diff --git a/ee/app/controllers/groups/scim_oauth_controller.rb b/ee/app/controllers/groups/scim_oauth_controller.rb new file mode 100644 index 00000000000000..1f46d26306e466 --- /dev/null +++ b/ee/app/controllers/groups/scim_oauth_controller.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class Groups::ScimOauthController < Groups::ApplicationController + before_action :require_top_level_group + before_action :authorize_manage_saml! + before_action :check_group_saml_available! + before_action :check_group_saml_configured + + def show + @saml_provider = @group.saml_provider || @group.build_saml_provider + end + + def create + @saml_provider = @group.build_saml_provider(saml_provider_params) + + @saml_provider.save + + render :show + end +end diff --git a/ee/app/models/scim_oauth_access_token.rb b/ee/app/models/scim_oauth_access_token.rb new file mode 100644 index 00000000000000..078eea706f1dc2 --- /dev/null +++ b/ee/app/models/scim_oauth_access_token.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class ScimOauthAccessToken < ActiveRecord::Base + include TokenAuthenticatable + + belongs_to :group + + add_authentication_token_field :token + + validates :group, presence: true + before_save :ensure_token +end diff --git a/ee/spec/models/scim_oauth_access_token_spec.rb b/ee/spec/models/scim_oauth_access_token_spec.rb new file mode 100644 index 00000000000000..1d131b0c856580 --- /dev/null +++ b/ee/spec/models/scim_oauth_access_token_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' + +describe ScimOauthAccessToken do + describe "Associations" do + it { is_expected.to belong_to :group } + end + + describe 'Validations' do + it { is_expected.to validate_presence_of(:group) } + end + + describe '#token' do + it 'generates a token on creation' do + scim_token = described_class.create(group: create(:group)) + + expect(scim_token.token).to be_a(String) + end + end +end -- GitLab From e9b2253fe3538234d1c4d173c4549a955233d836 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Fri, 1 Mar 2019 13:55:28 +0100 Subject: [PATCH 02/11] Update schema file --- db/schema.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/db/schema.rb b/db/schema.rb index 8fe0ee7fcc397f..cecf742c33a03f 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2773,6 +2773,14 @@ t.index ["group_id"], name: "index_saml_providers_on_group_id", using: :btree end + create_table "scim_oauth_access_tokens", force: :cascade do |t| + t.datetime_with_timezone "created_at", null: false + t.datetime_with_timezone "updated_at", null: false + t.integer "group_id", null: false + t.string "token", null: false + t.index ["group_id", "token"], name: "index_scim_oauth_access_tokens_on_group_id_and_token", unique: true, using: :btree + end + create_table "sent_notifications", force: :cascade do |t| t.integer "project_id" t.integer "noteable_id" -- GitLab From 1b350d994cee3a0f905a0b22615da5020c0b9e22 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Fri, 1 Mar 2019 15:51:54 +0100 Subject: [PATCH 03/11] Update controller and group --- .../groups/scim_oauth_controller.rb | 36 ++++++++++++++----- ee/app/models/ee/group.rb | 1 + ee/config/routes/group.rb | 2 ++ 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/ee/app/controllers/groups/scim_oauth_controller.rb b/ee/app/controllers/groups/scim_oauth_controller.rb index 1f46d26306e466..f82c84e609cd9c 100644 --- a/ee/app/controllers/groups/scim_oauth_controller.rb +++ b/ee/app/controllers/groups/scim_oauth_controller.rb @@ -1,20 +1,38 @@ # frozen_string_literal: true class Groups::ScimOauthController < Groups::ApplicationController - before_action :require_top_level_group - before_action :authorize_manage_saml! - before_action :check_group_saml_available! - before_action :check_group_saml_configured + # before_action :require_top_level_group + # before_action :authorize_manage_saml! + # before_action :check_group_saml_available! + # before_action :check_group_saml_configured + skip_before_filter :verify_authenticity_token + def show - @saml_provider = @group.saml_provider || @group.build_saml_provider + scim_token = ScimOauthAccessToken.find_by_group_id(@group.id) + + respond_to do |format| + format.json do + if scim_token + render json: ScimOauthAccessTokenEntity.new(scim_token).as_json + else + render json: {} + end + end + end end def create - @saml_provider = @group.build_saml_provider(saml_provider_params) - - @saml_provider.save + scim_token = ScimOauthAccessToken.safe_find_or_create_by(group: @group) - render :show + respond_to do |format| + format.json do + if scim_token&.valid? + render json: ScimOauthAccessTokenEntity.new(scim_token).as_json + else + render json: { errors: scim_token&.errors&.full_messages }, status: :unprocessable_entity + end + end + end end end diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index aa6d8602b021b3..8f7cd756e238dd 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -20,6 +20,7 @@ module Group has_one :saml_provider has_one :insight, foreign_key: :namespace_id accepts_nested_attributes_for :insight + has_one :scim_ouath_access_token, dependent: :destroy has_many :ldap_group_links, foreign_key: 'group_id', dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :hooks, dependent: :destroy, class_name: 'GroupHook' # rubocop:disable Cop/ActiveRecordDependent diff --git a/ee/config/routes/group.rb b/ee/config/routes/group.rb index fc9fcf53b3f941..ed548b05848ca7 100644 --- a/ee/config/routes/group.rb +++ b/ee/config/routes/group.rb @@ -89,6 +89,8 @@ delete :unlink, to: 'sso#unlink' end + resource :scim_oauth, only: [:show, :create], controller: :scim_oauth + resource :roadmap, only: [:show], controller: 'roadmap' legacy_ee_group_boards_redirect = redirect do |params, request| -- GitLab From f40ce03444f71a5bbbdcecad5741531d6d12d596 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Tue, 5 Mar 2019 12:05:58 +0100 Subject: [PATCH 04/11] Add SCIM specs --- .../groups/saml_providers_controller_spec.rb | 17 +++ .../groups/scim_oauth_controller_spec.rb | 110 ++++++++++++++++++ .../scim_oauth_access_token_entity_spec.rb | 17 +++ 3 files changed, 144 insertions(+) create mode 100644 ee/spec/controllers/groups/scim_oauth_controller_spec.rb create mode 100644 ee/spec/serializers/scim_oauth_access_token_entity_spec.rb diff --git a/ee/spec/controllers/groups/saml_providers_controller_spec.rb b/ee/spec/controllers/groups/saml_providers_controller_spec.rb index fafea97ae0bf77..c389b86ef27c1f 100644 --- a/ee/spec/controllers/groups/saml_providers_controller_spec.rb +++ b/ee/spec/controllers/groups/saml_providers_controller_spec.rb @@ -79,6 +79,23 @@ def stub_saml_config(enabled:) expect(response).to render_template 'groups/saml_providers/show' end + it 'has the SCIM token URL' do + group.add_owner(user) + + subject + + expect(assigns(:scim_token_url)).to eq('http://test.host/groups/group1/-/scim_oauth') + end + + it 'sets scim_token_exists if SCIM token is found' do + create(:scim_oauth_access_token, group: group) + group.add_owner(user) + + subject + + expect(assigns(:scim_token_exists)).to be true + end + context 'not on a top level group', :nested_groups do let(:group) { create(:group, :nested) } diff --git a/ee/spec/controllers/groups/scim_oauth_controller_spec.rb b/ee/spec/controllers/groups/scim_oauth_controller_spec.rb new file mode 100644 index 00000000000000..0ccdf7e6d5356d --- /dev/null +++ b/ee/spec/controllers/groups/scim_oauth_controller_spec.rb @@ -0,0 +1,110 @@ +require 'spec_helper' + +describe Groups::ScimOauthController do + let(:saml_provider) { create(:saml_provider, group: group) } + let(:group) { create(:group, :private, parent_id: nil) } + let(:user) { create(:user) } + + before do + sign_in(user) + end + + def stub_saml_config(enabled:) + providers = enabled ? %i(group_saml) : [] + allow(Devise).to receive(:omniauth_providers).and_return(providers) + end + + context 'when the feature is enabled' do + before do + stub_saml_config(enabled: true) + stub_licensed_features(group_saml: true) + stub_feature_flags(group_scim: true) + end + + describe 'GET #show' do + subject { get :show, params: { group_id: group }, format: :json } + + before do + group.add_owner(user) + end + + context 'without token' do + it 'shows an empty response' do + subject + + expect(json_response).to eq({}) + end + end + + context 'with token' do + let!(:scim_token) { create(:scim_oauth_access_token, group: group) } + + before do + subject + end + + it 'shows the token' do + expect(json_response['scim_token']).to eq(scim_token.token) + end + + it 'shows the url' do + expect(json_response['scim_api_url']).not_to be_empty + end + end + end + + describe 'POST #create' do + subject { post :create, params: { group_id: group }, format: :json } + + before do + group.add_owner(user) + end + + context 'without token' do + it 'creates a new SCIM token record' do + expect { subject }.to change { ScimOauthAccessToken.count }.by(1) + end + + context 'json' do + before do + subject + end + + it 'shows the token' do + expect(json_response['scim_token']).not_to be_empty + end + + it 'shows the url' do + expect(json_response['scim_api_url']).not_to be_empty + end + end + end + + context 'with token' do + let!(:scim_token) { create(:scim_oauth_access_token, group: group) } + + it 'does not create a new SCIM token record' do + expect { subject }.not_to change { ScimOauthAccessToken.count } + end + + it 'updates the token' do + expect { subject }.to change { scim_token.reload.token } + end + + context 'json' do + before do + subject + end + + it 'shows the token' do + expect(json_response['scim_token']).to eq(scim_token.reload.token) + end + + it 'shows the url' do + expect(json_response['scim_api_url']).not_to be_empty + end + end + end + end + end +end diff --git a/ee/spec/serializers/scim_oauth_access_token_entity_spec.rb b/ee/spec/serializers/scim_oauth_access_token_entity_spec.rb new file mode 100644 index 00000000000000..848fb75947eab5 --- /dev/null +++ b/ee/spec/serializers/scim_oauth_access_token_entity_spec.rb @@ -0,0 +1,17 @@ +require 'spec_helper' + +describe ScimOauthAccessTokenEntity do + let(:entity) do + described_class.new(create(:scim_oauth_access_token)) + end + + subject { entity.as_json } + + it "exposes the URL" do + is_expected.to include(:scim_api_url) + end + + it "exposes the token" do + is_expected.to include(:scim_token) + end +end -- GitLab From 30dde143de7924dd295bbd33d275bf3a5769ce59 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 14 Mar 2019 10:37:01 +0100 Subject: [PATCH 05/11] Add saml auth logic --- .../concerns/saml_authorization.rb | 18 +++++++++++ .../groups/saml_providers_controller.rb | 19 +++++------- .../groups/scim_oauth_controller.rb | 31 ++++++++++++++----- ee/app/models/ee/group.rb | 2 +- ee/app/models/scim_oauth_access_token.rb | 2 +- .../scim_oauth_access_token_entity.rb | 13 ++++++++ ee/spec/factories/scim_oauth_access_tokens.rb | 7 +++++ 7 files changed, 70 insertions(+), 22 deletions(-) create mode 100644 ee/app/controllers/concerns/saml_authorization.rb create mode 100644 ee/app/serializers/scim_oauth_access_token_entity.rb create mode 100644 ee/spec/factories/scim_oauth_access_tokens.rb diff --git a/ee/app/controllers/concerns/saml_authorization.rb b/ee/app/controllers/concerns/saml_authorization.rb new file mode 100644 index 00000000000000..747f4587f1bf02 --- /dev/null +++ b/ee/app/controllers/concerns/saml_authorization.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true +module SamlAuthorization + extend ActiveSupport::Concern + + private + + def authorize_manage_saml! + render_404 unless can?(current_user, :admin_group_saml, group) + end + + def check_group_saml_configured + render_404 unless Gitlab::Auth::GroupSaml::Config.enabled? + end + + def require_top_level_group + render_404 if group.subgroup? + end +end diff --git a/ee/app/controllers/groups/saml_providers_controller.rb b/ee/app/controllers/groups/saml_providers_controller.rb index 5e26b2df5f39cb..0b251a45487059 100644 --- a/ee/app/controllers/groups/saml_providers_controller.rb +++ b/ee/app/controllers/groups/saml_providers_controller.rb @@ -1,14 +1,21 @@ # frozen_string_literal: true +require_relative '../concerns/saml_authorization.rb' # frozen_string_literal: true class Groups::SamlProvidersController < Groups::ApplicationController + include SamlAuthorization before_action :require_top_level_group before_action :authorize_manage_saml! before_action :check_group_saml_available! before_action :check_group_saml_configured + # rubocop: disable CodeReuse/ActiveRecord def show @saml_provider = @group.saml_provider || @group.build_saml_provider + + @scim_token_exists = ScimOauthAccessToken.exists?(group: @group) + @scim_token_url = group_scim_oauth_url(@group) end + # rubocop: enable CodeReuse/ActiveRecord def create @saml_provider = @group.build_saml_provider(saml_provider_params) @@ -28,18 +35,6 @@ def update private - def authorize_manage_saml! - render_404 unless can?(current_user, :admin_group_saml, @group) - end - - def check_group_saml_configured - render_404 unless Gitlab::Auth::GroupSaml::Config.enabled? - end - - def require_top_level_group - render_404 if @group.subgroup? - end - def saml_provider_params allowed_params = %i[sso_url certificate_fingerprint enabled] diff --git a/ee/app/controllers/groups/scim_oauth_controller.rb b/ee/app/controllers/groups/scim_oauth_controller.rb index f82c84e609cd9c..faa3f41ac205d4 100644 --- a/ee/app/controllers/groups/scim_oauth_controller.rb +++ b/ee/app/controllers/groups/scim_oauth_controller.rb @@ -1,12 +1,13 @@ # frozen_string_literal: true class Groups::ScimOauthController < Groups::ApplicationController - # before_action :require_top_level_group - # before_action :authorize_manage_saml! - # before_action :check_group_saml_available! - # before_action :check_group_saml_configured - skip_before_filter :verify_authenticity_token + include SamlAuthorization + before_action :require_top_level_group + before_action :authorize_manage_saml! + before_action :check_group_saml_available! + before_action :check_group_saml_configured + before_action :check_group_scim_enabled def show scim_token = ScimOauthAccessToken.find_by_group_id(@group.id) @@ -22,17 +23,31 @@ def show end end + # rubocop: disable CodeReuse/ActiveRecord def create - scim_token = ScimOauthAccessToken.safe_find_or_create_by(group: @group) + scim_token = ScimOauthAccessToken.find_or_initialize_by(group: @group) + + if scim_token.new_record? + scim_token.save + else + scim_token.reset_token! + end respond_to do |format| format.json do - if scim_token&.valid? + if scim_token.valid? render json: ScimOauthAccessTokenEntity.new(scim_token).as_json else - render json: { errors: scim_token&.errors&.full_messages }, status: :unprocessable_entity + render json: { errors: scim_token.errors.full_messages }, status: :unprocessable_entity end end end end + # rubocop: enable CodeReuse/ActiveRecord + + private + + def check_group_scim_enabled + route_not_found unless Feature.enabled?(:group_scim, @group) + end end diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 8f7cd756e238dd..9679caa577a478 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -20,7 +20,7 @@ module Group has_one :saml_provider has_one :insight, foreign_key: :namespace_id accepts_nested_attributes_for :insight - has_one :scim_ouath_access_token, dependent: :destroy + has_one :scim_oauth_access_token, dependent: :destroy has_many :ldap_group_links, foreign_key: 'group_id', dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :hooks, dependent: :destroy, class_name: 'GroupHook' # rubocop:disable Cop/ActiveRecordDependent diff --git a/ee/app/models/scim_oauth_access_token.rb b/ee/app/models/scim_oauth_access_token.rb index 078eea706f1dc2..84df7b87c9fcb3 100644 --- a/ee/app/models/scim_oauth_access_token.rb +++ b/ee/app/models/scim_oauth_access_token.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class ScimOauthAccessToken < ActiveRecord::Base +class ScimOauthAccessToken < ApplicationRecord include TokenAuthenticatable belongs_to :group diff --git a/ee/app/serializers/scim_oauth_access_token_entity.rb b/ee/app/serializers/scim_oauth_access_token_entity.rb new file mode 100644 index 00000000000000..0d678ec598a27f --- /dev/null +++ b/ee/app/serializers/scim_oauth_access_token_entity.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class ScimOauthAccessTokenEntity < Grape::Entity + include ::API::Helpers::RelatedResourcesHelpers + + SCIM_PATH = '/api/scim/v2/groups' + + expose :scim_api_url do |scim| + expose_url("#{SCIM_PATH}/#{scim.group.full_path}") + end + + expose :token, as: :scim_token +end diff --git a/ee/spec/factories/scim_oauth_access_tokens.rb b/ee/spec/factories/scim_oauth_access_tokens.rb new file mode 100644 index 00000000000000..f8fd034ac5e43f --- /dev/null +++ b/ee/spec/factories/scim_oauth_access_tokens.rb @@ -0,0 +1,7 @@ +# Read about factories at https://github.com/thoughtbot/factory_bot + +FactoryBot.define do + factory :scim_oauth_access_token do + group + end +end -- GitLab From adbb823370138370e9666634834a56614dc53f43 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 14 Mar 2019 10:38:59 +0100 Subject: [PATCH 06/11] Fix spec failure --- ee/app/models/ee/group.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/group.rb b/ee/app/models/ee/group.rb index 9679caa577a478..00d5005457e5e9 100644 --- a/ee/app/models/ee/group.rb +++ b/ee/app/models/ee/group.rb @@ -20,7 +20,7 @@ module Group has_one :saml_provider has_one :insight, foreign_key: :namespace_id accepts_nested_attributes_for :insight - has_one :scim_oauth_access_token, dependent: :destroy + has_one :scim_oauth_access_token has_many :ldap_group_links, foreign_key: 'group_id', dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :hooks, dependent: :destroy, class_name: 'GroupHook' # rubocop:disable Cop/ActiveRecordDependent -- GitLab From e58a5e24836e89fd5e37121265959402a39fc465 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 14 Mar 2019 16:29:36 +0100 Subject: [PATCH 07/11] Refactor URL display logic --- ee/app/controllers/groups/saml_providers_controller.rb | 5 +++-- ee/app/controllers/groups/scim_oauth_controller.rb | 4 ++-- ee/app/models/scim_oauth_access_token.rb | 4 ++++ .../controllers/groups/saml_providers_controller_spec.rb | 8 ++++---- 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/ee/app/controllers/groups/saml_providers_controller.rb b/ee/app/controllers/groups/saml_providers_controller.rb index 0b251a45487059..a7dd49cd73b347 100644 --- a/ee/app/controllers/groups/saml_providers_controller.rb +++ b/ee/app/controllers/groups/saml_providers_controller.rb @@ -12,8 +12,9 @@ class Groups::SamlProvidersController < Groups::ApplicationController def show @saml_provider = @group.saml_provider || @group.build_saml_provider - @scim_token_exists = ScimOauthAccessToken.exists?(group: @group) - @scim_token_url = group_scim_oauth_url(@group) + scim_token = ScimOauthAccessToken.find_by_group_id(@group.id) + + @scim_token_url = scim_token.as_entity_json[:scim_api_url] if scim_token end # rubocop: enable CodeReuse/ActiveRecord diff --git a/ee/app/controllers/groups/scim_oauth_controller.rb b/ee/app/controllers/groups/scim_oauth_controller.rb index faa3f41ac205d4..5f3ad5db2d84b7 100644 --- a/ee/app/controllers/groups/scim_oauth_controller.rb +++ b/ee/app/controllers/groups/scim_oauth_controller.rb @@ -15,7 +15,7 @@ def show respond_to do |format| format.json do if scim_token - render json: ScimOauthAccessTokenEntity.new(scim_token).as_json + render json: scim_token.as_entity_json else render json: {} end @@ -36,7 +36,7 @@ def create respond_to do |format| format.json do if scim_token.valid? - render json: ScimOauthAccessTokenEntity.new(scim_token).as_json + render json: scim_token.as_entity_json else render json: { errors: scim_token.errors.full_messages }, status: :unprocessable_entity end diff --git a/ee/app/models/scim_oauth_access_token.rb b/ee/app/models/scim_oauth_access_token.rb index 84df7b87c9fcb3..5ae762b3a7c1e3 100644 --- a/ee/app/models/scim_oauth_access_token.rb +++ b/ee/app/models/scim_oauth_access_token.rb @@ -9,4 +9,8 @@ class ScimOauthAccessToken < ApplicationRecord validates :group, presence: true before_save :ensure_token + + def as_entity_json + ScimOauthAccessTokenEntity.new(self).as_json + end end diff --git a/ee/spec/controllers/groups/saml_providers_controller_spec.rb b/ee/spec/controllers/groups/saml_providers_controller_spec.rb index c389b86ef27c1f..eec2919c690798 100644 --- a/ee/spec/controllers/groups/saml_providers_controller_spec.rb +++ b/ee/spec/controllers/groups/saml_providers_controller_spec.rb @@ -79,21 +79,21 @@ def stub_saml_config(enabled:) expect(response).to render_template 'groups/saml_providers/show' end - it 'has the SCIM token URL' do + it 'has no SCIM token URL' do group.add_owner(user) subject - expect(assigns(:scim_token_url)).to eq('http://test.host/groups/group1/-/scim_oauth') + expect(assigns(:scim_token_url)).to be_nil end - it 'sets scim_token_exists if SCIM token is found' do + it 'has the SCIM token URL when it exists' do create(:scim_oauth_access_token, group: group) group.add_owner(user) subject - expect(assigns(:scim_token_exists)).to be true + expect(assigns(:scim_token_url)).to eq("http://localhost/api/scim/v2/groups/#{group.full_path}") end context 'not on a top level group', :nested_groups do -- GitLab From 5fca26fa3ee0c6aa25d9baa3f68791099e3dce29 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Mon, 18 Mar 2019 09:34:55 +0100 Subject: [PATCH 08/11] Update code based on feedback --- ee/app/controllers/groups/saml_providers_controller.rb | 2 +- .../20190301095211_create_scim_oauth_access_tokens.rb | 2 ++ ee/spec/controllers/groups/scim_oauth_controller_spec.rb | 6 +++--- 3 files changed, 6 insertions(+), 4 deletions(-) rename {db => ee/db}/migrate/20190301095211_create_scim_oauth_access_tokens.rb (95%) diff --git a/ee/app/controllers/groups/saml_providers_controller.rb b/ee/app/controllers/groups/saml_providers_controller.rb index a7dd49cd73b347..103f5e340c6dca 100644 --- a/ee/app/controllers/groups/saml_providers_controller.rb +++ b/ee/app/controllers/groups/saml_providers_controller.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true -require_relative '../concerns/saml_authorization.rb' # frozen_string_literal: true +require_relative '../concerns/saml_authorization.rb' class Groups::SamlProvidersController < Groups::ApplicationController include SamlAuthorization diff --git a/db/migrate/20190301095211_create_scim_oauth_access_tokens.rb b/ee/db/migrate/20190301095211_create_scim_oauth_access_tokens.rb similarity index 95% rename from db/migrate/20190301095211_create_scim_oauth_access_tokens.rb rename to ee/db/migrate/20190301095211_create_scim_oauth_access_tokens.rb index 13523b6c901324..c5b00c27101275 100644 --- a/db/migrate/20190301095211_create_scim_oauth_access_tokens.rb +++ b/ee/db/migrate/20190301095211_create_scim_oauth_access_tokens.rb @@ -1,4 +1,6 @@ class CreateScimOauthAccessTokens < ActiveRecord::Migration[5.0] + DOWNTIME = false + def change create_table :scim_oauth_access_tokens do |t| t.timestamps_with_timezone null: false diff --git a/ee/spec/controllers/groups/scim_oauth_controller_spec.rb b/ee/spec/controllers/groups/scim_oauth_controller_spec.rb index 0ccdf7e6d5356d..c6bc757a32835f 100644 --- a/ee/spec/controllers/groups/scim_oauth_controller_spec.rb +++ b/ee/spec/controllers/groups/scim_oauth_controller_spec.rb @@ -39,15 +39,15 @@ def stub_saml_config(enabled:) context 'with token' do let!(:scim_token) { create(:scim_oauth_access_token, group: group) } - before do + it 'shows the token' do subject - end - it 'shows the token' do expect(json_response['scim_token']).to eq(scim_token.token) end it 'shows the url' do + subject + expect(json_response['scim_api_url']).not_to be_empty end end -- GitLab From 56813c7f9490d49dad0777fcb36ce9326475e0d3 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Mon, 18 Mar 2019 11:39:55 +0100 Subject: [PATCH 09/11] Update migration to use encryption --- db/schema.rb | 4 ++-- .../20190301095211_create_scim_oauth_access_tokens.rb | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index cecf742c33a03f..41ac959dbb7dea 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2777,8 +2777,8 @@ t.datetime_with_timezone "created_at", null: false t.datetime_with_timezone "updated_at", null: false t.integer "group_id", null: false - t.string "token", null: false - t.index ["group_id", "token"], name: "index_scim_oauth_access_tokens_on_group_id_and_token", unique: true, using: :btree + t.string "token_encrypted", null: false + t.index ["group_id", "token_encrypted"], name: "index_scim_oauth_access_tokens_on_group_id_and_token_encrypted", unique: true, using: :btree end create_table "sent_notifications", force: :cascade do |t| diff --git a/ee/db/migrate/20190301095211_create_scim_oauth_access_tokens.rb b/ee/db/migrate/20190301095211_create_scim_oauth_access_tokens.rb index c5b00c27101275..17b9a4d38a4d92 100644 --- a/ee/db/migrate/20190301095211_create_scim_oauth_access_tokens.rb +++ b/ee/db/migrate/20190301095211_create_scim_oauth_access_tokens.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class CreateScimOauthAccessTokens < ActiveRecord::Migration[5.0] DOWNTIME = false @@ -5,9 +7,9 @@ def change create_table :scim_oauth_access_tokens do |t| t.timestamps_with_timezone null: false t.references :group, null: false, index: false - t.string "token", null: false + t.string :token_encrypted, null: false - t.index [:group_id, :token], unique: true + t.index [:group_id, :token_encrypted], unique: true t.foreign_key :namespaces, column: :group_id, on_delete: :cascade end end -- GitLab From a044bccbe7a912be5df27520914bd8e0a6602b07 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Mon, 18 Mar 2019 11:50:39 +0100 Subject: [PATCH 10/11] Refactor code and specs --- ee/app/controllers/groups/saml_providers_controller.rb | 2 -- ee/app/models/scim_oauth_access_token.rb | 2 +- ee/spec/controllers/groups/scim_oauth_controller_spec.rb | 2 ++ ee/spec/factories/scim_oauth_access_tokens.rb | 2 +- ee/spec/models/scim_oauth_access_token_spec.rb | 2 ++ ee/spec/serializers/scim_oauth_access_token_entity_spec.rb | 2 ++ 6 files changed, 8 insertions(+), 4 deletions(-) diff --git a/ee/app/controllers/groups/saml_providers_controller.rb b/ee/app/controllers/groups/saml_providers_controller.rb index 103f5e340c6dca..7e5cc1aac7a130 100644 --- a/ee/app/controllers/groups/saml_providers_controller.rb +++ b/ee/app/controllers/groups/saml_providers_controller.rb @@ -8,7 +8,6 @@ class Groups::SamlProvidersController < Groups::ApplicationController before_action :check_group_saml_available! before_action :check_group_saml_configured - # rubocop: disable CodeReuse/ActiveRecord def show @saml_provider = @group.saml_provider || @group.build_saml_provider @@ -16,7 +15,6 @@ def show @scim_token_url = scim_token.as_entity_json[:scim_api_url] if scim_token end - # rubocop: enable CodeReuse/ActiveRecord def create @saml_provider = @group.build_saml_provider(saml_provider_params) diff --git a/ee/app/models/scim_oauth_access_token.rb b/ee/app/models/scim_oauth_access_token.rb index 5ae762b3a7c1e3..aaf5833d56a8ca 100644 --- a/ee/app/models/scim_oauth_access_token.rb +++ b/ee/app/models/scim_oauth_access_token.rb @@ -5,7 +5,7 @@ class ScimOauthAccessToken < ApplicationRecord belongs_to :group - add_authentication_token_field :token + add_authentication_token_field :token, encrypted: :required validates :group, presence: true before_save :ensure_token diff --git a/ee/spec/controllers/groups/scim_oauth_controller_spec.rb b/ee/spec/controllers/groups/scim_oauth_controller_spec.rb index c6bc757a32835f..0f17e74702ad5c 100644 --- a/ee/spec/controllers/groups/scim_oauth_controller_spec.rb +++ b/ee/spec/controllers/groups/scim_oauth_controller_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe Groups::ScimOauthController do diff --git a/ee/spec/factories/scim_oauth_access_tokens.rb b/ee/spec/factories/scim_oauth_access_tokens.rb index f8fd034ac5e43f..0863415a1f092d 100644 --- a/ee/spec/factories/scim_oauth_access_tokens.rb +++ b/ee/spec/factories/scim_oauth_access_tokens.rb @@ -1,4 +1,4 @@ -# Read about factories at https://github.com/thoughtbot/factory_bot +# frozen_string_literal: true FactoryBot.define do factory :scim_oauth_access_token do diff --git a/ee/spec/models/scim_oauth_access_token_spec.rb b/ee/spec/models/scim_oauth_access_token_spec.rb index 1d131b0c856580..abbf8a06683379 100644 --- a/ee/spec/models/scim_oauth_access_token_spec.rb +++ b/ee/spec/models/scim_oauth_access_token_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe ScimOauthAccessToken do diff --git a/ee/spec/serializers/scim_oauth_access_token_entity_spec.rb b/ee/spec/serializers/scim_oauth_access_token_entity_spec.rb index 848fb75947eab5..b6515280a7564b 100644 --- a/ee/spec/serializers/scim_oauth_access_token_entity_spec.rb +++ b/ee/spec/serializers/scim_oauth_access_token_entity_spec.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'spec_helper' describe ScimOauthAccessTokenEntity do -- GitLab From 25deeaa366892097ab0e2a627eaa6d8e0630519f Mon Sep 17 00:00:00 2001 From: James Lopez Date: Mon, 18 Mar 2019 14:08:25 +0100 Subject: [PATCH 11/11] Fix schema file --- db/schema.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/db/schema.rb b/db/schema.rb index 41ac959dbb7dea..fc02cad3e53cb1 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -3621,6 +3621,7 @@ add_foreign_key "reviews", "projects", on_delete: :cascade add_foreign_key "reviews", "users", column: "author_id", on_delete: :nullify add_foreign_key "saml_providers", "namespaces", column: "group_id", on_delete: :cascade + add_foreign_key "scim_oauth_access_tokens", "namespaces", column: "group_id", on_delete: :cascade add_foreign_key "services", "projects", name: "fk_71cce407f9", on_delete: :cascade add_foreign_key "slack_integrations", "services", on_delete: :cascade add_foreign_key "smartcard_identities", "users", on_delete: :cascade -- GitLab