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

Fix order create permissions #4261

Conversation

spaghetticode
Copy link
Member

Description
Ref #4248

This PR aims at fixing permission checks for showing the new order button in the admin area.

We're currently using the manage permission, which seems to be a bit too much for just showing a button, given that manage is the maximum level of permission for a resource. Checking for the create permission would be much more appropriate.

In order to achieve the goal, the DefaultCustomer permission rule for creating orders has been made stricter, so by default users can create a order only when:

  • the order includes the email field (guest checkout);
  • the order they're creating is for the same user;
  • the order guest token is the same as the passed token;

A few specs were updated, as a consequence.

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)

Since `DefaultCustomer` is the default role that's mixed into every
ability, we basically had no restrictions in regards to creating
orders so far.

This change will allow order creation when:

* the order user is the same as the ability user;
* the order user and the ability user are both missing;
* order.email is present (guest checkout order);
* the order token matches the API token
We've been using `manage` because `can? :create` was always true
because of very lax `DefaultCustomer` permission definitions.

The Order create definition was restricted with 50393da, so now
we can start using `create` again.
This permission check was introduced in order to avoid showing
a link to the form for creating a new order from a partial that
uses the `can? :create, resource` permission check.
@spaghetticode spaghetticode self-assigned this Feb 9, 2022
@spaghetticode spaghetticode marked this pull request as ready for review February 9, 2022 16:10
@spaghetticode
Copy link
Member Author

Following @kennyadsl's suggestion, I tested solidus_auth_devise locally with this branch, and all specs are green.

Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

Thanks, @spaghetticode!

@waiting-for-dev waiting-for-dev merged commit fde58ec into solidusio:master Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants