From e8d73e8066b61f7704dc0f6cf200953cdf9a5e60 Mon Sep 17 00:00:00 2001 From: Lucas Di Pentima Date: Thu, 28 Jan 2021 17:44:59 -0300 Subject: [PATCH] 16736: Limit token's expires_at depending on the cluster config and user type. Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima --- .../api_client_authorizations_controller.rb | 1 + .../app/models/api_client_authorization.rb | 13 +++ services/api/config/arvados_config.rb | 1 + .../api_client_authorizations_api_test.rb | 91 +++++++++++++++++++ .../test/integration/user_sessions_test.rb | 6 +- 5 files changed, 109 insertions(+), 3 deletions(-) diff --git a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb index 59e359232e..99446688db 100644 --- a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb +++ b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb @@ -17,6 +17,7 @@ class Arvados::V1::ApiClientAuthorizationsController < ApplicationController scopes: {type: 'array', required: false} } end + def create_system_auth @object = ApiClientAuthorization. new(user_id: system_user.id, diff --git a/services/api/app/models/api_client_authorization.rb b/services/api/app/models/api_client_authorization.rb index 9290e01a1a..4218645d5d 100644 --- a/services/api/app/models/api_client_authorization.rb +++ b/services/api/app/models/api_client_authorization.rb @@ -13,6 +13,8 @@ class ApiClientAuthorization < ArvadosModel after_initialize :assign_random_api_token serialize :scopes, Array + before_validation :clamp_token_expiration + api_accessible :user, extend: :common do |t| t.add :owner_uuid t.add :user_id @@ -384,6 +386,17 @@ class ApiClientAuthorization < ArvadosModel protected + def clamp_token_expiration + if !current_user.andand.is_admin && Rails.configuration.API.MaxTokenLifetime > 0 + max_token_expiration = Time.now + Rails.configuration.API.MaxTokenLifetime + if self.new_record? && (self.expires_at.nil? || self.expires_at > max_token_expiration) + self.expires_at = max_token_expiration + elsif !self.new_record? && self.expires_at_changed? && (self.expires_at.nil? || self.expires_at > max_token_expiration) + self.expires_at = max_token_expiration + end + end + end + def permission_to_create current_user.andand.is_admin or (current_user.andand.id == self.user_id) end diff --git a/services/api/config/arvados_config.rb b/services/api/config/arvados_config.rb index 5327713f69..8f4395dada 100644 --- a/services/api/config/arvados_config.rb +++ b/services/api/config/arvados_config.rb @@ -92,6 +92,7 @@ arvcfg.declare_config "API.DisabledAPIs", Hash, :disable_api_methods, ->(cfg, k, arvcfg.declare_config "API.MaxRequestSize", Integer, :max_request_size arvcfg.declare_config "API.MaxIndexDatabaseRead", Integer, :max_index_database_read arvcfg.declare_config "API.MaxItemsPerResponse", Integer, :max_items_per_response +arvcfg.declare_config "API.MaxTokenLifetime", ActiveSupport::Duration arvcfg.declare_config "API.AsyncPermissionsUpdateInterval", ActiveSupport::Duration, :async_permissions_update_interval arvcfg.declare_config "Users.AutoSetupNewUsers", Boolean, :auto_setup_new_users arvcfg.declare_config "Users.AutoSetupNewUsersWithVmUUID", String, :auto_setup_new_users_with_vm_uuid diff --git a/services/api/test/integration/api_client_authorizations_api_test.rb b/services/api/test/integration/api_client_authorizations_api_test.rb index 296ab8a2ff..3a7b201312 100644 --- a/services/api/test/integration/api_client_authorizations_api_test.rb +++ b/services/api/test/integration/api_client_authorizations_api_test.rb @@ -74,4 +74,95 @@ class ApiClientAuthorizationsApiTest < ActionDispatch::IntegrationTest assert_response 403 end + [nil, Time.now + 2.hours].each do |desired_expiration| + test "expires_at gets clamped on non-admins when API.MaxTokenLifetime is set and desired expires_at #{desired_expiration.nil? ? 'is not set' : 'exceeds the limit'}" do + Rails.configuration.API.MaxTokenLifetime = 1.hour + + # Test token creation + start_t = Time.now + post "/arvados/v1/api_client_authorizations", + params: { + :format => :json, + :api_client_authorization => { + :owner_uuid => users(:active).uuid, + :expires_at => desired_expiration, + } + }, + headers: {'HTTP_AUTHORIZATION' => "OAuth2 #{api_client_authorizations(:active_trustedclient).api_token}"} + end_t = Time.now + assert_response 200 + expiration_t = json_response['expires_at'].to_time + assert_operator expiration_t.to_f, :>, (start_t + Rails.configuration.API.MaxTokenLifetime).to_f + if !desired_expiration.nil? + assert_operator expiration_t.to_f, :<, desired_expiration.to_f + else + assert_operator expiration_t.to_f, :<, (end_t + Rails.configuration.API.MaxTokenLifetime).to_f + end + + # Test token update + previous_expiration = expiration_t + token_uuid = json_response["uuid"] + start_t = Time.now + put "/arvados/v1/api_client_authorizations/#{token_uuid}", + params: { + :api_client_authorization => { + :expires_at => desired_expiration + } + }, + headers: {'HTTP_AUTHORIZATION' => "OAuth2 #{api_client_authorizations(:active_trustedclient).api_token}"} + end_t = Time.now + assert_response 200 + expiration_t = json_response['expires_at'].to_time + assert_operator previous_expiration.to_f, :<, expiration_t.to_f + assert_operator expiration_t.to_f, :>, (start_t + Rails.configuration.API.MaxTokenLifetime).to_f + if !desired_expiration.nil? + assert_operator expiration_t.to_f, :<, desired_expiration.to_f + else + assert_operator expiration_t.to_f, :<, (end_t + Rails.configuration.API.MaxTokenLifetime).to_f + end + end + + test "expires_at can be set to #{desired_expiration.nil? ? 'nil' : 'exceed the limit'} by admins when API.MaxTokenLifetime is set" do + Rails.configuration.API.MaxTokenLifetime = 1.hour + + # Test token creation + post "/arvados/v1/api_client_authorizations", + params: { + :format => :json, + :api_client_authorization => { + :owner_uuid => users(:admin).uuid, + :expires_at => desired_expiration, + } + }, + headers: {'HTTP_AUTHORIZATION' => "OAuth2 #{api_client_authorizations(:admin_trustedclient).api_token}"} + assert_response 200 + if desired_expiration.nil? + assert json_response['expires_at'].nil? + else + assert_equal json_response['expires_at'].to_time.to_i, desired_expiration.to_i + end + + # Test token update (reverse the above behavior) + previous_expiration = json_response['expires_at'] + token_uuid = json_response['uuid'] + if previous_expiration.nil? + desired_updated_expiration = Time.now + Rails.configuration.API.MaxTokenLifetime + 1.hour + else + desired_updated_expiration = nil + end + put "/arvados/v1/api_client_authorizations/#{token_uuid}", + params: { + :api_client_authorization => { + :expires_at => desired_updated_expiration, + } + }, + headers: {'HTTP_AUTHORIZATION' => "OAuth2 #{api_client_authorizations(:admin_trustedclient).api_token}"} + assert_response 200 + if desired_updated_expiration.nil? + assert json_response['expires_at'].nil? + else + assert_equal json_response['expires_at'].to_time.to_i, desired_updated_expiration.to_i + end + end + end end diff --git a/services/api/test/integration/user_sessions_test.rb b/services/api/test/integration/user_sessions_test.rb index fcc0ce4e52..6e951499ad 100644 --- a/services/api/test/integration/user_sessions_test.rb +++ b/services/api/test/integration/user_sessions_test.rb @@ -53,19 +53,19 @@ class UserSessionsApiTest < ActionDispatch::IntegrationTest test 'existing user login' do mock_auth_with(identity_url: "https://active-user.openid.local") u = assigns(:user) - assert_equal 'zzzzz-tpzed-xurymjxw79nv3jz', u.uuid + assert_equal users(:active).uuid, u.uuid end test 'user redirect_to_user_uuid' do mock_auth_with(identity_url: "https://redirects-to-active-user.openid.local") u = assigns(:user) - assert_equal 'zzzzz-tpzed-xurymjxw79nv3jz', u.uuid + assert_equal users(:active).uuid, u.uuid end test 'user double redirect_to_user_uuid' do mock_auth_with(identity_url: "https://double-redirects-to-active-user.openid.local") u = assigns(:user) - assert_equal 'zzzzz-tpzed-xurymjxw79nv3jz', u.uuid + assert_equal users(:active).uuid, u.uuid end test 'create new user during omniauth callback' do -- 2.30.2