devise icon indicating copy to clipboard operation
devise copied to clipboard

Local variable used instead of calling the setter in devise_controller#require_no_authentication

Open yasirazgar opened this issue 2 years ago • 3 comments

Pre-check

  • Do not use the issues tracker for help or support, try Stack Overflow.
  • For bugs, do a quick search and make sure the bug has not yet been reported
  • If you found a security bug, do not report it through GitHub. Please send an e-mail to [email protected] instead.
  • Finally, be nice and have fun!

Environment

  • Ruby [2.7.6]
  • Rails [6.0.4.8]
  • Devise [4.8.1]

Current behavior

Uses resource = warden.user(resource_name) instead of self.resource = warden.user(resource_name) in devise_controller#require_no_authentication due to this instance variable @user is not populated, as the setter resource = is not triggered.

Expected behavior

Should use self.resource in devise_controller#require_no_authentication so that the setter is triggered, and instance variable @user is populated

yasirazgar avatar Jul 03 '22 16:07 yasirazgar

I may miss something, but I think this is intentional.

This local variable is only used to determine after_signin_path value for the resource, instance variable set here won't be accessible by application code

  • The instance variable would only be set if a resource is authenticated.
  • If that is the case, we don't want to execute the controller action, so Devise redirects immediately to the after_signin_path configured for this resource, no more application code is executed for this request-response cycle.
  • Upon redirection, next request will be handled by a new instance of ApplicationController and application code won't have access to the previously set instance variable.

if no resource is authenticated, this code won't be executed and the instance variable will be nil (which is expected).

MatElGran avatar Jul 12 '22 12:07 MatElGran

@MatElGran Yes, but as you can see the affected code,

if authenticated && (resource = warden.user(resource_name))

then second condition will get executed only if the first condition returns true, so resource will be set only if authenticated returns true, and setting resource = will just creating a new local variable, whereas it should be self.resource = which will call this setter method.

yasirazgar avatar Jul 14 '22 03:07 yasirazgar

@yasirazgar Appreciate your work on this. I think there's no need to set the value since we're redirecting just after that, would there be any value in setting the instance var to nil for the non-redirect case? I'm guessing not, I can't think of one off the top of my head. Let me know if there's anything I may be missing there. Thanks!

carlosantoniodasilva avatar Mar 02 '23 00:03 carlosantoniodasilva