bitwarden/browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser synced 2025-12-15 15:53:27 +00:00
Code Issues Releases Wiki Activity

[PM-17154] Limit item deletion feature flag removal (#15094)

Browse Source 
* Refactor components to remove limitItemDeletion feature flag usage

This commit simplifies the logic in various components by removing the limitItemDeletion feature flag. The conditions for displaying restore and delete actions are now based solely on the cipher's permissions, enhancing code clarity and maintainability.

* Refactor cipher deletion logic to remove the feature flag and collection ID dependency

This commit updates the cipher deletion logic across multiple components and services by removing the unnecessary dependency on collection IDs. The `canDeleteCipher$` method now solely relies on the cipher's permissions, simplifying the code and improving maintainability.

* Remove LimitItemDeletion feature flag from feature-flag enum and default values

* Remove configService from ServiceContainer and MainBackground constructor parameters

* Remove configService from RestoreCommand instantiation in OssServeConfigurator and VaultProgram classes
This commit is contained in:
Rui Tomé
2025-06-10 09:57:34 +01:00
committed by GitHub
parent 159cca8cfa
commit b5bddd0b06
26 changed files with 55 additions and 339 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
libs/common/src/enums/feature-flag.enum.ts
View File

@@ -11,7 +11,6 @@ import { ServerConfig } from "../platform/abstractions/config/server-config";
// eslint-disable-next-line @bitwarden/platform/no-enums
export enum FeatureFlag {
/* Admin Console Team */
LimitItemDeletion = "pm-15493-restrict-item-deletion-to-can-manage-permission",
SeparateCustomRolePermissions = "pm-19917-separate-custom-role-permissions",
OptimizeNestedTraverseTypescript = "pm-21695-optimize-nested-traverse-typescript",
@@ -75,7 +74,6 @@ const FALSE = false as boolean;
*/
export const DefaultFeatureFlagValue = {
/* Admin Console Team */
[FeatureFlag.LimitItemDeletion]: FALSE,
[FeatureFlag.SeparateCustomRolePermissions]: FALSE,
[FeatureFlag.OptimizeNestedTraverseTypescript]: FALSE,

136
libs/common/src/vault/services/cipher-authorization.service.spec.ts
View File

@@ -7,10 +7,9 @@ import { CollectionService, CollectionView } from "@bitwarden/admin-console/comm
import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
import { Organization } from "@bitwarden/common/admin-console/models/domain/organization";
import { Utils } from "@bitwarden/common/platform/misc/utils";
import { CollectionId, UserId } from "@bitwarden/common/types/guid";
import { UserId } from "@bitwarden/common/types/guid";
import { FakeAccountService, mockAccountServiceWith } from "../../../spec";
import { ConfigService } from "../../platform/abstractions/config/config.service";
import { CipherPermissionsApi } from "../models/api/cipher-permissions.api";
import { CipherView } from "../models/view/cipher.view";
@@ -24,7 +23,6 @@ describe("CipherAuthorizationService", () => {
const mockCollectionService = mock<CollectionService>();
const mockOrganizationService = mock<OrganizationService>();
const mockConfigService = mock<ConfigService>();
const mockUserId = Utils.newGuid() as UserId;
let mockAccountService: FakeAccountService;
@@ -70,10 +68,7 @@ describe("CipherAuthorizationService", () => {
mockCollectionService,
mockOrganizationService,
mockAccountService,
mockConfigService,
);
mockConfigService.getFeatureFlag$.mockReturnValue(of(false));
});
describe("canRestoreCipher$", () => {
@@ -90,7 +85,7 @@ describe("CipherAuthorizationService", () => {
});
});
it("should return true if isAdminConsleAction and user can edit all ciphers in the org", (done) => {
it("should return true if isAdminConsoleAction and user can edit all ciphers in the org", (done) => {
const cipher = createMockCipher("org1", ["col1"]) as CipherView;
const organization = createMockOrganization({ canEditAllCiphers: true });
mockOrganizationService.organizations$.mockReturnValue(
@@ -145,15 +140,6 @@ describe("CipherAuthorizationService", () => {
});
describe("canDeleteCipher$", () => {
it("should return true if cipher has no organizationId", (done) => {
const cipher = createMockCipher(null, []) as CipherView;
cipherAuthorizationService.canDeleteCipher$(cipher).subscribe((result) => {
expect(result).toBe(true);
done();
});
});
it("should return true if isAdminConsoleAction is true and cipher is unassigned", (done) => {
const cipher = createMockCipher("org1", []) as CipherView;
const organization = createMockOrganization({ canEditUnassignedCiphers: true });
@@ -161,7 +147,7 @@ describe("CipherAuthorizationService", () => {
of([organization]) as Observable<Organization[]>,
);
cipherAuthorizationService.canDeleteCipher$(cipher, [], true).subscribe((result) => {
cipherAuthorizationService.canDeleteCipher$(cipher, true).subscribe((result) => {
expect(result).toBe(true);
done();
});
@@ -174,7 +160,7 @@ describe("CipherAuthorizationService", () => {
of([organization]) as Observable<Organization[]>,
);
cipherAuthorizationService.canDeleteCipher$(cipher, [], true).subscribe((result) => {
cipherAuthorizationService.canDeleteCipher$(cipher, true).subscribe((result) => {
expect(result).toBe(true);
expect(mockOrganizationService.organizations$).toHaveBeenCalledWith(mockUserId);
done();
@@ -186,136 +172,32 @@ describe("CipherAuthorizationService", () => {
const organization = createMockOrganization({ canEditUnassignedCiphers: false });
mockOrganizationService.organizations$.mockReturnValue(of([organization] as Organization[]));
cipherAuthorizationService.canDeleteCipher$(cipher, [], true).subscribe((result) => {
cipherAuthorizationService.canDeleteCipher$(cipher, true).subscribe((result) => {
expect(result).toBe(false);
done();
});
});
it("should return true if activeCollectionId is provided and has manage permission", (done) => {
const cipher = createMockCipher("org1", ["col1", "col2"]) as CipherView;
const activeCollectionId = "col1" as CollectionId;
const organization = createMockOrganization();
mockOrganizationService.organizations$.mockReturnValue(of([organization] as Organization[]));
const allCollections = [
createMockCollection("col1", true),
createMockCollection("col2", false),
];
mockCollectionService.decryptedCollectionViews$.mockReturnValue(
of(allCollections as CollectionView[]),
);
cipherAuthorizationService
.canDeleteCipher$(cipher, [activeCollectionId])
.subscribe((result) => {
expect(result).toBe(true);
expect(mockCollectionService.decryptedCollectionViews$).toHaveBeenCalledWith([
"col1",
"col2",
] as CollectionId[]);
done();
});
});
it("should return false if activeCollectionId is provided and manage permission is not present", (done) => {
const cipher = createMockCipher("org1", ["col1", "col2"]) as CipherView;
const activeCollectionId = "col1" as CollectionId;
const organization = createMockOrganization();
mockOrganizationService.organizations$.mockReturnValue(of([organization] as Organization[]));
const allCollections = [
createMockCollection("col1", false),
createMockCollection("col2", true),
];
mockCollectionService.decryptedCollectionViews$.mockReturnValue(
of(allCollections as CollectionView[]),
);
cipherAuthorizationService
.canDeleteCipher$(cipher, [activeCollectionId])
.subscribe((result) => {
expect(result).toBe(false);
expect(mockCollectionService.decryptedCollectionViews$).toHaveBeenCalledWith([
"col1",
"col2",
] as CollectionId[]);
done();
});
});
it("should return true if any collection has manage permission", (done) => {
const cipher = createMockCipher("org1", ["col1", "col2", "col3"]) as CipherView;
const organization = createMockOrganization();
mockOrganizationService.organizations$.mockReturnValue(of([organization] as Organization[]));
const allCollections = [
createMockCollection("col1", false),
createMockCollection("col2", true),
createMockCollection("col3", false),
];
mockCollectionService.decryptedCollectionViews$.mockReturnValue(
of(allCollections as CollectionView[]),
);
cipherAuthorizationService.canDeleteCipher$(cipher).subscribe((result) => {
expect(result).toBe(true);
expect(mockCollectionService.decryptedCollectionViews$).toHaveBeenCalledWith([
"col1",
"col2",
"col3",
] as CollectionId[]);
done();
});
});
it("should return false if no collection has manage permission", (done) => {
const cipher = createMockCipher("org1", ["col1", "col2"]) as CipherView;
const organization = createMockOrganization();
mockOrganizationService.organizations$.mockReturnValue(of([organization] as Organization[]));
const allCollections = [
createMockCollection("col1", false),
createMockCollection("col2", false),
];
mockCollectionService.decryptedCollectionViews$.mockReturnValue(
of(allCollections as CollectionView[]),
);
cipherAuthorizationService.canDeleteCipher$(cipher).subscribe((result) => {
expect(result).toBe(false);
expect(mockCollectionService.decryptedCollectionViews$).toHaveBeenCalledWith([
"col1",
"col2",
] as CollectionId[]);
done();
});
});
it("should return true if feature flag enabled and cipher.permissions.delete is true", (done) => {
it("should return true when cipher.permissions.delete is true", (done) => {
const cipher = createMockCipher("org1", [], true, {
delete: true,
} as CipherPermissionsApi) as CipherView;
const organization = createMockOrganization();
mockOrganizationService.organizations$.mockReturnValue(of([organization] as Organization[]));
mockConfigService.getFeatureFlag$.mockReturnValue(of(true));
cipherAuthorizationService.canDeleteCipher$(cipher, [], false).subscribe((result) => {
cipherAuthorizationService.canDeleteCipher$(cipher, false).subscribe((result) => {
expect(result).toBe(true);
expect(mockCollectionService.decryptedCollectionViews$).not.toHaveBeenCalled();
done();
});
});
it("should return false if feature flag enabled and cipher.permissions.delete is false", (done) => {
it("should return false when cipher.permissions.delete is false", (done) => {
const cipher = createMockCipher("org1", []) as CipherView;
const organization = createMockOrganization();
mockOrganizationService.organizations$.mockReturnValue(of([organization] as Organization[]));
mockConfigService.getFeatureFlag$.mockReturnValue(of(true));
cipherAuthorizationService.canDeleteCipher$(cipher, [], false).subscribe((result) => {
cipherAuthorizationService.canDeleteCipher$(cipher, false).subscribe((result) => {
expect(result).toBe(false);
expect(mockCollectionService.decryptedCollectionViews$).not.toHaveBeenCalled();
done();
});
});

46
libs/common/src/vault/services/cipher-authorization.service.ts
View File

@@ -1,15 +1,13 @@
import { combineLatest, map, Observable, of, shareReplay, switchMap } from "rxjs";
import { map, Observable, of, shareReplay, switchMap } from "rxjs";
// This import has been flagged as unallowed for this class. It may be involved in a circular dependency loop.
// eslint-disable-next-line no-restricted-imports
import { CollectionService } from "@bitwarden/admin-console/common";
import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { CollectionId } from "@bitwarden/common/types/guid";
import { getUserId } from "../../auth/services/account.service";
import { FeatureFlag } from "../../enums/feature-flag.enum";
import { Cipher } from "../models/domain/cipher";
import { CipherView } from "../models/view/cipher.view";
@@ -26,14 +24,12 @@ export abstract class CipherAuthorizationService {
* Determines if the user can delete the specified cipher.
*
* @param {CipherLike} cipher - The cipher object to evaluate for deletion permissions.
* @param {CollectionId[]} [allowedCollections] - Optional. The selected collection id from the vault filter.
* @param {boolean} isAdminConsoleAction - Optional. A flag indicating if the action is being performed from the admin console.
*
* @returns {Observable<boolean>} - An observable that emits a boolean value indicating if the user can delete the cipher.
*/
abstract canDeleteCipher$: (
cipher: CipherLike,
allowedCollections?: CollectionId[],
isAdminConsoleAction?: boolean,
) => Observable<boolean>;
@@ -72,7 +68,6 @@ export class DefaultCipherAuthorizationService implements CipherAuthorizationSer
private collectionService: CollectionService,
private organizationService: OrganizationService,
private accountService: AccountService,
private configService: ConfigService,
) {}
private organization$ = (cipher: CipherLike) =>
@@ -86,48 +81,21 @@ export class DefaultCipherAuthorizationService implements CipherAuthorizationSer
*
* {@link CipherAuthorizationService.canDeleteCipher$}
*/
canDeleteCipher$(
cipher: CipherLike,
allowedCollections?: CollectionId[],
isAdminConsoleAction?: boolean,
): Observable<boolean> {
return combineLatest([
this.organization$(cipher),
this.configService.getFeatureFlag$(FeatureFlag.LimitItemDeletion),
]).pipe(
switchMap(([organization, featureFlagEnabled]) => {
canDeleteCipher$(cipher: CipherLike, isAdminConsoleAction?: boolean): Observable<boolean> {
return this.organization$(cipher).pipe(
map((organization) => {
if (isAdminConsoleAction) {
// If the user is an admin, they can delete an unassigned cipher
if (!cipher.collectionIds || cipher.collectionIds.length === 0) {
return of(organization?.canEditUnassignedCiphers === true);
return organization?.canEditUnassignedCiphers === true;
}
if (organization?.canEditAllCiphers) {
return of(true);
return true;
}
}
if (featureFlagEnabled) {
return of(cipher.permissions.delete);
}
if (cipher.organizationId == null) {
return of(true);
}
return this.collectionService
.decryptedCollectionViews$(cipher.collectionIds as CollectionId[])
.pipe(
map((allCollections) => {
const shouldFilter = allowedCollections?.some(Boolean);
const collections = shouldFilter
? allCollections.filter((c) => allowedCollections?.includes(c.id as CollectionId))
: allCollections;
return collections.some((collection) => collection.manage);
}),
);
return cipher.permissions.delete;
}),
);
}