[AC-1126] Flexible collections: Deprecate manager role

[r-tome]

Type of change

- [ ] Bug fix
- [X] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

With Flexible Collections enabled the Manager role is deprecated.

Code changes

This is targeting #3360 so that should be merged first.

• src/Core/Context/CurrentContext.cs: Only add the organizations that the user is the manager of if Flexible Collections feature flag is not enabled
• src/Core/Context/ICurrentContext.cs: Marked the method OrganizationManager as obsolete
• src/Core/Services/Implementations/OrganizationService.cs: Reject any attempt to assign the Manager role to a user if the feature flag is enabled
• test/Core.Test/Services/OrganizationServiceTests.cs: Added unit tests to check that any attempt to assign the Manager role is blocked if the feature flag is enabled

Before you submit

• Please check for formatting errors (dotnet format --verify-no-changes) (required)
• If making database changes - make sure you also update Entity Framework queries and/or migrations
• Please add unit tests where it makes sense to do so (encouraged but not required)
• If this change requires a documentation update - notify the documentation team
• If this change has particular deployment requirements - notify the DevOps team

[eliykat]

This is a very small changeset, am I right in thinking that basically all the work was already done in #3360? That is, by deprecating those CurrentContext checks, we got this ticket for (almost) free?

In that case - lgtm! Although you've been following @vincentsalucci's feedback above in other PRs, but you haven't made that change here (it's still called UseFlexibleCollections - my wording, but I don't feel strongly about it). Once that's done, this should be good to go.

It's a small changeset behind flags, so I think it can merged without QA.

[r-tome]

I have renamed UseFlexibleCollections to FlexibleCollectionsIsEnabled. UseFlexibleCollections was the old name we were using everywhere which is why it was also here.
@eliykat yes all the work is mostly done in #3360 hence why this PR is targeting that branch, so that should be merged first.

The base branch was changed.

[bitwarden-bot]

Checkmarx One – Scan Summary & Details – 90678184-a18c-435c-8723-718b171bb4d3

New Issues

Severity	Issue	Source File / Package	Checkmarx Insight
	Unpinned Actions Full Length Commit SHA	/version-bump.yml: 47	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
	Unpinned Actions Full Length Commit SHA	/version-bump.yml: 26	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...

Fixed Issues

Severity	Issue	Source File / Package
	Unpinned Actions Full Length Commit SHA	/version-bump.yml: 33
	Unpinned Actions Full Length Commit SHA	/version-bump.yml: 80

[eliykat]

Unrelated to this PR: is it correct that we call ValidateOrganizationUserUpdatePermissions here:

server/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserGroupsCommand.cs

Lines 27 to 31 in 5152e4b

	if (loggedInUserId.HasValue)
	{
	await _organizationService.ValidateOrganizationUserUpdatePermissions(organizationUser.OrganizationId, organizationUser.Type, null, organizationUser.GetPermissions());
	}
	await _organizationUserRepository.UpdateGroupsAsync(organizationUser.Id, groupIds);

?

That command is updating user-group associations, not the user role.

[r-tome]

Unrelated to this PR: is it correct that we call ValidateOrganizationUserUpdatePermissions here:

server/src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateOrganizationUserGroupsCommand.cs

Lines 27 to 31 in 5152e4b

	if (loggedInUserId.HasValue)
	{
	await _organizationService.ValidateOrganizationUserUpdatePermissions(organizationUser.OrganizationId, organizationUser.Type, null, organizationUser.GetPermissions());
	}
	await _organizationUserRepository.UpdateGroupsAsync(organizationUser.Id, groupIds);

?
That command is updating user-group associations, not the user role.

That does look strange. On one hand we're passing oldType = null so it is simply checking if the user saving the group associations has permission to do that (Owner, Admin, or can Manage Users). But then it also checks custom user permissions (while they're not being saved here) so it could block saving group associations based on that. Should we create a separate ticket to look into this?

[eliykat]

Created a follow-up ticket here: https://bitwarden.atlassian.net/browse/AC-1961