diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index e661c9d8b71..0b905f61323 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -36,7 +36,10 @@ class Auth::DefaultCurrentUserProvider request = @request - auth_token = request.cookies[TOKEN_COOKIE] + user_api_key = @env[USER_API_KEY] + api_key = request[API_KEY] + + auth_token = request.cookies[TOKEN_COOKIE] unless user_api_key || api_key current_user = nil @@ -61,10 +64,6 @@ class Auth::DefaultCurrentUserProvider end end - if current_user && (current_user.suspended? || !current_user.active) - current_user = nil - end - if current_user && should_update_last_seen? u = current_user Scheduler::Defer.later "Updating Last Seen" do @@ -74,17 +73,18 @@ class Auth::DefaultCurrentUserProvider end # possible we have an api call, impersonate - if api_key = request[API_KEY] + if api_key current_user = lookup_api_user(api_key, request) raise Discourse::InvalidAccess unless current_user + raise Discourse::InvalidAccess if current_user.suspended? || !current_user.active @env[API_KEY_ENV] = true end # user api key handling - if api_key = @env[USER_API_KEY] + if user_api_key - limiter_min = RateLimiter.new(nil, "user_api_min_#{api_key}", SiteSetting.max_user_api_reqs_per_minute, 60) - limiter_day = RateLimiter.new(nil, "user_api_day_#{api_key}", SiteSetting.max_user_api_reqs_per_day, 86400) + limiter_min = RateLimiter.new(nil, "user_api_min_#{user_api_key}", SiteSetting.max_user_api_reqs_per_minute, 60) + limiter_day = RateLimiter.new(nil, "user_api_day_#{user_api_key}", SiteSetting.max_user_api_reqs_per_day, 86400) unless limiter_day.can_perform? limiter_day.performed! @@ -94,8 +94,9 @@ class Auth::DefaultCurrentUserProvider limiter_min.performed! end - current_user = lookup_user_api_user_and_update_key(api_key, @env[USER_API_CLIENT_ID]) + current_user = lookup_user_api_user_and_update_key(user_api_key, @env[USER_API_CLIENT_ID]) raise Discourse::InvalidAccess unless current_user + raise Discourse::InvalidAccess if current_user.suspended? || !current_user.active limiter_min.performed! limiter_day.performed! @@ -103,6 +104,12 @@ class Auth::DefaultCurrentUserProvider @env[USER_API_KEY_ENV] = true end + # keep this rule here as a safeguard + # under no conditions to suspended or inactive accounts get current_user + if current_user && (current_user.suspended? || !current_user.active) + current_user = nil + end + @env[CURRENT_USER_KEY] = current_user end @@ -112,7 +119,7 @@ class Auth::DefaultCurrentUserProvider # it could be an anonymous path, this would add cost return if is_api? || !@env.key?(CURRENT_USER_KEY) - if @user_token && @user_token.user == user + if !is_user_api? && @user_token && @user_token.user == user rotated_at = @user_token.rotated_at needs_rotation = @user_token.auth_token_seen ? rotated_at < UserAuthToken::ROTATE_TIME.ago : rotated_at < UserAuthToken::URGENT_ROTATE_TIME.ago diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb index cd191ec6126..e666d4c8460 100644 --- a/spec/components/auth/default_current_user_provider_spec.rb +++ b/spec/components/auth/default_current_user_provider_spec.rb @@ -26,6 +26,18 @@ describe Auth::DefaultCurrentUserProvider do user = Fabricate(:user) ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1) expect(provider("/?api_key=hello").current_user.id).to eq(user.id) + + user.update_columns(active: false) + + expect{ + provider("/?api_key=hello").current_user + }.to raise_error(Discourse::InvalidAccess) + + user.update_columns(active: true, suspended_till: 1.day.from_now) + + expect{ + provider("/?api_key=hello").current_user + }.to raise_error(Discourse::InvalidAccess) end it "raises for a user pretending" do @@ -248,6 +260,13 @@ describe Auth::DefaultCurrentUserProvider do provider("/", params.merge({"REQUEST_METHOD" => "POST"})).current_user }.to raise_error(Discourse::InvalidAccess) + + user.update_columns(suspended_till: 1.year.from_now) + + expect { + provider("/", params).current_user + }.to raise_error(Discourse::InvalidAccess) + end it "rate limits api usage" do