From e9be38b3e9537d0f784caa8f1d7ce86a73eaada3 Mon Sep 17 00:00:00 2001 From: wvengen Date: Tue, 5 Feb 2019 20:53:02 +0100 Subject: [PATCH] Add OAuth scopes https://github.com/foodcoops/foodsoft/issues/582#issuecomment-442513237 --- app/controllers/api/v1/base_controller.rb | 24 ++++--- app/controllers/api/v1/configs_controller.rb | 2 + .../api/v1/user/users_controller.rb | 9 +++ app/controllers/api/v1/users_controller.rb | 7 -- app/controllers/concerns/auth_api.rb | 66 +++++++++++++++++++ config/initializers/doorkeeper.rb | 9 ++- config/routes.rb | 5 +- doc/API.md | 2 + doc/swagger.v1.yml | 28 ++++++-- spec/api/v1/swagger_spec.rb | 12 ++-- spec/factories/user.rb | 8 +++ spec/support/api_helper.rb | 22 ++++++- 12 files changed, 162 insertions(+), 32 deletions(-) create mode 100644 app/controllers/api/v1/user/users_controller.rb delete mode 100644 app/controllers/api/v1/users_controller.rb create mode 100644 app/controllers/concerns/auth_api.rb diff --git a/app/controllers/api/v1/base_controller.rb b/app/controllers/api/v1/base_controller.rb index 9a6cc1d0..7b92a287 100644 --- a/app/controllers/api/v1/base_controller.rb +++ b/app/controllers/api/v1/base_controller.rb @@ -1,4 +1,6 @@ class Api::V1::BaseController < ApplicationController + include Concerns::AuthApi + protect_from_forgery with: :null_session before_action :skip_session @@ -11,16 +13,6 @@ class Api::V1::BaseController < ApplicationController private - def authenticate - doorkeeper_authorize! - super if current_user - end - - # @return [User] Current user, or +nil+ if no valid token. - def current_user - @current_user ||= User.undeleted.find(doorkeeper_token.resource_owner_id) if doorkeeper_token - end - # @return [Ordergroup] Current user's ordergroup, or +nil+ if no valid token or user has no ordergroup. def current_ordergroup current_user.try(:ordergroup) @@ -28,7 +20,9 @@ class Api::V1::BaseController < ApplicationController def require_ordergroup authenticate - raise Api::Errors::PermissionRequired unless current_user.ordergroup.present? + unless current_ordergroup.present? + raise Api::Errors::PermissionRequired.new('Forbidden, must be in an ordergroup') + end end def skip_session @@ -42,13 +36,18 @@ class Api::V1::BaseController < ApplicationController end def not_acceptable_handler(e) - render status: 422, json: {error: 'not_acceptable', error_description: e.message || 'Data not acceptable' } + msg = e.message || 'Data not acceptable' + render status: 422, json: {error: 'not_acceptable', error_description: msg} end def doorkeeper_unauthorized_render_options(error:) {json: {error: error.name, error_description: error.description}} end + def doorkeeper_forbidden_render_options(error:) + {json: {error: error.name, error_description: error.description}} + end + def permission_required_handler(e) msg = e.message || 'Forbidden, user has no access' render status: 403, json: {error: 'forbidden', error_description: msg} @@ -58,5 +57,4 @@ class Api::V1::BaseController < ApplicationController def show_user(user = current_user, **options) user.display end - end diff --git a/app/controllers/api/v1/configs_controller.rb b/app/controllers/api/v1/configs_controller.rb index 3ba0ed4b..8edb73c4 100644 --- a/app/controllers/api/v1/configs_controller.rb +++ b/app/controllers/api/v1/configs_controller.rb @@ -1,5 +1,7 @@ class Api::V1::ConfigsController < Api::V1::BaseController + before_action ->{ doorkeeper_authorize! 'config:user', 'config:read', 'config:write' } + def show render json: FoodsoftConfig, serializer: ConfigSerializer, root: 'config' end diff --git a/app/controllers/api/v1/user/users_controller.rb b/app/controllers/api/v1/user/users_controller.rb new file mode 100644 index 00000000..e95e4790 --- /dev/null +++ b/app/controllers/api/v1/user/users_controller.rb @@ -0,0 +1,9 @@ +class Api::V1::User::UsersController < Api::V1::BaseController + + before_action ->{ doorkeeper_authorize! 'user:read', 'user:write' } + + def show + render json: current_user + end + +end diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb deleted file mode 100644 index f260c0c9..00000000 --- a/app/controllers/api/v1/users_controller.rb +++ /dev/null @@ -1,7 +0,0 @@ -class Api::V1::UsersController < Api::V1::BaseController - - def show - render json: current_user - end - -end diff --git a/app/controllers/concerns/auth_api.rb b/app/controllers/concerns/auth_api.rb new file mode 100644 index 00000000..be3389ec --- /dev/null +++ b/app/controllers/concerns/auth_api.rb @@ -0,0 +1,66 @@ +# Controller concern for authentication methods +# +# Split off from main +ApplicationController+ to allow e.g. +# Doorkeeper to use it too. +module Concerns::AuthApi + extend ActiveSupport::Concern + + protected + + def authenticate + # authenticate does not look at scopes, we just want to know if there's a valid token + # @see https://github.com/doorkeeper-gem/doorkeeper/blob/v5.0.1/lib/doorkeeper/models/access_token_mixin.rb#L218 + doorkeeper_render_error unless doorkeeper_token && doorkeeper_token.accessible? + super if current_user + end + + # @return [User] Current user, or +nil+ if no valid token. + def current_user + @current_user ||= User.undeleted.find(doorkeeper_token.resource_owner_id) if doorkeeper_token + end + + def doorkeeper_authorize!(*scopes) + super(*scopes) + # In addition to Doorkeeper's authorization and scope check, we also verify + # that the user has permissions for the scope (through its workgroups). + # Unless no scopes were supplied, which means we only want to make sure there + # is a valid user. + # + # If Doorkeeper's +handle_auth_errors+ is set to +:raise+, we don't get here, + # but otherwise we need to check whether +super+ rendered an error. + doorkeeper_authorize_roles!(*scopes) if scopes.present? && !performed? + end + + private + + # Make sure that at least one the given OAuth scopes is valid for the current user's permissions. + # @raise Api::Errors::PermissionsRequired + def doorkeeper_authorize_roles!(*scopes) + unless scopes.any? {|scope| doorkeeper_scope_permitted?(scope) } + raise Api::Errors::PermissionRequired.new('Forbidden, no permission') + end + end + + # Check whether a given OAuth scope is permitted for the current user. + # @note This does not validate whether the given scope is a valid scope. + # @return [Boolean] Whether the given scope is valid for the current user. + # @see https://github.com/foodcoops/foodsoft/issues/582#issuecomment-442513237 + def doorkeeper_scope_permitted?(scope) + scope_parts = scope.split(':') + # user sub-scopes like +config:user+ are always permitted + if scope_parts.last == 'user' + return true + end + + case scope_parts.first + when 'user' then true # access to the current user's own profile + when 'config' then current_user.role_admin? + when 'users' then current_user.role_admin? + when 'workgroups' then current_user.role_admin? + when 'suppliers' then current_user.role_suppliers? + when 'group_orders' then current_user.role_orders? + when 'finance' then current_user.role_finance? + # please note that offline_access does not belong here, since it is not used for permission checking + end + end +end diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index ad5be092..253e7b42 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -47,8 +47,13 @@ Doorkeeper.configure do # Define access token scopes for your provider # For more information go to # https://github.com/doorkeeper-gem/doorkeeper/wiki/Using-Scopes - # default_scopes :public - # optional_scopes :write, :update + + # default is a collection of read-only scopes + default_scopes 'config:user', 'user:read' + + optional_scopes 'config:read', 'config:write', + 'user:write', + 'offline_access' # Change the way client credentials are retrieved from the request object. # By default it retrieves first from the `HTTP_AUTHORIZATION` header, then diff --git a/config/routes.rb b/config/routes.rb index 94037c63..9e3cad02 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -245,9 +245,12 @@ Foodsoft::Application.routes.draw do namespace :api do namespace :v1 do - resource :user, only: [:show] resource :config, only: [:show] resource :navigation, only: [:show] + + namespace :user do + root to: 'users#show' + end end end diff --git a/doc/API.md b/doc/API.md index 5c544149..2e09cfa4 100644 --- a/doc/API.md +++ b/doc/API.md @@ -9,6 +9,8 @@ The API is documented using [Open API 2.0](https://github.com/OAI/OpenAPI-Specif / [Swagger](https://swagger.io/) in [swagger.v1.yml](swagger.v1.yml). This provides a machine-readable reference that is used to provide documentation. +**Note:** the current OAuth scopes may be subject to change, until the next release of Foodsoft. + ## API endpoint documentation >> [View API documentation](http://petstore.swagger.io/?url=https%3A%2F%2Fraw.githubusercontent.com%2Ffoodcoops%2Ffoodsoft%2Fmaster%2Fdoc%2Fswagger.v1.yml) << diff --git a/doc/swagger.v1.yml b/doc/swagger.v1.yml index eeee5f35..62321496 100644 --- a/doc/swagger.v1.yml +++ b/doc/swagger.v1.yml @@ -46,8 +46,12 @@ paths: description: not logged-in schema: $ref: '#/definitions/Error401' + 403: + description: missing scope + schema: + $ref: '#/definitions/Error403' security: - - foodsoft_auth: ['all'] + - foodsoft_auth: ['user:read', 'user:write'] /config: get: summary: configuration variables @@ -62,8 +66,12 @@ paths: description: not logged-in schema: $ref: '#/definitions/Error401' + 403: + description: missing scope or no permission + schema: + $ref: '#/definitions/Error403' security: - - foodsoft_auth: ['all'] + - foodsoft_auth: ['config:user', 'config:read', 'config:write'] /navigation: get: summary: navigation @@ -82,7 +90,7 @@ paths: schema: $ref: '#/definitions/Error401' security: - - foodsoft_auth: ['all'] + - foodsoft_auth: [] definitions: # models @@ -142,6 +150,14 @@ definitions: description: 'unauthorized' error_description: $ref: '#/definitions/Error/properties/error_description' + Error403: + type: object + properties: + error: + type: string + description: 'forbidden or invalid_scope' + error_description: + $ref: '#/definitions/Error/properties/error_description' securityDefinitions: foodsoft_auth: @@ -149,5 +165,9 @@ securityDefinitions: flow: implicit authorizationUrl: http://localhost:3000/f/oauth/authorize scopes: - all: full access to user functions + config:user: reading Foodsoft configuration for regular users + config:read: reading Foodsoft configuration values + config:write: reading and updating Foodsoft configuration values + user:read: reading your own user profile + user:write: reading and updating your own user profile offline_access: retain access after user has logged out diff --git a/spec/api/v1/swagger_spec.rb b/spec/api/v1/swagger_spec.rb index ab0197aa..33bf4851 100644 --- a/spec/api/v1/swagger_spec.rb +++ b/spec/api/v1/swagger_spec.rb @@ -15,6 +15,7 @@ describe 'API v1', type: :apivore, order: :defined do context 'has valid paths' do context 'user' do + let(:api_scopes) { ['user:read'] } # create multiple users to make sure we're getting the authenticated user, not just any let!(:other_user_1) { create :user } let!(:user) { create :user } @@ -23,20 +24,23 @@ describe 'API v1', type: :apivore, order: :defined do it { is_expected.to validate(:get, '/user', 200, api_auth) } it { is_expected.to validate(:get, '/user', 401) } - context 'with invalid access token' do - let(:api_access_token) { 'abc' } - it { is_expected.to validate(:get, '/user', 401, api_auth) } - end + it_handles_invalid_token_and_scope(:get, '/user') end context 'config' do + let(:api_scopes) { ['config:user'] } + it { is_expected.to validate(:get, '/config', 200, api_auth) } it { is_expected.to validate(:get, '/config', 401) } + + it_handles_invalid_token_and_scope(:get, '/config') end context 'navigation' do it { is_expected.to validate(:get, '/navigation', 200, api_auth) } it { is_expected.to validate(:get, '/navigation', 401) } + + it_handles_invalid_token(:get, '/navigation') end end diff --git a/spec/factories/user.rb b/spec/factories/user.rb index 51b35809..c4bcb6a2 100644 --- a/spec/factories/user.rb +++ b/spec/factories/user.rb @@ -21,6 +21,14 @@ FactoryBot.define do create :ordergroup, user_ids: [user.id] end end + + [:ordergroup, :finance, :invoices, :article_meta, :suppliers, :pickups, :orders].each do |role| + trait "role_#{role}".to_sym do + after :create do |user, evaluator| + create :workgroup, "role_#{role}" => true, user_ids: [user.id] + end + end + end end factory :group do diff --git a/spec/support/api_helper.rb b/spec/support/api_helper.rb index 4ab6a55f..66f9bd95 100644 --- a/spec/support/api_helper.rb +++ b/spec/support/api_helper.rb @@ -3,8 +3,28 @@ module ApiHelper included do let(:user) { create(:user) } - let(:api_access_token) { create(:oauth2_access_token, resource_owner_id: user.id).token } + let(:api_scopes) { [] } # empty scopes for stricter testing (in reality this would be default_scopes) + let(:api_access_token) { create(:oauth2_access_token, resource_owner_id: user.id, scopes: api_scopes&.join(' ')).token } let(:api_authorization) { "Bearer #{api_access_token}" } + + def self.it_handles_invalid_token(method, path, params_block = ->{ api_auth }) + context 'with invalid access token' do + let(:api_access_token) { 'abc' } + it { is_expected.to validate(method, path, 401, instance_exec(¶ms_block)) } + end + end + + def self.it_handles_invalid_scope(method, path, params_block = ->{ api_auth }) + context 'with invalid scope' do + let(:api_scopes) { ['none'] } + it { is_expected.to validate(method, path, 403, instance_exec(¶ms_block)) } + end + end + + def self.it_handles_invalid_token_and_scope(*args) + it_handles_invalid_token(*args) + it_handles_invalid_scope(*args) + end end # Add authentication to parameters for {Swagger::RspecHelpers#validate}