SECURITY: inactive/suspended accounts should be banned from api

Also fixes edge cases around users presenting multiple credentials

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_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_#{api_key}", SiteSetting.max_user_api_reqs_per_day, 86400)
+      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

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