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

[Debt] Update how roles are set as part of communities changes #10791

Merged
merged 35 commits into from
Jul 18, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
7e2d841
remove sync from RoleAssignmentHasMany
vd1992 Jun 25, 2024
7c660bb
deprecate/replace updateUserTeamRoles
vd1992 Jun 25, 2024
b1fbe03
policy function written out
vd1992 Jun 25, 2024
7f03c47
condense policy
vd1992 Jun 25, 2024
a17b338
fix test
vd1992 Jun 25, 2024
4d92c26
Merge branch 'main' into 10351-updateUserRoles-changes
vd1992 Jun 27, 2024
dcceb4d
Merge branch 'main' into 10351-updateUserRoles-changes
vd1992 Jul 3, 2024
1d80d07
Merge branch 'main' into 10351-updateUserRoles-changes
vd1992 Jul 4, 2024
174582c
change RoleAssignmentHasMany
vd1992 Jul 4, 2024
6187473
fix test formatting and improve validation
vd1992 Jul 4, 2024
4980080
adjust policy to fit mutation change
vd1992 Jul 4, 2024
b909155
add guard
vd1992 Jul 4, 2024
6cd2094
make updateRoles work again
vd1992 Jul 5, 2024
c5522c1
playwright fix
vd1992 Jul 5, 2024
3af0b17
interface change
vd1992 Jul 5, 2024
fc1e51b
add assign any role
vd1992 Jul 5, 2024
ef143ea
add todo for later
vd1992 Jul 5, 2024
d7214cf
make nullable
vd1992 Jul 5, 2024
24350ae
Merge branch 'main' into 10351-updateUserRoles-changes
vd1992 Jul 5, 2024
77159ef
Update apps/playwright/fixtures/AdminPage.ts
vd1992 Jul 8, 2024
9768145
use process operator only
vd1992 Jul 8, 2024
0812f65
Merge branch 'main' into 10351-updateUserRoles-changes
vd1992 Jul 8, 2024
42c5e49
lint
vd1992 Jul 8, 2024
5515299
early escape permission check
vd1992 Jul 8, 2024
3524d65
add team id
vd1992 Jul 8, 2024
a5f8521
Merge branch 'main' into 10351-updateUserRoles-changes
vd1992 Jul 10, 2024
423d590
Merge branch 'main' into 10351-updateUserRoles-changes
vd1992 Jul 12, 2024
f808353
teamId added
vd1992 Jul 15, 2024
de7a408
Merge branch 'main' into 10351-updateUserRoles-changes
vd1992 Jul 15, 2024
b9787c1
null safe
vd1992 Jul 15, 2024
e4beaf0
don't use name teamId
vd1992 Jul 15, 2024
83c6f35
typo fix
vd1992 Jul 15, 2024
819b41a
move up early return
vd1992 Jul 15, 2024
0f8c697
rename again
vd1992 Jul 16, 2024
9edb149
Merge branch 'main' into 10351-updateUserRoles-changes
vd1992 Jul 16, 2024
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
120 changes: 118 additions & 2 deletions api/app/Policies/UserPolicy.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
namespace App\Policies;

use App\Models\PoolCandidate;
use App\Models\Role;
use App\Models\Team;
use App\Models\User;
use Illuminate\Auth\Access\HandlesAuthorization;

Expand Down Expand Up @@ -80,11 +82,72 @@ public function updateSub(User $user)
/**
* Determine whether the user can update roles.
*
* @param UpdateUserRolesInput $args
* @return \Illuminate\Auth\Access\Response|bool
*/
public function updateRoles(User $user)
public function updateRoles(User $user, $args)
{
return $user->isAbleTo('assign-any-role');
$targetUserId = isset($args['id']) ? $args['id'] : null;
$attachRoles = isset($args['roleAssignmentsInput']) && isset($args['roleAssignmentsInput']['attach']) ?
$args['roleAssignmentsInput']['attach'] : null;
$detachRoles = isset($args['roleAssignmentsInput']) && isset($args['roleAssignmentsInput']['detach']) ?
$args['roleAssignmentsInput']['detach'] : null;

if (is_null($targetUserId)) {
return false;
}

// adding or removing team based roles
if (isset($attachRoles['team']) || isset($detachRoles['team'])) {
Copy link
Member

Choose a reason for hiding this comment

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

I take back my previous comment about decoupling this policy from the schema. However, if we follow my suggested change to the schema, then it probably makes more sense to simply loop through all the attach roles then all the detach roles, instead of dealing with team roles first then non-team roles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusting the policy to fit the new mutation input type simplifies the policy file a fair bit
4980080


if ($attachRoles && isset($attachRoles['roles']) && isset($attachRoles['team'])) {
$rolesArray = $attachRoles['roles'];

// check every role being operated on and only return true if none failed
foreach ($rolesArray as $roleId) {
if (! $this->teamAbleToCheck($user, $roleId, $attachRoles['team'])) {
return false;
}
}
}

if ($detachRoles && isset($detachRoles['roles']) && isset($detachRoles['team'])) {
$rolesArray = $detachRoles['roles'];

// check every role being operated on and only return true if none failed
foreach ($rolesArray as $roleId) {
if (! $this->teamAbleToCheck($user, $roleId, $detachRoles['team'])) {
return false;
}
}
}

return true;
}

// adding or removing individual roles
if (empty($attachRoles['team']) && empty($detachRoles['team'])) {

$rolesArray = [];
if ($attachRoles && isset($attachRoles['roles'])) {
$rolesArray = [...$rolesArray, ...$attachRoles['roles']];
}
if ($detachRoles && isset($detachRoles['roles'])) {
$rolesArray = [...$rolesArray, ...$detachRoles['roles']];
}

// check every role being operated on and only return true if none failed
foreach ($rolesArray as $roleId) {
if (! $this->individualAbleToCheck($user, $roleId)) {
return false;
}
}

return true;
}

// default to reject
return false;
}

/**
Expand Down Expand Up @@ -118,4 +181,57 @@ protected function applicantHasAppliedToPoolInTeams(User $applicant, $teamIds)
})
->exists();
}

/******************* ROLE CHECKING *******************/

/**
* Function to check if the acting user can update a team based role
*
* @return bool
*/
protected function teamAbleToCheck(User $actor, string $roleId, string $teamId)
{
tristan-orourke marked this conversation as resolved.
Show resolved Hide resolved
$role = Role::findOrFail($roleId);
$team = Team::findOrFail($teamId);

switch ($role->name) {
case 'pool_operator':
return $actor->isAbleTo('assign-any-teamRole');
case 'process_operator':
return $actor->isAbleTo('update-any-processOperatorMembership') || $actor->isAbleTo('update-team-processOperatorMembership', $team);
tristan-orourke marked this conversation as resolved.
Show resolved Hide resolved
case 'community_recruiter':
return $actor->isAbleTo('update-any-communityRecruiterMembership ') || $actor->isAbleTo('update-team-communityRecruiterMembership ', $team);
case 'community_admin':
return $actor->isAbleTo('update-any-communityAdminMembership ') || $actor->isAbleTo('update-team-communityAdminMembership ', $team);
}

return false; // reject unknown roles
}

/**
* Function to check if the acting user can update an individual role
*
* @return bool
*/
protected function individualAbleToCheck(User $actor, string $roleId)
{
$role = Role::findOrFail($roleId);

switch ($role->name) {
case 'guest':
return $actor->isAbleTo('assign-any-role');
case 'base_user':
return $actor->isAbleTo('assign-any-role');
case 'applicant':
return $actor->isAbleTo('assign-any-role');
case 'request_responder':
return $actor->isAbleTo('assign-any-role');
case 'community_manager':
return $actor->isAbleTo('assign-any-role');
case 'platform_admin':
return $actor->isAbleTo('update-any-platformAdminMembership ');
tristan-orourke marked this conversation as resolved.
Show resolved Hide resolved
}

return false; // reject unknown roles
}
}
8 changes: 5 additions & 3 deletions api/graphql/schema.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -1679,7 +1679,6 @@ input RolesInput
input RoleAssignmentHasMany {
attach: RolesInput
detach: RolesInput
sync: RolesInput
}
tristan-orourke marked this conversation as resolved.
Show resolved Hide resolved

# This input only accepts Team-Based roles. It is assumed that a team id will be provided at a higher level of input.
Expand Down Expand Up @@ -1793,7 +1792,7 @@ type Mutation {
updateUserRolesInput: UpdateUserRolesInput! @spread
): UserAuthInfo
@update(model: "User")
Copy link
Member

Choose a reason for hiding this comment

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

If we change the UpdateUserRolesInput, we'll need to remove the @update directive which lets Lighthouse magically handle the update, and do it in a mutation class.

@canModel(ability: "updateRoles", model: "User")
Copy link
Member

Choose a reason for hiding this comment

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

We should also add a @guard directive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure b909155

@canModel(ability: "updateRoles", model: "User", injectArgs: true)
deleteUser(id: ID! @whereKey): User
@delete
@guard
Expand Down Expand Up @@ -2110,7 +2109,10 @@ type Mutation {
@canFind(ability: "delete", find: "id")
updateUserTeamRoles(
teamRoleAssignments: UpdateUserTeamRolesInput! @spread
): Team @guard @canFind(ability: "assignTeamMembers", find: "teamId")
): Team
@guard
@canFind(ability: "assignTeamMembers", find: "teamId")
@deprecated(reason: "use updateUserRoles")

# Notifications
markNotificationAsRead(id: UUID!): Notification @guard # can only affect notifications belonging to the logged-in user
Expand Down
3 changes: 1 addition & 2 deletions api/storage/app/lighthouse-schema.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ type Mutation {
createTeam(team: CreateTeamInput!): Team
updateTeam(id: UUID!, team: UpdateTeamInput!): Team
deleteTeam(id: UUID!): Team
updateUserTeamRoles(teamRoleAssignments: UpdateUserTeamRolesInput!): Team
updateUserTeamRoles(teamRoleAssignments: UpdateUserTeamRolesInput!): Team @deprecated(reason: "use updateUserRoles")
markNotificationAsRead(id: UUID!): Notification
markNotificationAsUnread(id: UUID!): Notification
deleteNotification(id: UUID!): Notification
Expand Down Expand Up @@ -1457,7 +1457,6 @@ input RolesInput {
input RoleAssignmentHasMany {
attach: RolesInput
detach: RolesInput
sync: RolesInput
}

input RolesForTeamInput {
Expand Down
9 changes: 5 additions & 4 deletions api/tests/Feature/UserRoleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ protected function setUp(): void
->asApplicant()
->asRequestResponder()
->asAdmin()
->asCommunityManager()
->create([
'email' => 'admin-user@test.com',
'sub' => 'admin-user@test.com',
Expand Down Expand Up @@ -205,8 +206,8 @@ public function testAdminCanSeeUsersTeams()
// Create a user with an old role. Assert that the admin can remove the old role and add the new role.
public function testAdminCanAddAndRemoveNonTeamRoleToUser()
{
$oldRole = Role::factory()->create(['is_team_based' => false]);
$newRole = Role::factory()->create(['is_team_based' => false]);
$oldRole = Role::where('name', 'guest')->sole();
$newRole = Role::where('name', 'base_user')->sole();
$user = User::factory()->create()->syncRoles([$oldRole]);

$this->actingAs($this->adminUser, 'api')->graphQL(
Expand Down Expand Up @@ -249,8 +250,8 @@ public function testAdminCanAddAndRemoveNonTeamRoleToUser()
// Create a user with an old role. Assert that the admin can remove the old role and add the new role, now with teams!
public function testAdminCanAddAndRemoveTeamRoleToUser()
{
$oldRole = Role::factory()->create(['is_team_based' => true]);
$newRole = Role::factory()->create(['is_team_based' => true]);
$oldRole = Role::where('name', 'pool_operator')->sole();
$newRole = Role::where('name', 'process_operator')->sole();
tristan-orourke marked this conversation as resolved.
Show resolved Hide resolved
$oldTeam = Team::factory()->create();
$newTeam = Team::factory()->create();
$user = User::factory()->create()->syncRoles([$oldRole], $oldTeam);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,12 @@ AddTeamMemberDialogProps) => {

const handleSave = async (formValues: TeamMemberFormValues) => {
await executeMutation({
teamRoleAssignments: {
updateUserRolesInput: {
userId: formValues.userId,
teamId: formValues.teamId,
roleAssignments: {
roleAssignmentsInput: {
attach: {
roles: formValues.roles,
team: formValues.teamId,
},
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,15 @@ const EditTeamMemberDialog = ({ user, team }: EditTeamMemberDialogProps) => {
const { roles, fetching } = useAvailableRoles();
const [, executeMutation] = useMutation(UpdateUserTeamRoles_Mutation);
const [isOpen, setIsOpen] = useState<boolean>(false);
const initialRolesIds = user.roles.map((role) => role.id);

const methods = useForm<TeamMemberFormValues>({
defaultValues: {
userId: user.id,
userDisplay: user.id,
teamId: team.id,
teamDisplay: team.id,
roles: user.roles.map((role) => role.id),
roles: initialRolesIds,
},
});

Expand All @@ -52,14 +53,29 @@ const EditTeamMemberDialog = ({ user, team }: EditTeamMemberDialogProps) => {
} = methods;

const handleSave = async (formValues: TeamMemberFormValues) => {
const rolesToAttach = formValues.roles.filter(
(role) => !initialRolesIds.includes(role),
);
const rolesToDetach = initialRolesIds.filter(
(role) => !formValues.roles.includes(role),
);

await executeMutation({
teamRoleAssignments: {
updateUserRolesInput: {
userId: formValues.userId,
teamId: formValues.teamId,
roleAssignments: {
attach: {
roles: formValues.roles,
},
roleAssignmentsInput: {
attach: rolesToAttach.length
? {
roles: rolesToAttach,
team: team.id,
}
: undefined,
detach: rolesToDetach.length
? {
roles: rolesToDetach,
team: team.id,
}
: undefined,
},
},
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,12 @@ const RemoveTeamMemberDialog = ({

const handleRemove = async () => {
await executeMutation({
teamRoleAssignments: {
updateUserRolesInput: {
userId: user.id,
teamId: team.id,
roleAssignments: {
roleAssignmentsInput: {
detach: {
roles: user.roles.map((role) => role.id),
team: team.id,
},
},
},
Expand Down
23 changes: 16 additions & 7 deletions apps/web/src/pages/Teams/TeamMembersPage/components/operations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,27 @@ import { graphql } from "@gc-digital-talent/graphql";

// eslint-disable-next-line import/prefer-default-export
export const UpdateUserTeamRoles_Mutation = graphql(/* GraphQL */ `
mutation UpdateUserTeamRoles(
$teamRoleAssignments: UpdateUserTeamRolesInput!
) {
updateUserTeamRoles(teamRoleAssignments: $teamRoleAssignments) {
mutation UpdateUserRoles($updateUserRolesInput: UpdateUserRolesInput!) {
updateUserRoles(updateUserRolesInput: $updateUserRolesInput) {
id
roleAssignments {
id
role {
id
name
isTeamBased
displayName {
en
fr
}
}
user {
team {
id
firstName
lastName
name
displayName {
en
fr
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,11 @@ const EditTeamRoleDialog = ({
const [isOpen, setIsOpen] = useState<boolean>(false);
const userDisplayName = getFullNameHtml(user.firstName, user.lastName, intl);
const teamDisplayName = getLocalizedName(team.displayName, intl);
const initialRolesIds = initialRoles.map((role) => role.id);

const methods = useForm<FormValues>({
defaultValues: {
roles: initialRoles.map((role) => role.id),
roles: initialRolesIds,
},
});

Expand All @@ -61,13 +62,28 @@ const EditTeamRoleDialog = ({
} = methods;

const handleEditRoles = async (formValues: FormValues) => {
const rolesToAttach = formValues.roles.filter(
(role) => !initialRolesIds.includes(role),
);
const rolesToDetach = initialRolesIds.filter(
(role) => !formValues.roles.includes(role),
);

return onEditRoles({
userId: user.id,
roleAssignmentsInput: {
sync: {
roles: formValues.roles,
team: team.id,
},
attach: rolesToAttach.length
? {
roles: rolesToAttach,
team: team.id,
}
: undefined,
detach: rolesToDetach.length
? {
roles: rolesToDetach,
team: team.id,
}
: undefined,
},
})
.then(() => {
Expand Down
Loading