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

Jump to bottom

Fire jwt invalidated callback when receiving 401 errors #1478

Merged
merged 34 commits into from
Oct 4, 2024
Merged

Fire jwt invalidated callback when receiving 401 errors #1478

merged 34 commits into from
Oct 4, 2024

Conversation

emawby
Copy link
Contributor

@emawby emawby commented Sep 4, 2024 •
edited
Loading

Currently the executors don't pause trying to execute their requests, but they aren't sent because they are stopped in prepareForExecution. We can optimize to have them wait until the token has been updated.

There is also no protection against firing the invalidated callback multiple times on the same token.

Description

One Line Summary

REQUIRED - Very short description that summaries the changes in this PR.

Details

Motivation

REQUIRED - Why is this code change being made? Or what is the goal of this PR? Examples: Fixes a specific bug, provides additional logging to debug future issues, feature to allow X.

Scope

RECOMMEND - OPTIONAL - What is intended to be effected. What is known not to change. Example: Notifications are grouped when parameter X is set, not enabled by default.

OPTIONAL - Other

OPTIONAL - Feel free to add any other sections or sub-sections that can explain your PR better.

Testing

Unit testing

OPTIONAL - Explain unit tests added, if not clear in the code.

Manual testing

RECOMMEND - OPTIONAL - Explain what scenarios were tested and the environment.
Example: Tested opening a notification while the app was foregrounded, app build with Android Studio 2020.3 with a fresh install of the OneSignal example app on a Pixel 6 with Android 12.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

emawby added 10 commits September 3, 2024 13:01
@emawby
create simple way to fire invalid Jwt callback
e7eeba5
@emawby
Adding OSJwtInvalidatedEvent and updating the JWT callback
c0ad6c3 
This uses the callback in the UserExecutor only
@emawby
refactor in UserExecutor to handle 401s
f926109
@emawby
Unit tests for UserExecutor with identity verification on
02acc2a
@emawby
Fire callback for 401 error in property executor
2530e2f 
Also adds tests for the property executor and refactors some testing code that can be shared with other executor tests
@emawby
fire Jwt callback from subscription executor
c0156d7 
Includes tests

Currently delete and update requests don't have an identity model attached. This may need to be changed for JWT
@emawby
fire Jwt callback for add and remove aliases 401
d5514e4 
Includes tests
@emawby
Remove message for invalid handler
5afab13
@emawby
Fire JWT callback from IAMs
1d0b97f 
This will always fire it for 401 even if JWT is not required, so validate the JWT config in the user manager before firing the callback
@emawby
Store UserExecutor requests that need auth
f3cb3d8 
This PR adds a pendingAuthRequests dictionary that stores the requests that are waiting for an updated JWT keyed on externalId.

When a requests fails with a 401 due to JWT or fails when preparing for execution we remove the request from the request queue and add it to the pending dictionary.

Once we get the onJWTUpdated callback for that externalId we requeue the pending requests and try again.

Also update tests to account for the callback object change and add tests for the new case
@emawby emawby force-pushed the identity_verification_callback branch from 10f6adc to f3cb3d8 Compare September 11, 2024 21:19
@emawby
store property update requests that need auth
aca978e 
When a requests fails with a 401 due to JWT or fails when preparing for execution we remove the request from the request queue and add it to the pending dictionary.

Once we get the onJWTUpdated callback for that externalId we requeue the pending requests and try again.

fixup property operations
@emawby
store alias update requests that need auth
1ae0deb 
When a requests fails with a 401 due to JWT or fails when preparing for execution we remove the request from the request queue and add it to the pending dictionary.

Once we get the onJWTUpdated callback for that externalId we requeue the pending requests and try again.
@emawby emawby force-pushed the identity_verification_callback branch from 6ebfcfc to 1ae0deb Compare September 11, 2024 23:10
@emawby
Adding work in progress Subscription executor auth pends
3542d34 
adds handling for pending unauthorized subscription executor requests.
Doesn't yet handle prepare for execution properly
No unit tests yet
@nan-li
[nit] run swiftlint
d90f0d2 
Run swiftlint and make a log more helpful
@nan-li nan-li force-pushed the identity_verification_callback branch 6 times, most recently from 8b95d0c to da4adcb Compare September 26, 2024 04:55
nan-li and others added 6 commits September 26, 2024 10:35
@nan-li
cache the pending auth requests in executors
01c2069 
* Uncaching now involves more queues, can be refactored when op repo is refactored
* Some executors added a helper to remove requests from the active queue and cache the queue after removal.
@nan-li
Add pending to fetch user requests
d579f06
@nan-li
Don't fire JWT invalid listeners multiple times
19df3db 
* Use a string constant `OS_JWT_TOKEN_INVALID` for a jwt token when we internally invalidated it, instead of setting to `nil`.
* OSIdentityModelRepo will not notify user manager when a token has been set to `OS_JWT_TOKEN_INVALID`. The user manager will already be notified of invalidation by executors.
@nan-li
Subscription executor wrap up, add some more tests
cccb58c 
* The delete subscription request now has identity model, similar to the Create subscription request
* The update subscription request is used only for the push sub, and it does not use User JWT, only a push token header
* The "Device-Auth-Push-Token" header has to be base 64 encoded
* Move some auth helpers into the JWT extension, and move execute request methods into an extension to address swiflint type_body_length violation
@nan-li
[nits] Move public protocols out of User Manager file
c27842c 
* OneSignalUserManagerImpl.swift violated the 1000 line file limit of Swiftlint
* Options include modifing the rule but let's pull out 2 public protocols.
* Additionally add more folders to organize the top-level files: MODELING for models and listeners, PUBLIC for publicly accessed objects and protocols
@nan-li
Update tests
74e4ef7 
* Remove test on Update Subscription with JWT; it does not use User JWT
* Make some changes to existing tests
@nan-li nan-li force-pushed the identity_verification_callback branch from da4adcb to 74e4ef7 Compare September 26, 2024 17:36
nan-li added 10 commits October 1, 2024 08:31
@nan-li
Update the remote params JWT key
0156328 
* Remote params returns `jwt_required` as the key to use
@nan-li
Re-use existing identity models for new users
a2420ee 
* If logging into an external ID that already exists in the SDK, re-use that one to keep the same model.
@nan-li
Remove duplicate Create User requests
5433ebb 
* If multiple create user requests are enqueued for the same external ID, only keep the most recent one, and remove the previous.
* These requests should all have the same identity model since they share external IDs, so only keeping the latest is adequate.
* This prevents multiple Create User requests with the same external ID from being executed simultaneously, which is possible when JWT is on, as we allow future logins to be sent before past user's login succeeds.
* An example of this is login(a) > login(b) > login(a) > login(b) but user A has an expired token. Once the token is updated for userA, potentially both logins could be executed if we don't prevent duplicates.
@nan-li
Don't send push sub for previous users to avoid transfer
01fde6f 
* Remove the push subscription if not current user; we don't want to transfer the push sub.
* This detail is meant to handle JWT on, and previous failed user creates can be sent even though the user has changed successfully.
* However, don't remove the push sub if the user is anonymous or else the create will fail. Also, when JWT is off and anonymous users can be created, this will block requests until it succeeds so there is no risk of accidentally transferring the push sub to an old user.
@nan-li
Extract out User Manager Loggable extension for file length
8d0d088
@nan-li
Update JWT invalidated listener and event API
b5ae642 
* Update the API for the listener, add and removal function names, event name
* The listener API is OSUserJwtInvalidatedListener
* The event is OSUserJwtInvalidatedEvent
@nan-li
Update tests after JWT listener API change
7d5db5e
@nan-li
Disable push sub when logout called (JWT on)
0dc2cee
@nan-li
Fire user observer on logout (JWT on)
5ad412f 
* Usually, on logout, the user observer will fire once the anonymous user is created to the backend and returns with an OSID. However, when Identity Verification is on, that will not happen, so fire the observer early with `nil` values to represent there is no user in the SDK currently.
* Firing the observer will save the state and necessary to know when the user logs back in. This is used by the messaging controller to fetch IAM appropriately. On a new session, it will not fetch IAM if logged out, but as the user observer, it will fetch once a user logs in.
@nan-li
[nit] move a method to extension for swiftlint
0d9c62d
@emawby emawby changed the title [WiP] Fire jwt invalidated callback when receiving 401 errors Fire jwt invalidated callback when receiving 401 errors Oct 3, 2024
@emawby emawby requested a review from nan-li October 3, 2024 18:03
nan-li added 4 commits October 4, 2024 09:22
@nan-li
[nit] Clean up logging, remove hardcoded prints
1f60b08
@nan-li
Revert back to production servers
0a81b7a 
* Revert back to prod servers
* Add app clips back
@nan-li
Merge pull request #1488 from OneSignal/identity_verification_logout
1e7bce1 
[JWT] Handle logout when Identity verification is on
@nan-li
Merge pull request #1487 from OneSignal/identity_verification_multipl…
d4588f5 
…e_users

[JWT] Improve management of multiple users + finalize API
@nan-li nan-li merged commit 0bc08eb into identity_verification_get_iams Oct 4, 2024
1 of 2 checks passed
@nan-li nan-li deleted the identity_verification_callback branch October 4, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nan-li nan-li nan-li approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emawby @nan-li