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): Add permission checks to template method and publication #3606

Merged
merged 4 commits into from
Feb 22, 2018

Conversation

spencern
Copy link
Contributor

Adds permission checks to the template/email/update server method.

I've tested and verified that as an admin you can still view and modify email templates.
I've also tested and verified that users without the reaction-templates role for the primary shop cannot view or subscribe to the templates, and cannot modify them.

We currently do not use shop specific templates, the default marketplace install of Reaction disables template editing for shops and uses the primary shop's templates for all emails. I've added this code to be compatible with marketplace shops, but it's not enabled right now anyway, and there's not a great way to test that.

To test:

  1. Verify that as the admin user or user with the reaction-templates role you can still view and modify templates.
  2. As an unauthenticated user. (incognito window works)
  3. Attempt to subscribe to the templates publication, verify that you are not sent any template documents.
  4. Attempt to modify a template by calling the template/email/update method from the client console. Verify that you are not able to do so successfully.

image

Copy link
Contributor

@aaronjudd aaronjudd left a comment

Choose a reason for hiding this comment

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

Problem with this, is that if you cannot subscribe to Templates as guest, you cannot load the PDP page. Templates are not just email templates, but layout templates as well.

basic_reaction_product_and_slack_-_reaction_commerce

@spencern
Copy link
Contributor Author

@aaronjudd good catch. Pretty sloppy of me not to remember that we use templates for the PDP as well. Fixed by permitting all users to subscribe to templates (previous functionality) but keeping the role check in the update method.

return Templates.update({
_id: templateId,
type: "email"
type: "email",
shopId: shopId // Ensure that the template we're attempting to update is owned by the active shop.
Copy link
Collaborator

Choose a reason for hiding this comment

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

💯

@pmn4
Copy link
Collaborator

pmn4 commented Jan 29, 2018

lgtm

@brent-hoover
Copy link
Collaborator

As a shop admin I am able to execute the method for templates that are not mine but the method uses the active shop ID so I am prevented from actually doing anything harmful.

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

Tested. Verified fixed.

@spencern spencern changed the base branch from master to release-1.8.0 February 22, 2018 02:47
@spencern spencern dismissed aaronjudd’s stale review February 22, 2018 02:49

Approved by @zenweasel and @jshimko

@spencern spencern merged commit fd05d4a into release-1.8.0 Feb 22, 2018
@spencern spencern deleted the fix-spencern-email-template-methods branch February 22, 2018 02:57
@spencern spencern mentioned this pull request Feb 22, 2018
@spencern spencern mentioned this pull request Mar 9, 2018
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.

5 participants