Merge pull request #1153 from openstax/more-external-id

Added new API to find user by external_id

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? || (

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

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

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

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

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

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