-
-
Notifications
You must be signed in to change notification settings - Fork 725
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
[Spree Upgrade] Authenticate API user through logged in user #3009
[Spree Upgrade] Authenticate API user through logged in user #3009
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
# Authenticate API user | ||
authenticate_user_without_spree_current_user | ||
end | ||
alias_method_chain :authenticate_user, :spree_current_user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this solves the problem, IMO there's no benefit on using alias_method_chain
here. There are two things that lead me to think we should implement this differently:
- We've been actively removing uses of
alias_method_chain
in the upgrade and this goes in the opposite direction. - It's been removed from Rails codebase itself and it's somewhat discouraged in the Rails community, for the reasons explained in https://yehudakatz.com/2009/03/06/alias_method_chain-in-models/ and https://www.justinweiss.com/articles/rails-5-module-number-prepend-and-the-end-of-alias-method-chain/.
If I'm not mistaken we can leverage the inheritance we're already using here. What about?
alias_method_chain :authenticate_user, :spree_current_user | |
def authenticate_user | |
@current_api_user = try_spree_current_user | |
super | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, yes, we are extending not class_evaling.... this was a SUPER suggestion @sauloperez :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol @luisramos0
259ea35
to
bc7674e
Compare
bc7674e
to
3dd981c
Compare
What? Why?
Related to #3000
Use inheritance to set the
current_api_user
tospree_current_user
when a call to the API is made.What should we test?
When calling the API while logged in in the app, the
current_api_user
should be the logged in user.Release notes
The API user is now the app's logged in user, if any user is logged in. This prevents action with
authorize!
checks to fail, but parts of the API with no such check remain open for non authenticated users.Changelog Category: Added
How is this related to the Spree upgrade?
This comes from the fact that this line now doesn't have the
try_spree_current_user
setting, which means that if no API key is provided, the API user will be set toSpree.user_class.new
, making anyauthorize!
check fail.