Add access token-related functionality including auto-refresh #83
Conversation
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
floehopper
force-pushed
the
extract-refresh-credentials-functionality-from-code-club-frontend
branch
2 times, most recently
from
f222bbe to
c2b9489
Compare
Test coverage: 100.0% |
floehopper
force-pushed
the
extract-refresh-credentials-functionality-from-code-club-frontend
branch
from
c2b9489 to
30d3d02
Compare
floehopper
changed the title
floehopper
changed the title
floehopper
force-pushed
the
extract-refresh-credentials-functionality-from-code-club-frontend
branch
from
30d3d02 to
4ecd786
Compare
This is more idiomatic use of an `ActiveSupport::Concern`. Instance methods defined on the concern module will be available in the class that includes the concern and the method will also be available in the ancestor chain for overriding and the use of `super`. The `included` block is intended for calling class-level "macros" like `attr_accessor`; not for defining instance methods.
So I can override it in modules or classes later in the ancestor chain.
Previously `RpiAuth::Controllers::CurrentUser` (which is in the `lib` directory) was only being required somewhat by accident via `RpiAuth::AuthController` which is required automatically, because it's in the `app/controllers` directory. I think a better solution would be to move the `CurrentUser` concern into the more idiomatic `app/controllers` directory (or perhaps `app/controllera/concerns`) where it would be automatically loaded and there would be no need for an expicit `require`. However, that feels like a bigger piece of work.
floehopper
force-pushed
the
extract-refresh-credentials-functionality-from-code-club-frontend
branch
2 times, most recently
from
125a195 to
e67a422
Compare
In `code-club-frontend`, `RpiAuth::Controllers::CurrentUser` is being included into `AuthenticationConcern` which is in turn used in `ApplicationComponent` which is *not* a Rails controller and so doesn't support `AbstractController::Helpers::ClassMethods#helper_method`.
This has been copied as closely as possible from `code-club-frontend`: - `lib/oauth_client.rb` [1] - `spec/lib/oauth_client_spec.rb` [2] Some things that I've changed: - Moved into the `RpiAuth` namespace. - Use `RpiAuth.configuration.bypass_auth` instead of reading `BYPASS_AUTH` env var directly as per this PR [3] - Use `RpiAuth.configure` in the `before` block instead of using `ClimateControl.modify` in an `around` block. The former is possible, because the `RpiAuth` configuration is reset before every example [4]. [1]: https://github.com/RaspberryPiFoundation/code-club-frontend/blob/f7e965798d910584fed0d1eb7867f32a899f9ce8/lib/oauth_client.rb [2]: https://github.com/RaspberryPiFoundation/code-club-frontend/blob/main/spec/lib/oauth_client_spec.rb [3]: https://github.com/RaspberryPiFoundation/experience-cs/pull/315 [4]: https://github.com/RaspberryPiFoundation/rpi-auth/blob/e54cc7a30c61cf43874a38c79f1e2f3e2b6d2103/spec/spec_helper.rb#L132-L136
floehopper
force-pushed
the
extract-refresh-credentials-functionality-from-code-club-frontend
branch
2 times, most recently
from
1213926 to
7224a41
Compare
patch0
requested changes
Apr 28, 2025
Contributor
patch0
left a comment
patch0
left a comment
There was a problem hiding this comment.
A couple of small comments. I think I'm maybe going OTT, but there's one typo that needs correcting I think.
This can optionally be included into your user model in order to obtain an access token, a refresh token, and an expiry time when logging in. These attributes are set by the same mechanism in `AuthController#callback` that populates `user_id` on the user via `Authenticatable.from_omniauth`, but instead calls `WithTokens.from_omniauth` which in turn calls `Authenticatable.from_omniauth` via the ancestor chain. This also relies on: - `RpiAuth.configuration.scope` including the "offline" scope in the Rails app which is using the `rpi_auth` gem. - In the `profile` app `hydra_client` config for the Rails app, `grant_types` must include "refresh_token" and `scope` must include "offline". Again this has been substantially copied from `code-club-frontend`: - `app/models/user.rb` [1] - `spec/models/user_spec.rb` [2] Some things that I've changed: - Moved methods on `User` into an `RpiAuth` namespaced concern which can be included into the user model in the Rails app. - Override the recently added `Authenticatable#attribute_keys` method, but call `super` to avoid duplication. - I haven't included the aliases for `#id` & `#id=` methods, because it wasn't immediately obvious that these are relevant for projects other than `code-club-frontend`. - Use `RpiAuth.configuration.bypass_auth` instead of reading `BYPASS_AUTH` env var directly as per this PR [3] - Use `RpiAuth.configure` in the `before` block instead of using `ClimateControl.modify` in an `around` block. The former is possible, because the `RpiAuth` configuration is reset before every example [4]. [1]: https://github.com/RaspberryPiFoundation/code-club-frontend/blob/f7e965798d910584fed0d1eb7867f32a899f9ce8/app/models/user.rb [2]: https://github.com/RaspberryPiFoundation/code-club-frontend/blob/f7e965798d910584fed0d1eb7867f32a899f9ce8/spec/models/user_spec.rb [3]: https://github.com/RaspberryPiFoundation/experience-cs/pull/315 [4]: https://github.com/RaspberryPiFoundation/rpi-auth/blob/e54cc7a30c61cf43874a38c79f1e2f3e2b6d2103/spec/spec_helper.rb#L132-L136
This can optionally be included into your controller to automatically use the user's refresh token to obtain a new access token when the old one expires. It adds a before action to the target controller which automatically calls `OauthClient#refresh_credentials!` if the user is signed in and their access token has expired. Again this has been substantially copied from `code-club-frontend`: - `app/controllers/application_controller.rb` [1] - `spec/requests/refresh_credentials_spec.rb` [2] Some things that I've changed: - Moved functionality from `ApplicationController` into an `RpiAuth` namespaced concern which can be included into the controller in the Rails app. - Make `AutoRefreshingToken#refresh_credentials_if_needed` private since it's not a controller action. - Use the home page in the dummy app in the request spec instead of the home page in `code-club-frontend`. - Construct a user directly in the request spec; there's no user factory in the `rpi_auth` gem. - Set `RpiAuth.configuration.user_model = 'User'` in the `before` block as per the other request specs. - Only call `before_action` if available. In `code-club-frontend`, `RpiAuth::Controllers::CurrentUser` is being included into `AuthenticationConcern` which is in turn used in `ApplicationComponent` which is *not* a Rails controller and so doesn't support `AbstractController::Callbacks::ClassMethods#before_action`. I'm slightly concerned about a few details of the implementation of `AutoRefreshingToken#refresh_credentials_if_needed`: - Rescuing all `ArgumentError` exceptions seems risky, because it's quite a common exception. It would be better to be more specific. - Even rescuing all `OAuth2::Error` exceptions seems a bit broad, although I haven't investigated what this encompasses. - While resetting the session in the event of an exception seems OK security-wise, it probably doesn't result in a great user experience. It would be better to redirect and/or display an error message to the user to explain what's happened. [1]: https://github.com/RaspberryPiFoundation/code-club-frontend/blob/f7e965798d910584fed0d1eb7867f32a899f9ce8/app/controllers/application_controller.rb#L8 [2]: https://github.com/RaspberryPiFoundation/code-club-frontend/blob/f7e965798d910584fed0d1eb7867f32a899f9ce8/spec/requests/refresh_credentials_spec.rb
Contributor
Author
Many thanks for reviewing. I've fixed the typo and explained that, while I agree with your other suggestion, I'd prefer defer making that change for now. I hope that makes sense. |
floehopper
force-pushed
the
extract-refresh-credentials-functionality-from-code-club-frontend
branch
from
81bab8d to
2a54a8d
Compare
patch0
approved these changes
May 1, 2025
Contributor
patch0
left a comment
patch0
left a comment
There was a problem hiding this comment.
Thanks @floehopper appreciate the work!
floehopper
deleted the
extract-refresh-credentials-functionality-from-code-club-frontend
branch
floehopper
added a commit
that referenced
this pull request
May 7, 2025
The changes in #83 (which were released in v4.1.0) moved the `OauthClient` class (which uses the `oauth2` gem) from the Rails applications into the `rpi_auth` gem. At that point the `oauth2` gem was made a direct dependency of `rpi_auth` and the direct dependency on `oauth2` was removed from the Rails applications. I _think_ this meant that the `oauth2` gem was no longer being required automatically. And, although individual files in the `oauth2` gem were being required from individual files in `rpi_auth`, this meant that not all of the files in `oauth2` were being required. In particular, the `lib/oauth2.rb` file which defines the top-level `OAuth2` module and the `OAuth2.config` method were no longer being required. This led to exceptions like this one [1] in `experience-cs` when `RpiAuth::OauthClient#refresh_credentials` was called: NoMethodError: undefined method 'config' for module OAuth2 This commit changes the require statements to just require `oauth2` which in turn requires all the relevant files [2] in the `oauth2` gem. The require statement in `spec/rpi_auth/oauth_client_spec.rb` is unnecessary, because it's automatically loading `RpiAuth::OauthClient` which in turn loads `oauth2`. This should mean we no longer see exceptions like the one above. [1]: https://raspberrypifoundation.sentry.io/issues/6590882516?project=4508953237651456 [2]: https://gitlab.com/oauth-xx/oauth2/-/blob/v2.0.9/lib/oauth2.rb?ref_type=tags#L11-23
Merged
floehopper
added a commit
that referenced
this pull request
May 7, 2025
The changes in #83 (which were released in v4.1.0) moved the `OauthClient` class (which uses the `oauth2` gem) from the Rails applications into the `rpi_auth` gem. At that point the `oauth2` gem was made a direct dependency of `rpi_auth` and the direct dependency on `oauth2` was removed from the Rails applications. I _think_ this meant that the `oauth2` gem was no longer being required automatically. And, although individual files in the `oauth2` gem were being required from individual files in `rpi_auth`, this meant that not all of the files in `oauth2` were being required. In particular, the `lib/oauth2.rb` file which defines the top-level `OAuth2` module and the `OAuth2.config` method was no longer being required. This led to exceptions like [this one][1] in `experience-cs` when `RpiAuth::OauthClient#refresh_credentials` was called: NoMethodError: undefined method 'config' for module OAuth2 This commit changes the require statements to just require `oauth2` which in turn [requires all the relevant files][2] in the `oauth2` gem. The require statement in `spec/rpi_auth/oauth_client_spec.rb` is unnecessary, because it's automatically loading `RpiAuth::OauthClient` which in turn loads `oauth2`. This should mean we no longer see exceptions like the one above. I've tested that this fix works in `experience-cs` in https://github.com/RaspberryPiFoundation/experience-cs/pull/421 - I ran the Rails app and triggered a token refresh to check that `NoMethodError` was no longer being raised. [1]: https://raspberrypifoundation.sentry.io/issues/6590882516?project=4508953237651456 [2]: https://gitlab.com/oauth-xx/oauth2/-/blob/v2.0.9/lib/oauth2.rb?ref_type=tags#L11-23
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.
The following new functionality has been extracted from
code-club-frontendandexperience-csto reduce duplication and hopefully make useful functionality available to other Rails apps using this gem.This pull request where it was recently added to
experience-csand this follow-up pull request are good references.rpi-authcode in the context ofexperience-csin this pull request and it seems to be OK.rpi-authcode in the context ofcode-club-frontendin this pull request and all the test pass.Add
RpiAuth::Models::WithTokensconcernThis can optionally be included into your user model in order to obtain an access token, a refresh token, and an expiry time when logging in. These attributes are set by the same mechanism in
AuthController#callbackthat populatesuser_idon the user viaAuthenticatable.from_omniauth, but instead callsWithTokens.from_omniauthwhich in turn callsAuthenticatable.from_omniauthvia the ancestor chain.This also relies on:
RpiAuth.configuration.scopeincluding the "offline" scope in the Rails app which is using therpi_authgem.profileapphydra_clientconfig for the Rails app,grant_typesmust include "refresh_token" andscopemust include "offline".This has been substantially copied from
code-club-frontend:app/models/user.rbspec/models/user_spec.rbAdd
RpiAuth::Controllers::AutoRefreshingTokenconcernThis can optionally be included into your controller to automatically use the user's refresh token to obtain a new access token when the old one expires. It adds a before action to the target controller which automatically calls
OauthClient#refresh_credentials!if the user is signed in and their access token has expired. The latter makes a request to theprofileapp using the refresh token to obtain a new access token.Again this has been substantially copied from
code-club-frontend:app/controllers/application_controller.rbspec/requests/refresh_credentials_spec.rblib/oauth_client.rbspec/lib/oauth_client_spec.rbQuestions/issues
In general I'd prefer to postpone tackling any of the following to separate PRs, because I think it's useful to have a version of the code which matches what's currently in
code-club-frontend&experience-csas closely as possible before we start changing things further. However, hopefully the following notes are useful:#user_id&#user_id=to#id&#id=respectively, because I'm not yet sure whether it's more generally useful - we don't seem to need it inexperience-csyet.libinto relevant directories underapp, e.g. I think that movinglib/rpi_auth/models/authenticatable.rb->app/models/rpi_auth/authenticatable.rb(or maybeapp/models/concerns/rpi_auth/authenticatable.rb) would mean the classes were automatically loaded. This seems like a more idiomatic use of a Rails engine. However, that's probably a bigger piece of work, especially to make sure it works with the relevant versions of Rails.AutoRefreshingToken#refresh_credentials_if_needed:ArgumentErrorexceptions seems risky, because it's quite a common exception. It would be better to be more specific.OAuth2::Errorexceptions seems a bit broad, although I haven't investigated what this encompasses. At the very least it would be good to log/report the details of the exception.