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

[PM-11360] Remove export permission for providers #12062

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion apps/cli/src/tools/import.command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export class ImportCommand {
);
}

if (!organization.canAccessImportExport) {
if (!organization.canAccessImport) {
return Response.badRequest(
"You are not authorized to import into the provided organization.",
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,12 @@
<bit-nav-item
[text]="'importData' | i18n"
route="settings/tools/import"
*ngIf="organization.canAccessImportExport"
*ngIf="organization.canAccessImport"
></bit-nav-item>
<bit-nav-item
[text]="'exportVault' | i18n"
route="settings/tools/export"
*ngIf="organization.canAccessImportExport"
*ngIf="canAccessExport$ | async"
></bit-nav-item>
<bit-nav-item
[text]="'domainVerification' | i18n"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { CommonModule } from "@angular/common";
import { Component, OnDestroy, OnInit } from "@angular/core";
import { Component, OnInit } from "@angular/core";
import { ActivatedRoute, RouterModule } from "@angular/router";
import { combineLatest, map, mergeMap, Observable, Subject, switchMap, takeUntil } from "rxjs";
import { combineLatest, filter, map, Observable, switchMap } from "rxjs";

import { JslibModule } from "@bitwarden/angular/jslib.module";
import {
Expand All @@ -12,7 +12,6 @@
canAccessReportingTab,
canAccessSettingsTab,
canAccessVaultTab,
getOrganizationById,
OrganizationService,
} from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction";
Expand All @@ -22,6 +21,7 @@
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { getById } from "@bitwarden/common/platform/misc";
import { BannerModule, IconModule } from "@bitwarden/components";

import { OrgSwitcherComponent } from "../../../layouts/org-switcher/org-switcher.component";
Expand All @@ -42,19 +42,18 @@
BannerModule,
],
})
export class OrganizationLayoutComponent implements OnInit, OnDestroy {
export class OrganizationLayoutComponent implements OnInit {
protected readonly logo = AdminConsoleLogo;

protected orgFilter = (org: Organization) => canAccessOrgAdmin(org);

organization$: Observable<Organization>;
canAccessExport$: Observable<boolean>;
showPaymentAndHistory$: Observable<boolean>;
hideNewOrgButton$: Observable<boolean>;
organizationIsUnmanaged$: Observable<boolean>;
isAccessIntelligenceFeatureEnabled = false;

private _destroy = new Subject<void>();

constructor(
private route: ActivatedRoute,
private organizationService: OrganizationService,
Expand All @@ -71,23 +70,23 @@
FeatureFlag.AccessIntelligence,
);

this.organization$ = this.route.params
.pipe(takeUntil(this._destroy))
.pipe<string>(map((p) => p.organizationId))
.pipe(
mergeMap((id) => {
return this.organizationService.organizations$
.pipe(takeUntil(this._destroy))
.pipe(getOrganizationById(id));
}),
);
this.organization$ = this.route.params.pipe(
map((p) => p.organizationId),
switchMap((id) => this.organizationService.organizations$.pipe(getById(id))),
filter((org) => org != null),

Check warning on line 76 in apps/web/src/app/admin-console/organizations/layouts/organization-layout.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/admin-console/organizations/layouts/organization-layout.component.ts#L73-L76

Added lines #L73 - L76 were not covered by tests
);

this.canAccessExport$ = combineLatest([

Check warning on line 79 in apps/web/src/app/admin-console/organizations/layouts/organization-layout.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/admin-console/organizations/layouts/organization-layout.component.ts#L79

Added line #L79 was not covered by tests
this.organization$,
this.configService.getFeatureFlag$(FeatureFlag.PM11360RemoveProviderExportPermission),
]).pipe(map(([org, removeProviderExport]) => org.canAccessExport(removeProviderExport)));

Check warning on line 82 in apps/web/src/app/admin-console/organizations/layouts/organization-layout.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/admin-console/organizations/layouts/organization-layout.component.ts#L82

Added line #L82 was not covered by tests

this.showPaymentAndHistory$ = this.organization$.pipe(
map(
(org) =>
!this.platformUtilsService.isSelfHost() &&
org?.canViewBillingHistory &&
org?.canEditPaymentMethods,
org.canViewBillingHistory &&
org.canEditPaymentMethods,
),
);

Expand All @@ -107,11 +106,6 @@
);
}

ngOnDestroy() {
this._destroy.next();
this._destroy.complete();
}

canShowVaultTab(organization: Organization): boolean {
return canAccessVaultTab(organization);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import { NgModule } from "@angular/core";
import { RouterModule, Routes } from "@angular/router";
import { inject, NgModule } from "@angular/core";
import { CanMatchFn, RouterModule, Routes } from "@angular/router";
import { map } from "rxjs";

Check warning on line 3 in apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts#L1-L3

Added lines #L1 - L3 were not covered by tests

import { canAccessSettingsTab } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction";
import { Organization } from "@bitwarden/common/admin-console/models/domain/organization";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";

Check warning on line 8 in apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts#L7-L8

Added lines #L7 - L8 were not covered by tests

import { organizationPermissionsGuard } from "../../organizations/guards/org-permissions.guard";
import { organizationRedirectGuard } from "../../organizations/guards/org-redirect.guard";
Expand All @@ -11,6 +14,11 @@
import { AccountComponent } from "./account.component";
import { TwoFactorSetupComponent } from "./two-factor-setup.component";

const removeProviderExportPermission$: CanMatchFn = () =>
inject(ConfigService)

Check warning on line 18 in apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts#L17-L18

Added lines #L17 - L18 were not covered by tests
.getFeatureFlag$(FeatureFlag.PM11360RemoveProviderExportPermission)
.pipe(map((removeProviderExport) => removeProviderExport === true));

Check warning on line 20 in apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts#L20

Added line #L20 was not covered by tests

const routes: Routes = [
{
path: "",
Expand Down Expand Up @@ -53,18 +61,32 @@
path: "import",
loadComponent: () =>
import("./org-import.component").then((mod) => mod.OrgImportComponent),
canActivate: [organizationPermissionsGuard((org) => org.canAccessImportExport)],
canActivate: [organizationPermissionsGuard((org) => org.canAccessImport)],

Check warning on line 64 in apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts#L64

Added line #L64 was not covered by tests
data: {
titleId: "importData",
},
},

// Export routing is temporarily duplicated to set the flag value passed into org.canAccessExport
{
path: "export",
loadComponent: () =>
import("../tools/vault-export/org-vault-export.component").then(
(mod) => mod.OrganizationVaultExportComponent,

Check warning on line 75 in apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts#L74-L75

Added lines #L74 - L75 were not covered by tests
),
canMatch: [removeProviderExportPermission$], // if this matches, the flag is ON
canActivate: [organizationPermissionsGuard((org) => org.canAccessExport(true))],

Check warning on line 78 in apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts#L78

Added line #L78 was not covered by tests
data: {
titleId: "exportVault",
},
},
{
path: "export",
loadComponent: () =>
import("../tools/vault-export/org-vault-export.component").then(
(mod) => mod.OrganizationVaultExportComponent,
),
canActivate: [organizationPermissionsGuard((org) => org.canAccessImportExport)],
canActivate: [organizationPermissionsGuard((org) => org.canAccessExport(false))],

Check warning on line 89 in apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/admin-console/organizations/settings/organization-settings-routing.module.ts#L89

Added line #L89 was not covered by tests
data: {
titleId: "exportVault",
},
Expand All @@ -82,7 +104,7 @@
if (organization.canManagePolicies) {
return "policies";
}
if (organization.canAccessImportExport) {
if (organization.canAccessImport) {
return ["tools", "import"];
}
if (organization.canManageSso) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,12 @@ describe("ProductSwitcherService", () => {

it("is included in bento when there is an organization with SM", async () => {
organizationService.organizations$ = of([
{ id: "1234", canAccessSecretsManager: true, enabled: true },
{
id: "1234",
canAccessSecretsManager: true,
enabled: true,
canAccessExport: (_) => true,
},
] as Organization[]);

initiateService();
Expand Down Expand Up @@ -220,8 +225,20 @@ describe("ProductSwitcherService", () => {
router.url = "/sm/4243";

organizationService.organizations$ = of([
{ id: "23443234", canAccessSecretsManager: true, enabled: true, name: "Org 2" },
{ id: "4243", canAccessSecretsManager: true, enabled: true, name: "Org 32" },
{
id: "23443234",
canAccessSecretsManager: true,
enabled: true,
name: "Org 2",
canAccessExport: (_) => true,
},
{
id: "4243",
canAccessSecretsManager: true,
enabled: true,
name: "Org 32",
canAccessExport: (_) => true,
},
] as Organization[]);

initiateService();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { map, Observable } from "rxjs";

import { I18nService } from "../../../platform/abstractions/i18n.service";
import { Utils } from "../../../platform/misc/utils";
import { UserId } from "../../../types/guid";
import { OrganizationData } from "../../models/data/organization.data";
import { Organization } from "../../models/domain/organization";
Expand All @@ -16,7 +14,8 @@ export function canAccessSettingsTab(org: Organization): boolean {
org.canManagePolicies ||
org.canManageSso ||
org.canManageScim ||
org.canAccessImportExport ||
org.canAccessImport ||
org.canAccessExport(false) || // Feature flag value doesn't matter here, providers will have access to this group anyway
org.canManageDeviceApprovals
);
}
Expand Down Expand Up @@ -56,32 +55,6 @@ export function getOrganizationById(id: string) {
return map<Organization[], Organization | undefined>((orgs) => orgs.find((o) => o.id === id));
}

export function canAccessAdmin(i18nService: I18nService) {
return map<Organization[], Organization[]>((orgs) =>
orgs.filter(canAccessOrgAdmin).sort(Utils.getSortFunction(i18nService, "name")),
);
}

/**
* @deprecated
* To be removed after Flexible Collections.
**/
export function canAccessImportExport(i18nService: I18nService) {
return map<Organization[], Organization[]>((orgs) =>
orgs
.filter((org) => org.canAccessImportExport)
.sort(Utils.getSortFunction(i18nService, "name")),
);
}

export function canAccessImport(i18nService: I18nService) {
return map<Organization[], Organization[]>((orgs) =>
orgs
.filter((org) => org.canAccessImportExport || org.canCreateNewCollections)
.sort(Utils.getSortFunction(i18nService, "name")),
);
}

/**
* Returns `true` if a user is a member of an organization (rather than only being a ProviderUser)
* @deprecated Use organizationService.organizations$ with a filter instead
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { map, Observable } from "rxjs";

import { I18nService } from "../../../platform/abstractions/i18n.service";
import { Utils } from "../../../platform/misc/utils";
import { UserId } from "../../../types/guid";
import { OrganizationData } from "../../models/data/organization.data";
import { Organization } from "../../models/domain/organization";
Expand All @@ -16,7 +14,8 @@ export function canAccessSettingsTab(org: Organization): boolean {
org.canManagePolicies ||
org.canManageSso ||
org.canManageScim ||
org.canAccessImportExport ||
org.canAccessImport ||
org.canAccessExport(false) || // Feature flag value doesn't matter here, providers will have access to this group anyway
org.canManageDeviceApprovals
);
}
Expand Down Expand Up @@ -56,20 +55,6 @@ export function getOrganizationById(id: string) {
return map<Organization[], Organization | undefined>((orgs) => orgs.find((o) => o.id === id));
}

export function canAccessAdmin(i18nService: I18nService) {
return map<Organization[], Organization[]>((orgs) =>
orgs.filter(canAccessOrgAdmin).sort(Utils.getSortFunction(i18nService, "name")),
);
}

export function canAccessImport(i18nService: I18nService) {
return map<Organization[], Organization[]>((orgs) =>
orgs
.filter((org) => org.canAccessImportExport || org.canCreateNewCollections)
.sort(Utils.getSortFunction(i18nService, "name")),
);
}

/**
* Publishes an observable stream of organizations. This service is meant to
* be used widely across Bitwarden as the primary way of fetching organizations.
Expand Down
23 changes: 21 additions & 2 deletions libs/common/src/admin-console/models/domain/organization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,27 @@
return (this.isAdmin || this.permissions.accessEventLogs) && this.useEvents;
}

get canAccessImportExport() {
return this.isAdmin || this.permissions.accessImportExport;
get canAccessImport() {
return (

Check warning on line 172 in libs/common/src/admin-console/models/domain/organization.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/admin-console/models/domain/organization.ts#L172

Added line #L172 was not covered by tests
this.isProviderUser ||
this.type === OrganizationUserType.Owner ||
this.type === OrganizationUserType.Admin ||
this.permissions.accessImportExport ||
this.canCreateNewCollections // To allow users to create collections and then import into them
);
}

canAccessExport(removeProviderExport: boolean) {
Copy link
Member Author

Choose a reason for hiding this comment

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

It's a bit awkward to make callers fetch their own feature flag value to pass in. I tried some other approaches, but introducing any observable or promise here makes pretty large waves through all its callers. This remains the simplest approach for now.

if (!removeProviderExport && this.isProviderUser) {
return true;

Check warning on line 183 in libs/common/src/admin-console/models/domain/organization.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/admin-console/models/domain/organization.ts#L183

Added line #L183 was not covered by tests
}

return (

Check warning on line 186 in libs/common/src/admin-console/models/domain/organization.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/admin-console/models/domain/organization.ts#L186

Added line #L186 was not covered by tests
this.isMember &&
Copy link
Member Author

@eliykat eliykat Nov 20, 2024

Choose a reason for hiding this comment

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

This is necessary because some of our model mapping is wrong. I've logged a bug here: https://bitwarden.atlassian.net/browse/PM-15078

(this.type === OrganizationUserType.Owner ||
this.type === OrganizationUserType.Admin ||
this.permissions.accessImportExport)
);
}

get canAccessReports() {
Expand Down
2 changes: 2 additions & 0 deletions libs/common/src/enums/feature-flag.enum.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export enum FeatureFlag {
NewDeviceVerificationTemporaryDismiss = "new-device-temporary-dismiss",
NewDeviceVerificationPermanentDismiss = "new-device-permanent-dismiss",
DisableFreeFamiliesSponsorship = "PM-12274-disable-free-families-sponsorship",
PM11360RemoveProviderExportPermission = "pm-11360-remove-provider-export-permission",
}

export type AllowedFeatureFlagTypes = boolean | number | string;
Expand Down Expand Up @@ -90,6 +91,7 @@ export const DefaultFeatureFlagValue = {
[FeatureFlag.NewDeviceVerificationTemporaryDismiss]: FALSE,
[FeatureFlag.NewDeviceVerificationPermanentDismiss]: FALSE,
[FeatureFlag.DisableFreeFamiliesSponsorship]: FALSE,
[FeatureFlag.PM11360RemoveProviderExportPermission]: FALSE,
} satisfies Record<FeatureFlag, AllowedFeatureFlagTypes>;

export type DefaultFeatureFlagValueType = typeof DefaultFeatureFlagValue;
Expand Down
Loading
Loading