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.

Sign up for GitHub

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-17168] Sync organization user revoked/restored status immediately via push notification #13101

Closed
wants to merge 15 commits into from
+23 −0
Closed

[PM-17168] Sync organization user revoked/restored status immediately via push notification #13101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4ddfc1e
Refresh identity token on SyncOrgKeys notification message
r-tome Jan 27, 2025
1398c49
Fix floating promise in signalrConnection onclose handler by adding a…
r-tome Jan 27, 2025
cfe0ea4
Refactor NotificationsService to encapsulate SignalR connection setup…
r-tome Jan 27, 2025
8acbc16
Set up SignalR connection on reconnect
r-tome Jan 27, 2025
fffbb5b
Add PushSyncOrgKeysOnRevokeRestore feature flag
r-tome Jan 27, 2025
f4e4625
Add feature flag check for PushSyncOrgKeysOnRevokeRestore in Notifica…
r-tome Jan 27, 2025
4a4e35c
Merge branch 'main' into ac/pm-17168/revoking-user-is-not-synced-imme…
r-tome Jan 28, 2025
934791c
Add ConfigService for NotificationsService in jslib-services.module.ts
r-tome Jan 28, 2025
839eacc
Update NotificationsService to await feature flag checks for PushSync…
r-tome Jan 28, 2025
ff7eef9
Add ConfigService to MainBackground
r-tome Jan 28, 2025
eb30531
Merge branch 'main' into ac/pm-17168/revoking-user-is-not-synced-imme…
r-tome Feb 3, 2025
9f6ecfb
Refresh identity token on SyncOrgKeys notification if feature flag is…
r-tome Feb 3, 2025
1764a8b
Add comment to clarify why the identity token needs to be refreshed
r-tome Feb 4, 2025
3b144ba
Merge branch 'main' into ac/pm-17168/revoking-user-is-not-synced-imme…
r-tome Feb 4, 2025
57d9548
Add mocks for ApiService and ConfigService in NotificationsService tests
r-tome Feb 5, 2025
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: 2 additions & 0 deletions apps/browser/src/background/main.background.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1067,6 +1067,8 @@ export default class MainBackground {
new SignalRConnectionService(this.apiService, this.logService),
this.authService,
this.webPushConnectionService,
this.apiService,
this.configService,
);

this.fido2UserInterfaceService = new BrowserFido2UserInterfaceService(this.authService);
Expand Down
2 changes: 2 additions & 0 deletions libs/angular/src/services/jslib-services.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -890,6 +890,8 @@ const safeProviders: SafeProvider[] = [
SignalRConnectionService,
AuthServiceAbstraction,
WebPushConnectionService,
ApiServiceAbstraction,
ConfigService,
],
}),
safeProvider({
Expand Down
2 changes: 2 additions & 0 deletions libs/common/src/enums/feature-flag.enum.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export enum FeatureFlag {
VerifiedSsoDomainEndpoint = "pm-12337-refactor-sso-details-endpoint",
PM14505AdminConsoleIntegrationPage = "pm-14505-admin-console-integration-page",
LimitItemDeletion = "pm-15493-restrict-item-deletion-to-can-manage-permission",
PushSyncOrgKeysOnRevokeRestore = "pm-17168-push-sync-org-keys-on-revoke-restore",

/* Autofill */
BlockBrowserInjectionsByDomain = "block-browser-injections-by-domain",
Expand Down Expand Up @@ -69,6 +70,7 @@ export const DefaultFeatureFlagValue = {
[FeatureFlag.VerifiedSsoDomainEndpoint]: FALSE,
[FeatureFlag.PM14505AdminConsoleIntegrationPage]: FALSE,
[FeatureFlag.LimitItemDeletion]: FALSE,
[FeatureFlag.PushSyncOrgKeysOnRevokeRestore]: FALSE,

/* Autofill */
[FeatureFlag.BlockBrowserInjectionsByDomain]: FALSE,
Expand Down
8 changes: 8 additions & 0 deletions libs/common/src/platform/notifications/internal/default-notifications.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { mock, MockProxy } from "jest-mock-extended";
import { BehaviorSubject, bufferCount, firstValueFrom, ObservedValueOf, Subject } from "rxjs";

import { LogoutReason } from "@bitwarden/auth/common";
import { ApiService } from "@bitwarden/common/abstractions/api.service";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";

import { awaitAsync } from "../../../../spec";
import { Matrix } from "../../../../spec/matrix";
Expand Down Expand Up @@ -36,6 +38,8 @@ describe("NotificationsService", () => {
let signalRNotificationConnectionService: MockProxy<SignalRConnectionService>;
let authService: MockProxy<AuthService>;
let webPushNotificationConnectionService: MockProxy<WebPushConnectionService>;
let apiService: MockProxy<ApiService>;
let configService: MockProxy<ConfigService>;
justindbaur marked this conversation as resolved.
Show resolved Hide resolved

let activeAccount: BehaviorSubject<ObservedValueOf<AccountService["activeAccount$"]>>;

Expand All @@ -62,6 +66,8 @@ describe("NotificationsService", () => {
signalRNotificationConnectionService = mock<SignalRConnectionService>();
authService = mock<AuthService>();
webPushNotificationConnectionService = mock<WorkerWebPushConnectionService>();
apiService = mock<ApiService>();
configService = mock<ConfigService>();

activeAccount = new BehaviorSubject<ObservedValueOf<AccountService["activeAccount$"]>>(null);
accountService.activeAccount$ = activeAccount.asObservable();
Expand Down Expand Up @@ -102,6 +108,8 @@ describe("NotificationsService", () => {
signalRNotificationConnectionService,
authService,
webPushNotificationConnectionService,
apiService,
configService,
);
});

Expand Down
9 changes: 9 additions & 0 deletions libs/common/src/platform/notifications/internal/default-notifications.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
} from "rxjs";

import { LogoutReason } from "@bitwarden/auth/common";
import { ApiService } from "@bitwarden/common/abstractions/api.service";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";

import { AccountService } from "../../../auth/abstractions/account.service";
import { AuthService } from "../../../auth/abstractions/auth.service";
Expand Down Expand Up @@ -52,6 +55,8 @@
private readonly signalRConnectionService: SignalRConnectionService,
private readonly authService: AuthService,
private readonly webPushConnectionService: WebPushConnectionService,
private readonly apiService: ApiService,
private readonly configService: ConfigService,
) {
this.notifications$ = this.accountService.activeAccount$.pipe(
map((account) => account?.id),
Expand Down Expand Up @@ -182,6 +187,10 @@
await this.syncService.fullSync(true);
break;
case NotificationType.SyncOrgKeys:
if (await this.configService.getFeatureFlag(FeatureFlag.PushSyncOrgKeysOnRevokeRestore)) {
// Refresh the identity token to ensure organization roles in JWT claims are up-to-date
await this.apiService.refreshIdentityToken();

Check warning on line 192 in libs/common/src/platform/notifications/internal/default-notifications.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/platform/notifications/internal/default-notifications.service.ts#L192 

Added line #L192 was not covered by tests
r-tome marked this conversation as resolved.
Show resolved Hide resolved
}
Comment on lines +190 to +193
Copy link
Member

Choose a reason for hiding this comment

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

I'm actually going to keep the rejection only because I think your server change should be enough for now. SyncService.fullSync(true) will automatically call ApiService.refreshIdentityToken(). We are thinking about decoupling that behavior and the change here you have would be right but we are not going to be merging that right now. So please check my assumption here but you should get the desired outcome with only the server PR. I've linked the tickets and mentioned the PR on our decoupling work so that if we go ahead with that we will add the ApiService.refreshIdentityToken call for you.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't know fullSync already did this for us, so fwiw I'm OK with that suggestion. Thanks @justindbaur

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So please check my assumption here but you should get the desired outcome with only the server PR

Yes that is true, the server PR changes alone are enough. Thanks for looking into this, @justindbaur!

I'll close this PR.

await this.syncService.fullSync(true);
this.activitySubject.next("inactive"); // Force a disconnect
this.activitySubject.next("active"); // Allow a reconnect
Expand Down
Loading