From 4c0b7a1f0ccaa74cd57e27140a327d91f55e4d18 Mon Sep 17 00:00:00 2001 From: Dante Soares Date: Mon, 20 Feb 2023 20:58:55 -0600 Subject: [PATCH] Added new API to find user by external_id that can also return an SSO token --- app/access_policies/user_access_policy.rb | 2 +- app/controllers/api/v1/users_controller.rb | 65 +++++++++++--- .../api/v1/find_user_representer.rb | 46 ++++++++++ config/routes.rb | 5 +- .../user_access_policy_spec.rb | 30 +++++-- .../api/v1/users_controller_spec.rb | 48 ++++++++++ .../api/v1/find_user_representer_spec.rb | 89 +++++++++++++++++++ 7 files changed, 263 insertions(+), 22 deletions(-) create mode 100644 app/representers/api/v1/find_user_representer.rb create mode 100644 spec/representers/api/v1/find_user_representer_spec.rb diff --git a/app/access_policies/user_access_policy.rb b/app/access_policies/user_access_policy.rb index 2681a3814a..01fbfa4431 100644 --- a/app/access_policies/user_access_policy.rb +++ b/app/access_policies/user_access_policy.rb @@ -14,7 +14,7 @@ def self.action_allowed?(action, requestor, user) requestor.is_human? && (requestor.is_anonymous? || !requestor.activated?) && requestor == user # Temp or unclaimed users only - when :create + when :find, :create # find-or-create accounts that are a stand-in for a person who's not yet signed up # only selected applications can access this via client credentials Rails.env.development? || ( diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb index 25846912fb..d68e4c197a 100644 --- a/app/controllers/api/v1/users_controller.rb +++ b/app/controllers/api/v1/users_controller.rb @@ -146,6 +146,39 @@ def update standard_update(User.find(current_human_user.id)) end + ############################################################### + # find + ############################################################### + + api :POST, '/user/find', 'Finds a user account.' + description <<-EOS + Finds a user. + + An external_id must be supplied. + + If the external_id is already in use, that existing user will be returned. + If the sso parameter is also set, an SSO token for the user will also be returned. + + Otherwise, a 404 error is returned. + + #{json_schema(Api::V1::FindUserRepresenter, include: [:readable, :writable])} + EOS + def find + OSU::AccessPolicy.require_action_allowed!(:find, current_api_user, User) + # OpenStax::Api#standard_(update|create) require an ActiveRecord model, which we don't have + # Substitue a Hashie::Mash to read the JSON encoded body + payload = consume!(Hashie::Mash.new, represent_with: Api::V1::FindUserRepresenter) + + user = User.joins(:external_ids).find_by!(external_ids: { external_id: payload.external_id }) + + token = get_sso_token(current_api_user.application, user) if payload.sso.present? + + respond_with user, represent_with: Api::V1::FindUserRepresenter, + status: :ok, + location: nil, + user_options: { sso: token } + end + ############################################################### # find_or_create ############################################################### @@ -156,13 +189,13 @@ def update state will be "unclaimed" meaning it is a place-holder account for an user who has not yet completed the sign up process. - An email address or username must be supplied. + An email address, username or external_id must be supplied. - If the username or email is already in use, that existing user's ID + If the username, email or external_id is already in use, that existing user's ID will be returned. - If an account is created with only an email and no username, it cannot be logged - into directly. It will merged with the user's account when they complete the + If an account is created with no username, it cannot be logged + into directly. It will merged with the user's account when they complete the standard sign up process using a matching email address. If an account is created with a username and password, it may be signed into and used @@ -175,20 +208,13 @@ def find_or_create # OpenStax::Api#standard_(update|create) require an ActiveRecord model, which we don't have # Substitue a Hashie::Mash to read the JSON encoded body payload = consume!(Hashie::Mash.new, represent_with: Api::V1::FindOrCreateUserRepresenter) - payload.application = current_api_user.application + result = FindOrCreateUser.call(payload.except(:sso)) if result.errors.any? render json: { errors: result.errors }, status: :conflict else - # Note: this method changes to keyword arguments in a future Doorkeeper version - token = Doorkeeper::AccessToken.find_or_create_for( - payload.application, - result.outputs.user.id, - '', - 1.hour, - false, - ).token if payload.sso.present? + token = get_sso_token(payload.application, result.outputs.user) if payload.sso.present? respond_with result.outputs.user, represent_with: Api::V1::FindOrCreateUserRepresenter, location: nil, @@ -200,4 +226,17 @@ def create_external_id standard_create ExternalId.new end + private + + def get_sso_token(application, user) + # Note: this method changes to keyword arguments in a future Doorkeeper version + Doorkeeper::AccessToken.find_or_create_for( + application, + user.id, + '', + 1.hour, + false, + ).token + end + end diff --git a/app/representers/api/v1/find_user_representer.rb b/app/representers/api/v1/find_user_representer.rb new file mode 100644 index 0000000000..e2c2f3bf30 --- /dev/null +++ b/app/representers/api/v1/find_user_representer.rb @@ -0,0 +1,46 @@ +module Api::V1 + class FindUserRepresenter < Roar::Decorator + include ::Roar::JSON + + property :id, + type: Integer, + readable: true, + writeable: false + + property :uuid, + type: String, + readable: true, + writeable: false + + property :support_identifier, + type: String, + readable: true, + writeable: false + + property :external_id, + type: String, + readable: false, + writeable: true, + schema_info: { + description: "External ID to search by" + } + + property :is_test, + type: :boolean, + readable: true, + writeable: false, + schema_info: { + description: 'Whether or not this is a test user' + } + + property :sso, + type: :string, + readable: true, + writeable: true, + getter: ->(user_options:, **) { user_options[:sso] }, + schema_info: { + description: 'Set to a non-empty string to request an sso cookie for the user' + } + + end +end diff --git a/config/routes.rb b/config/routes.rb index 4fc71a238b..8c2d0f9a3e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -222,8 +222,9 @@ resources :users, only: [:index] resource :user, only: [:show, :update] do - post '/find-or-create', action: 'find_or_create' - post '/external-ids', action: 'create_external_id' + post 'find' + post 'find-or-create', action: 'find_or_create' + post 'external-ids', action: 'create_external_id' end resources :application_users, only: [:index] do diff --git a/spec/access_policies/user_access_policy_spec.rb b/spec/access_policies/user_access_policy_spec.rb index 59a4c12a74..1b5244ef5f 100644 --- a/spec/access_policies/user_access_policy_spec.rb +++ b/spec/access_policies/user_access_policy_spec.rb @@ -2,12 +2,13 @@ RSpec.describe UserAccessPolicy do - let!(:anon) { AnonymousUser.instance } - let!(:temp) { FactoryBot.create :temp_user } - let!(:user) { FactoryBot.create :user } - let!(:new_social) { FactoryBot.create :new_social_user } - let!(:admin) { FactoryBot.create :user, :admin } - let!(:app) { FactoryBot.create :doorkeeper_application } + let!(:anon) { AnonymousUser.instance } + let!(:temp) { FactoryBot.create :temp_user } + let!(:user) { FactoryBot.create :user } + let!(:new_social) { FactoryBot.create :new_social_user } + let!(:admin) { FactoryBot.create :user, :admin } + let!(:app) { FactoryBot.create :doorkeeper_application } + let!(:trusted_app) { FactoryBot.create :doorkeeper_application, :trusted } context 'search' do it 'cannot be accessed by anonymous or temp users' do @@ -72,4 +73,21 @@ end end + [ :find, :create ].each do |action| + context action do + it 'cannot be accessed by users or unstrusted apps' do + expect(OSU::AccessPolicy.action_allowed?(action, anon, anon)).to eq false + expect(OSU::AccessPolicy.action_allowed?(action, temp, user)).to eq false + expect(OSU::AccessPolicy.action_allowed?(action, user, user)).to eq false + expect(OSU::AccessPolicy.action_allowed?(action, new_social, user)).to eq false + expect(OSU::AccessPolicy.action_allowed?(action, admin, user)).to eq false + expect(OSU::AccessPolicy.action_allowed?(action, app, user)).to eq false + end + + it 'can be accessed by trusted apps' do + expect(OSU::AccessPolicy.action_allowed?(action, trusted_app, user)).to eq true + end + end + end + end diff --git a/spec/controllers/api/v1/users_controller_spec.rb b/spec/controllers/api/v1/users_controller_spec.rb index 1312d0d924..d8a59dfb0e 100644 --- a/spec/controllers/api/v1/users_controller_spec.rb +++ b/spec/controllers/api/v1/users_controller_spec.rb @@ -258,6 +258,54 @@ end end + context "find" do + let(:trusted_application) do + FactoryBot.create :doorkeeper_application, can_find_or_create_accounts: true + end + let(:trusted_application_token) do + FactoryBot.create :doorkeeper_access_token, application: trusted_application, + resource_owner_id: nil + end + + let(:user) { FactoryBot.create :user } + let(:external_id) { FactoryBot.create :external_id, user: user } + let(:valid_body) { { external_id: external_id.external_id, sso: 'true' } } + + it "should find a user for a trusted app" do + api_post :find, + trusted_application_token, + body: valid_body + expect(response).to have_http_status :ok + expect(response.body_as_hash).to match( + id: user.id, + sso: kind_of(String), + support_identifier: user.support_identifier, + uuid: user.uuid + ) + end + + it "should not find a user that does not exist" do + api_post :find, + trusted_application_token, + body: { external_id: SecureRandom.uuid, sso: 'true' } + expect(response).to have_http_status :not_found + end + + it "should not find a user for anonymous" do + api_post :find, + nil, + body: valid_body + expect(response).to have_http_status :forbidden + end + + it "should not find a user for another user" do + api_post :find, + user_2_token, + body: valid_body + expect(response).to have_http_status :forbidden + end + end + context "find or create" do let!(:foc_trusted_application) do FactoryBot.create :doorkeeper_application, can_find_or_create_accounts: true diff --git a/spec/representers/api/v1/find_user_representer_spec.rb b/spec/representers/api/v1/find_user_representer_spec.rb new file mode 100644 index 0000000000..d432db23ef --- /dev/null +++ b/spec/representers/api/v1/find_user_representer_spec.rb @@ -0,0 +1,89 @@ +require 'rails_helper' + +RSpec.describe Api::V1::FindUserRepresenter, type: :representer do + let(:payload) { Hashie::Mash.new } + subject(:representer) { described_class.new payload } + + context 'id' do + it 'can be read' do + payload.id = 123 + expect(representer.to_hash['id']).to eq payload.id + end + + it 'cannot be written (attempts are silently ignored)' do + hash = { 'id' => 456 } + + expect(payload).not_to receive(:id=) + expect { representer.from_hash(hash) }.not_to change { payload.id } + end + end + + context 'uuid' do + it 'can be read' do + payload.uuid = SecureRandom.uuid + expect(representer.to_hash['uuid']).to eq payload.uuid + end + + it 'cannot be written (attempts are silently ignored)' do + hash = { 'uuid' => SecureRandom.uuid } + + expect(payload).not_to receive(:uuid=) + expect { representer.from_hash(hash) }.not_to change { payload.uuid } + end + end + + context 'support_identifier' do + it 'can be read' do + payload.support_identifier = SecureRandom.uuid + expect(representer.to_hash['support_identifier']).to eq payload.support_identifier + end + + it 'cannot be written (attempts are silently ignored)' do + hash = { 'support_identifier' => SecureRandom.uuid } + + expect(payload).not_to receive(:support_identifier=) + expect { representer.from_hash(hash) }.not_to change { payload.support_identifier } + end + end + + context 'external_id' do + it 'cannot be read' do + payload.external_id = SecureRandom.uuid + expect(representer.to_hash['external_id']).to be_nil + end + + it 'can be written' do + expect(payload).to receive(:external_id=).and_call_original + expect { representer.from_hash 'external_id' => SecureRandom.uuid }.to( + change { payload.external_id } + ) + end + end + + context 'is_test' do + it 'can be read' do + payload.is_test = true + expect(representer.to_hash['is_test']).to eq payload.is_test + end + + it 'cannot be written (attempts are silently ignored)' do + hash = { 'is_test' => false } + + expect(payload).not_to receive(:is_test=) + expect { representer.from_hash(hash) }.not_to change { payload.is_test } + end + end + + context 'sso' do + it 'can be read' do + expect(representer.to_hash(user_options: { sso: '123' })['sso']).to eq '123' + end + + it 'can be written' do + hash = { 'sso' => '456' } + + expect(payload).to receive(:sso=).and_call_original + expect { representer.from_hash(hash) }.to change { payload.sso } + end + end +end