20750: Reorder remote token validation strategy
authorBrett Smith <brett.smith@curii.com>
Wed, 23 Aug 2023 01:47:33 +0000 (21:47 -0400)
committerBrett Smith <brett.smith@curii.com>
Thu, 24 Aug 2023 18:37:14 +0000 (14:37 -0400)
commit9717f88d29310067fa70e19fb9afb3679c036bc4
tree829a6aea430f678cc0a4f67565237f04b9208748
parent17c1f9f6bd6eea9321d36a45b9dfc1ca1f4fb51d
20750: Reorder remote token validation strategy

See updated comments for an explanation of why we do queries in the
order that we do. Briefly, now that getting the current API client
authorization is no longer subject to token scopes, that query has the
best chance of telling us immediately whether or not the token is
valid. If that query succeeds, but the current user query fails, we
might still be able to load a user record from the database from the
token's owner_uuid, and use that.

The immediate application for this change is sharing tokens with limited
scopes. When the user creates such a token, we expect they're doing so
using a token with unlimited scopes, and their user record will have
been loaded into the database at that time. Then visitors using the
sharing token will be able to authorize from that database record,
without having permission to query the full user record (and see the
creator's name and email address) directly.

This may also provide better behavior in cases of network hiccups or
other remote server turbulence. Now only one query *needs* to succeed,
the current token API call.

With these changes, it seemed appropriate to propagate any remote error
status codes back to the client, so they can also distinguish permanent
authorization failures from temporary ones. To do that, we attach an
`http_status` singleton method to the original
HTTPClient::BadResponseError. ApplicationController#render_error already
uses that method to determine the status code to return to the client,
and this commit extends CurrentApiToken with similar behavior. Before
this commit, there were definitely other cases that could raise
unhandled exceptions; I assume they were previously all returning
generic error codes, and probably untested. I think this is improved
behavior that makes sense with the rest of the change.

Arvados-DCO-1.1-Signed-off-by: Brett Smith <brett.smith@curii.com>
services/api/app/middlewares/arvados_api_token.rb
services/api/app/models/api_client_authorization.rb
services/api/test/integration/remote_user_test.rb