solidus_social icon indicating copy to clipboard operation
solidus_social copied to clipboard

Utilize Devise Location helpers

Open cpfergus1 opened this issue 3 years ago • 7 comments

Description Removes #redirect_back_or_default in favor of solidus_auth_devise helper methods

Motivation and Context The method #redirect_back_or_default and the class user_last_url_storer will be deprecated in Solidus https://github.com/solidusio/solidus/pull/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_or which was introduced in https://github.com/solidusio/solidus_auth_devise/pull/228 and will be utilized instead.

How Has This Been Tested? The current test suite covers the changes made in the PR

cpfergus1 avatar Sep 13 '22 17:09 cpfergus1

@cpfergus1 @kennyadsl

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?

fthobe avatar Jan 08 '25 21:01 fthobe

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

elia avatar Jan 13 '25 10:01 elia

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?

fthobe avatar Jan 13 '25 11:01 fthobe

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.

kennyadsl avatar Jan 13 '25 11:01 kennyadsl

@rahulsingh321 this should work out of the box, please integrate into the current PR and test it.

fthobe avatar Jan 13 '25 15:01 fthobe

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

rahulsingh321 avatar Jan 13 '25 18:01 rahulsingh321

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

fthobe avatar Jan 20 '25 09:01 fthobe