[openstreetmap/openstreetmap-website] Merge ApiCapability class into ApiAbility (PR #5429)

[Tom Hughes]

@tomhughes commented on this pull request.



> @@ -64,11 +64,7 @@ def authorize(errormessage = "Couldn't authenticate you")
 
   def current_ability
     # Use capabilities from the oauth token if it exists and is a valid access token
-    if doorkeeper_token&.accessible?
-      ApiAbility.new(nil).merge(ApiCapability.new(doorkeeper_token))
-    else
-      ApiAbility.new(current_user)
-    end
+    ApiAbility.new((doorkeeper_token if doorkeeper_token&.accessible?))

Do we not need to take `current_user` into account when there is no token? or is that redundant now that OAuth 2 is the only authentication method?

Whatever the answer to that is I think I'd rather see this kept as an if block rather than having to use double parentheses to embed a conditional expression in the argument list.

>      can :read, [:version, :capability, :permission, :map]
 
     if Settings.status != "database_offline"
+      user = (User.find(token.resource_owner_id) if token)

I don't think the outer parentheses are needed here? They would be if `user` already had a value and you wanted to make sure it was overwritten but that's not the case here.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5429#pullrequestreview-2517786781
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/5429/review/2517786781 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20241220/1d7ab523/attachment.htm>