From 86596233aede29895df559977c2a62a8f5a6f5c2 Mon Sep 17 00:00:00 2001 From: James Mead Date: Mon, 14 Apr 2025 13:48:49 +0100 Subject: [PATCH] Don't use cached user if session has been reset This is a bit of an edge case, but if the user is logged in and then the session is reset (or at least its `current_user` value is reset) and `RpiAuth::Controllers::CurrentUser#current_user` is called within the *same* request, then previously it would have returned the user "cached" in the `@current_user`. Now it will return `nil` which seems less surprising. This was highlighted by this example [1] in code-club-frontend. In that case the example passes, because that app has its own implementation of `#current_user` [2]. However, when I added a similar example in experience-cs where we're using `RpiAuth::Controllers::CurrentUser#current_user`, the example failed. I've added a rather contrived example by adding a new `HomeController#reset_user` action to the dummy app used in the specs. However, hopefully it helps explain a bit about why the change is necessary. It looks as if the only two RPF repos that use `RpiAuth::Controllers::CurrentUser` are experience-ai [3,4] and experience-cs [5]. I'm pretty confident this change won't affect either of those apps, so this can probably be a minor version bump. I think a possible alternative to making this change would be to provide a `#reset_user` method which could both reset the session *and* reset the ivar-cached user. However, given that the code-club-frontend implementation already uses this approach, I think that probably makes more sense for now. [1]: https://github.com/RaspberryPiFoundation/code-club-frontend/blob/901f2200e926741fd170858286d64f1dd49994b4/spec/requests/refresh_credentials_spec.rb#L90 [2]: https://github.com/RaspberryPiFoundation/code-club-frontend/blob/901f2200e926741fd170858286d64f1dd49994b4/app/controllers/concerns/authentication_concern.rb#L6-L11 [3]: https://github.com/RaspberryPiFoundation/experience-ai/blob/ad729cefe060fcc0f0c345b3297247069fe55867/app/controllers/application_controller.rb#L6 [4]: https://github.com/RaspberryPiFoundation/experience-ai/blob/ad729cefe060fcc0f0c345b3297247069fe55867/app/controllers/api/v1/zipped_resources_controller.rb#L6 [5]: https://github.com/RaspberryPiFoundation/experience-cs/blob/33fa4756371fa3a550f32ac7613130e0925c17d3/app/controllers/application_controller.rb#L4 --- lib/rpi_auth/controllers/current_user.rb | 2 +- spec/dummy/app/controllers/home_controller.rb | 6 ++++++ spec/dummy/config/routes.rb | 1 + spec/dummy/spec/requests/auth_request_spec.rb | 9 +++++++++ 4 files changed, 17 insertions(+), 1 deletion(-) diff --git a/lib/rpi_auth/controllers/current_user.rb b/lib/rpi_auth/controllers/current_user.rb index 44e3c89..fc75779 100644 --- a/lib/rpi_auth/controllers/current_user.rb +++ b/lib/rpi_auth/controllers/current_user.rb @@ -10,8 +10,8 @@ module CurrentUser end def current_user - return @current_user if @current_user return nil unless session[:current_user] + return @current_user if @current_user @current_user = RpiAuth.user_model.new(session[:current_user]) end diff --git a/spec/dummy/app/controllers/home_controller.rb b/spec/dummy/app/controllers/home_controller.rb index 89ff5b1..0f319e4 100644 --- a/spec/dummy/app/controllers/home_controller.rb +++ b/spec/dummy/app/controllers/home_controller.rb @@ -1,4 +1,10 @@ class HomeController < ApplicationController def show end + + def reset_user + current_user + reset_session + render :show + end end diff --git a/spec/dummy/config/routes.rb b/spec/dummy/config/routes.rb index 2fff33c..f449321 100644 --- a/spec/dummy/config/routes.rb +++ b/spec/dummy/config/routes.rb @@ -1,6 +1,7 @@ Rails.application.routes.draw do # For details on the DSL available within this file, see https://guides.rubyonrails.org/routing.html root to: 'home#show' + get '/reset-user', to: 'home#reset_user' resource :session, only: %i[create] diff --git a/spec/dummy/spec/requests/auth_request_spec.rb b/spec/dummy/spec/requests/auth_request_spec.rb index 1eafd3b..b91cf5d 100644 --- a/spec/dummy/spec/requests/auth_request_spec.rb +++ b/spec/dummy/spec/requests/auth_request_spec.rb @@ -182,6 +182,15 @@ expect(session.id).not_to eq previous_id end + it 'does not use cached user if session is reset' do + post '/auth/rpi' + follow_redirect! + + get reset_user_path + + expect(response.body).to include('Log in') + end + context 'when session_keys_to_persist is set' do let(:session_keys_to_persist) { 'foo' }