Utilize Devise Location helpers #109
Conversation
| if request.env['omniauth.error'].present? | ||
| flash[:error] = I18n.t('devise.omniauth_callbacks.failure', kind: auth_hash['provider'], reason: I18n.t('spree.user_was_not_valid')) | ||
| redirect_back_or_default(root_url) | ||
| redirect_to stored_spree_user_location_or(root_url) |
There was a problem hiding this comment.
This assumes we are using the latest version of solidus_auth_devise, which contains the solidusio/solidus_auth_devise#228 and it's not always true. We should probably make it conditional, isn't it?
There was a problem hiding this comment.
We should always be using the latest solidus_auth_devise as the version number is not specified in the gemspec and it should resolve to the latest I would expect. It is true this would break if the user specified solidus_auth_devise 2.5.4, but I feel like we may want to specify in the gemspec rather than create a conditional here. WDYT?
With the deprecation of #redirect_back_or_default in solidus 4.0, we can utilize the SolidusAuthDevise helper stored_location_for to provide the same functionality.
cpfergus1
force-pushed
the
nebulab/remove-redirect-back-or-default
branch
from
31ed644 to
aa75c33
Compare
|
Hey, I climbed down the rabbit hole that is the history of the devise location helpers and understood that the initial PR by @elia caused a lot of trouble reverting and remaking PRs starting with the modification in Solidus #4533 (Deprecate method #redirect_back_or_default ). Given that we are right now touching auth by restoring solidus_social and touching also starter_frontend for that, wouldn't this be something that we can push through with a new major or is it something that is officially abandoned as a nice to have causing too much trouble? |
|
I'm fully in favor of pushing a new major with an explicit dependency on solidus_auth_devise. Most of the extensions secretly assumed it to be there anyway, much better to just state it explicitly. 👍 |
@kennyadsl is that acceptable for you? |
|
Yes definitely. Solidus Social could depends on Devise. If anyone needs to build social logins on top of other auth providers, I doubt this gem will be useful anyway. |
|
@rahulsingh321 this should work out of the box, please integrate into the current PR and test it. |
@fthobe These changes are reverted here so its not in solidus_auth_devise latest version. Adding this causing failure in the specs to me. |
|
@kennyadsl we are waiting on a PR in Devise to offer password complexity implementation in Devise to be merged making many dependencies of devise to set password complexity unneccessary before layong hand on solidus devise. Also MFA is required in many settings by PCI compliance and we would like to look into this issue Inside a larger context of separating admin and user auth to prevent escalations and allow impersonations effectively. |
5 participants
Description
Removes
#redirect_back_or_defaultin favor ofsolidus_auth_devisehelper methodsMotivation and Context
The method
#redirect_back_or_defaultand the classuser_last_url_storerwill be deprecated in Solidus solidusio/solidus#4533 which would break the current build without these changes. SolidusAuthDevise has similar a helper method to #redirect_back_or_default,#stored_spree_user_location_orwhich was introduced in solidusio/solidus_auth_devise#228 and will be utilized instead.How Has This Been Tested?
The current test suite covers the changes made in the PR