From 731c237d0c70cf53749da6e4316fc69adbd69843 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Tue, 16 Aug 2016 10:00:44 +0200 Subject: [PATCH] Improved permission check of personal tokens. --- app/controllers/application_controller.rb | 10 ++++++- app/controllers/roles_controller.rb | 5 +--- .../user_access_token_controller.rb | 12 +++++++-- app/models/token.rb | 2 +- test/controllers/api_auth_controller_test.rb | 26 ++++++++++++++----- test/unit/token_test.rb | 5 +--- 6 files changed, 42 insertions(+), 18 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 3b0fcde82..ec8833f97 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -265,9 +265,17 @@ class ApplicationController < ActionController::Base user = Token.check( action: 'api', name: token, - permission: auth_param[:permission], inactive_user: true, ) + if user && auth_param[:permission] + user = Token.check( + action: 'api', + name: token, + permission: auth_param[:permission], + inactive_user: true, + ) + raise Exceptions::NotAuthorized, 'No permission!' if !user + end @_token_auth = token # remember for permission_check return authentication_check_prerequesits(user, 'token_auth', auth_param) if user end diff --git a/app/controllers/roles_controller.rb b/app/controllers/roles_controller.rb index 19317a51a..49930cbbe 100644 --- a/app/controllers/roles_controller.rb +++ b/app/controllers/roles_controller.rb @@ -1,7 +1,7 @@ # Copyright (C) 2012-2014 Zammad Foundation, http://zammad-foundation.org/ class RolesController < ApplicationController - before_action :authentication_check + before_action { authentication_check(permission: 'admin.role') } =begin @@ -95,7 +95,6 @@ curl http://localhost/api/v1/roles.json -v -u #{login}:#{password} -H "Content-T =end def create - permission_check('admin.role') model_create_render(Role, params) end @@ -124,7 +123,6 @@ curl http://localhost/api/v1/roles.json -v -u #{login}:#{password} -H "Content-T =end def update - permission_check('admin.role') model_update_render(Role, params) end @@ -139,7 +137,6 @@ Test: =end def destroy - permission_check('admin.role') model_destory_render(Role, params) end end diff --git a/app/controllers/user_access_token_controller.rb b/app/controllers/user_access_token_controller.rb index 906a55da8..9f31e71e3 100644 --- a/app/controllers/user_access_token_controller.rb +++ b/app/controllers/user_access_token_controller.rb @@ -17,14 +17,22 @@ class UserAccessTokenController < ApplicationController local_permissions.each { |key, _value| keys = Object.const_get('Permission').with_parents(key) keys.each { |local_key| - next if local_permissions_new[local_key] + next if local_permissions_new.key?([local_key]) + if local_permissions[local_key] == true + local_permissions_new[local_key] = true + next + end local_permissions_new[local_key] = false } } permissions = [] Permission.all.order(:name).each { |permission| next if !local_permissions_new.key?(permission.name) - permissions.push permission + permission_attributes = permission.attributes + if local_permissions_new[permission.name] == false + permission_attributes['preferences']['disabled'] = true + end + permissions.push permission_attributes } render json: { diff --git a/app/models/token.rb b/app/models/token.rb index 4e84b96db..258a421ba 100644 --- a/app/models/token.rb +++ b/app/models/token.rb @@ -79,7 +79,7 @@ returns if data[:permission] return if !user.permissions?(data[:permission]) return if !token.preferences[:permission] - return if token.preferences[:permission][data[:permission]] != true + return if !token.preferences[:permission].include?(data[:permission]) end # return token user diff --git a/test/controllers/api_auth_controller_test.rb b/test/controllers/api_auth_controller_test.rb index ee8d34c07..155446f89 100644 --- a/test/controllers/api_auth_controller_test.rb +++ b/test/controllers/api_auth_controller_test.rb @@ -114,9 +114,7 @@ class ApiAuthControllerTest < ActionDispatch::IntegrationTest persistent: true, user_id: @admin.id, preferences: { - permission: { - 'admin.session' => true, - } + permission: ['admin.session'], }, ) admin_credentials = "Token token=#{admin_token.name}" @@ -135,7 +133,7 @@ class ApiAuthControllerTest < ActionDispatch::IntegrationTest assert_equal(Hash, result.class) assert(result) - admin_token.preferences[:permission]['admin.session'] = false + admin_token.preferences[:permission] = ['admin.session_not_existing'] admin_token.save! get '/api/v1/sessions', {}, @headers.merge('Authorization' => admin_credentials) @@ -144,7 +142,7 @@ class ApiAuthControllerTest < ActionDispatch::IntegrationTest assert_equal(Hash, result.class) assert_equal('No permission!', result['error']) - admin_token.preferences[:permission] = {} + admin_token.preferences[:permission] = [] admin_token.save! get '/api/v1/sessions', {}, @headers.merge('Authorization' => admin_credentials) @@ -162,7 +160,7 @@ class ApiAuthControllerTest < ActionDispatch::IntegrationTest assert_equal(Hash, result.class) assert_equal('User is inactive!', result['error']) - admin_token.preferences[:permission]['admin.session'] = true + admin_token.preferences[:permission] = ['admin.session'] admin_token.save! get '/api/v1/sessions', {}, @headers.merge('Authorization' => admin_credentials) @@ -179,6 +177,22 @@ class ApiAuthControllerTest < ActionDispatch::IntegrationTest result = JSON.parse(@response.body) assert_equal(Hash, result.class) assert(result) + + get '/api/v1/roles', {}, @headers.merge('Authorization' => admin_credentials) + assert_response(401) + result = JSON.parse(@response.body) + assert_equal(Hash, result.class) + assert_equal('No permission!', result['error']) + + admin_token.preferences[:permission] = ['admin.session_not_existing', 'admin.role'] + admin_token.save! + + get '/api/v1/roles', {}, @headers.merge('Authorization' => admin_credentials) + assert_response(200) + result = JSON.parse(@response.body) + assert_equal(Array, result.class) + assert(result) + end test 'token auth - agent' do diff --git a/test/unit/token_test.rb b/test/unit/token_test.rb index e13b4a2bb..ddf0ca338 100644 --- a/test/unit/token_test.rb +++ b/test/unit/token_test.rb @@ -83,10 +83,7 @@ class TokenTest < ActiveSupport::TestCase persistent: true, user_id: agent1.id, preferences: { - permission: { - 'admin' => true, # agent has no access to admin.* - 'ticket.agent' => true, - } + permission: ['admin', 'ticket.agent'], # agent has no access to admin.* } ) user = Token.check(