Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepare for upgrade to rails 7.1 #468

Merged
merged 2 commits into from
Oct 12, 2023
Merged

Conversation

chrisroos
Copy link
Contributor

These changes preemptively avoid two deprecation warnings that appear when we upgrade to Rails 7.1:

DEPRECATION WARNING: Passing the class as positional argument is deprecated and will be removed in Rails 7.2.

and

DEPRECATION WARNING: Rails.application.secrets is deprecated in favor of Rails.application.credentials and will be removed in Rails 7.2. (called from block in class:Application at /govuk/authenticating-proxy/config/application.rb:32)

In order to avoid the following deprecation warning when upgrading to
Rails 7.1.0:

> DEPRECATION WARNING: Passing the class as positional argument is
> deprecated and will be removed in Rails 7.2.
Rails 7 changed the default digest class for the key generator from SHA1
to SHA256[1].

The code removed in this commit was added on 15 Mar 2022 in
05a41d8. The intention was to allow any
cookies encrypted using the SHA1 key generator to be read and
re-encrypted using SHA256. I can't see any record of us manually setting
encrypted cookies using `cookies.encrypted`[2] so I think this would
have only applied to our sessions which are stored using the (encrypted)
CookieStore[3]. Given that this rotator was added 17 months ago I think
it's safe to remove it (i.e. I would expect all existing sessions from
17 months ago to have been rotated).

Note that although we don't make any calls to `session` in
authenticating-proxy, there are calls to `session` in the gds-sso Gem
that's included in this app:

- GDS::SSO::FailureApp#store_location![4]
- AuthenticationsController#callback[5]
- lib/gds-sso/warden_config.rb[6]

[1]: https://guides.rubyonrails.org/v7.0/upgrading_ruby_on_rails.html#key-generator-digest-class-changing-to-use-sha256
[2]: https://api.rubyonrails.org/v7.1.1/classes/ActionDispatch/Cookies.html
[3]: https://api.rubyonrails.org/classes/ActionDispatch/Session/CookieStore.html
[4]: https://github.com/alphagov/gds-sso/blob/v18.1.0/lib/gds-sso/failure_app.rb#L48
[5]: https://github.com/alphagov/gds-sso/blob/v18.1.0/app/controllers/authentications_controller.rb#L8
[6]: https://github.com/alphagov/gds-sso/blob/v18.1.0/lib/gds-sso/warden_config.rb#L15
@floehopper floehopper self-assigned this Oct 12, 2023
Copy link
Contributor

@floehopper floehopper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@chrisroos chrisroos merged commit 81d31ee into main Oct 12, 2023
4 checks passed
@chrisroos chrisroos deleted the prepare-for-upgrade-to-rails-7.1 branch October 12, 2023 14:32
@chrisroos
Copy link
Contributor Author

Thanks @floehopper! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants