[AC-1747] deprecate access control indicator

[willmartian]

Type of change

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

Objective

This PR hides the ability to grant a user or group access to all collections, based on FeatureFlag.FlexibleCollections.

Code changes

• apps/web/src/app/admin-console/organizations/core/services/group/group.service.ts: When creating a GroupView, set accessAll to false if the feature flag is enabled.
• apps/web/src/app/admin-console/organizations/core/services/user-admin.service.ts: When creating a OrganizationUserAdminView, set accessAll to false if the feature flag is enabled.
• apps/web/src/app/admin-console/organizations/core/services/group/responses/group.response.ts: Add deprecation comment to accessAll field.
• apps/web/src/app/admin-console/organizations/core/views/group.view.ts: Add deprecation comment to accessAll field.
• apps/web/src/app/admin-console/organizations/core/views/organization-user-admin-view.ts: Add deprecation comment to accessAll field.
• libs/common/src/admin-console/abstractions/organization-user/responses/organization-user.response.ts: Add deprecation comment to accessAll field.
• apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.html: Hide checkbox if feature flag is true
• apps/web/src/app/admin-console/organizations/manage/group-add-edit.component.html: Hide checkbox if feature flag is true

Screenshots

FeatureFlag.FlexibleCollections === true

Screen.Recording.2023-11-03.at.4.54.01.PM.mov

FeatureFlag.FlexibleCollections === false

Screen.Recording.2023-11-03.at.4.52.45.PM.mov

Before you submit

• 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
• Ensure that all UI additions follow WCAG AA requirements

[bitwarden-bot]

Checkmarx One – Scan Summary & Details – 4188b818-d559-443a-9c72-37871f8c62a3

No New Or Fixed Issues Found

[eliykat]

These changes generally look good to me, thanks @willmartian!

OrganizationUserView also has an AccessAll property which doesn't look like it's dealt with here. It's consumed in PeopleComponent: https://github.com/bitwarden/clients/blob/ac/AC-1747/deprecate-access-control-indicator/apps/web/src/app/admin-console/organizations/members/people.component.ts#L244-L254

Do you think there's any benefit to overriding the value in the static fromResponse methods, so that you cannot actually instantiate it without having to deal with the feature flag logic? I don't feel strongly about it because it looks like you've caught it in all places anyway. But it would be extra protection against someone instantiating the object without properly handling this.

[willmartian]

OrganizationUserView also has an AccessAll property which doesn't look like it's dealt with here. It's consumed in PeopleComponent: https://github.com/bitwarden/clients/blob/ac/AC-1747/deprecate-access-control-indicator/apps/web/src/app/admin-console/organizations/members/people.component.ts#L244-L254

Good catch, fixed in 86bcc6a and 582a524.

Do you think there's any benefit to overriding the value in the static fromResponse methods, so that you cannot actually instantiate it without having to deal with the feature flag logic? I don't feel strongly about it because it looks like you've caught it in all places anyway. But it would be extra protection against someone instantiating the object without properly handling this.

Agreed, the extra protection would be nice, but:

1. Not all view models have fromResponse methods (such as the OrganizationUserAdminView) and would need to be dealt with wherever they are instantiated via constructor.
2. Even if a view models has a fromResponse method, there is nothing stopping a dev from just using the constructor instead.

So I think we would need to be equally careful whether or not a parameter is added to the fromResponse method... which says to me to not even add it in the first place.