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

Add permission check for admins updating user passwords #3394

Merged
merged 1 commit into from Nov 6, 2019

Conversation

@JDutil
Copy link
Contributor

JDutil commented Oct 23, 2019

For security purposes administrators should not be able to set a
users password. Only the accounts owner should be able to directly set
their password. administrators should only have the ability to send a
password reset email to the account owner. Otherwise someone working in
customer service or another role could take over a users account in
order to make illegal purchases with their stored credit card
information.

In order to maintain backwards compatibility, and leave more power in
control of the store owner this will leave the current admin role
behavior the same, but anyone creating custom roles will no longer
be able to update passwords unless they explicitly add a change
password permission.

Fixes #438

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)
@@ -58,21 +58,4 @@
<% end %>

</div>

<div data-hook="admin_user_form_password_fields" class="col-6">

This comment has been minimized.

Copy link
@JDutil

JDutil Oct 23, 2019

Author Contributor

I'm not sure if we should delete this div with the form fields or not. There maybe stores using this data-hook so I'm thinking that perhaps we should leave it in for backwards compatibility. Although I'm also annoyed the data-hook would have a bad name.

This comment has been minimized.

Copy link
@kennyadsl

kennyadsl Oct 28, 2019

Member

If I got it correctly, I think this div should go into the if statement. I think it will still be evaluated even if the if statement is false, since Deface should do its magic before the erb is rendered.

This comment has been minimized.

Copy link
@JDutil

JDutil Oct 29, 2019

Author Contributor

My thought here was that someone may have a deface customization that's adding non-password related fields here simply because it would be a convenient place to add more user fields. I'm fine putting the div into the conditional statement as well, but I think that may cause customizations to be hidden unintentionally if people are using this data hook for non-password stuff.

This comment has been minimized.

Copy link
@kennyadsl

kennyadsl Nov 6, 2019

Member

I think that if someone is adding some non-password related fields into the password dedicated div it's ok to make this change, we can't handle every scenario. The problem is if someone is using this hook in the insert_after or insert_before deface action, which is legit and the added parts will be hidden at that point. :(

That said, your solution is probably the safest one in terms of backward compatibility.

@JDutil JDutil force-pushed the JDutil:fixes-438 branch 5 times, most recently from d7e3ca2 to 6fc62e3 Oct 23, 2019
Copy link
Member

tvdeyen left a comment

I don't agree with removing the feature completely. Can we do this with permissions instead?

At least we should have a button that allows admins to send a password reset mail.

@JDutil

This comment has been minimized.

Copy link
Contributor Author

JDutil commented Oct 24, 2019

@tvdeyen a button to send a password reset email was added to solidusio/solidus_auth_devise#146 I can modify this to work based on permissions though.

@JDutil JDutil force-pushed the JDutil:fixes-438 branch 2 times, most recently from 1b006f3 to edd342a Oct 24, 2019
@JDutil JDutil requested a review from tvdeyen Oct 24, 2019
@JDutil JDutil changed the title Remove ability to set user passwords Add permission check for admins updating user passwords Oct 24, 2019
@JDutil

This comment has been minimized.

Copy link
Contributor Author

JDutil commented Oct 24, 2019

@tvdeyen updated to be permission based 👍

Copy link
Member

tvdeyen left a comment

Awesome. Thanks.

Note for further PRs. I think it makes sense to ship Solidus with a Customer Support role that must not change

  • users passwords
  • Everything thats under settings
  • store settings

But this is 🤘🏻

@JDutil

This comment has been minimized.

Copy link
Contributor Author

JDutil commented Oct 24, 2019

@tvdeyen Yeah I also think it'd be great to start shipping Solidus with some sort of customer service role. Giving every "admin" super user permissions is a bit much by default. That would also help for testing many permission related features.

@tvdeyen tvdeyen mentioned this pull request Oct 25, 2019
0 of 6 tasks complete
@tvdeyen

This comment has been minimized.

Copy link
Member

tvdeyen commented Oct 25, 2019

@JDutil agreed, I made #3404

Copy link
Member

kennyadsl left a comment

Left a quick comment about Rubocop, but I'm 👍with the PR's content, thanks!

backend/spec/features/admin/users_spec.rb Outdated Show resolved Hide resolved
For security purposes administrators should not be able to set a
users password. Only the accounts owner should be able to directly set
their password. administrators should only have the ability to send a
password reset email to the account owner. Otherwise someone working in
customer service or another role could take over a users account in
order to make illegal purchases with their stored credit card
information.

In order to maintain backwards compatibility, and leave more power in
control of the store owner this will leave the current admin role
behavior the same, but anyone creating custom roles will no longer
be able to update passwords unless they explicitly add a change
password permission.
@JDutil JDutil force-pushed the JDutil:fixes-438 branch from edd342a to 253641e Oct 29, 2019
@JDutil JDutil requested a review from kennyadsl Oct 29, 2019
Copy link
Member

kennyadsl left a comment

Thanks!

@kennyadsl kennyadsl merged commit 4cd6736 into solidusio:master Nov 6, 2019
15 checks passed
15 checks passed
Header rules - solidus-docs No header rules processed
Details
Header rules - solidus-guides No header rules processed
Details
Pages changed - solidus-docs 308 new files uploaded
Details
Pages changed - solidus-guides All files already uploaded
Details
Redirect rules - solidus-docs No redirect rules processed
Details
Redirect rules - solidus-guides No redirect rules processed
Details
Hound No violations found. Woof!
Mixed content - solidus-docs No mixed content detected
Details
Mixed content - solidus-guides No mixed content detected
Details
ci/circleci: mysql Your tests passed on CircleCI!
Details
ci/circleci: mysql_rails52 Your tests passed on CircleCI!
Details
ci/circleci: postgres Your tests passed on CircleCI!
Details
ci/circleci: postgres_rails52 Your tests passed on CircleCI!
Details
netlify/solidus-docs/deploy-preview Deploy preview ready!
Details
netlify/solidus-guides/deploy-preview Deploy preview ready!
Details
@JDutil JDutil deleted the JDutil:fixes-438 branch Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.