Syncing a role removes roles across ALL teams

[judgej]

This doesn'r seem right, and is certainly inconsistent. Is it a bug? Feels like it.

I have teams set up, omn release 5.10.1, so the latest at this time. I have a bunch of global roles, that is roles not assigned to a particular team, so are common to all teams and used by all teams.

I and logged in and have a team selected, and want to remove a user from the current team. Before removing the user, I want to remove all their roles from the current team. The user is a member of other teams, and has roles on those other teams too.

So the obvious way to remove the roles is:

$user->syncRoles([]);

That will remove all roles and add back in no roles.

However, it removes the user's roles from ALL the teams. It does not use the team context to detatch those roles. It does this using:

$this->roles()->detach(); // $this = user

That does not take into account the pivot table context, which will have the team ID in, so roles for all teams are removed.

Now, the next step is to add the new synced roles back in:

$this->assignRole($roles);

Looking at assignRole(), that method does take the current team into account when assigning the new roles. That's the inconsistency.

I think removeRole() is the same - does not look at the team context.

[judgej]

The relationship between users and roles is a many-to-many, with the team ID on the pivot context and a part of the primary key, so any pair of user and role instances can have multiple relationships - one for each team that user is a member of.

[judgej]

There is another level to the team-based functionality too - being able to work with specified teams. A super-admin may have access to a full list of users, and they may need to add or remove roles from those users for specific teams they are on. Switching the context to the team is one way to do this, but not needing to switch context would be useful. Perhaps working in a closure that switches team context for what runs in it, then switches back on finishing could be one approach.

I can go direct to the models for my application, but I'm trying to avoid that where possible and stick to the public interface to reduce surprises in future releases.

[parallels999]

Read other threads before opening new ones, everything you write has already been discussed before, and possible solutions have been proposed.

[parallels999]

I'm sure that's caused by your customizations in #2446 (comment), try it on a fresh installation

Related: #2088 (reply in thread)

Tests:

laravel-permission/tests/TeamHasRolesTest.php

Lines 87 to 118 in 9d7ea6d

	public function it_can_sync_or_remove_roles_without_detach_on_different_teams()
	{
	app(Role::class)->create(['name' => 'testRole3', 'team_test_id' => 2]);
	setPermissionsTeamId(1);
	$this->testUser->syncRoles('testRole', 'testRole2');
	setPermissionsTeamId(2);
	$this->testUser->syncRoles('testRole', 'testRole3');
	setPermissionsTeamId(1);
	$this->testUser->load('roles');
	$this->assertEquals(
	collect(['testRole', 'testRole2']),
	$this->testUser->getRoleNames()->sort()->values()
	);
	$this->testUser->removeRole('testRole');
	$this->assertEquals(
	collect(['testRole2']),
	$this->testUser->getRoleNames()->sort()->values()
	);
	setPermissionsTeamId(2);
	$this->testUser->load('roles');
	$this->assertEquals(
	collect(['testRole', 'testRole3']),
	$this->testUser->getRoleNames()->sort()->values()
	);
	}

[judgej]

Looks like it. The detach needs the exlicit pivot to filter on the correct pivot records. Replacing the wherePivot() with a join interferes with this. The only reason for replacing it with a join is that eloqeuent only supports

pivot_condition AND pivot_condition [ AND ...]

It does not support OR conditions, which is a shame. The result on the SQL for a many-to-many join is the same whether using a wherePivot() or a where(), but the first method must tell eloquent to use the pivot query in a detach() while the second method does not, and just works from the list of roles.

So we are stuck, out of the box, needing to add special users to roles in every single team that needs it, apart from the single super admin that has god-like privileges. It also means having to add those special users to each team for those roles to work, which I didn't want to do.

Another approach may be to keep this permissions package only for privileges of team members, and use a completely separate guard for non-team members (with elevated privileges).

Or maybe I sleep on it and think of another approach.

My problem remains: I have a set of roles that can be assigned to users within a team. All teams share those same roles. I have a set of roles that can be assigned to users globally, that work across all teams and do not require a user to be a member of those teams.

[parallels999]

the detach needs the exlicit pivot to filter on the correct pivot records .... It does not support OR conditions

maybe they accept PRs in laravel for this

[judgej]

I'll have a play with it and see what I can come up with. I can see what the pivot methods are trying to do - they are (I suspect) trying to enforce the pivot table name usage against the conditions, because those columns may clash with the main table column (like team_id does). I think is also allows the pivot conditions to be constructed in the query in a slightly different way for the detach() function. But it does mean bunch of methods just for the pivot that are trying to do any filtering on the pivot that anyone may want to do, which is never going to cover all bases.

Each pivot method has both an and and an or version. What it doesn't do, is wrap multiple conditions in parenthesis when generating the query, so the or variants break the pivot query completely. That would probably be my first line of attack. Still doesn't cover using both or and and, but it's a step.

Anyway, just thinking out loud, and going way off-topic, but hopefully putting ideas out in public will help others that may be coming to similar problems, or have solved it other ways.

[drbyte]

Responding to your first post above:

You are correct: syncRoles() is not team-aware. Nor is removeRole().

While not exactly perfect, I believe you could accomplish something akin to syncRoles by looping through all the teams the particular user is assigned to and attaching back the necessary records/pivots. (Obviously querying and remembering them before detaching everything, and using that to attach what you want to keep.)

$this->roles()->attach($roles_to_restore, $teamPivot);

I haven't explored it in detail, but an alternate approach might be: instead of wiping all and adding new, you could first query which records to specifically detach and pass those to the detach($ids) call:
https://github.com/laravel/framework/blob/f3bc33e1043139632098c924fa97a6afb8037bab/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php#L431-L466

EDIT: I read further, and I see that you are indeed exploring more about working directly with the detach() call.

[judgej]

It actually turns out it is team aware. The team relationship needs to be implemented through an explicit pivot condition, and that's what tells laravel to take the team into account when unallocating. When the relationship doesn't use a pivot, but simple inner joins instead (which I was doing because laravel pivot conditions did not seem to implement an "or" logic without breaking the relationship) then laravel totally ignores any conditions on the pivot table when unallocationg (resulting in deleting the joins for ALL teams).

I've worked around this now, detailed in a layer post, but in summary:

1. Set global (all team) permissions on team 0 (zero).
2. Sync changes to team 0 with all other teams. This isn't a sync(), it is just adding and removing a subset of "global" admin roles, not normally assigned within teams.

I go direct to the database for (2) since the team conditions on the relationship would otherwise get in the way.