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

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

Open
yasirazgar opened this issue Jul 3, 2022 · 3 comments · May be fixed by #5506
Open

Comments

@yasirazgar
Copy link

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 heartcombo@googlegroups.com 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 yasirazgar linked a pull request Jul 4, 2022 that will close this issue
@MatElGran
Copy link

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).

@yasirazgar
Copy link
Author

@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.

@carlosantoniodasilva
Copy link
Member

@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!

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

Successfully merging a pull request may close this issue.

3 participants