reactioncommerce / reaction

Mailchimp Open Commerce is an API-first, headless commerce platform built using Node.js, React, GraphQL. Deployed via Docker and Kubernetes.
https://mailchimp.com/developer/open-commerce/
GNU General Public License v3.0
12.34k stars 2.17k forks source link

Surchages query missing permission validation #6634

Open tedraykov opened 2 years ago

tedraykov commented 2 years ago

Prerequisites

Issue Description

The surcharges query in api-plugin-surcharges is missing the read permission validation. https://github.com/reactioncommerce/reaction/blob/b11e47f05a3d3042b76385e992960dfafb36a286/packages/api-plugin-surcharges/src/queries/surcharges.js#L15

This means every user can query the surcharges regardless the permission they have.

Possible Solution

An example of a query that has the desired permission validation. https://github.com/reactioncommerce/reaction/blob/b11e47f05a3d3042b76385e992960dfafb36a286/packages/api-plugin-accounts/src/queries/groups.js#L14

Vinitvh commented 2 years ago

Hi @tedraykov, I would like to work on this.

tedraykov commented 2 years ago

You can take onto the issue. In order to add a new role, you need to add a new migration in the authorization plugin as so: https://github.com/reactioncommerce/reaction/blob/trunk/packages/api-plugin-authorization-simple/migrations/3.js

You also have to update the migration in the following repo: https://github.com/reactioncommerce/api-migrations/blob/trunk/migrator.config.js

Vinitvh commented 2 years ago

@tedraykov Need some help here. I'm getting this error while starting dev server with pnpm run start:dev. ERROR Reaction: Module "file:///home/vinit/Desktop/opensource/reaction/apps/reaction/plugins.json" needs an import assertion of type "json". I tried const plugins = await importPluginsJSONFile("../plugins.json", { assert: { type: "json" }, }); but this doesn't seem to work.

Vinitvh commented 2 years ago

@tedraykov Never mind, I have fixed the above error. But there seems to be a new one Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './collectionIndex.js' is not defined by "exports" in /home/vinit/Desktop/opensource/reaction/packages/api-core/node_modules/@reactioncommerce/api-utils/package.json imported from /home/vinit/Desktop/opensource/reaction/packages/api-core/src/ReactionAPICore.js

Vinitvh commented 2 years ago

@tedraykov Do I have to write any additional tests? I ran npm run test and it seems there were no failed tests with my changes.

brent-hoover commented 2 years ago

You should write two additional tests. One that it works when you have the correct permissions (which might be covered by existing tests if you changed them) and one that it fails when you don't have the correct permissions

Vinitvh commented 2 years ago

@zenweasel Okay, so if I got this right, It's for unauthenticated and authenticated with read permissions just like groups tests, right?

test("authenticated with `reaction:legacy:groups/read` permissions, gets all groups", async () => {
  await testApp.setLoggedInUser(mockAdminAccount);

  const nodes = allGroups.map(groupMongoSchemaToGraphQL);

  // Default sortBy is createdAt ascending
  nodes.sort((item1, item2) => {
    if (item1.createdAt > item2.createdAt) return 1;
    if (item1.createdAt < item2.createdAt) return -1;
    return 0;
  });

  const result = await groupsQuery({ shopId: opaqueShopId });
  expect(result).toEqual({
    groups: {
      nodes
    }
  });

  await testApp.clearLoggedInUser();
});
brent-hoover commented 2 years ago

@tedraykov Can we confirm that this query without permissions is not used anywhere in the storefront by customer-facing code

@Vinitvh Yes that seems right. Not sure if this particular query returns a cursor or not though

tedraykov commented 2 years ago

I checked that. The only public access to the surcharges are trough the cart, but the cart resolvers does not use this query but rather directly fetch the data from the database.