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

Surchages query missing permission validation #6634

Open
3 tasks done
tedraykov opened this issue Nov 7, 2022 · 9 comments · May be fixed by #6716
Open
3 tasks done

Surchages query missing permission validation #6634

tedraykov opened this issue Nov 7, 2022 · 9 comments · May be fixed by #6716
Assignees
Labels
bug For issues that describe a defect or regression in the released software good first issue For issues that a new contributor could likely submit a pull request for without needing much help needs triage For issues that are awaiting triage by the core development team

Comments

@tedraykov
Copy link
Collaborator

tedraykov commented Nov 7, 2022

Prerequisites

  • Are you running the latest version?
  • Are you able to consistently reproduce the issue?
  • Did you search the issue queue for existing issue? Search issues

Issue Description

The surcharges query in api-plugin-surcharges is missing the read permission validation.

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.

await context.validatePermissions("reaction:legacy:groups", "read", { shopId });

@tedraykov tedraykov added bug For issues that describe a defect or regression in the released software needs triage For issues that are awaiting triage by the core development team good first issue For issues that a new contributor could likely submit a pull request for without needing much help labels Nov 7, 2022
@Vinitvh
Copy link

Vinitvh commented Nov 7, 2022

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

@tedraykov
Copy link
Collaborator Author

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
Copy link

Vinitvh commented Nov 8, 2022

@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
Copy link

Vinitvh commented Nov 8, 2022

@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
Copy link

Vinitvh commented Nov 9, 2022

@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.

@zenweasel
Copy link
Collaborator

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
Copy link

Vinitvh commented Nov 9, 2022

@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();
});

@zenweasel
Copy link
Collaborator

@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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For issues that describe a defect or regression in the released software good first issue For issues that a new contributor could likely submit a pull request for without needing much help needs triage For issues that are awaiting triage by the core development team
Projects
None yet
4 participants