Rewrite addAccountToGroup to not call through to Meteor method

[aldeed]

Resolves #5336
Impact: minor
Type: refactor

Changes

• Rewrites addAccountToGroup GraphQL resolver to not use context.callMeteorMethod. Adds addAccountToGroup internal mutation function and calls that.
• Updates Accounts page in Reaction Admin to use GraphQL mutations for remove/change account groups
• Removes some browser-side permission checks. It's possible you'll now see buttons on Accounts page even if you don't have permission to use them. You'll get an error if you try.
• Removes the "group/addUser" Meteor method
• It's now possible to have multiple shop owners, which makes it easier to transition ownership.

Breaking changes

Removes the "group/addUser" Meteor method

Testing

• Code is covered by integration tests, but you can manually test addAccountToGroup mutation.
• Verify that the Accounts page in Reaction Admin works as expected.

[kieckhafer]

I just want to clarify best practices on how to import GraphQL mutations inside other files.

You are creating a mutation in a .graphql file, that's imported into a js file: https://github.com/reactioncommerce/reaction/pull/5431/files#diff-e5c13dca4cfed4d0f3a7926fa30d36f3

The way I've been doing it is using a js file to do basically the same thing: https://github.com/reactioncommerce/reaction/blob/master/imports/plugins/core/orders/client/graphql/queries/orderByReferenceId.js

I don't know which one is "correct", or if either of these are seen as "correct" and one as "incorrect", or if they are both OK, we just do things in multiple ways and we should probably have a consistent style.

[kieckhafer]

Just making a note to refactor this without simpleGraphQLClient in the future.

[aldeed]

Yeah I didn't want to go too far down the rabbit hole.

[kieckhafer]

Looks familiar 😁

[kieckhafer]

Do we need to importlodash here for this?

Could we do something like groupPermissions.filter((permission) => !user.roles[shopId].includes(permission)) instead?

I know at one point our policy was "lodash only if needed", if that's changed, then difference is cleaner looking.

[aldeed]

No, lodash isn't needed.

[kieckhafer]

These were some of the new rules added when we did the recommended import of node lint rules.

We should discuss if we want to keep these rules in our eslint config, I know @mikemurray has run across these lint errors a few times too.

[kieckhafer]

@aldeed

Removes some browser-side permission checks. It's possible you'll now see buttons on Accounts page even if you don't have permission to use them. You'll get an error if you try.

Is this what we'll want to do going forward? In my recent updates, especially in the orders panel, I've done the opposite and purposely removed UI elements like buttons if a user doesn't have permission.

[kieckhafer]

When logged in as an owner (admin@localhost), I invited a new user to be a shop admin, and also created a new group. These worked as expected.

I then tried to move the new user into the new operator group I had just created, and I get the following error (as well as a toast error)

ERROR Reaction: Access Denied (errorId=cjz1w9ytu0002j6pp4jltagoq)
  path: [
    "addAccountToGroup"
  ]
  --
  stack: ReactionError: Access Denied
      at Promise.asyncApply (imports/plugins/core/accounts/server/no-meteor/mutations/addAccountToGroup.js:46:11)
      at /home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/fiber_pool.js:43:40
   => awaited here:
      at Function.Promise.await (/home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/promise_server.js:56:12)
      at Promise.asyncApply (imports/plugins/core/accounts/server/no-meteor/resolvers/Mutation/addAccountToGroup.js:23:14)
      at /home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/fiber_pool.js:43:40

[aldeed]

@kieckhafer I have always been using .graphql for the client gql unless it was tiny and could be gql template in the same file. My thought was that it's easier that way since editors support formatting, linting, etc. in .graphql files. That being said, I believe I now have an editor extension that does the same formatting and highlighting in gql tags, so it may not matter. Using a js file has the benefit of working without Babel or Webpack. It should probably be discussed more widely and document a decision. In the meantime, it doesn't bother me to have two different ways.

Regarding client side permissions, I do think we should have a way for people to have UI permissions, but it will eventually need to be a completely separate permission system. We're working toward the API permissions being a completely separate auth layer, so when that happens, there will be no way to find out a user's auth permissions in browser code. In the meantime it seemed easiest to just remove such checks, but maybe we should prioritize building a simple UI permissions system? It could be as simple as an array of strings (similar to roles) on an account, which would correspond with admin UI areas they're allowed to see.

Regarding the error you got, what permissions are on the new group? It should only show access denied if you are missing ANY permission that you're trying to grant to another person. I don't know if that's the best way to do it, but that's how it's been. The only difference is that I got rid of the logic that said "owner can do anything" because I assumed "owner" would have all the permissions they're trying to grant. Maybe that isn't true?

[kieckhafer]

@aldeed I think the "Owner should be able to do anything" might be it. I am an owner trying to move the user from Shop Manager to Operator, which is a new group I made, with less permissions than the user had before.

[kieckhafer]

Adding back an "owner override" fixes the issues I was seeing.