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.

Sign up for GitHub

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

Jump to bottom

When a controller action fails to be authorized, redirect back or default to /unauthorized #3118

Merged
merged 2 commits into from
Jun 26, 2020
Merged

When a controller action fails to be authorized, redirect back or default to /unauthorized #3118

merged 2 commits into from
Jun 26, 2020

Conversation

genarorg
Copy link
Contributor

Description
This change allows for a user to be redirected back whenever a request is not authorized. The current behavior redirects the user to /unauthorized all the time, but this is not ideal when browsing the admin, since you need to manually go back to where you came from.

Changes:

  • Spree::Core::ControllerHelpers::Auth: In self.unauthorized_redirect, use redirect_back with a fallback location.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)

@genarorg genarorg force-pushed the genarorg/redirect-back-when-not-authorized branch from 75d3b36 to a3ac7bd Compare February 22, 2019 01:17
@kennyadsl
Copy link
Member

Thanks! Are you using solidus_auth_devise as well? Because I think that Proc is overridden in that extension as well. Maybe something we should take into account?

@genarorg
Copy link
Contributor Author

ah! thanks for pointing that out. I can make the change in solidus_auth_devise, so that the behavior would be consistent to that of core. How does that sound?

@kennyadsl
Copy link
Member

Sound good to me, please note that for the Admin controller there's another point where this is set, other than what I've previously linked: https://github.com/solidusio/solidus_auth_devise/blob/335991f79557bc36ea08943b9e1fbb151154d020/lib/spree/auth/engine.rb#L37

@kennyadsl kennyadsl mentioned this pull request May 20, 2020
Merged
@kennyadsl
Copy link
Member

If we create a preference for letting people opt-in for this new behavior, we can deprecate the old one and use this preference into solidusio/solidus_auth_devise#192 as well.

@kennyadsl kennyadsl added this to the 2.11 milestone Jun 15, 2020
@kennyadsl kennyadsl force-pushed the genarorg/redirect-back-when-not-authorized branch from a3ac7bd to 4a3ac73 Compare June 16, 2020 15:43
@kennyadsl kennyadsl self-assigned this Jun 18, 2020
@kennyadsl kennyadsl force-pushed the genarorg/redirect-back-when-not-authorized branch from 73058e1 to 9cc67d8 Compare June 18, 2020 12:52
spaghetticode
spaghetticode reviewed Jun 24, 2020
View reviewed changes
Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kennyadsl @genarorg thanks for this PR 👍

There's one small typo to fix, otherwise looks good!

core/lib/spree/app_configuration.rb Outdated
# Whether to try to redirect users back when they try to access
# unauthorized routes, before redirect them to /unauthorized.
# @return [Boolean] (default: +false+)
preference :redirect_back_on_unauthorized, :boolena, default: false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be :boolean

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, and it's working?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works because that information is used for typecasting. In that case, no typecasting would occur as the type would not be recognized.

genarorg and others added 2 commits June 24, 2020 12:08
@genarorg @kennyadsl
When a controller action fails to be autorized, redirect back if a re…
734ce03 
…ferrer is present or redirect to /unauthorized
@kennyadsl
Add a preference to determine if redirect back when unauthorized
34408c2 
With this preference we'll be able to communicate with store
owners that the behavior is changing and they have to manually
change the preference and be sure that their store is still
working as expected.

New app will have the new behavior, with the preference set
to true, while existing app will keep having the old behavior
(false), so they can see the deprecation warning.
@kennyadsl kennyadsl force-pushed the genarorg/redirect-back-when-not-authorized branch from 9cc67d8 to 34408c2 Compare June 24, 2020 10:09
@aldesantis
Copy link
Member

@kennyadsl this is good to merge I think?

@kennyadsl kennyadsl merged commit fa7aa92 into solidusio:master Jun 26, 2020
kennyadsl added a commit to nebulab/solidus_auth_devise that referenced this pull request Oct 6, 2020
@kennyadsl
Try to redirect back on unauthorized accesses
9e149c3 
Only when the `redirect_back_on_unauthorized` preference
exists and is set to true.

This preference has been introduced in core with solidusio/solidus#3118
and we can rely on that preference to drive the behavior change here
as well.

The extra

	if Spree::Config.redirect_back_on_unauthorized == false

check might seem useless but it's needed to avoid printing this
deprecation warning on Solidus versions that still do not have
the preference.

If the Solidus verion used does not have the preference yet, the old
behavior will be preserved.
kennyadsl added a commit to nebulab/solidus_auth_devise that referenced this pull request Oct 6, 2020
@kennyadsl
Try to redirect back on unauthorized accesses
d700b53 
Only when the `redirect_back_on_unauthorized` preference
exists and is set to true.

This preference has been introduced in core with solidusio/solidus#3118
and we can rely on that preference to drive the behavior change here
as well.

The extra

	if Spree::Config.respond_to?(:redirect_back_on_unauthorized)

check might seem useless but it's needed to avoid printing this
deprecation warning on Solidus versions that still do not have
the preference.

If the Solidus verion used does not have the preference yet, the old
behavior will be preserved.
kennyadsl added a commit to nebulab/solidus_auth_devise that referenced this pull request Oct 6, 2020
@kennyadsl
Try to redirect back on unauthorized accesses
4e5a2dc 
Only when the `redirect_back_on_unauthorized` preference
exists and is set to true.

This preference has been introduced in core with solidusio/solidus#3118
and we can rely on that preference to drive the behavior change here
as well.

The extra

	if Spree::Config.respond_to?(:redirect_back_on_unauthorized)

check might seem useless but it's needed to avoid printing this
deprecation warning on Solidus versions that still do not have
the preference.

If the Solidus verion used does not have the preference yet, the old
behavior will be preserved.
kennyadsl added a commit to nebulab/solidus_auth_devise that referenced this pull request Oct 6, 2020
@kennyadsl
Try to redirect back on unauthorized accesses
3a639e0 
Only when the `redirect_back_on_unauthorized` preference
exists and is set to true.

This preference has been introduced in core with solidusio/solidus#3118
and we can rely on that preference to drive the behavior change here
as well.

The extra

	if Spree::Config.respond_to?(:redirect_back_on_unauthorized)

check might seem useless but it's needed to avoid printing this
deprecation warning on Solidus versions that still do not have
the preference.

If the Solidus verion used does not have the preference yet, the old
behavior will be preserved.
kennyadsl added a commit to nebulab/solidus_auth_devise that referenced this pull request Oct 7, 2020
@kennyadsl
Try to redirect back on unauthorized accesses
10df4ee 
Only when the `redirect_back_on_unauthorized` preference
exists and is set to true.

This preference has been introduced in core with solidusio/solidus#3118
and we can rely on that preference to drive the behavior change here
as well.

The extra

	if Spree::Config.respond_to?(:redirect_back_on_unauthorized)

check might seem useless but it's needed to avoid printing this
deprecation warning on Solidus versions that still do not have
the preference.

If the Solidus verion used does not have the preference yet, the old
behavior will be preserved.
kennyadsl added a commit to nebulab/solidus_auth_devise that referenced this pull request Oct 7, 2020
@kennyadsl
Try to redirect back on unauthorized accesses
ff57ff5 
Only when the `redirect_back_on_unauthorized` preference
exists and is set to true.

This preference has been introduced in core with solidusio/solidus#3118
and we can rely on that preference to drive the behavior change here
as well.

The extra

	if Spree::Config.respond_to?(:redirect_back_on_unauthorized)

check might seem useless but it's needed to avoid printing this
deprecation warning on Solidus versions that still do not have
the preference.

If the Solidus verion used does not have the preference yet, the old
behavior will be preserved.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennyadsl kennyadsl kennyadsl left review comments

@spaghetticode spaghetticode spaghetticode approved these changes

@jarednorman jarednorman jarednorman approved these changes

@aldesantis aldesantis Awaiting requested review from aldesantis

@tvdeyen tvdeyen Awaiting requested review from tvdeyen

Assignees

@kennyadsl kennyadsl

Labels
None yet
Projects
None yet
Milestone
2.11
Development

Successfully merging this pull request may close these issues.
5 participants
@genarorg @kennyadsl @aldesantis @spaghetticode @jarednorman