Open
Conversation
Test coverage89.86% line coverage reported by SimpleCov. |
I noticed that if a token is invalid because it has expired, we might call `from_token` multiple times. This is because `@current_user` is set to `nil` so won't be cached. `from_token` calls `HydraPublicApiClient.fetch_oauth_user` which does an API request. Since we might call current_user many times in a controller, we shouldn't be doing a API request from each one. Since we're currently recording unauthorized errors in Sentry, this often resulted in 3-5 errors being recorded for the same request. Instead, load the current user once. This might mean in some API calls we're loading the user when we don't need to, but since almost all API actions should validate the current user I don't see this as a problem. The identifible by cookie file doesn't have this problem, but I've updated it for consistency.
Sentry should be for exceptional errors. Having this reported to sentry was creating a lot of noise (~20k events a day) and using up a large proportion of our Sentry limits. If in the future we want to track this, we should instead log it or track unauthorized responses as a proxy. Before doing this I did some testing to see if the tokens were long lasting and are automatically refresh - as far as I can see they are and wasn't able to reproduce any issues by leaving tabs open or by offloading tabs. This would point to most tokens being invalid due to expiry. This could be due to the refresh token not working in inactive tabs.
zetter-rpf
force-pushed
the
reduce-auth-errors
branch
from
6e81a4c to
c0cc72c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce authentication-related noise by (1) preventing repeated user lookups when a token is invalid, and (2) avoiding Sentry reporting for expected 401s from Hydra.
Changes:
- Stop reporting
Faraday::UnauthorizedError(401) fromHydraPublicApiClient#fetch_oauth_userto Sentry. - Change controller authentication concerns to load
current_uservia abefore_actioninstead of@current_user ||= ...memoization. - Update
User.from_tokenspec to reflect the new unauthorized behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
spec/models/user_spec.rb |
Removes Sentry expectation for invalid-token path; keeps nil-return behavior check. |
lib/hydra_public_api_client.rb |
Stops capturing 401 Unauthorized exceptions to Sentry and returns nil. |
app/controllers/concerns/identifiable_by_cookie.rb |
Switches cookie-based identification to a before_action + attr_reader :current_user. |
app/controllers/concerns/identifiable.rb |
Switches header-based identification to a before_action + attr_reader :current_user. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zetter-rpf
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Status
What's changed?
See commits for more
Steps to perform after deploying to production