From 91f5b14daa8334d58d5eaa1818a424ff91b01fff Mon Sep 17 00:00:00 2001 From: Eric Dobbertin Date: Tue, 25 Feb 2020 19:16:00 -0600 Subject: [PATCH] feat: remove group.permissions code Also cleanup of createAccountGroup and updateAccountGroup mutations Signed-off-by: Eric Dobbertin --- .../account/mutations/createAccountGroup.js | 59 ++++++++------ .../mutations/createAccountGroup.test.js | 54 +++++-------- .../mutations/createAuthGroupsForShop.js | 38 ++------- .../account/mutations/updateAccountGroup.js | 77 ++++++------------- .../account/schemas/group.graphql | 27 +++---- src/core-services/account/simpleSchemas.js | 32 +++----- .../util/ensureAccountsManagerGroup.js | 20 ++--- .../account/util/ensureSystemManagerGroup.js | 20 ++--- .../createAccountGroup.test.js | 4 +- .../updateAccountGroup.test.js.snap | 57 -------------- 10 files changed, 118 insertions(+), 270 deletions(-) delete mode 100644 tests/integration/api/mutations/updateAccountGroup/__snapshots__/updateAccountGroup.test.js.snap diff --git a/src/core-services/account/mutations/createAccountGroup.js b/src/core-services/account/mutations/createAccountGroup.js index b0d0cf7a788..2801ae575af 100644 --- a/src/core-services/account/mutations/createAccountGroup.js +++ b/src/core-services/account/mutations/createAccountGroup.js @@ -1,4 +1,5 @@ import Logger from "@reactioncommerce/logger"; +import Random from "@reactioncommerce/random"; import ReactionError from "@reactioncommerce/reaction-error"; import getSlug from "@reactioncommerce/api-utils/getSlug.js"; @@ -10,57 +11,65 @@ import getSlug from "@reactioncommerce/api-utils/getSlug.js"; * It creates permission group for a given shop with passed in roles * This method also effectively creates a role in reaction authorization service with the same * name as the supplied group if kafka connect mongo has been configured on the mongo db - * @param {object} context - The GraphQL context - * @param {String} context.shopId - id of the shop the group belongs to - * @param {object} input - The input supplied from GraphQL - * @param {Object} input.groupData - info about group to create - * @param {String} input.groupData.name - name of the group to be created - * @param {String} input.groupData.description - Optional description of the group to be created - * @param {Array} input.groupData.permissions - permissions to assign to the group being created - * @param {Array} input.groupData.members - members of the - * @returns {Object} - `object.status` of 200 on success or Error object on failure + * @param {Object} context - The GraphQL context + * @param {Object} input - The input supplied from GraphQL + * @param {String} input.shopId - ID of the shop the group belongs to + * @param {Object} input.group - info about group to create + * @param {String} input.group.name - name of the group to be created + * @param {String} input.group.description - Optional description of the group to be created + * @returns {Object} An object where `group` is the newly created group. */ export default async function createAccountGroup(context, input) { - const { group, shopId } = input; - let newlyCreatedGroup; - const { Groups } = context.collections; + const { group, shopId = null } = input; + const { + accountId, + appEvents, + collections: { Groups }, + simpleSchemas: { Group }, + userId + } = context; // we are limiting group method actions to only users within the account managers role await context.validatePermissions("reaction:legacy:groups", "create", { shopId }); const nowDate = new Date(); - const newGroupData = Object.assign({}, group, { - slug: getSlug(group.name), - shopId, + const newGroup = Object.assign({}, group, { + _id: Random.id(), createdAt: nowDate, + createdBy: accountId, + shopId, + slug: group.slug || getSlug(group.name), updatedAt: nowDate }); - // TODO: Remove when we move away from legacy permission verification - if (!newGroupData.permissions) { - newGroupData.permissions = []; - } - // ensure one group type per shop - const groupExists = await Groups.findOne({ slug: newGroupData.slug, shopId }); + const groupExists = await Groups.findOne({ slug: newGroup.slug, shopId }); if (groupExists) { throw new ReactionError("conflict", "Group already exist for this shop"); } + Group.validate(newGroup); + + Logger.debug(`creating group ${newGroup.slug} for shop ${shopId}`); + try { /** Kafka connect mongo should be listening for insert events - and should place the newly created group on the kakfa groups topic. + and should place the newly created group on the Kafka groups topic. reaction authorization listens on the topic and creates role (group) in reaction authorization */ - const result = await Groups.insertOne(newGroupData); - newlyCreatedGroup = result.ops ? result.ops[0] : {}; + await Groups.insertOne(newGroup); } catch (error) { Logger.error(error); throw new ReactionError("invalid-parameter", "Bad request"); } - return { group: newlyCreatedGroup }; + await appEvents.emit("afterAccountGroupCreate", { + createdBy: userId, + group: newGroup + }); + + return { group: newGroup }; } diff --git a/src/core-services/account/mutations/createAccountGroup.test.js b/src/core-services/account/mutations/createAccountGroup.test.js index 0777e93cad6..4fac4150e57 100644 --- a/src/core-services/account/mutations/createAccountGroup.test.js +++ b/src/core-services/account/mutations/createAccountGroup.test.js @@ -1,11 +1,12 @@ import mockContext from "@reactioncommerce/api-utils/tests/mockContext.js"; -import getSlug from "@reactioncommerce/api-utils/getSlug.js"; import ReactionError from "@reactioncommerce/reaction-error"; +import { Group } from "../simpleSchemas.js"; import createAccountGroup from "./createAccountGroup.js"; mockContext.validatePermissions = jest.fn("validatePermissions"); mockContext.collections.Groups.insert = jest.fn("collections.Groups.insertOne"); mockContext.collections.Groups.findOne = jest.fn("collections.Groups.findOne"); +mockContext.simpleSchemas = { Group }; test("should create a group for the shop", async () => { const groupName = "test group"; @@ -17,37 +18,27 @@ test("should create a group for the shop", async () => { const input = { group, shopId }; - const fakeResult = { - _id: "test-group-id", - name: "test group", - slug: "test group", - permissions: [], - shopId: "test-shop-id" - }; - - - const insertOneRes = { - ops: [fakeResult] - }; - mockContext.collections.Groups.insertOne.mockReturnValueOnce(Promise.resolve(insertOneRes)); + mockContext.collections.Groups.insertOne.mockReturnValueOnce(Promise.resolve(undefined)); mockContext.collections.Groups.findOne.mockReturnValueOnce(Promise.resolve(undefined)); - mockContext.validatePermissions.mockReturnValueOnce(Promise.resolve(undefined)); const result = await createAccountGroup(mockContext, input); - const expected = Object.assign({}, { group: fakeResult }); - await expect(result).toEqual(expected); - expect(mockContext.validatePermissions).toHaveBeenCalledWith("reaction:legacy:groups", "create", { shopId }); - expect(mockContext.collections.Groups.findOne).toHaveBeenNthCalledWith(1, { slug: "test-group", shopId }); - expect(mockContext.collections.Groups.insertOne).toHaveBeenCalledWith({ + const expectedGroup = { + _id: jasmine.any(String), + createdAt: jasmine.any(Date), + createdBy: "FAKE_ACCOUNT_ID", name: "test group", - slug: getSlug("test group"), - permissions: [], shopId: "test-shop-id", - updatedAt: expect.any(Date), - createdAt: expect.any(Date) - }); + slug: "test group", + updatedAt: jasmine.any(Date) + }; + + await expect(result).toEqual({ group: expectedGroup }); + + expect(mockContext.validatePermissions).toHaveBeenCalledWith("reaction:legacy:groups", "create", { shopId }); + expect(mockContext.collections.Groups.findOne).toHaveBeenNthCalledWith(1, { slug: "test group", shopId }); + expect(mockContext.collections.Groups.insertOne).toHaveBeenCalledWith(expectedGroup); }); test("should throw if group already exists", async () => { @@ -64,20 +55,11 @@ test("should throw if group already exists", async () => { _id: "test-group-id", name: "test group", slug: "test group", - permissions: [ - "dashboard" - ], shopId: "test-shop-id" }; - const insertOneRes = { - ops: [fakeResult] - }; - mockContext.collections.Groups.insertOne.mockReturnValueOnce(Promise.resolve(insertOneRes)); - mockContext.collections.Groups.findOne - .mockReturnValueOnce(Promise.resolve(fakeResult)) - .mockReturnValueOnce(Promise.resolve(undefined)); - + mockContext.collections.Groups.insertOne.mockReturnValueOnce(Promise.resolve(undefined)); + mockContext.collections.Groups.findOne.mockReturnValueOnce(Promise.resolve(fakeResult)); mockContext.validatePermissions.mockReturnValueOnce(Promise.resolve(undefined)); await expect(createAccountGroup(mockContext, input)).rejects.toThrow(new ReactionError("conflict", "Group already exist for this shop")); diff --git a/src/core-services/account/mutations/createAuthGroupsForShop.js b/src/core-services/account/mutations/createAuthGroupsForShop.js index 8e5a05df21c..58410eabbeb 100644 --- a/src/core-services/account/mutations/createAuthGroupsForShop.js +++ b/src/core-services/account/mutations/createAuthGroupsForShop.js @@ -1,10 +1,3 @@ -import Logger from "@reactioncommerce/logger"; -import Random from "@reactioncommerce/random"; -import { - defaultShopOwnerRoles, - defaultShopManagerRoles -} from "../util/defaultRoles.js"; - /** * @name createAuthGroupsForShop * @method @@ -15,31 +8,16 @@ import { * @returns {undefined} */ export default async function createAuthGroupsForShop(context, shopId) { - const { - collections: { - Groups, - Shops - } - } = context; - - const roles = { - "shop manager": defaultShopManagerRoles, - "owner": defaultShopOwnerRoles - }; - - const primaryShop = await Shops.findOne({ shopType: "primary" }); + const { collections: { Groups } } = context; - const promises = Object.keys(roles).map(async (slug) => { + const promises = ["shop manager", "owner"].map(async (slug) => { const existingGroup = await Groups.findOne({ shopId, slug }); - if (!existingGroup) { // create group only if it doesn't exist before - Logger.debug(`creating group ${slug} for shop ${shopId}`); - // get roles from the default groups of the primary shop; we try to use this first before using default roles - const primaryShopGroup = primaryShop ? await Groups.findOne({ shopId: primaryShop._id, slug }) : null; - await Groups.insertOne({ - _id: Random.id(), - name: slug, - slug, - permissions: (primaryShopGroup && primaryShopGroup.permissions) || roles[slug], + if (!existingGroup) { + await context.mutations.createAccountGroup(context.getInternalContext(), { + group: { + name: slug, + slug + }, shopId }); } diff --git a/src/core-services/account/mutations/updateAccountGroup.js b/src/core-services/account/mutations/updateAccountGroup.js index f7cb5798f1c..ef18813f1e1 100644 --- a/src/core-services/account/mutations/updateAccountGroup.js +++ b/src/core-services/account/mutations/updateAccountGroup.js @@ -1,16 +1,4 @@ import ReactionError from "@reactioncommerce/reaction-error"; -import SimpleSchema from "simpl-schema"; -import getSlug from "@reactioncommerce/api-utils/getSlug.js"; -import defaultAccountGroups from "../util/defaultAccountGroups.js"; - -const inputSchema = new SimpleSchema({ - "slug": { type: String, optional: true }, - "name": { type: String, optional: true }, - "description": { type: String, optional: true }, - "permissions": { type: Array, optional: true }, - "permissions.$": String, - "updatedAt": Date -}); /** * @name group/updateAccountGroup @@ -21,70 +9,49 @@ const inputSchema = new SimpleSchema({ * This method also effectively updates a role in reaction authorization service with the same * name as the supplied group if kafka connect mongo has been configured on the mongo db * @param {object} context - The GraphQL context - * @param {String} context.shopId - id of the shop the group belongs to * @param {object} input - The input supplied from GraphQL - * @param {Object} input.groupData - info about group to updated - * @param {String} input.groupData.name - name of the group to be updated - * @param {String} input.groupData.description - Optional description of the group to be updated - * @param {Array} input.groupData.permissions - permissions to assign to the group being updated - * @param {Array} input.groupData.members - members of the - * @returns {Object} - `object.status` of 200 on success or Error object on failure + * @param {String} input.shopId - id of the shop the group belongs to + * @param {Object} input.group - info about group to updated + * @param {String} input.group.name - name of the group to be updated + * @param {String} input.group.description - Optional description of the group to be updated + * @returns {Object} Updated group */ export default async function updateAccountGroup(context, input) { const { group, groupId, shopId } = input; - const { appEvents, user } = context; - const { Groups } = context.collections; + const { + appEvents, + collections: { Groups }, + simpleSchemas: { Group }, + user + } = context; // we are limiting group method actions to only users within the account managers role await context.validatePermissions(`reaction:legacy:groups:${groupId}`, "update", { shopId }); // Ensure group exists before proceeding - const existingGroup = await Groups.findOne({ _id: groupId }); - + const existingGroup = await Groups.findOne({ _id: groupId, shopId }); if (!existingGroup) { throw new ReactionError("not-found", `Group with ID (${groupId}) doesn't exist`); } - const updateGroupData = { - updatedAt: new Date() + const modifier = { + $set: { + ...group, + updatedAt: new Date() + } }; - // Update the name if provided - if (group.name) { - updateGroupData.name = group.name; - } - - // Prevent updating the slug of the default groups. - // For example, changing the slug of the shop manager group could cause various features of the application to stop working as intended. - if (defaultAccountGroups.includes(existingGroup.slug) && group.slug && group.slug !== existingGroup.slug) { - throw new ReactionError("access-denied", `Field 'slug' cannot be updated for default group with ID (${groupId}) and name (${existingGroup.name}).`); - } else if (group.slug) { // Update the slug if available for other groups - updateGroupData.slug = getSlug(group.slug); - } - - // Update description - if (group.description) { - updateGroupData.description = group.description; - } - - // Update the permissions on the group and any user in those groups - if (Array.isArray(group.permissions)) { - updateGroupData.permissions = group.permissions; - } - // Validate final group object - inputSchema.validate(updateGroupData); + Group.validate(modifier, { modifier: true }); /** Kafka connect mongo should be listening for update events - and should place the updated group on the kakfa groups topic. + and should place the updated group on the Kafka groups topic. reaction authorization listens on the topic and updates role (group) in reaction authorization */ const { value: updatedGroup } = await Groups.findOneAndUpdate( { _id: groupId }, - { - $set: updateGroupData - }, + modifier, { returnOriginal: false } @@ -93,9 +60,9 @@ export default async function updateAccountGroup(context, input) { if (!updatedGroup) throw new ReactionError("server-error", `Unable to update Group ${group._id}`); await appEvents.emit("afterAccountGroupUpdate", { - account: updatedGroup, + group: updatedGroup, updatedBy: user._id, - updatedFields: Object.keys(updateGroupData) + updatedFields: Object.keys(modifier.$set) }); return updatedGroup; diff --git a/src/core-services/account/schemas/group.graphql b/src/core-services/account/schemas/group.graphql index dc042b397ee..71a2fca7316 100644 --- a/src/core-services/account/schemas/group.graphql +++ b/src/core-services/account/schemas/group.graphql @@ -21,14 +21,11 @@ input GroupInput { "A unique name for the group" name: String! - "A list of the account permissions implied by membership in this group" - permissions: [String] - "A unique URL-safe string representing this group" - slug: String! + slug: String } -"A type definition for updating account groups" +"Fields to update for an existing account group" input UpdateGroupInput { "A free text description of this group" description: String @@ -36,9 +33,6 @@ input UpdateGroupInput { "A unique name for the group" name: String - "A list of the account permissions implied by membership in this group" - permissions: [String] - "A unique URL-safe string representing this group" slug: String } @@ -52,7 +46,7 @@ input CreateAccountGroupInput { group: GroupInput! "The ID of the shop this group belongs to" - shopId: ID! + shopId: ID } "The details for updating a group" @@ -67,7 +61,7 @@ input UpdateAccountGroupInput { groupId: ID! "The ID of the shop this group belongs to" - shopId: ID! + shopId: ID } "The details for removing a group" @@ -79,7 +73,7 @@ input RemoveAccountGroupInput { groupId: ID! "The ID of the shop this group belongs to" - shopId: ID! + shopId: ID } "Defines a group and account that should be linked" @@ -123,9 +117,6 @@ type Group implements Node { "A unique name for the group" name: String! - "A list of the account permissions implied by membership in this group" - permissions: [String] - "The shop to which this group belongs" shop: Shop @@ -240,7 +231,7 @@ extend type Shop { } extend type Account { - "A paged list of the permission groups in which this account is listed" + "A paged list of the account groups in which this account is listed" groups( "Return only results that come after this cursor. Use this with `first` to specify the number of results to return." after: ConnectionCursor, @@ -269,7 +260,7 @@ extend type Mutation { input: AddAccountToGroupInput! ): AddAccountToGroupPayload - "Create a new account group. These are usually used for account group permissions" + "Create a new account group. These are usually used for account permissions" createAccountGroup( "Mutation input" input: CreateAccountGroupInput! @@ -281,13 +272,13 @@ extend type Mutation { input: RemoveAccountFromGroupInput! ): RemoveAccountFromGroupPayload - "Remove an existing permission group" + "Remove an existing account group" removeAccountGroup( "Mutation input" input: RemoveAccountGroupInput! ): RemoveAccountGroupPayload - "Update an existing permission group" + "Update an existing account group" updateAccountGroup( "Mutation input" input: UpdateAccountGroupInput! diff --git a/src/core-services/account/simpleSchemas.js b/src/core-services/account/simpleSchemas.js index 5fde7fe4b98..eea4a39ceec 100644 --- a/src/core-services/account/simpleSchemas.js +++ b/src/core-services/account/simpleSchemas.js @@ -377,39 +377,27 @@ export const Account = new SimpleSchema({ * @property {String} name required * @property {String} description optional * @property {String} slug required - * @property {String[]} permissions optional - * @property {String} shopId required + * @property {String} shopId optional * @property {String} createdBy optional * @property {Date} createdAt required * @property {Date} updatedAt required */ export const Group = new SimpleSchema({ - "name": { - type: String - }, - "description": { + _id: String, + name: String, + description: { type: String, optional: true }, - "slug": { - type: String - }, - "permissions": { - type: Array, - optional: true - }, - "permissions.$": { - type: String - }, - "shopId": { + slug: String, + shopId: { type: String, optional: true }, - "createdBy": { + createdBy: { type: String, - optional: true, - regEx: SimpleSchema.RegEx.Id + optional: true }, - "createdAt": Date, - "updatedAt": Date + createdAt: Date, + updatedAt: Date }); diff --git a/src/core-services/account/util/ensureAccountsManagerGroup.js b/src/core-services/account/util/ensureAccountsManagerGroup.js index 17286179dc9..4060a6c534a 100644 --- a/src/core-services/account/util/ensureAccountsManagerGroup.js +++ b/src/core-services/account/util/ensureAccountsManagerGroup.js @@ -1,6 +1,3 @@ -import Random from "@reactioncommerce/random"; -import { defaultAccountsManagerRoles } from "./defaultRoles.js"; - /** * @name ensureAccountsManagerGroup * @summary Ensure accounts manager group exists @@ -9,22 +6,19 @@ import { defaultAccountsManagerRoles } from "./defaultRoles.js"; */ export default async function ensureAccountsManagerGroup(context) { const { collections: { Groups } } = context; - // get IDs of `accounts-manager` and `account-manager` groups const group = await Groups.findOne({ slug: "accounts-manager" }, { projection: { _id: 1 } }); let groupId = (group && group._id) || null; + // if accounts-manager group doesn't exist, create it now if (!group) { - groupId = Random.id(); - const currentDate = Date(); - await Groups.insertOne({ - _id: groupId, - createdAt: currentDate, - updatedAt: currentDate, - name: "accounts manager", - slug: "accounts-manager", - permissions: defaultAccountsManagerRoles, + const { group: newGroup } = await context.mutations.createAccountGroup(context.getInternalContext(), { + group: { + name: "accounts manager", + slug: "accounts-manager" + }, shopId: null }); + groupId = newGroup._id; } return groupId; diff --git a/src/core-services/account/util/ensureSystemManagerGroup.js b/src/core-services/account/util/ensureSystemManagerGroup.js index ff242f06998..6b177a2df51 100644 --- a/src/core-services/account/util/ensureSystemManagerGroup.js +++ b/src/core-services/account/util/ensureSystemManagerGroup.js @@ -1,6 +1,3 @@ -import Random from "@reactioncommerce/random"; -import { defaultSystemManagerRoles } from "./defaultRoles.js"; - /** * @name ensureSystemManagerGroup * @summary Ensure system manager group exists @@ -9,22 +6,19 @@ import { defaultSystemManagerRoles } from "./defaultRoles.js"; */ export default async function ensureSystemManagerGroup(context) { const { collections: { Groups } } = context; - // get IDs of `system-manager` and `account-manager` groups const group = await Groups.findOne({ slug: "system-manager" }, { projection: { _id: 1 } }); let groupId = (group && group._id) || null; + // if system-manager group doesn't exist, create it now if (!group) { - groupId = Random.id(); - const currentDate = Date(); - await Groups.insertOne({ - _id: groupId, - createdAt: currentDate, - updatedAt: currentDate, - name: "system manager", - slug: "system-manager", - permissions: defaultSystemManagerRoles, + const { group: newGroup } = await context.mutations.createAccountGroup(context.getInternalContext(), { + group: { + name: "system manager", + slug: "system-manager" + }, shopId: null }); + groupId = newGroup._id; } return groupId; diff --git a/tests/integration/api/mutations/createAccountGroup/createAccountGroup.test.js b/tests/integration/api/mutations/createAccountGroup/createAccountGroup.test.js index 795a91d189f..92ff728c69c 100644 --- a/tests/integration/api/mutations/createAccountGroup/createAccountGroup.test.js +++ b/tests/integration/api/mutations/createAccountGroup/createAccountGroup.test.js @@ -72,7 +72,9 @@ test("anyone can add account to group if they have ALL the group permissions", a expect(result.createAccountGroup.group).toMatchObject({ _id: expect.any(String), - createdBy: null, + createdBy: { + _id: "cmVhY3Rpb24vYWNjb3VudDptb2NrQWRtaW5BY2NvdW50" + }, description: "a group for testing purposes", name: "test-int-group", permissions: ["test-perm-1", "test-perm-2"], diff --git a/tests/integration/api/mutations/updateAccountGroup/__snapshots__/updateAccountGroup.test.js.snap b/tests/integration/api/mutations/updateAccountGroup/__snapshots__/updateAccountGroup.test.js.snap deleted file mode 100644 index c277371815c..00000000000 --- a/tests/integration/api/mutations/updateAccountGroup/__snapshots__/updateAccountGroup.test.js.snap +++ /dev/null @@ -1,57 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`an admin account should not be able to change the slug of a default group if is doesn't match 1`] = ` -Array [ - Object { - "extensions": Object { - "code": "FORBIDDEN", - "exception": Object { - "details": Object {}, - "error": "access-denied", - "eventData": Object {}, - "isClientSafe": true, - "reason": "Field 'slug' cannot be updated for default group with ID (shopManagerGroup) and name (shopManager).", - }, - }, - "locations": Array [ - Object { - "column": 5, - "line": 3, - }, - ], - "message": "Field 'slug' cannot be updated for default group with ID (shopManagerGroup) and name (shopManager).", - "path": Array [ - "updateAccountGroup", - "group", - ], - }, -] -`; - -exports[`an admin account should not be able to change the slug of a default group if is doesn't match 2`] = ` -Array [ - Object { - "extensions": Object { - "code": "FORBIDDEN", - "exception": Object { - "details": Object {}, - "error": "access-denied", - "eventData": Object {}, - "isClientSafe": true, - "reason": "Field 'slug' cannot be updated for default group with ID (ownerGroup) and name (ownerGroup).", - }, - }, - "locations": Array [ - Object { - "column": 5, - "line": 3, - }, - ], - "message": "Field 'slug' cannot be updated for default group with ID (ownerGroup) and name (ownerGroup).", - "path": Array [ - "updateAccountGroup", - "group", - ], - }, -] -`;