@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/2517786...@github.com>_______________________________________________
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
