Skip to content

Commit 8659623

Browse files
committed
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
1 parent 2017252 commit 8659623

File tree

4 files changed

+17
-1
lines changed

4 files changed

+17
-1
lines changed

lib/rpi_auth/controllers/current_user.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,8 @@ module CurrentUser
1010
end
1111

1212
def current_user
13-
return @current_user if @current_user
1413
return nil unless session[:current_user]
14+
return @current_user if @current_user
1515

1616
@current_user = RpiAuth.user_model.new(session[:current_user])
1717
end
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
11
class HomeController < ApplicationController
22
def show
33
end
4+
5+
def reset_user
6+
current_user
7+
reset_session
8+
render :show
9+
end
410
end

spec/dummy/config/routes.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
Rails.application.routes.draw do
22
# For details on the DSL available within this file, see https://guides.rubyonrails.org/routing.html
33
root to: 'home#show'
4+
get '/reset-user', to: 'home#reset_user'
45

56
resource :session, only: %i[create]
67

spec/dummy/spec/requests/auth_request_spec.rb

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,15 @@
182182
expect(session.id).not_to eq previous_id
183183
end
184184

185+
it 'does not use cached user if session is reset' do
186+
post '/auth/rpi'
187+
follow_redirect!
188+
189+
get reset_user_path
190+
191+
expect(response.body).to include('Log in')
192+
end
193+
185194
context 'when session_keys_to_persist is set' do
186195
let(:session_keys_to_persist) { 'foo' }
187196

0 commit comments

Comments
 (0)