Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion apps/ui-community/src/App.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { RequireAuth } from '@cellix/ui-core';
import { Accounts } from '@ocom/ui-community-route-accounts';
import { Accounts, Member } from '@ocom/ui-community-route-accounts';
import { Admin } from '@ocom/ui-community-route-admin';
import { Root } from '@ocom/ui-community-route-root';
import { Route, Routes } from 'react-router-dom';
Expand Down Expand Up @@ -27,6 +27,10 @@ export default function App() {
path="/accounts/*"
element={<Accounts />}
/>
<Route
path="/:communityId/member/:memberId/*"
element={<Member />}
/>
<Route
path="/:communityId/admin/:memberId/*"
element={<Admin />}
Expand Down
4 changes: 2 additions & 2 deletions packages/ocom-verification/acceptance-ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@
},
"dependencies": {
"@apollo/client": "^3.13.9",
"@cellix/serenity-framework": "workspace:*",
"@cucumber/cucumber": "catalog:",
"@dr.pogodin/react-helmet": "^3.0.4",
"@cellix/serenity-framework": "workspace:*",
"@serenity-js/console-reporter": "catalog:",
"@serenity-js/core": "catalog:",
"@serenity-js/cucumber": "catalog:",
Expand All @@ -33,7 +33,7 @@
"@types/node": "catalog:",
"@types/react": "^19.1.8",
"@types/react-dom": "^19.1.6",
"c8": "^10.1.3",
"c8": "^11.0.0",
"tsx": "catalog:",
"typescript": "catalog:"
}
Expand Down

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The change in the application-services command file for updateMemberProfile(...) is correct, for removing that unnecessary permission check. But I still expect the command to throw an error if a member attempts to update a member document that is not their own (i.e different ObjectID).

We still want to test here to assert that behavior in the situation where a member is trying to edit not their own member document. The difference is we want the aggregate/entity classes in the domain layer (@ocom/domain) to be the source of truth for permissions, using their visas. The setter methods for the fields like memberName and profile.name should be checking the visa on that class for the permissions.isEdititngOwnMember predicate (which automatically does the ObjectID comparison you removed from the application-services code, that's why we don't need it there).

I don't see any changes to the member classes in @ocom/domain so once you adjust the behavior there, this test scenario can be restored to check for the correct expected behavior that the command throws when a member tries updating another member. Let me know if you have questions about enforcing the domain permissions in the Member classes, it should already be configured for you to do so in the code there.

Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,58 @@ describe('member-management operations', () => {
expect(member.profile.showProfile).toBe(true);
});

it('updates member profile without service-level actor permission checks', async () => {
const member = {
memberName: 'Old Name',
profile: {
name: '',
email: '',
bio: '',
showProfile: false,
showEmail: false,
showInterests: false,
showLocation: false,
showProperties: false,
},
};
memberRepository.getById.mockResolvedValue(member);
memberRepository.save.mockResolvedValue({ id: 'member-1' });

const result = await updateMemberProfile(dataSources)({
memberId: 'member-1',
profile: {
name: 'Jane Doe',
},
});

expect(result.id).toBe('member-1');
expect(member.profile.name).toBe('Jane Doe');
expect(member.memberName).toBe('Jane Doe');
});

it('throws when a member tries to update another member profile', async () => {
const profile = {} as Record<string, unknown>;
Object.defineProperty(profile, 'name', {
get: () => 'Old Name',
set: () => {
throw new Error('You do not have permission to update this profile');
},
enumerable: true,
});
const member = {
memberName: 'Old Name',
profile,
};
memberRepository.getById.mockResolvedValue(member);

await expect(
updateMemberProfile(dataSources)({
memberId: 'member-1',
profile: { name: 'New Name' },
}),
).rejects.toThrow('You do not have permission to update this profile');
});

it('throws when update member role save returns nothing', async () => {
const role = { id: 'role-1', community: { id: 'community-1' } };
const member = { communityId: 'community-1', role: null };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ Feature: <AggregateRoot> Member
When I set the memberName to "Bob"
Then the member's memberName should be "Bob"

Scenario: Changing the memberName with permission to edit own member profile and is editing own member account
Given a Member aggregate with permission to edit own member profile and is editing own member account
When I set the memberName to "Bob"
Then the member's memberName should be "Bob"

Scenario: Changing the memberName without permission
Given a Member aggregate without permission to manage members or system account
When I try to set the memberName to "Bob"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,21 @@ const feature = await loadFeature(path.resolve(__dirname, 'features/member.featu
function makePassport(
overrides: Partial<{
canManageMembers: boolean;
canEditOwnMemberProfile: boolean;
isEditingOwnMemberAccount: boolean;
isSystemAccount: boolean;
}> = {},
) {
return vi.mocked({
community: {
forCommunity: vi.fn(() => ({
determineIf: (fn: (p: { canManageMembers: boolean; isSystemAccount: boolean }) => boolean) =>
determineIf: (
fn: (p: { canManageMembers: boolean; canEditOwnMemberProfile: boolean; isEditingOwnMemberAccount: boolean; isSystemAccount: boolean }) => boolean,
) =>
fn({
canManageMembers: overrides.canManageMembers ?? true,
canEditOwnMemberProfile: overrides.canEditOwnMemberProfile ?? false,
isEditingOwnMemberAccount: overrides.isEditingOwnMemberAccount ?? false,
isSystemAccount: overrides.isSystemAccount ?? false,
}),
})),
Expand Down Expand Up @@ -216,6 +222,24 @@ test.for(feature, ({ Scenario, Background, BeforeEachScenario }) => {
});
});

Scenario('Changing the memberName with permission to edit own member profile and is editing own member account', ({ Given, When, Then }) => {
Given('a Member aggregate with permission to edit own member profile and is editing own member account', () => {
passport = makePassport({
canManageMembers: false,
canEditOwnMemberProfile: true,
isEditingOwnMemberAccount: true,
isSystemAccount: false,
});
member = new Member(makeBaseProps(), passport);
});
When('I set the memberName to "Bob"', () => {
member.memberName = 'Bob';
});
Then('the member\'s memberName should be "Bob"', () => {
expect(member.memberName).toBe('Bob');
});
});

Scenario('Changing the memberName without permission', ({ Given, When, Then }) => {
let changeMemberNameWithoutPermission: () => void;
Given('a Member aggregate without permission to manage members or system account', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ export class Member<props extends MemberProps> extends AggregateRoot<props, Pass
return this.props.memberName;
}
set memberName(memberName: string) {
if (!this.isNew && !this.visa.determineIf((domainPermissions) => domainPermissions.canManageMembers || domainPermissions.isSystemAccount)) {
if (!this.isNew && !this.visa.determineIf((domainPermissions) => domainPermissions.canManageMembers || domainPermissions.isSystemAccount || (domainPermissions.canEditOwnMemberProfile && domainPermissions.isEditingOwnMemberAccount))) {
throw new PermissionError('Cannot set member name');
}
this.props.memberName = new ValueObjects.MemberName(memberName).valueOf();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,16 @@ Feature: <Visa> MemberCommunityVisa
And I call determineIf with a function that returns canManageCommunitySettings
Then the result should be false

Scenario: determineIf sets isEditingOwnMemberAccount to true when the actor has a matching member account
Given a MemberCommunityVisa for the member and community with a matching user account
When I call determineIf with a function that returns isEditingOwnMemberAccount
Then the result should be true

Scenario: determineIf sets isEditingOwnMemberAccount to false when the actor does not own the member account
Given a MemberCommunityVisa for the member and community with a different user account
When I call determineIf with a function that returns isEditingOwnMemberAccount
Then the result should be false

Scenario: determineIf sets isEditingOwnMemberAccount to false
Given a MemberCommunityVisa for the member and community
When I call determineIf with a function that returns isEditingOwnMemberAccount
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@ import { MemberCommunityVisa } from './member.community.visa.ts';

export class MemberCommunityPassport extends MemberPassportBase implements CommunityPassport {
forCommunity(root: CommunityEntityReference): CommunityVisa {
return new MemberCommunityVisa(root, this._member);
return new MemberCommunityVisa(root, this._member, this._user);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@ function makeMember(
roleOverrides: Partial<{
communityPermissions: Record<string, unknown>;
}> = {},
accountUserIds: string[] = [],
) {
return {
id,
community: makeCommunity(communityId),
accounts: accountUserIds.map((userId) => ({ user: { id: userId } })),
role: {
permissions: {
communityPermissions: {
Expand Down Expand Up @@ -151,6 +153,36 @@ test.for(feature, ({ Scenario, Background, BeforeEachScenario }) => {
});
});

Scenario('determineIf sets isEditingOwnMemberAccount to true when the actor has a matching member account', ({ Given, When, Then }) => {
let result: boolean;
const currentUser = { id: 'user-42' } as any;
Given('a MemberCommunityVisa for the member and community with a matching user account', () => {
member = makeMember('member-1', 'community-1', {}, ['user-42']);
visa = new MemberCommunityVisa(community, member, currentUser);
});
When('I call determineIf with a function that returns isEditingOwnMemberAccount', () => {
result = visa.determineIf((p) => p.isEditingOwnMemberAccount);
});
Then('the result should be true', () => {
expect(result).toBe(true);
});
});

Scenario('determineIf sets isEditingOwnMemberAccount to false when the actor does not own the member account', ({ Given, When, Then }) => {
let result: boolean;
const currentUser = { id: 'another-user' } as any;
Given('a MemberCommunityVisa for the member and community with a different user account', () => {
member = makeMember('member-1', 'community-1', {}, ['user-42']);
visa = new MemberCommunityVisa(community, member, currentUser);
});
When('I call determineIf with a function that returns isEditingOwnMemberAccount', () => {
result = visa.determineIf((p) => p.isEditingOwnMemberAccount);
});
Then('the result should be false', () => {
expect(result).toBe(false);
});
});

Scenario('determineIf sets isEditingOwnMemberAccount to false', ({ Given, When, Then }) => {
let result: boolean;
Given('a MemberCommunityVisa for the member and community', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,17 @@ import type { CommunityEntityReference } from '../../../contexts/community/commu
import type { CommunityDomainPermissions } from '../../../contexts/community/community.domain-permissions.ts';
import type { CommunityVisa } from '../../../contexts/community/community.visa.ts';
import type { MemberEntityReference } from '../../../contexts/community/member/member.ts';
import type { EndUserEntityReference } from '../../../contexts/user/end-user/end-user.ts';

export class MemberCommunityVisa<root extends CommunityEntityReference> implements CommunityVisa {
private readonly root: root;
private readonly member: MemberEntityReference;
private readonly user?: EndUserEntityReference;

constructor(root: root, member: MemberEntityReference) {
constructor(root: root, member: MemberEntityReference, user?: EndUserEntityReference) {
this.root = root;
this.member = member;
this.user = user;
}

determineIf(func: (permissions: CommunityDomainPermissions) => boolean): boolean {
Expand All @@ -32,7 +35,9 @@ export class MemberCommunityVisa<root extends CommunityEntityReference> implemen
canEditOwnMemberAccounts: communityPermissions.canEditOwnMemberAccounts,
canManageEndUserRolesAndPermissions: communityPermissions.canManageEndUserRolesAndPermissions,
canManageSiteContent: communityPermissions.canManageSiteContent,
isEditingOwnMemberAccount: false,
isEditingOwnMemberAccount: this.user
? this.member.accounts.some((account) => account.user.id === this.user?.id)
: false,
canCreateCommunities: true, //TODO: add a more complext rule here like can only create one community for free, otherwise need a paid plan
canManageVendorUserRolesAndPermissions: false, // end user roles cannot manage vendor user roles
isSystemAccount: false,
Expand Down
26 changes: 26 additions & 0 deletions packages/ocom/graphql/src/schema/types/member.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,18 @@ type MemberProfile {
showProperties: Boolean
}

type MemberProfileVisibility {
showEmail: Boolean!
showBio: Boolean!
showInterests: Boolean!
}

extend type Query {
member(id: ObjectID!): Member!
membersByCommunityId(communityId: ObjectID!): [Member!]!
memberForCurrentCommunity(communityId: ObjectID!): Member
membersForCurrentEndUser: [Member!]!
memberMyProfile(communityId: ID!): Member
}

extend type Mutation {
Expand All @@ -69,6 +76,8 @@ extend type Mutation {

# Role Management
memberRoleUpdate(input: UpdateMemberRoleInput!): MemberMutationResult!

memberUpdateMyProfile(communityId: ID!, input: UpdateMyMemberProfileInput!): MemberMutationResult!
}

type MemberMutationResult implements MutationResult {
Expand Down Expand Up @@ -207,3 +216,20 @@ input MemberProfileInput {
showLocation: Boolean
showProperties: Boolean
}

input UpdateMemberProfileVisibilityInput {
showEmail: Boolean!
showBio: Boolean!
showInterests: Boolean!
showProfile: Boolean
showLocation: Boolean
showProperties: Boolean
}

input UpdateMyMemberProfileInput {
name: String!
email: String!
bio: String
interests: [String!]!
visibility: UpdateMemberProfileVisibilityInput!
}
Loading
Loading