diff --git a/app/access_policies/message_access_policy.rb b/app/access_policies/message_access_policy.rb index b4c7d18ef..404e68db7 100644 --- a/app/access_policies/message_access_policy.rb +++ b/app/access_policies/message_access_policy.rb @@ -3,8 +3,8 @@ class MessageAccessPolicy # with which Message objects. def self.action_allowed?(action, requestor, msg) - # Only trusted apps can access this API - return false unless requestor.is_application? && requestor.trusted + # Only selected apps can access this API + return false unless requestor.is_application? && requestor.can_message_users # Create is currently the only available action [:create].include?(action) diff --git a/app/access_policies/user_access_policy.rb b/app/access_policies/user_access_policy.rb index 8182b198a..3286a3b60 100644 --- a/app/access_policies/user_access_policy.rb +++ b/app/access_policies/user_access_policy.rb @@ -16,8 +16,8 @@ def self.action_allowed?(action, requestor, user) requestor == user # Temp or unclaimed users only when :unclaimed # find-or-create accounts that are a stand-in for a person who's not yet signed up - # only trusted applications can access this via client credentials - Rails.env.development? || (requestor.is_application? && requestor.trusted?) + # only selected applications can access this via client credentials + Rails.env.development? || (requestor.is_application? && requestor.can_find_or_create_accounts?) end end end diff --git a/app/controllers/api/v1/application_users_controller.rb b/app/controllers/api/v1/application_users_controller.rb index 40d126c29..c25876ca3 100644 --- a/app/controllers/api/v1/application_users_controller.rb +++ b/app/controllers/api/v1/application_users_controller.rb @@ -91,7 +91,7 @@ def index app = current_api_user.application respond_with outputs, represent_with: Api::V1::UserSearchRepresenter, user_options: { - include_private_data: app && app.trusted? + include_private_data: app && app.can_access_private_user_data? }, location: nil end @@ -185,7 +185,7 @@ def updates outputs = GetUpdatedApplicationUsers.call(current_application, params[:limit]).outputs respond_with outputs[:application_users], represent_with: Api::V1::ApplicationUsersRepresenter, - user_options: { include_private_data: current_application && current_application.trusted? }, + user_options: { include_private_data: current_application && current_application.can_access_private_user_data? }, location: nil end diff --git a/app/controllers/api/v1/messages_controller.rb b/app/controllers/api/v1/messages_controller.rb index 2d64585a0..c3aa6a76b 100644 --- a/app/controllers/api/v1/messages_controller.rb +++ b/app/controllers/api/v1/messages_controller.rb @@ -7,7 +7,7 @@ class Api::V1::MessagesController < Api::V1::ApiController description <<-EOS All actions in this controller require an Oauth token obtained through the Client Credentials flow. - Only trusted applications can currently access this API. + Only selected applications can currently access this API. Messages belong to applications. They represent messages sent to Users through their ContactInfos. @@ -44,7 +44,7 @@ class Api::V1::MessagesController < Api::V1::ApiController api :POST, '/messages', 'Creates and sends a new Message.' description <<-EOS Creates and sends a new Message to the given users. - Can only be called by a trusted application, using a token + Can only be called by a selected application, using a token obtained via the Client Credentials Oauth flow. Returns a JSON representation of the sent message. diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb index d1d7d28cd..2d5665178 100644 --- a/app/controllers/api/v1/users_controller.rb +++ b/app/controllers/api/v1/users_controller.rb @@ -95,7 +95,7 @@ def index respond_with outputs, represent_with: Api::V1::UserSearchRepresenter, location: nil, user_options: { - include_private_data: current_application.try(:trusted?) + include_private_data: current_application.try(:can_access_private_user_data?) } end diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb index c14ec6f39..c56f22755 100644 --- a/app/controllers/oauth/applications_controller.rb +++ b/app/controllers/oauth/applications_controller.rb @@ -111,8 +111,13 @@ def set_user def app_params params.require(:doorkeeper_application).permit( :name, :redirect_uri, :scopes, :email_subject_prefix, :lead_application_source, - :trusted, :email_from_address, :confidential + :trusted, :email_from_address, :confidential, + :can_access_private_user_data, :can_find_or_create_accounts, :can_message_users, + :can_skip_oauth_screen, ) end end end + + + diff --git a/app/views/doorkeeper/applications/_form.html.erb b/app/views/doorkeeper/applications/_form.html.erb index 5c5f5ce12..d78cf0162 100644 --- a/app/views/doorkeeper/applications/_form.html.erb +++ b/app/views/doorkeeper/applications/_form.html.erb @@ -78,15 +78,25 @@ <% end %> + <% [ + :trusted, + :can_access_private_user_data, + :can_find_or_create_accounts, + :can_message_users, + :can_skip_oauth_screen, + ].each do |priviledge| %> + <%= content_tag :div, - class: "form-group#{' has-error' if application.errors[:trusted].present?}" do %> - <%= f.label :trusted?, class: 'col-sm-2 control-label', - for: 'doorkeeper_application_trusted' %> + class: "form-group#{' has-error' if application.errors[priviledge].present?}" do %> + <%= f.label priviledge.to_s.humanize + "?", class: 'col-sm-2 control-label', + for: "doorkeeper_application_#{priviledge}" %>
- <%= f.check_box :trusted %> - <%= doorkeeper_errors_for application, :trusted %> + <%= f.check_box priviledge %> + <%= doorkeeper_errors_for application, priviledge %>
<% end %> + + <% end %> <% end %>
diff --git a/app/views/doorkeeper/applications/show.html.erb b/app/views/doorkeeper/applications/show.html.erb index da5fddf08..49d627979 100644 --- a/app/views/doorkeeper/applications/show.html.erb +++ b/app/views/doorkeeper/applications/show.html.erb @@ -44,9 +44,20 @@

<%= @application.lead_application_source %>

-

Trusted?

+ <% [ + :trusted, + :can_access_private_user_data, + :can_find_or_create_accounts, + :can_message_users, + :can_skip_oauth_screen, + ].each do |priviledge| %> + +

<%= priviledge.to_s.humanize %>?

+ +

<%= @application.send(priviledge) ? 'Yes' : 'No' %>

+ + <% end %> -

<%= @application.trusted ? 'Yes' : 'No' %>

diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index a402d7b20..1d710e9b6 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -192,7 +192,7 @@ # For example if dealing with a trusted application. # skip_authorization do |resource_owner, client| - client.application.trusted + client.application.can_skip_oauth_screen end # WWW-Authenticate Realm (default "Doorkeeper"). diff --git a/config/initializers/doorkeeper_models.rb b/config/initializers/doorkeeper_models.rb index 07e21bd73..1c54f6498 100644 --- a/config/initializers/doorkeeper_models.rb +++ b/config/initializers/doorkeeper_models.rb @@ -22,6 +22,30 @@ def is_redirect_url?(url) # Let doorkeeper do the work of checking the URL against the app's redirect_uris Doorkeeper::OAuth::Helpers::URIChecker.valid_for_authorization?(url, redirect_uri) end + + ## TODO REMOVE THE FOLLOWING AFTER THESE NEW PERMISSIONS FIELDS ARE DEPLOYED + + def respond_to_missing?(method, include_private=false) + [ + :can_access_private_user_data, + :can_find_or_create_accounts, + :can_message_users, + :can_skip_oauth_screen, + ].include?(method.to_s.gsub(/\?/,'').to_sym) || super + end + + def method_missing(method, *args, &block) + if [ + :can_access_private_user_data, + :can_find_or_create_accounts, + :can_message_users, + :can_skip_oauth_screen, + ].include?(method.to_s.gsub(/\?/,'').to_sym) + trusted + else + super + end + end end Doorkeeper::AccessToken.class_eval do diff --git a/db/migrate/20190911171944_add_fine_grained_access_to_oauth_applications.rb b/db/migrate/20190911171944_add_fine_grained_access_to_oauth_applications.rb new file mode 100644 index 000000000..6c0040eb1 --- /dev/null +++ b/db/migrate/20190911171944_add_fine_grained_access_to_oauth_applications.rb @@ -0,0 +1,24 @@ +class AddFineGrainedAccessToOauthApplications < ActiveRecord::Migration[5.2] + def up + add_column :oauth_applications, :can_access_private_user_data, :boolean, default: false, null: false + add_column :oauth_applications, :can_find_or_create_accounts, :boolean, default: false, null: false + add_column :oauth_applications, :can_message_users, :boolean, default: false, null: false + add_column :oauth_applications, :can_skip_oauth_screen, :boolean, default: false, null: false + + Doorkeeper::Application.reset_column_information + + Doorkeeper::Application.where(trusted: true).update_all( + can_access_private_user_data: true, + can_find_or_create_accounts: true, + can_message_users: true, + can_skip_oauth_screen: true + ) + end + + def down + remove_column :oauth_applications, :can_access_private_user_data + remove_column :oauth_applications, :can_find_or_create_accounts + remove_column :oauth_applications, :can_message_users + remove_column :oauth_applications, :can_skip_oauth_screen + end +end diff --git a/db/schema.rb b/db/schema.rb index 17e1ac844..53a94a008 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2019_02_06_185243) do +ActiveRecord::Schema.define(version: 2019_09_11_171944) do # These are extensions that must be enabled in order to support this database enable_extension "citext" @@ -247,6 +247,10 @@ t.string "scopes", default: "", null: false t.string "lead_application_source", default: "", null: false t.boolean "confidential", default: true, null: false + t.boolean "can_access_private_user_data", default: false + t.boolean "can_find_or_create_accounts", default: false + t.boolean "can_message_users", default: false + t.boolean "can_skip_oauth_screen", default: false t.index ["owner_id", "owner_type"], name: "index_oauth_applications_on_owner_id_and_owner_type" t.index ["uid"], name: "index_oauth_applications_on_uid", unique: true end diff --git a/spec/controllers/api/v1/application_users_controller_spec.rb b/spec/controllers/api/v1/application_users_controller_spec.rb index 028f6083e..e9c24c5e3 100644 --- a/spec/controllers/api/v1/application_users_controller_spec.rb +++ b/spec/controllers/api/v1/application_users_controller_spec.rb @@ -3,7 +3,7 @@ RSpec.describe Api::V1::ApplicationUsersController, type: :controller, api: true, version: :v1 do let!(:untrusted_application) { FactoryBot.create :doorkeeper_application } - let!(:trusted_application) { FactoryBot.create :doorkeeper_application, :trusted } + let!(:trusted_application) { FactoryBot.create :doorkeeper_application, can_access_private_user_data: true } let!(:untrusted_application_token) do FactoryBot.create :doorkeeper_access_token, application: untrusted_application, diff --git a/spec/controllers/api/v1/messages_controller_spec.rb b/spec/controllers/api/v1/messages_controller_spec.rb index 72ee315fc..cd949ec77 100644 --- a/spec/controllers/api/v1/messages_controller_spec.rb +++ b/spec/controllers/api/v1/messages_controller_spec.rb @@ -7,7 +7,7 @@ email_from_address: 'app@example.com' } let!(:trusted_application) { - FactoryBot.create :doorkeeper_application, :trusted, + FactoryBot.create :doorkeeper_application, can_message_users: true, email_from_address: 'app@example.com' } let!(:user_1) { FactoryBot.create :user } diff --git a/spec/controllers/api/v1/users_controller_spec.rb b/spec/controllers/api/v1/users_controller_spec.rb index f95d8220e..d4b2d8dec 100644 --- a/spec/controllers/api/v1/users_controller_spec.rb +++ b/spec/controllers/api/v1/users_controller_spec.rb @@ -62,6 +62,19 @@ api_get :index, trusted_application_token, params: {q: 'first_name:bob last_name:Michaels'} expect(response.code).to eq('200') + expected_response = { + total_count: 1, + items: [ user_matcher(user_2, include_private_data: false) ] + } + + expect(response.body_as_hash).to match(expected_response) + end + + it "returns a single result well with private data" do + trusted_application.update_attribute(:can_access_private_user_data, true) + api_get :index, trusted_application_token, params: {q: 'first_name:bob last_name:Michaels'} + expect(response.code).to eq('200') + expected_response = { total_count: 1, items: [ user_matcher(user_2, include_private_data: true) ] @@ -249,10 +262,16 @@ end context "find or create" do + let!(:foc_trusted_application) { FactoryBot.create :doorkeeper_application, can_find_or_create_accounts: true } + let!(:foc_trusted_application_token) do + FactoryBot.create :doorkeeper_access_token, application: foc_trusted_application, + resource_owner_id: nil + end + it "should create a new user for an app" do expect{ api_post :find_or_create, - trusted_application_token, + foc_trusted_application_token, body: {email: 'a-new-email@test.com', first_name: 'Ezekiel', last_name: 'Jones'} }.to change{User.count}.by(1) expect(response.code).to eq('201') @@ -265,7 +284,7 @@ it 'creates a new user with first name, last name and full name if given' do expect { api_post :find_or_create, - trusted_application_token, + foc_trusted_application_token, body: { email: 'a-new-email@test.com', first_name: 'Sarah', @@ -279,7 +298,7 @@ expect(new_user.last_name).to eq 'Test' expect(new_user.full_name).to eq 'Sarah Test' expect(new_user.role).to eq 'instructor' - expect(new_user.applications).to eq [ trusted_application ] + expect(new_user.applications).to eq [ foc_trusted_application ] expect(new_user.uuid).not_to be_blank end @@ -303,7 +322,7 @@ context "should return only IDs for a user" do it "does so for unclaimed users" do - api_post :find_or_create, trusted_application_token, + api_post :find_or_create, foc_trusted_application_token, body: {email: unclaimed_user.contact_infos.first.value} expect(response.code).to eq('201') expect(response.body_as_hash).to eq( @@ -314,7 +333,7 @@ end it "does so for claimed users" do api_post :find_or_create, - trusted_application_token, + foc_trusted_application_token, body: {email: user_2.contact_infos.first.value} expect(response.code).to eq('201') expect(response.body_as_hash).to eq( diff --git a/spec/handlers/messages_create_spec.rb b/spec/handlers/messages_create_spec.rb index 06428f84f..b89e70e43 100644 --- a/spec/handlers/messages_create_spec.rb +++ b/spec/handlers/messages_create_spec.rb @@ -3,7 +3,7 @@ describe MessagesCreate, type: :handler do let!(:trusted_application) { - FactoryBot.create :doorkeeper_application, :trusted, + FactoryBot.create :doorkeeper_application, can_message_users: true, email_from_address: 'app@example.com' } let!(:trusted_application_token) { FactoryBot.create :doorkeeper_access_token, diff --git a/spec/support/feature_helpers.rb b/spec/support/feature_helpers.rb index c364c867f..5875df29c 100644 --- a/spec/support/feature_helpers.rb +++ b/spec/support/feature_helpers.rb @@ -88,7 +88,9 @@ def link_in_last_email end def create_application(skip_terms: false) - app = FactoryBot.create(:doorkeeper_application, :trusted, skip_terms: skip_terms) + app = FactoryBot.create(:doorkeeper_application, skip_terms: skip_terms, + can_access_private_user_data: true, + can_skip_oauth_screen: true) # We want to provide a local "external" redirect uri so our specs aren't actually # making HTTP calls against real external URLs like "example.com"