[PM 22968] Enforce UI Restrictions When MSP/BUP Is Disabled #15752
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #15752 +/- ##
=======================================
Coverage 37.84% 37.84%
=======================================
Files 3309 3309
Lines 94014 94034 +20
Branches 14248 14252 +4
=======================================
+ Hits 35579 35587 +8
- Misses 56932 56948 +16
+ Partials 1503 1499 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -92,7 +118,7 @@ | |||
| </bit-table-scroll> | |||
| <div *ngIf="dataSource.data.length === 0" class="tw-mt-10"> | |||
| <app-no-clients | |||
| [showAddOrganizationButton]="isProviderAdmin" | |||
| [showAddOrganizationButton]="isSuspensionActive" | |||
There was a problem hiding this comment.
I believe this change will start showing the Add Organization button to service users when the provider is enabled.
|
There was a problem hiding this comment.
We still need to disable the Add Organization button and provide the tooltip when the Provider is suspended, but when the Provider is not suspended, we only want the button to show for Provider Admins.
Something like this:
<app-no-clients
[showAddOrganizationButton]="isProviderAdmin"
[disableAddOrganizationButton]="isSuspensionActive"
(addNewOrganizationClicked)="createClient()"
/>And then make the according changes in the NoClientsComponent.
There was a problem hiding this comment.
🎨 non blocker: We should favor using @if syntax over *ngIf
| map((provider) => { | ||
| this.provider = provider; | ||
| this.isAdminOrServiceUser = | ||
| provider.type === ProviderUserType.ProviderAdmin || | ||
| provider.type === ProviderUserType.ServiceUser; | ||
| return provider?.providerStatus === ProviderStatusType.Billable; | ||
| }), |
There was a problem hiding this comment.
We really should not be extracting values out of Observables like this. We can solve this by deconstructing this into separate Observables since its doing way too many unrelated things. It could look something like this:
this.providerId$ = this.activatedRoute.parent.params.pipe(
map((params) => params.providerId as ProviderId),
);
this.provider$ = this.providerId$.pipe(
switchMap((providerId) => this.providerService.get$(providerId)),
);
this.isAdminOrServiceUser$ = this.provider$.pipe(
map(
(provider) =>
provider?.type === ProviderUserType.ProviderAdmin ||
provider?.type === ProviderUserType.ServiceUser,
),
);
this.suspensionActive$ = combineLatest([
this.isAdminOrServiceUser$,
this.providerPortalTakeover$,
this.provider$.pipe(map((provider) => provider?.enabled ?? false)),
]).pipe(
map(
([isAdminOrServiceUser, portalTakoverEnabled, providerEnabled]) =>
isAdminOrServiceUser && portalTakoverEnabled && providerEnabled,
),
);
this.provider$
.pipe(
map((provider) => {
if (provider?.providerStatus === ProviderStatusType.Billable) {
return from(
this.router.navigate(["../manage-client-organizations"], {
relativeTo: this.activatedRoute,
}),
);
}
return from(this.load());
}),
takeUntilDestroyed(),
)
.subscribe();This lets you get rid of the two getters added here in favor of having suspensionActive$ which we can use in the template with the async pipe to also add our i18n'd tooltip:
@let isSuspensionActive = (suspensionActive$ | async);
...
<button
type="button"
bitButton
[disabled]="isSuspensionActive"
[title]="isSuspensionActive ? ('providerIsDisabled' | i18n) : '' "
(click)="addExistingOrganization()"
*ngIf="manageOrganizations && showAddExisting"
>
<i class="bwi bwi-plus bwi-fw" aria-hidden="true"></i>
{{ "addExistingOrganization" | i18n }}
</button>
...There was a problem hiding this comment.
We should really be following the same pattern in the clients.component. I would advise to align them both in the way I described in the previous file.
There was a problem hiding this comment.
🎨 non blocking: we should use @if syntax instead of *ngIf
| protected provider$ = this.providerId$.pipe( | ||
| switchMap((providerId) => { | ||
| this.providerId = providerId; | ||
| return this.providerService.get$(providerId); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
| protected provider$ = this.providerId$.pipe( | |
| switchMap((providerId) => { | |
| this.providerId = providerId; | |
| return this.providerService.get$(providerId); | |
| }), | |
| ); | |
| protected provider$ = this.providerId$.pipe( | |
| switchMap((providerId) => this.providerService.get$(providerId)), | |
| ); |
| this.provider = provider; | ||
| this.manageOrganizations = | ||
| provider.type === ProviderUserType.ProviderAdmin || | ||
| provider.type === ProviderUserType.ServiceUser; |
| [disabled]="isSuspensionActive" | ||
| [title]="isSuspensionActive ? ('providerIsDisabled' | i18n) : ''" | ||
| appA11yTitle="{{ | ||
| isSuspensionActive ? ('add' | i18n) : isSuspensionActive ? ('providerIsDisabled' | i18n) : '' |
There was a problem hiding this comment.
This is confusing, if isSuspensionActive is true then we display 'add' but if its false both ternary statements evaluate to "". We never use 'providerIsDisabled'. Unless I'm reading this wrong, this doesn't seem correct.
| relativeTo: this.activatedRoute, | ||
| }), | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
else is unnecessary here due to early return
|
|
||
| protected provider$ = this.providerId$.pipe( | ||
| switchMap((providerId) => { | ||
| this.providerId = providerId; |
There was a problem hiding this comment.
same feedback as the other file. Use a Signal or await firstValueFrom when providerId is needed in a promise based workflow. We should only have this.providerId$ and this.provider$.
| this.isProviderAdmin = this.provider?.type === ProviderUserType.ProviderAdmin; | ||
| this.isServiceUser = this.provider?.type === ProviderUserType.ServiceUser; |
There was a problem hiding this comment.
I think we can just get rid of these and directly check the properties in the template. Ex:
@let provider = (provider$ | async);
...
@if(provider?.type === ProviderUserType.ProviderAdmin){
<button type="button" bitMenuItem (click)="remove(row)">
<span class="tw-text-danger">
<i aria-hidden="true" class="bwi bwi-close"></i> {{ "unlinkOrganization" | i18n }}
</span>
</button>
}There was a problem hiding this comment.
Also, I don't think we are even using isServiceUser, but double check me on that one.
| @@ -55,6 +58,7 @@ const DisallowedPlanTypes = [ | |||
| }) | |||
| export class ClientsComponent { | |||
| providerId: string = ""; | |||
| provider: Provider | undefined; | |||
There was a problem hiding this comment.
Remove this, use a Signal or await firstValueFrom on the provider$ class member
There was a problem hiding this comment.
I recognize this is a billing owned file, so consider the feedback here strongly recommended but non blocking 👍
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-22968
📔 Objective
This PR introduces logic to automatically suspend a provider when their subscription becomes unpaid, and restricts client management capabilities when MSP/BUP is disabled.
🔁 Changes Introduced
Automatic Provider Suspension
When a provider’s subscription status changes to unpaid, the provider is automatically suspended, preventing further usage until payment is resolved.
MSP/BUP Disabled Behavior
When the MSP/BUP feature is disabled, the following UI and permission changes are applied for provider admins and service users:
A warning icon is displayed next to the provider name.
The following actions are disabled, with a tooltip explaining why:
Unlinking clients is still allowed for provider admins.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes