From ad0c460687803af81b2b2b7aea084536f1ae4f61 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Mon, 17 Apr 2023 13:09:53 +1000 Subject: [PATCH] [EC-850] ProviderUser permissions should prevail over member permissions (#5162) * Apply provider permissions even if also member * Add org.isMember * Refactor: extract syncProfileOrganizations method * Change isNotProvider logic to isMember * Fix cascading org permissions * Add memberOrganizations$ observable --- .../vault/vault-select.component.ts | 12 +++---- .../organization-switcher.component.ts | 4 +-- apps/web/src/app/layouts/navbar.component.ts | 6 ++-- .../services/vault-filter.service.spec.ts | 1 + .../services/vault-filter.service.ts | 4 +-- .../angular/src/components/share.component.ts | 12 ++----- .../vault/components/add-edit.component.ts | 4 +-- .../services/vault-filter.service.ts | 6 ++-- .../organization/organization.service.spec.ts | 2 +- .../spec/services/policy.service.spec.ts | 2 +- .../organization.service.abstraction.ts | 13 +++++-- .../models/data/organization.data.ts | 12 ++++++- .../models/domain/organization.ts | 34 ++++++++++++++---- .../organization/organization.service.ts | 6 +++- .../services/policy/policy.service.ts | 4 +-- .../src/vault/services/sync/sync.service.ts | 36 ++++++++++++------- 16 files changed, 99 insertions(+), 59 deletions(-) diff --git a/apps/browser/src/vault/popup/components/vault/vault-select.component.ts b/apps/browser/src/vault/popup/components/vault/vault-select.component.ts index eb058913c7..aea920ad2a 100644 --- a/apps/browser/src/vault/popup/components/vault/vault-select.component.ts +++ b/apps/browser/src/vault/popup/components/vault/vault-select.component.ts @@ -17,11 +17,9 @@ import { BehaviorSubject, concatMap, map, merge, Observable, Subject, takeUntil import { I18nService } from "@bitwarden/common/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/abstractions/platformUtils.service"; -import { - isNotProviderUser, - OrganizationService, -} from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; +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/misc/utils"; import { VaultFilterService } from "../../../services/vault-filter.service"; @@ -101,11 +99,9 @@ export class VaultSelectComponent implements OnInit, OnDestroy { } async ngOnInit() { - this.organizations$ = this.organizationService.organizations$ + this.organizations$ = this.organizationService.memberOrganizations$ .pipe(takeUntil(this._destroy)) - .pipe( - map((orgs) => orgs.filter(isNotProviderUser).sort((a, b) => a.name.localeCompare(b.name))) - ); + .pipe(map((orgs) => orgs.sort(Utils.getSortFunction(this.i18nService, "name")))); this.organizations$ .pipe( diff --git a/apps/web/src/app/admin-console/components/organization-switcher.component.ts b/apps/web/src/app/admin-console/components/organization-switcher.component.ts index b3fd8ef8b6..518b99240d 100644 --- a/apps/web/src/app/admin-console/components/organization-switcher.component.ts +++ b/apps/web/src/app/admin-console/components/organization-switcher.component.ts @@ -4,7 +4,6 @@ import { map, Observable } from "rxjs"; import { I18nService } from "@bitwarden/common/abstractions/i18n.service"; import { canAccessAdmin, - isNotProviderUser, OrganizationService, } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; @@ -23,8 +22,7 @@ export class OrganizationSwitcherComponent implements OnInit { loaded = false; async ngOnInit() { - this.organizations$ = this.organizationService.organizations$.pipe( - map((orgs) => orgs.filter(isNotProviderUser)), + this.organizations$ = this.organizationService.memberOrganizations$.pipe( canAccessAdmin(this.i18nService), map((orgs) => orgs.sort(Utils.getSortFunction(this.i18nService, "name"))) ); diff --git a/apps/web/src/app/layouts/navbar.component.ts b/apps/web/src/app/layouts/navbar.component.ts index 2147024ef3..0b23093dc9 100644 --- a/apps/web/src/app/layouts/navbar.component.ts +++ b/apps/web/src/app/layouts/navbar.component.ts @@ -1,12 +1,11 @@ import { Component, OnInit } from "@angular/core"; -import { map, Observable } from "rxjs"; +import { Observable } from "rxjs"; import { I18nService } from "@bitwarden/common/abstractions/i18n.service"; import { MessagingService } from "@bitwarden/common/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/abstractions/platformUtils.service"; import { canAccessAdmin, - isNotProviderUser, OrganizationService, } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { ProviderService } from "@bitwarden/common/admin-console/abstractions/provider.service"; @@ -54,8 +53,7 @@ export class NavbarComponent implements OnInit { } this.providers = await this.providerService.getAll(); - this.organizations$ = this.organizationService.organizations$.pipe( - map((orgs) => orgs.filter(isNotProviderUser)), + this.organizations$ = this.organizationService.memberOrganizations$.pipe( canAccessAdmin(this.i18nService) ); } diff --git a/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.spec.ts b/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.spec.ts index c56d780f06..23aa0f7ffb 100644 --- a/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.spec.ts +++ b/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.spec.ts @@ -251,6 +251,7 @@ describe("vault filter service", () => { org.id = id; org.name = name; org.identifier = name; + org.isMember = true; return org; } diff --git a/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.ts b/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.ts index e4f605caf8..a36c41b81e 100644 --- a/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.ts +++ b/apps/web/src/app/vault/individual-vault/vault-filter/services/vault-filter.service.ts @@ -14,7 +14,7 @@ import { import { I18nService } from "@bitwarden/common/abstractions/i18n.service"; import { StateService } from "@bitwarden/common/abstractions/state.service"; import { - isNotProviderUser, + isMember, OrganizationService, } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; @@ -139,7 +139,7 @@ export class VaultFilterService implements VaultFilterServiceAbstraction { } if (orgs) { const orgNodes: TreeNode[] = []; - orgs.filter(isNotProviderUser).forEach((org) => { + orgs.filter(isMember).forEach((org) => { const orgCopy = org as OrganizationFilter; orgCopy.icon = "bwi-business"; const node = new TreeNode(orgCopy, headNode, orgCopy.name); diff --git a/libs/angular/src/components/share.component.ts b/libs/angular/src/components/share.component.ts index c70a7ddf25..286e4da00d 100644 --- a/libs/angular/src/components/share.component.ts +++ b/libs/angular/src/components/share.component.ts @@ -5,10 +5,7 @@ import { I18nService } from "@bitwarden/common/abstractions/i18n.service"; import { LogService } from "@bitwarden/common/abstractions/log.service"; import { PlatformUtilsService } from "@bitwarden/common/abstractions/platformUtils.service"; import { CollectionService } from "@bitwarden/common/admin-console/abstractions/collection.service"; -import { - isNotProviderUser, - OrganizationService, -} from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; +import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { OrganizationUserStatusType } from "@bitwarden/common/admin-console/enums"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { CollectionView } from "@bitwarden/common/admin-console/models/view/collection.view"; @@ -54,13 +51,10 @@ export class ShareComponent implements OnInit, OnDestroy { const allCollections = await this.collectionService.getAllDecrypted(); this.writeableCollections = allCollections.map((c) => c).filter((c) => !c.readOnly); - this.organizations$ = this.organizationService.organizations$.pipe( + this.organizations$ = this.organizationService.memberOrganizations$.pipe( map((orgs) => { return orgs - .filter( - (o) => - o.enabled && o.status === OrganizationUserStatusType.Confirmed && isNotProviderUser(o) - ) + .filter((o) => o.enabled && o.status === OrganizationUserStatusType.Confirmed) .sort(Utils.getSortFunction(this.i18nService, "name")); }) ); diff --git a/libs/angular/src/vault/components/add-edit.component.ts b/libs/angular/src/vault/components/add-edit.component.ts index 31fdecbd91..98cc02de1d 100644 --- a/libs/angular/src/vault/components/add-edit.component.ts +++ b/libs/angular/src/vault/components/add-edit.component.ts @@ -10,7 +10,7 @@ import { PlatformUtilsService } from "@bitwarden/common/abstractions/platformUti import { StateService } from "@bitwarden/common/abstractions/state.service"; import { CollectionService } from "@bitwarden/common/admin-console/abstractions/collection.service"; import { - isNotProviderUser, + isMember, OrganizationService, } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; @@ -192,7 +192,7 @@ export class AddEditComponent implements OnInit, OnDestroy { const orgs = await this.organizationService.getAll(); orgs - .filter(isNotProviderUser) + .filter(isMember) .sort(Utils.getSortFunction(this.i18nService, "name")) .forEach((o) => { if (o.enabled && o.status === OrganizationUserStatusType.Confirmed) { diff --git a/libs/angular/src/vault/vault-filter/services/vault-filter.service.ts b/libs/angular/src/vault/vault-filter/services/vault-filter.service.ts index 38ff921781..7bb216983d 100644 --- a/libs/angular/src/vault/vault-filter/services/vault-filter.service.ts +++ b/libs/angular/src/vault/vault-filter/services/vault-filter.service.ts @@ -4,7 +4,7 @@ import { firstValueFrom, from, mergeMap, Observable } from "rxjs"; import { StateService } from "@bitwarden/common/abstractions/state.service"; import { CollectionService } from "@bitwarden/common/admin-console/abstractions/collection.service"; import { - isNotProviderUser, + isMember, OrganizationService, } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; @@ -44,9 +44,7 @@ export class VaultFilterService implements DeprecatedVaultFilterServiceAbstracti async buildOrganizations(): Promise { let organizations = await this.organizationService.getAll(); if (organizations != null) { - organizations = organizations - .filter(isNotProviderUser) - .sort((a, b) => a.name.localeCompare(b.name)); + organizations = organizations.filter(isMember).sort((a, b) => a.name.localeCompare(b.name)); } return organizations; diff --git a/libs/common/spec/services/organization/organization.service.spec.ts b/libs/common/spec/services/organization/organization.service.spec.ts index 81ae60f947..df875d7748 100644 --- a/libs/common/spec/services/organization/organization.service.spec.ts +++ b/libs/common/spec/services/organization/organization.service.spec.ts @@ -160,7 +160,7 @@ describe("Organization Service", () => { }); function organizationData(id: string, name: string) { - const data = new OrganizationData({} as any); + const data = new OrganizationData({} as any, {} as any); data.id = id; data.name = name; data.identifier = "test"; diff --git a/libs/common/spec/services/policy.service.spec.ts b/libs/common/spec/services/policy.service.spec.ts index ab52aabc29..1cdb7c4931 100644 --- a/libs/common/spec/services/policy.service.spec.ts +++ b/libs/common/spec/services/policy.service.spec.ts @@ -401,7 +401,7 @@ describe("PolicyService", () => { status: OrganizationUserStatusType, managePolicies: boolean ) { - const organizationData = new OrganizationData({} as any); + const organizationData = new OrganizationData({} as any, {} as any); organizationData.id = id; organizationData.enabled = enabled; organizationData.usePolicies = usePolicies; diff --git a/libs/common/src/admin-console/abstractions/organization/organization.service.abstraction.ts b/libs/common/src/admin-console/abstractions/organization/organization.service.abstraction.ts index 912728828e..1fdfb260d2 100644 --- a/libs/common/src/admin-console/abstractions/organization/organization.service.abstraction.ts +++ b/libs/common/src/admin-console/abstractions/organization/organization.service.abstraction.ts @@ -56,13 +56,22 @@ export function canAccessAdmin(i18nService: I18nService) { ); } -export function isNotProviderUser(org: Organization): boolean { - return !org.isProviderUser; +/** + * Returns `true` if a user is a member of an organization (rather than only being a ProviderUser) + * @deprecated Use organizationService.memberOrganizations$ instead + */ +export function isMember(org: Organization): boolean { + return org.isMember; } export abstract class OrganizationService { organizations$: Observable; + /** + * Organizations that the user is a member of (excludes organizations that they only have access to via a provider) + */ + memberOrganizations$: Observable; + get$: (id: string) => Observable; get: (id: string) => Organization; getByIdentifier: (identifier: string) => Organization; diff --git a/libs/common/src/admin-console/models/data/organization.data.ts b/libs/common/src/admin-console/models/data/organization.data.ts index cfaaff37c7..36ebb0f3df 100644 --- a/libs/common/src/admin-console/models/data/organization.data.ts +++ b/libs/common/src/admin-console/models/data/organization.data.ts @@ -38,6 +38,7 @@ export class OrganizationData { providerName: string; providerType?: ProviderType; isProviderUser: boolean; + isMember: boolean; familySponsorshipFriendlyName: string; familySponsorshipAvailable: boolean; planProductType: ProductType; @@ -48,7 +49,13 @@ export class OrganizationData { familySponsorshipToDelete?: boolean; accessSecretsManager: boolean; - constructor(response: ProfileOrganizationResponse) { + constructor( + response: ProfileOrganizationResponse, + options: { + isMember: boolean; + isProviderUser: boolean; + } + ) { this.id = response.id; this.name = response.name; this.status = response.status; @@ -91,5 +98,8 @@ export class OrganizationData { this.familySponsorshipValidUntil = response.familySponsorshipValidUntil; this.familySponsorshipToDelete = response.familySponsorshipToDelete; this.accessSecretsManager = response.accessSecretsManager; + + this.isMember = options.isMember; + this.isProviderUser = options.isProviderUser; } } diff --git a/libs/common/src/admin-console/models/domain/organization.ts b/libs/common/src/admin-console/models/domain/organization.ts index b29a9fa7c8..11e0ccc034 100644 --- a/libs/common/src/admin-console/models/domain/organization.ts +++ b/libs/common/src/admin-console/models/domain/organization.ts @@ -9,7 +9,14 @@ export class Organization { id: string; name: string; status: OrganizationUserStatusType; + + /** + * The member's role in the organization. + * Avoid using this for permission checks - use the getters instead (e.g. isOwner, isAdmin, canManageX), because they + * properly handle permission inheritance and relationships. + */ type: OrganizationUserType; + enabled: boolean; usePolicies: boolean; useGroups: boolean; @@ -39,7 +46,14 @@ export class Organization { providerId: string; providerName: string; providerType?: ProviderType; + /** + * Indicates that a user is a ProviderUser for the organization + */ isProviderUser: boolean; + /** + * Indicates that a user is a member for the organization (may be `false` if they have access via a Provider only) + */ + isMember: boolean; familySponsorshipFriendlyName: string; familySponsorshipAvailable: boolean; planProductType: ProductType; @@ -89,6 +103,7 @@ export class Organization { this.providerName = obj.providerName; this.providerType = obj.providerType; this.isProviderUser = obj.isProviderUser; + this.isMember = obj.isMember; this.familySponsorshipFriendlyName = obj.familySponsorshipFriendlyName; this.familySponsorshipAvailable = obj.familySponsorshipAvailable; this.planProductType = obj.planProductType; @@ -101,24 +116,29 @@ export class Organization { } get canAccess() { - if (this.type === OrganizationUserType.Owner) { + if (this.isOwner) { return true; } return this.enabled && this.status === OrganizationUserStatusType.Confirmed; } + /** + * Whether a user has Manager permissions or greater + */ get isManager() { - return ( - this.type === OrganizationUserType.Manager || - this.type === OrganizationUserType.Owner || - this.type === OrganizationUserType.Admin - ); + return this.type === OrganizationUserType.Manager || this.isAdmin; } + /** + * Whether a user has Admin permissions or greater + */ get isAdmin() { - return this.type === OrganizationUserType.Owner || this.type === OrganizationUserType.Admin; + return this.type === OrganizationUserType.Admin || this.isOwner; } + /** + * Whether a user has Owner permissions (including ProviderUsers) + */ get isOwner() { return this.type === OrganizationUserType.Owner || this.isProviderUser; } diff --git a/libs/common/src/admin-console/services/organization/organization.service.ts b/libs/common/src/admin-console/services/organization/organization.service.ts index 94cc4b5c35..8d33684c20 100644 --- a/libs/common/src/admin-console/services/organization/organization.service.ts +++ b/libs/common/src/admin-console/services/organization/organization.service.ts @@ -1,7 +1,10 @@ import { BehaviorSubject, concatMap, map, Observable } from "rxjs"; import { StateService } from "../../../abstractions/state.service"; -import { InternalOrganizationService as InternalOrganizationServiceAbstraction } from "../../abstractions/organization/organization.service.abstraction"; +import { + InternalOrganizationService as InternalOrganizationServiceAbstraction, + isMember, +} from "../../abstractions/organization/organization.service.abstraction"; import { OrganizationData } from "../../models/data/organization.data"; import { Organization } from "../../models/domain/organization"; @@ -9,6 +12,7 @@ export class OrganizationService implements InternalOrganizationServiceAbstracti protected _organizations = new BehaviorSubject([]); organizations$ = this._organizations.asObservable(); + memberOrganizations$ = this.organizations$.pipe(map((orgs) => orgs.filter(isMember))); constructor(private stateService: StateService) { this.stateService.activeAccountUnlocked$ diff --git a/libs/common/src/admin-console/services/policy/policy.service.ts b/libs/common/src/admin-console/services/policy/policy.service.ts index 8fa0d72298..75b0e5bcef 100644 --- a/libs/common/src/admin-console/services/policy/policy.service.ts +++ b/libs/common/src/admin-console/services/policy/policy.service.ts @@ -260,7 +260,7 @@ export class PolicyService implements InternalPolicyServiceAbstraction { await this.stateService.setEncryptedPolicies(null, { userId: userId }); } - private isExcemptFromPolicies(organization: Organization, policyType: PolicyType) { + private isExemptFromPolicies(organization: Organization, policyType: PolicyType) { if (policyType === PolicyType.MaximumVaultTimeout) { return organization.type === OrganizationUserType.Owner; } @@ -291,7 +291,7 @@ export class PolicyService implements InternalPolicyServiceAbstraction { o.status >= OrganizationUserStatusType.Accepted && o.usePolicies && policySet.has(o.id) && - !this.isExcemptFromPolicies(o, policyType) + !this.isExemptFromPolicies(o, policyType) ); } } diff --git a/libs/common/src/vault/services/sync/sync.service.ts b/libs/common/src/vault/services/sync/sync.service.ts index b5a25390c5..2476ca034a 100644 --- a/libs/common/src/vault/services/sync/sync.service.ts +++ b/libs/common/src/vault/services/sync/sync.service.ts @@ -314,24 +314,13 @@ export class SyncService implements SyncServiceAbstraction { await this.stateService.setForcePasswordReset(response.forcePasswordReset); await this.keyConnectorService.setUsesKeyConnector(response.usesKeyConnector); - const organizations: { [id: string]: OrganizationData } = {}; - response.organizations.forEach((o) => { - organizations[o.id] = new OrganizationData(o); - }); + await this.syncProfileOrganizations(response); const providers: { [id: string]: ProviderData } = {}; response.providers.forEach((p) => { providers[p.id] = new ProviderData(p); }); - response.providerOrganizations.forEach((o) => { - if (organizations[o.id] == null) { - organizations[o.id] = new OrganizationData(o); - organizations[o.id].isProviderUser = true; - } - }); - - await this.organizationService.replace(organizations); await this.providerService.save(providers); if (await this.keyConnectorService.userNeedsMigration()) { @@ -342,6 +331,29 @@ export class SyncService implements SyncServiceAbstraction { } } + private async syncProfileOrganizations(response: ProfileResponse) { + const organizations: { [id: string]: OrganizationData } = {}; + response.organizations.forEach((o) => { + organizations[o.id] = new OrganizationData(o, { + isMember: true, + isProviderUser: false, + }); + }); + + response.providerOrganizations.forEach((o) => { + if (organizations[o.id] == null) { + organizations[o.id] = new OrganizationData(o, { + isMember: false, + isProviderUser: true, + }); + } else { + organizations[o.id].isProviderUser = true; + } + }); + + await this.organizationService.replace(organizations); + } + private async syncFolders(response: FolderResponse[]) { const folders: { [id: string]: FolderData } = {}; response.forEach((f) => {