Skip to content

Update push token on login to allow multiple users on mobile devices#2404

Merged
trmartin4 merged 16 commits intomasterfrom
bug/SG-814-passwordless-multiple-users
Nov 16, 2022
Merged

Update push token on login to allow multiple users on mobile devices#2404
trmartin4 merged 16 commits intomasterfrom
bug/SG-814-passwordless-multiple-users

Conversation

@trmartin4
Copy link
Member

@trmartin4 trmartin4 commented Nov 11, 2022

https://bitwarden.atlassian.net/browse/SG-816
https://bitwarden.atlassian.net/browse/SG-814
https://bitwarden.atlassian.net/browse/SG-823

Type of change

- [X] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

We are experiencing a problem today where push notifications are not being sent to mobile devices when there are multiple users logged in.

There is still a scenario where no push token would get set for a given user + device combination, and that would be if a user logs in to a device and gets an access token prior to the PNS providing a push token to the device. However, if such a scenario occurs, the user can log out and log back in with that user and the token would be updated on the subsequent authentication request.

Code changes

BaseRequestValidator.cs:

  • When request for a token comes to Identity, we used to only call CreateOrUpdateRegistrationAsync() (as a part of SaveAsync() on the DeviceService) when it was a new device. This meant that subsequent login requests never updated the push token.
  • I changed this so regardless of whether the device is new or existing, every login request will attempt to set the push token for that logged-in user in the Azure Notification Hub.

AuthRequestsController:

  • Added userId to lookup for the correct device, in case there are multiple logged in

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

(cherry picked from commit 75d299ae269eeb8ac272c96458815a359ea6d085)
(cherry picked from commit f1cf54ebef2019743834f667861f9b34c1661e11)
@trmartin4 trmartin4 marked this pull request as ready for review November 15, 2022 14:58
@trmartin4 trmartin4 enabled auto-merge (squash) November 16, 2022 15:30
@trmartin4 trmartin4 merged commit 24469e2 into master Nov 16, 2022
@trmartin4 trmartin4 deleted the bug/SG-814-passwordless-multiple-users branch November 16, 2022 15:30
trmartin4 added a commit that referenced this pull request Nov 21, 2022
trmartin4 added a commit that referenced this pull request Dec 12, 2022
* Revert "Set Id property on existing devices so we don't try to create a new one instead of updating existing. (#2420)"

This reverts commit 02e4b10.

* Revert "Update push token on login to allow multiple users on mobile devices (#2404)"

This reverts commit 24469e2.

* Added back test changes.
eliykat pushed a commit that referenced this pull request Jan 10, 2023
* Revert "Set Id property on existing devices so we don't try to create a new one instead of updating existing. (#2420)"

This reverts commit 02e4b10.

* Revert "Update push token on login to allow multiple users on mobile devices (#2404)"

This reverts commit 24469e2.

* Added back test changes.
eliykat pushed a commit that referenced this pull request Jan 10, 2023
* Revert "Set Id property on existing devices so we don't try to create a new one instead of updating existing. (#2420)"

This reverts commit 02e4b10.

* Revert "Update push token on login to allow multiple users on mobile devices (#2404)"

This reverts commit 24469e2.

* Added back test changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants