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

Closed
1 change: 1 addition & 0 deletions apps/browser/src/background/main.background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1045,6 +1045,7 @@ export default class MainBackground {
this.authService,
this.messagingService,
this.taskSchedulerService,
this.configService,
);

this.fido2UserInterfaceService = new BrowserFido2UserInterfaceService(this.authService);
Expand Down
1 change: 1 addition & 0 deletions libs/angular/src/services/jslib-services.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -892,6 +892,7 @@ const safeProviders: SafeProvider[] = [
AuthServiceAbstraction,
MessagingServiceAbstraction,
TaskSchedulerService,
ConfigService,
],
}),
safeProvider({
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 @@ -9,6 +9,7 @@ export enum FeatureFlag {
AccountDeprovisioning = "pm-10308-account-deprovisioning",
VerifiedSsoDomainEndpoint = "pm-12337-refactor-sso-details-endpoint",
PM14505AdminConsoleIntegrationPage = "pm-14505-admin-console-integration-page",
PushSyncOrgKeysOnRevokeRestore = "pm-17168-push-sync-org-keys-on-revoke-restore",

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

/* Autofill */
[FeatureFlag.BlockBrowserInjectionsByDomain]: FALSE,
Expand Down
58 changes: 35 additions & 23 deletions libs/common/src/services/notifications.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,15 @@
import { AuthService } from "../auth/abstractions/auth.service";
import { AuthenticationStatus } from "../auth/enums/authentication-status";
import { NotificationType } from "../enums";
import { FeatureFlag } from "../enums/feature-flag.enum";

Check warning on line 14 in libs/common/src/services/notifications.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/services/notifications.service.ts#L14

Added line #L14 was not covered by tests
import {
NotificationResponse,
SyncCipherNotification,
SyncFolderNotification,
SyncSendNotification,
} from "../models/response/notification.response";
import { AppIdService } from "../platform/abstractions/app-id.service";
import { ConfigService } from "../platform/abstractions/config/config.service";
import { EnvironmentService } from "../platform/abstractions/environment.service";
import { LogService } from "../platform/abstractions/log.service";
import { MessagingService } from "../platform/abstractions/messaging.service";
Expand Down Expand Up @@ -46,6 +48,7 @@
private authService: AuthService,
private messagingService: MessagingService,
private taskSchedulerService: TaskSchedulerService,
private configService: ConfigService,

Check warning on line 51 in libs/common/src/services/notifications.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/services/notifications.service.ts#L51

Added line #L51 was not covered by tests
) {
this.taskSchedulerService.registerTaskHandler(
ScheduledTaskNames.notificationsReconnectTimeout,
Expand Down Expand Up @@ -80,29 +83,7 @@
this.signalrConnection = null;
}

this.signalrConnection = new signalR.HubConnectionBuilder()
.withUrl(this.url + "/hub", {
accessTokenFactory: () => this.apiService.getActiveBearerToken(),
skipNegotiation: true,
transport: signalR.HttpTransportType.WebSockets,
})
.withHubProtocol(new signalRMsgPack.MessagePackHubProtocol() as signalR.IHubProtocol)
// .configureLogging(signalR.LogLevel.Trace)
.build();

this.signalrConnection.on("ReceiveMessage", (data: any) =>
this.processNotification(new NotificationResponse(data)),
);
// eslint-disable-next-line
this.signalrConnection.on("Heartbeat", (data: any) => {
/*console.log('Heartbeat!');*/
});
this.signalrConnection.onclose(() => {
this.connected = false;
// FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
// eslint-disable-next-line @typescript-eslint/no-floating-promises
this.reconnect(true);
});
Comment on lines -100 to -105
Copy link
Member

Choose a reason for hiding this comment

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

I believe this handler will automatically reconnect after the connection is closed, so I'm not sure that refactor is necessary. If anything, maybe best to not trigger competing reconnections (not sure how it would handle that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought so too but that only happens if the connection drops, it doesn't reconnect if we manually close the connection

this.signalrConnection = this.setupConnection();

Check warning on line 86 in libs/common/src/services/notifications.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/services/notifications.service.ts#L86

Added line #L86 was not covered by tests
this.inited = true;
if (await this.isAuthedAndUnlocked()) {
await this.reconnect(false);
Expand Down Expand Up @@ -192,6 +173,9 @@
break;
case NotificationType.SyncOrgKeys:
if (isAuthenticated) {
if (await this.configService.getFeatureFlag(FeatureFlag.PushSyncOrgKeysOnRevokeRestore)) {
await this.apiService.refreshIdentityToken();

Check warning on line 177 in libs/common/src/services/notifications.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/services/notifications.service.ts#L177

Added line #L177 was not covered by tests
}
await this.syncService.fullSync(true);
// Stop so a reconnect can be made
await this.signalrConnection.stop();
Expand Down Expand Up @@ -249,6 +233,10 @@
}

try {
if (await this.configService.getFeatureFlag(FeatureFlag.PushSyncOrgKeysOnRevokeRestore)) {
this.signalrConnection = this.setupConnection();

Check warning on line 237 in libs/common/src/services/notifications.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/services/notifications.service.ts#L237

Added line #L237 was not covered by tests
}

await this.signalrConnection.start();
this.connected = true;
if (sync) {
Expand Down Expand Up @@ -277,4 +265,28 @@
max = Math.floor(max);
return Math.floor(Math.random() * (max - min + 1)) + min;
}

private setupConnection(): signalR.HubConnection {
const connection = new signalR.HubConnectionBuilder()

Check warning on line 270 in libs/common/src/services/notifications.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/services/notifications.service.ts#L270

Added line #L270 was not covered by tests
.withUrl(this.url + "/hub", {
accessTokenFactory: () => this.apiService.getActiveBearerToken(),

Check warning on line 272 in libs/common/src/services/notifications.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/services/notifications.service.ts#L272

Added line #L272 was not covered by tests
skipNegotiation: true,
transport: signalR.HttpTransportType.WebSockets,
})
.withHubProtocol(new signalRMsgPack.MessagePackHubProtocol() as signalR.IHubProtocol)
.build();

connection.on("ReceiveMessage", (data: any) =>
this.processNotification(new NotificationResponse(data)),

Check warning on line 280 in libs/common/src/services/notifications.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/services/notifications.service.ts#L279-L280

Added lines #L279 - L280 were not covered by tests
);
connection.on("Heartbeat", (data: any) => {

Check warning on line 282 in libs/common/src/services/notifications.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/services/notifications.service.ts#L282

Added line #L282 was not covered by tests
/*console.log('Heartbeat!');*/
});
connection.onclose(async () => {
this.connected = false;
await this.reconnect(true);

Check warning on line 287 in libs/common/src/services/notifications.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/services/notifications.service.ts#L285-L287

Added lines #L285 - L287 were not covered by tests
});

return connection;

Check warning on line 290 in libs/common/src/services/notifications.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/services/notifications.service.ts#L290

Added line #L290 was not covered by tests
}
}
Loading