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

refactor: remove "owner" and other default permissions / groups #6063

Merged
merged 32 commits into from
Jan 31, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
0df8f2e
refactor: remove changing of groupSlug to owner
kieckhafer Jan 29, 2020
0bdc5db
refactor: remove code which automatically adds a user to "customer"
kieckhafer Jan 29, 2020
cc345ef
refactor: add user to (and create if needed) global shops when they a…
kieckhafer Jan 29, 2020
b7c9084
refactor: remove default "customer" and "guest" groups
kieckhafer Jan 29, 2020
189273b
refactor: remove restriction of inviting owners
kieckhafer Jan 29, 2020
620ca46
refactor: remove isOwnerAccount logic from `canAddAccountToGroup`
kieckhafer Jan 29, 2020
8b66b32
refactor: remove "owner" allowance in hasPermission
kieckhafer Jan 29, 2020
41fbf30
refactor: remove references to "owner" where applicable
kieckhafer Jan 30, 2020
75fe0fd
tests: refactor test to make sure "owner" does not get a free pass fo…
kieckhafer Jan 30, 2020
d1dcdea
tests: update permissions in queries tests
kieckhafer Jan 30, 2020
95d9134
Merge branch 'release-3.0.0' into refactor-removeOwner
kieckhafer Jan 30, 2020
e065c37
refactor: update default groups and create ensure utils for each
kieckhafer Jan 30, 2020
b2d9229
refactor: move canAddAccountToGroup checks into each individual call
kieckhafer Jan 30, 2020
ff58f58
refactor: remove owner roles
kieckhafer Jan 30, 2020
11b4036
refactor: remove requirement of customer group from removeAccountGroup
kieckhafer Jan 30, 2020
6ed4485
tests: fix account permissions now that owner is gone
kieckhafer Jan 30, 2020
8b55f6b
refactor: refactor permission building for global shops
kieckhafer Jan 30, 2020
e924f60
refactor: remove defaultCustomerGroup
kieckhafer Jan 30, 2020
94538a0
style: remove unused import
kieckhafer Jan 30, 2020
ec87fe5
refactor: remove reference to default customer group
kieckhafer Jan 30, 2020
31d9f02
refactor: remove default legacy roles
kieckhafer Jan 30, 2020
c578ea2
tests: fix unit tests with no more customer group
kieckhafer Jan 30, 2020
1fd8318
style: fix eslint issue
kieckhafer Jan 30, 2020
d011afe
tests: update tests with global permissions
kieckhafer Jan 31, 2020
53915ae
tests: add test for global group getting
kieckhafer Jan 31, 2020
8f0dc2c
tests: add shopId into tests
kieckhafer Jan 31, 2020
ece3574
tests: update mutations with new permissions
kieckhafer Jan 31, 2020
6eb6e6a
Merge remote-tracking branch 'origin/release-3.0.0' into refactor-rem…
kieckhafer Jan 31, 2020
a7354e1
testsL: add global group to test
kieckhafer Jan 31, 2020
f5e4925
tests: add permissions to tests
kieckhafer Jan 31, 2020
a040f61
tests: update snapsthots
kieckhafer Jan 31, 2020
6819daf
style: lint fix
kieckhafer Jan 31, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/core-services/account/mutations/addAccountToGroup.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import ReactionError from "@reactioncommerce/reaction-error";
import SimpleSchema from "simpl-schema";
import canAddAccountToGroup from "../util/canAddAccountToGroup.js";

const inputSchema = new SimpleSchema({
accountId: String,
Expand Down Expand Up @@ -34,8 +33,7 @@ export default async function addAccountToGroup(context, input) {
const groupToAddUserTo = await Groups.findOne({ _id: groupId });
if (!groupToAddUserTo) throw new ReactionError("not-found", "No group found with that ID");

const isAllowed = await canAddAccountToGroup(context, groupToAddUserTo);
if (!isAllowed) throw new ReactionError("access-denied", "Access Denied");
await context.validatePermissions("reaction:legacy:groups", "manage:accounts", { shopId: groupToAddUserTo.shopId });

const account = await Accounts.findOne({ _id: accountId });
if (!account) throw new ReactionError("not-found", "No account found with that ID");
Expand Down
45 changes: 15 additions & 30 deletions src/core-services/account/mutations/createAccount.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import SimpleSchema from "simpl-schema";
import Logger from "@reactioncommerce/logger";
import ensureAccountsManagerGroup from "../util/ensureAccountsManagerGroup.js";
import ensureSystemManagerGroup from "../util/ensureSystemManagerGroup.js";
import sendWelcomeEmail from "../util/sendWelcomeEmail.js";

const inputSchema = new SimpleSchema({
Expand Down Expand Up @@ -42,7 +44,7 @@ export default async function createAccount(context, input) {

const {
appEvents,
collections: { Accounts, AccountInvites, Groups },
collections: { Accounts, AccountInvites },
simpleSchemas: {
Account: AccountSchema
},
Expand Down Expand Up @@ -76,42 +78,25 @@ export default async function createAccount(context, input) {
userId
};

let groupSlug = "customer"; // Default is to put new accounts into the "customer" permission group
let groups;
let groups = [];
let invites;

// The identity provider service gives the first created user the global "owner" role. When we
// create an account for this user, they should be assigned to the "owner" group.
if (authUserId === userId) {
const isGlobalOwner = await context.userHasPermission("reaction:legacy:shops", "owner", { shopId }); // TODO(pod-auth): update this permissions check
if (isGlobalOwner) groupSlug = "owner";
}

// If we didn't already upgrade them to the "owner" group, see if they're been invited to any groups
if (groupSlug === "customer") {
// if this is the first user created overall, add them to the
// `system-manager` and `accounts-manager` groups
const anyAccount = await Accounts.findOne();
if (!anyAccount) {
const accountsManagerGroupId = await ensureAccountsManagerGroup(context);
const systemManagerGroupId = await ensureSystemManagerGroup(context);
groups.push(systemManagerGroupId);
groups.push(accountsManagerGroupId);
} else {
// if this isn't the first account see if they were invited by another user
// find all invites for this email address, for all shops, and add to all groups
const emailAddresses = emails.map((emailRecord) => emailRecord.address.toLowerCase());
// Find all invites for all shops and add to all groups
invites = await AccountInvites.find({ email: { $in: emailAddresses } }).toArray();
groups = invites.map((invite) => invite.groupId);
}

// If they weren't invited to any groups, put them in the customer or owner group as determined above
if (!groups || groups.length === 0) {
if (shopId) {
const group = await Groups.findOne({ slug: groupSlug, shopId });
groups = group ? [group._id] : [];
} else {
// Put them in a group for the primary shop
const primaryShopId = await context.queries.primaryShopId(context);
if (primaryShopId) {
const primaryShopGroup = await Groups.findOne({ slug: groupSlug, shopId: primaryShopId });
groups = primaryShopGroup ? [primaryShopGroup._id] : [];
} else {
groups = [];
}
}
}

AccountSchema.validate(account);

await Accounts.insertOne(account);
Expand Down
9 changes: 0 additions & 9 deletions src/core-services/account/mutations/createAccountGroup.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import _ from "lodash";
import Logger from "@reactioncommerce/logger";
import ReactionError from "@reactioncommerce/reaction-error";
import getSlug from "@reactioncommerce/api-utils/getSlug.js";
Expand Down Expand Up @@ -29,11 +28,6 @@ export default async function createAccountGroup(context, input) {
// we are limiting group method actions to only users within the account managers role
await context.validatePermissions("reaction:legacy:groups", "create", { shopId });

const defaultCustomerGroupForShop = await Groups.findOne({ slug: "customer", shopId }) || {};

// TODO: Remove when we move away from legacy permission verification
const defaultAdminPermissions = (defaultCustomerGroupForShop.permissions || []).concat("dashboard");

const nowDate = new Date();
const newGroupData = Object.assign({}, group, {
slug: getSlug(group.name),
Expand All @@ -47,9 +41,6 @@ export default async function createAccountGroup(context, input) {
newGroupData.permissions = [];
}

// TODO: Remove when we move away from legacy permission verification
newGroupData.permissions = _.uniq([...newGroupData.permissions, ...defaultAdminPermissions]);

// ensure one group type per shop
const groupExists = await Groups.findOne({ slug: newGroupData.slug, shopId });

Expand Down
19 changes: 6 additions & 13 deletions src/core-services/account/mutations/createAccountGroup.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,7 @@ test("should create a group for the shop", async () => {
_id: "test-group-id",
name: "test group",
slug: "test group",
permissions: [
"dashboard"
],
permissions: [],
shopId: "test-shop-id"
};

Expand All @@ -32,9 +30,7 @@ test("should create a group for the shop", async () => {
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.findOne.mockReturnValueOnce(Promise.resolve(undefined));

mockContext.validatePermissions.mockReturnValueOnce(Promise.resolve(undefined));

Expand All @@ -43,14 +39,11 @@ test("should create a group for the shop", async () => {
await expect(result).toEqual(expected);

expect(mockContext.validatePermissions).toHaveBeenCalledWith("reaction:legacy:groups", "create", { shopId });
expect(mockContext.collections.Groups.findOne).toHaveBeenNthCalledWith(1, { slug: "customer", shopId });
expect(mockContext.collections.Groups.findOne).toHaveBeenNthCalledWith(2, { slug: "test-group", shopId });
expect(mockContext.collections.Groups.findOne).toHaveBeenNthCalledWith(1, { slug: "test-group", shopId });
expect(mockContext.collections.Groups.insertOne).toHaveBeenCalledWith({
name: "test group",
slug: getSlug("test group"),
permissions: [
"dashboard"
],
permissions: [],
shopId: "test-shop-id",
updatedAt: expect.any(Date),
createdAt: expect.any(Date)
Expand Down Expand Up @@ -82,8 +75,8 @@ test("should throw if group already exists", async () => {
};
mockContext.collections.Groups.insertOne.mockReturnValueOnce(Promise.resolve(insertOneRes));
mockContext.collections.Groups.findOne
.mockReturnValueOnce(Promise.resolve(undefined))
.mockReturnValueOnce(Promise.resolve(fakeResult));
.mockReturnValueOnce(Promise.resolve(fakeResult))
.mockReturnValueOnce(Promise.resolve(undefined));

mockContext.validatePermissions.mockReturnValueOnce(Promise.resolve(undefined));

Expand Down
10 changes: 3 additions & 7 deletions src/core-services/account/mutations/createAuthGroupsForShop.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
import Logger from "@reactioncommerce/logger";
import Random from "@reactioncommerce/random";
import {
defaultCustomerRoles,
defaultOwnerRoles,
defaultShopManagerRoles,
defaultVisitorRoles
defaultShopOwnerRoles,
defaultShopManagerRoles
} from "../util/defaultRoles.js";

/**
Expand All @@ -26,9 +24,7 @@ export default async function createAuthGroupsForShop(context, shopId) {

const roles = {
"shop manager": defaultShopManagerRoles,
"customer": defaultCustomerRoles,
"guest": defaultVisitorRoles,
"owner": defaultOwnerRoles
"owner": defaultShopOwnerRoles
};

const primaryShop = await Shops.findOne({ shopType: "primary" });
Expand Down
9 changes: 1 addition & 8 deletions src/core-services/account/mutations/inviteShopMember.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import SimpleSchema from "simpl-schema";
import Random from "@reactioncommerce/random";
import ReactionError from "@reactioncommerce/reaction-error";
import config from "../config.js";
import canAddAccountToGroup from "../util/canAddAccountToGroup.js";
import getCurrentUserName from "../util/getCurrentUserName.js";

const { REACTION_ADMIN_PUBLIC_ACCOUNT_REGISTRATION_URL } = config;
Expand Down Expand Up @@ -49,11 +48,6 @@ export default async function inviteShopMember(context, input) {
const group = await Groups.findOne({ _id: groupId });
if (!group) throw new ReactionError("not-found", "No group found");

// we don't allow direct invitation of "owners", throw an error if that is the group
if (group.slug === "owner") {
throw new ReactionError("bad-request", "Cannot directly invite owner");
}

const lowercaseEmail = email.toLowerCase();

// check to see if invited email has an account
Expand All @@ -71,8 +65,7 @@ export default async function inviteShopMember(context, input) {

// This check is part of `addAccountToGroup` mutation for existing users. For new users,
// we do it here before creating an invite record and sending the invite email.
const isAllowed = await canAddAccountToGroup(context, group);
if (!isAllowed) throw new ReactionError("access-denied", "Access Denied");
await context.validatePermissions("reaction:legacy:groups", "manage:accounts", { shopId: group.shopId });

// Create an AccountInvites document. If a person eventually creates an account with this email address,
// it will be automatically added to this group instead of the default group for this shop.
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import ReactionError from "@reactioncommerce/reaction-error";
import SimpleSchema from "simpl-schema";
import canAddAccountToGroup from "../util/canAddAccountToGroup.js";

const inputSchema = new SimpleSchema({
accountId: String,
Expand Down Expand Up @@ -36,8 +35,7 @@ export default async function removeAccountFromGroup(context, input) {

const { shopId } = groupToRemoveUserFrom;

const isAllowed = await canAddAccountToGroup(context, groupToRemoveUserFrom);
if (!isAllowed) throw new ReactionError("access-denied", "Access Denied");
await context.validatePermissions("reaction:legacy:groups", "manage:accounts", { shopId });

const account = await Accounts.findOne({ _id: accountId });
if (!account) throw new ReactionError("not-found", "No account found with that ID");
Expand Down
16 changes: 4 additions & 12 deletions src/core-services/account/mutations/removeAccountGroup.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import ReactionError from "@reactioncommerce/reaction-error";
import moveAccountsToGroup from "../util/moveAccountsToGroup.js";
import defaultAccountGroups, { defaultCustomerGroupSlug } from "../util/defaultAccountGroups.js";
import defaultAccountGroups from "../util/defaultAccountGroups.js";
import removeAccountsFromGroup from "../util/removeAccountsFromGroup.js";

/**
* @name group/removeAccountGroup
Expand All @@ -20,7 +20,6 @@ export default async function removeAccountGroup(context, input) {
const { appEvents, user } = context;
const { Groups } = context.collections;

// we are limiting group method actions to only users within the account managers role
await context.validatePermissions(`reaction:legacy:groups:${groupId}`, "remove", { shopId });

const defaultGroupsForShop = await Groups.find({
Expand All @@ -30,22 +29,15 @@ export default async function removeAccountGroup(context, input) {
}
}).toArray();

const defaultCustomerGroupForShop = defaultGroupsForShop.find(({ slug }) => slug === defaultCustomerGroupSlug);
const forbiddenGroupIds = defaultGroupsForShop.map(({ _id }) => _id);

if (forbiddenGroupIds.includes(groupId)) {
throw new ReactionError("access-denied", `Cannot remove default group with ID ${groupId}.`);
}

if (!defaultCustomerGroupForShop) {
throw new ReactionError("server-error", `Cannot remove group ${groupId}. Default "customer" group doesn't exist to move account to.`);
}

// Move accounts from their old group to their new group
await moveAccountsToGroup(context, {
await removeAccountsFromGroup(context, {
shopId,
fromGroupId: groupId,
toGroupId: defaultCustomerGroupForShop._id
fromGroupId: groupId
});

/** Kafka connect mongo should be listening for update events
Expand Down
16 changes: 4 additions & 12 deletions src/core-services/account/mutations/updateAccountGroup.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import _ from "lodash";
import ReactionError from "@reactioncommerce/reaction-error";
import SimpleSchema from "simpl-schema";
import getSlug from "@reactioncommerce/api-utils/getSlug.js";
import defaultAccountGroups, { defaultCustomerGroupSlug } from "../util/defaultAccountGroups.js";
import defaultAccountGroups from "../util/defaultAccountGroups.js";

const inputSchema = new SimpleSchema({
"slug": { type: String, optional: true },
Expand Down Expand Up @@ -39,11 +38,6 @@ export default async function updateAccountGroup(context, input) {
// we are limiting group method actions to only users within the account managers role
await context.validatePermissions(`reaction:legacy:groups:${groupId}`, "update", { shopId });

const defaultCustomerGroupForShop = await Groups.findOne({ slug: defaultCustomerGroupSlug, shopId }) || {};

// TODO: Remove when we move away from legacy permission verification
const defaultCustomerPermissions = defaultCustomerGroupForShop.permissions;

// Ensure group exists before proceeding
const existingGroup = await Groups.findOne({ _id: groupId });

Expand All @@ -61,7 +55,7 @@ export default async function updateAccountGroup(context, input) {
}

// Prevent updating the slug of the default groups.
// For example, changing the slug of the customer group could cause various features of the application to sop working as intended.
// 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
Expand All @@ -73,11 +67,9 @@ export default async function updateAccountGroup(context, input) {
updateGroupData.description = group.description;
}

// TODO: Remove when we move away from legacy permission verification
// Update the roles on the group and any user in those groups
// Update the permissions on the group and any user in those groups
if (Array.isArray(group.permissions)) {
const roles = _.uniq([...group.permissions, ...defaultCustomerPermissions]);
updateGroupData.permissions = roles;
updateGroupData.permissions = group.permissions;
}

// Validate final group object
Expand Down
2 changes: 1 addition & 1 deletion src/core-services/account/queries/group.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export default async function groupQuery(context, id) {

// If the user has sufficient permissions, then allow them to find any group by ID
// TODO: Break this query up into one for all groups (for admins only) and one for user's groups
const allowed = await context.userHasPermission("reaction:legacy:groups", "read");
const allowed = await context.userHasPermission("reaction:legacy:groups", "read", { shopId: group.shopId });
if (allowed) return group;

// Otherwise, only let users see groups that they are members of
Expand Down
6 changes: 4 additions & 2 deletions src/core-services/account/queries/group.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ test("returns the group if userHasPermission returns true", async () => {
expect(mockContext.collections.Groups.findOne).toHaveBeenCalledWith({ _id: fakeGroup._id });
expect(mockContext.userHasPermission).toHaveBeenCalledWith(
"reaction:legacy:groups",
"read"
"read",
{ shopId: "FAKE_SHOP_ID" }
);
expect(result).toEqual(fakeGroup);
});
Expand All @@ -35,7 +36,8 @@ test("returns the group if userHasPermission returns false but the current user
expect(mockContext.collections.Groups.findOne).toHaveBeenCalledWith({ _id: fakeGroup._id });
expect(mockContext.userHasPermission).toHaveBeenCalledWith(
"reaction:legacy:groups",
"read"
"read",
{ shopId: "FAKE_SHOP_ID" }
);
expect(result).toEqual(fakeGroup);
});
Expand Down
3 changes: 2 additions & 1 deletion src/core-services/account/simpleSchemas.js
Original file line number Diff line number Diff line change
Expand Up @@ -402,7 +402,8 @@ export const Group = new SimpleSchema({
type: String
},
"shopId": {
type: String
type: String,
optional: true
},
"createdBy": {
type: String,
Expand Down
10 changes: 3 additions & 7 deletions src/core-services/account/startup.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import ensureRoles from "./util/ensureRoles.js";
import {
defaultCustomerRoles,
defaultOwnerRoles,
defaultShopManagerRoles,
defaultVisitorRoles
defaultShopOwnerRoles,
defaultShopManagerRoles
} from "./util/defaultRoles.js";

/**
Expand All @@ -14,8 +12,6 @@ import {
*/
export default async function accountStartup(context) {
// Add missing roles to `roles` collection if needed
await ensureRoles(context, defaultCustomerRoles);
await ensureRoles(context, defaultOwnerRoles);
await ensureRoles(context, defaultShopOwnerRoles);
await ensureRoles(context, defaultShopManagerRoles);
await ensureRoles(context, defaultVisitorRoles);
}
Loading