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: get roles / permissions directly from group(s), not user object #6031

Merged
merged 51 commits into from
Jan 24, 2020
Merged
Show file tree
Hide file tree
Changes from 50 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
b3efdc1
feat: get roles via group for a user, and add to context.account.perm…
kieckhafer Jan 15, 2020
b6324a3
feat: permissionsbyUserId util
kieckhafer Jan 15, 2020
325a193
Merge remote-tracking branch 'origin/release-3.0.0' into refactor-kie…
kieckhafer Jan 15, 2020
e3d1098
refactor: rename "role" to "permission" where applicable in hasPermis…
kieckhafer Jan 15, 2020
25a603f
refactor: move context.acocunt.permissions to context.userPermissions
kieckhafer Jan 15, 2020
4b7aba1
refactor: move setting userPermissions to outside of account check
kieckhafer Jan 15, 2020
3ff8712
remove permission check from util function
kieckhafer Jan 15, 2020
e0bd15f
tests: add userPermissions to test
kieckhafer Jan 15, 2020
d91a45c
tests: update query integration tests with groups for permissions
kieckhafer Jan 16, 2020
9836daf
tests: remove groups after tests
kieckhafer Jan 16, 2020
78ef3ef
tests: update tests with permissions
kieckhafer Jan 16, 2020
cf07c80
tests: update tests with new permissions
kieckhafer Jan 16, 2020
2b185f8
Merge remote-tracking branch 'origin/release-3.0.0' into refactor-kie…
kieckhafer Jan 17, 2020
1568642
Merge remote-tracking branch 'origin/feat-kieckhafer-allowUserToBeInM…
kieckhafer Jan 17, 2020
0584997
tests: update integration tests with new permissions
kieckhafer Jan 17, 2020
d3732a5
tests: update integration tests with new permissions
kieckhafer Jan 17, 2020
89c69df
tests: update integration tests with new permissions
kieckhafer Jan 17, 2020
a173799
tests: update integration tests with new permissions
kieckhafer Jan 18, 2020
49f3c29
tests: update integration tests with new permissions
kieckhafer Jan 18, 2020
20d93b2
refactor: update groups query
kieckhafer Jan 18, 2020
b705751
refactor: remove administrators queires as they are no longer relevent
kieckhafer Jan 20, 2020
eddda56
refactor: remove shop administrator queries
kieckhafer Jan 20, 2020
9c0c14b
tests: update inviteShopMember test with new permissions
kieckhafer Jan 20, 2020
e579d51
fix: typo in error messaging
kieckhafer Jan 20, 2020
2b1c08c
tests: update addAccountToGroup test with new permissions
kieckhafer Jan 21, 2020
67cc089
tests: update removeAccountGroup with new permissions
kieckhafer Jan 21, 2020
0c961cb
tests: remove irrelevent test
kieckhafer Jan 21, 2020
672ddea
fix: duplicate input item
kieckhafer Jan 21, 2020
dd9c148
tests: update updateAccountGroup test with new permissions
kieckhafer Jan 21, 2020
d19d881
style: lint fix
kieckhafer Jan 21, 2020
c08ae8b
tests: remove "roles" from user object in tests
kieckhafer Jan 21, 2020
8121d6f
tests: remove roles from tags query
kieckhafer Jan 21, 2020
2e9b7f8
remove extraneous legacy roles
kieckhafer Jan 21, 2020
97a69b0
refactor permission check to use `rolesThatCanEdit`
kieckhafer Jan 21, 2020
ddea6c1
refactor: rename rolesThatCanEdit and refactor for new permissions style
kieckhafer Jan 22, 2020
2e593d6
style: lint fix
kieckhafer Jan 22, 2020
ffd6769
refactor: remove auth check in appSettings
kieckhafer Jan 22, 2020
ab5e7a0
style: clean up comments
kieckhafer Jan 22, 2020
d97e22b
update snapshots
kieckhafer Jan 22, 2020
957fe31
chore: clean up TODOs
kieckhafer Jan 22, 2020
a9c7ac3
style: fix comments
kieckhafer Jan 22, 2020
22e4566
tests: fix tags unit tests
kieckhafer Jan 22, 2020
77fb9bb
tests: refactor tests with new permissions
kieckhafer Jan 22, 2020
c4ccfd9
refactor: move permissionsByUserId into legacy-authorization plugin
kieckhafer Jan 23, 2020
aef626b
refactor: rename allowedRoles to permissionsNeededToEdit
kieckhafer Jan 23, 2020
99e1d1f
refactor: revert `__global_permissions__` to `__global_roles__`
kieckhafer Jan 23, 2020
6954076
refactor: tag query to get tag first, then filter after the fact
kieckhafer Jan 23, 2020
750958a
fix: move check for resource above permissions check
kieckhafer Jan 23, 2020
a480fd4
style: lint fix
kieckhafer Jan 24, 2020
dd27500
Merge remote-tracking branch 'origin/release-3.0.0' into refactor-kie…
kieckhafer Jan 24, 2020
91bae0f
tests: update tests with correct permissions
kieckhafer Jan 24, 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
92 changes: 0 additions & 92 deletions propel-feat.yaml

This file was deleted.

33 changes: 0 additions & 33 deletions propel.yaml

This file was deleted.

4 changes: 3 additions & 1 deletion src/core-services/account/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import resolvers from "./resolvers/index.js";
import schemas from "./schemas/index.js";
import startup from "./startup.js";
import accountByUserId from "./util/accountByUserId.js";
import permissionsByUserId from "./util/permissionsByUserId.js";
import { Account } from "./simpleSchemas.js";

/**
Expand Down Expand Up @@ -48,7 +49,8 @@ export default async function register(app) {
}
},
auth: {
accountByUserId
accountByUserId,
permissionsByUserId
kieckhafer marked this conversation as resolved.
Show resolved Hide resolved
},
functionsByType: {
startup: [startup]
Expand Down
23 changes: 0 additions & 23 deletions src/core-services/account/mutations/addAccountToGroup.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import _ from "lodash";
import ReactionError from "@reactioncommerce/reaction-error";
import SimpleSchema from "simpl-schema";
import canAddAccountToGroup from "../util/canAddAccountToGroup.js";
import ensureRoles from "../util/ensureRoles.js";

const inputSchema = new SimpleSchema({
accountId: String,
Expand Down Expand Up @@ -37,8 +35,6 @@ 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 { permissions: groupPermissions = [], shopId } = groupToAddUserTo;

const isAllowed = await canAddAccountToGroup(context, groupToAddUserTo);
if (!isAllowed) throw new ReactionError("access-denied", "Access Denied");

Expand All @@ -51,25 +47,6 @@ export default async function addAccountToGroup(context, input) {
const groupToAdd = account.groups.includes(groupId);
if (groupToAdd) throw new ReactionError("group-found", "Account is already in this group");

// Get existing roles from a user
const newAccountUserRoles = new Set((accountUser.roles || {})[shopId] || []);

// Merge existing roles on user and roles from new group
// TODO(pod-auth): this will likely be removed in #6031, where we no longer get roles from user.roles
const newRoles = [...newAccountUserRoles, ...groupPermissions];
const uniqueRoles = _.uniq(newRoles);

// Add all group roles to the user. Make sure this stays in this order.
await ensureRoles(context, newRoles);
await users.updateOne({
_id: account.userId
}, {
$set: {
[`roles.${shopId}`]: uniqueRoles
}
});
// TODO(pod-auth): this will likely be removed in #6031, where we no longer get roles from user.roles

// Add new group to Account
const accountGroups = Array.isArray(account.groups) ? account.groups : [];
accountGroups.push(groupId);
Expand Down
4 changes: 1 addition & 3 deletions src/core-services/account/mutations/createAccount.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ export default async function createAccount(context, input) {
// 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, legacyRoles: ["owner"] }); // TODO(pod-auth): update this permissions check
const isGlobalOwner = await context.userHasPermission("reaction:legacy:shops", "owner", { shopId }); // TODO(pod-auth): update this permissions check
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you are planning to tackle this later, but I'd like to see "owner" go away as a permission and instead just give the first user "create" permission for "shops" as well as maybe a global "update:settings" permission.

So I guess that would mean there should be a global permission group (role) auto-created called "owner" which should have these two permissions, and the first account should be placed in that group upon creation (here)?

As is, this is putting them in a shop-specific group, which we actually may not need to do here (unless there's a specific invite). It seems like part of createShop mutation should put the creator account into that shop's owner group. Then after that invites and groups are done explicitly by that first shop user.

Yikes, I should probably draw this out in a diagram.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this isn't new in this PR, so we can fix in a subsequent PR if you want.

if (isGlobalOwner) groupSlug = "owner";
}

Expand Down Expand Up @@ -115,8 +115,6 @@ export default async function createAccount(context, input) {

await Accounts.insertOne(account);

// Add all group permissions to the user roles. Because of the complexity of this
// and potential security concerns if done incorrectly, it's best to use the mutation.
try {
await Promise.all(groups.map((groupId) => (
context.mutations.addAccountToGroup(context.getInternalContext(), {
Expand Down
15 changes: 0 additions & 15 deletions src/core-services/account/mutations/removeAccountFromGroup.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import _ from "lodash";
import ReactionError from "@reactioncommerce/reaction-error";
import SimpleSchema from "simpl-schema";
import canAddAccountToGroup from "../util/canAddAccountToGroup.js";
import ensureRoles from "../util/ensureRoles.js";

const inputSchema = new SimpleSchema({
accountId: String,
Expand Down Expand Up @@ -55,19 +53,6 @@ export default async function removeAccountFromGroup(context, input) {

// Get all other groups user belongs to, and all permissions from these groups, and flatten into single array
const remainingGroupsUserBelongsTo = allGroupsUserBelongsTo.filter((group) => group.shopId === shopId && group._id !== groupId);
const remainingRolesToGiveUser = _.uniq(remainingGroupsUserBelongsTo.map((group) => group.permissions).flat());

// update user to only have roles from other groups they belong
// TODO(pod-auth): this will likely be removed in #6031, where we no longer get roles from user.roles
await ensureRoles(context, remainingRolesToGiveUser);
await users.updateOne({
_id: account.userId
}, {
$set: {
[`roles.${shopId}`]: remainingRolesToGiveUser
}
});
// TODO(pod-auth): this will likely be removed in #6031, where we no longer get roles from user.roles

const remainingGroupIds = remainingGroupsUserBelongsTo.map((group) => group._id);
await Accounts.updateOne({ _id: accountId }, { $set: { groups: remainingGroupIds } });
Expand Down
5 changes: 2 additions & 3 deletions src/core-services/account/mutations/removeAccountGroup.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export default async function removeAccountGroup(context, input) {
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, legacyRoles: ["admin"] });
await context.validatePermissions(`reaction:legacy:groups:${groupId}`, "remove", { shopId });

const defaultGroupsForShop = await Groups.find({
shopId,
Expand All @@ -34,14 +34,13 @@ export default async function removeAccountGroup(context, input) {
const forbiddenGroupIds = defaultGroupsForShop.map(({ _id }) => _id);

if (forbiddenGroupIds.includes(groupId)) {
throw new ReactionError("accedd-denied", `Cannot remove default group with ID ${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 users to.`);
}

// TODO: Remove when we move away from legacy permission verification
// Move accounts from their old group to their new group
await moveAccountsToGroup(context, {
shopId,
Expand Down
2 changes: 1 addition & 1 deletion src/core-services/account/mutations/updateAccountGroup.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export default async function updateAccountGroup(context, input) {
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}`, "update", { shopId, legacyRoles: ["admin"] });
await context.validatePermissions(`reaction:legacy:groups:${groupId}`, "update", { shopId });

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

Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`throws access-denied if not allowed 1`] = `"User does not have permissions to view groups"`;
exports[`throws access-denied if not allowed 1`] = `"Access Denied"`;

This file was deleted.

32 changes: 5 additions & 27 deletions src/core-services/account/queries/groups.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import ReactionError from "@reactioncommerce/reaction-error";

/**
* @name groups
* @method
Expand All @@ -10,31 +8,11 @@ import ReactionError from "@reactioncommerce/reaction-error";
* @returns {Object} Groups collection cursor
*/
export default async function groups(context, shopId) {
const { collections, userId } = context;
const { Accounts, Groups } = collections;

// TODO: Break this query up into one for all groups (for admins only) and one for user's groups
if (context.userHasPermission("reaction:legacy:accounts", "read", { shopId })) { // TODO(pod-auth): update this permissions check
// find groups by shop ID
return Groups.find({ shopId });
}
const { collections } = context;
const { Groups } = collections;

const userAccount = await Accounts.findOne({
userId
}, {
projection: {
groups: 1
}
});
await context.validatePermissions("reaction:legacy:groups", "read", { shopId });

// If user is not found, throw an error
if (!userAccount) throw new ReactionError("access-denied", "User does not have permissions to view groups");

// find groups by shop ID limited to those the current user is in
return Groups.find({
_id: {
$in: userAccount.groups || []
},
shopId
});
// TODO: Break this query up into one for all groups (for admins only) and one for user's groups
return Groups.find({ shopId });
}
31 changes: 10 additions & 21 deletions src/core-services/account/queries/groups.test.js
Original file line number Diff line number Diff line change
@@ -1,45 +1,34 @@
import mockContext from "@reactioncommerce/api-utils/tests/mockContext.js";
import ReactionError from "@reactioncommerce/reaction-error";
import groupsQuery from "./groups.js";

const fakeShopId = "FAKE_SHOP_ID";
const fakeAccount = { _id: "FAKE_ACCOUNT_ID", groups: ["group1", "group2"] };
mockContext.validatePermissions = jest.fn("validatePermissions");


beforeEach(() => {
jest.resetAllMocks();
});

test("returns the groups cursor if userHasPermission returns true", async () => {
mockContext.collections.Groups.find.mockReturnValueOnce("CURSOR");
mockContext.userHasPermission.mockReturnValueOnce(true);
mockContext.validatePermissions.mockReturnValueOnce(Promise.resolve(undefined));
const result = await groupsQuery(mockContext, fakeShopId);

expect(mockContext.collections.Groups.find).toHaveBeenCalledWith({ shopId: fakeShopId });
expect(mockContext.userHasPermission).toHaveBeenCalledWith(
"reaction:legacy:accounts",
expect(mockContext.validatePermissions).toHaveBeenCalledWith(
"reaction:legacy:groups",
"read",
{ shopId: fakeShopId }
);
expect(result).toBe("CURSOR");
});

test("returns the groups cursor for groups the current user is in, if userHasPermission returns false", async () => {
mockContext.collections.Groups.find.mockReturnValueOnce("CURSOR");
mockContext.userHasPermission.mockReturnValueOnce(false);
mockContext.collections.Accounts.findOne.mockReturnValueOnce(fakeAccount);
const result = await groupsQuery(mockContext, fakeShopId);
expect(mockContext.collections.Groups.find).toHaveBeenCalledWith({
_id: { $in: fakeAccount.groups },
shopId: fakeShopId
});
expect(mockContext.userHasPermission).toHaveBeenCalledWith(
"reaction:legacy:accounts",
"read",
{ shopId: fakeShopId }
);
expect(result).toBe("CURSOR");
});

test("throws access-denied if not allowed", async () => {
mockContext.userHasPermission.mockReturnValueOnce(false);
mockContext.validatePermissions.mockImplementation(() => {
throw new ReactionError("access-denied", "Access Denied");
});
mockContext.collections.Accounts.findOne.mockReturnValueOnce(undefined);
const result = groupsQuery(mockContext, fakeShopId);
return expect(result).rejects.toThrowErrorMatchingSnapshot();
Expand Down
2 changes: 0 additions & 2 deletions src/core-services/account/queries/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,12 @@ import accounts from "./accounts.js";
import group from "./group.js";
import groups from "./groups.js";
import roles from "./roles.js";
import shopAdministrators from "./shopAdministrators.js";
import userAccount from "./userAccount.js";

export default {
accounts,
group,
groups,
roles,
shopAdministrators,
userAccount
};
Loading