Add Web Push Support

[justindbaur]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-9609

📔 Objective

Main objective is adding WebPush support to our MV3 browser clients but it does add the ability to relatively easily add support for it in other clients. We only attempt to use web push if we call out to the server and the server says that it supports it, effectively being a feature flag.

Part of this, notifications goes more observable where it will listen to the observables it cares about, making it not need it's updateConnection method anymore.

📸 Screenshots

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

[codecov]

Codecov Report

Attention: Patch coverage is 22.02797% with 223 lines in your changes missing coverage. Please review.

Project coverage is 33.85%. Comparing base (b3155d1) to head (57c6eec).

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ions/internal/worker-webpush-connection.service.ts	0.00%	51 Missing ⚠️
...tifications/internal/signalr-connection.service.ts	0.00%	48 Missing ⚠️
...ications/internal/default-notifications.service.ts	47.61%	44 Missing ⚠️
...ications/permissions-webpush-connection.service.ts	0.00%	15 Missing ⚠️
apps/browser/src/background/main.background.ts	0.00%	11 Missing ⚠️
...ommon/src/platform/notifications/internal/index.ts	0.00%	8 Missing ⚠️
...tifications/internal/noop-notifications.service.ts	0.00%	7 Missing ⚠️
...ons/internal/web-push-notifications-api.service.ts	0.00%	6 Missing ⚠️
...latform/notifications/internal/web-push.request.ts	0.00%	6 Missing ⚠️
apps/web/src/app/app.component.ts	0.00%	5 Missing ⚠️
... and 11 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11346      +/-   ##
==========================================
+ Coverage   33.78%   33.85%   +0.07%     
==========================================
  Files        2912     2924      +12     
  Lines       90708    90824     +116     
  Branches    17155    17163       +8     
==========================================
+ Hits        30646    30749     +103     
- Misses      57674    57685      +11     
- Partials     2388     2390       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[MGibson1]

I think this might need a lot more documentation. I'm not understanding what this is for from the code and tests

[justindbaur]

I added some more documentation to the method but here is a snippet from it:

interface MyService {
  event$(userId: UserId) => Observable<UserEvent>
}

// Test
const myService = mock<MyService>();
const eventGetter = Matrix.autoMockMethod(myService.event$, (userId) => BehaviorSubject<UserEvent>());

eventGetter("userOne").next(new UserEvent());
eventGetter("userTwo").next(new UserEvent());

This replaces a more manual way of doing things like:

const myService = mock<MyService>();
const userOneSubject = new BehaviorSubject<UserEvent>();
const userTwoSubject = new BehaviorSubject<UserEvent>();
myService.event$.mockImplementation((userId) => {
   if (userId === "userOne") {
     return userOneSubject;
   } else if (userId === "userTwo") {
     return userTwoSubject;
   }
   return new BehaviorSubject<UserEvent>();
});

userOneSubject.next(new UserEvent());
userTwoSubject.next(new UserEvent());

Basically so I can easily emit values for different users easily in tests.

[MGibson1]

I see, the problem it's solving is that the pattern we're using of parametrized observables is pretty wordy to set up.

[MGibson1]

is this barrel file really doing anything for you if it's

1. already internal and
2. exporting everything from every file in the folder?

[justindbaur]

Yeah it makes importing internal things a lot easier. It's also supposed to automatically make it a restricted import outside of platform code but it appears that (all?) our restricted import eslint rules aren't working.

[MGibson1]

I've wanted that behavior for barrel files, but I don't believe we've ever found or built a rule to enforce that.

[justindbaur]

It was working:

clients/.eslintrc.json

Lines 331 to 346 in 18f7d64

	{
	"files": ["**/*.ts"],
	"excludedFiles": ["**/platform/**/*.ts"],
	"rules": {
	"no-restricted-imports": [
	"error",
	{
	"patterns": [
	"**/platform/**/internal", // General internal pattern
	// All features that have been converted to barrel files
	"**/platform/messaging/**"
	]
	}
	]
	}
	},

I should just be able to add **/platform/notifications/** but it's not working now and I don't want to add it now in case it spontaneously starts working again.

[MGibson1]

nit: I'd love to change this interface to something that makes more sense.
really notifications connect or disconnect due to account switching, locking, etc.

What's more, there's not a big reason to disconnect notifications for non active (but logged in) users once we use web push

[justindbaur]

I do too, I wanted to yank them out alongside the updateConnection method but didn't want to expand the scope that much.

I envision there would probably be a ActivityService with activity$ and that could be part of the chain instead of it tracking it's own BehaviorSubject. Then as you say, we could move it into just the SignalR code so that web push would be allowed to stay connected when there isn't activity.

[MGibson1]

Do we already have tickets tracking that? Either way we should add some TODOs here to document the planned interface changes

[justindbaur]

Added one & and comment. https://bitwarden.atlassian.net/browse/PM-14264

[MGibson1]

Do we currently require unlocked? I think all the notifications could be done on a locked vault, and listening for logout events on locked is useful.

[justindbaur]

Mostly, to do any sort of reconnect or updateConnection you do have to be authed and unlocked. But technically the very first connect does NOT enforce that. So if your application called init while you were locked you can connect. I'm happy to change it because receiving logout events is very useful but I do believe there are some preliminary changes to make the access token be stored encrypted by the user key which would make it unattainable unless you are unlocked.

clients/libs/common/src/services/notifications.service.ts

Lines 223 to 251 in 073ee47

	private async reconnect(sync: boolean) {
	this.reconnectTimerSubscription?.unsubscribe();
	if (this.connected || !this.inited || this.inactive) {
	return;
	}
	const authedAndUnlocked = await this.isAuthedAndUnlocked();
	if (!authedAndUnlocked) {
	return;
	}
	try {
	await this.signalrConnection.start();
	this.connected = true;
	if (sync) {
	await this.syncService.fullSync(false);
	}
	} catch (e) {
	this.logService.error(e);
	}
	if (!this.connected) {
	this.isSyncingOnReconnect = sync;
	this.reconnectTimerSubscription = this.taskSchedulerService.setTimeout(
	ScheduledTaskNames.notificationsReconnectTimeout,
	this.random(120000, 300000),
	);
	}
	}

[MGibson1]

hmm. encrypting the access token with the user key effectively kills the locked state and would split it into two states:

1. just authed (you just authed but haven't yet unlocked; so you have the auth token, but not the user key)
2. maybe authed (you just locked; so you don't have the auth token or user key)

@JaredSnider-Bitwarden let's sync up about this when you're available. Don't want to derail here too badly.

[JaredSnider-Bitwarden]

@MGibson1 , the idea to encrypt the access token with the user key was born out of a mistaken assumption about when the access token was cleared in web in prod. We are proceeding with the web process reload ticket #11781 without worrying about encrypting the access token at this time.

[MGibson1]

nit: yikes. this is rough logic. I would be significantly improved if we did one of

1. just allow multiple subscriptions to servers and filter some other way -- I believe all notifications include tag information
2. extract some of this logic into methods as you did with pushManagerSubscribe$
3. not be so dedicated to all of these being observables at every level

I personally like 1 the most, but it's also probably the most work. At the least you'd want to carefully unsubscribe a user on logout

[justindbaur]

I went with 3 for now, I can simplify it a whole lot by just doing all of it in a single defer, it does make it much simpler. Regarding 1 though, I think this will already reuse the same subscription. I intentionally left out any eager unsubscribe and non-active user notifications are filtered out in notifications service.

[MGibson1]

Not sure where I was coming from with 1, I'm pretty sure I tried to create multiple subscriptions and you can't. Worth double checking, but you're probably right we need to update existing subscriptions, which means rethinking our tags.

[coroiu]

Everything addressed 👍

[coroiu]

thought: If we get issues with people making changes to this file and forgets these unsubscribes we could look into cleaning in afterEach or beforeEach

[JaredSnider-Bitwarden]

A few tiny questions (per discussion, I'm not actually a required reviewer):

[JaredSnider-Bitwarden]

Nit (non-blocking): pushConfig is a more clear name vs just push.