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

[JWT] App startup #1473

Open
wants to merge 61 commits into
base: internal_dump_method
Choose a base branch
from

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Aug 22, 2024

Description

One Line Summary

Handles basic-level app startup for Identity Verification - on new install and minor migrations on upgrades.

Details

Internal JWT Config management and listeners

  • One of the primary complexities is handling the state when an app is first installed or updated, but JWT is unknown. Once this value is received, it can be cached and future runs will be simple.
  • Make OSUserJwtConfig object to handle auth required status, etc.
  • User manager owns an instance of the OSUserJwtConfig and passes it to sub-components like Operation Repo and Executors who will basically be "paused" until they know if auth is required or not.
  • Operations can enqueue while the SDK is in an "unknown" state, they will just be blocked from sending.
  • Once remote params returns, the SDK learns if auth is required, and this value will be cached for the future. The OSUserJwtConfig fires listeners waiting on this value. If components learn that auth is on, they will process their queues and remove operations that are not valid. Then, the components will start operating normally.
  • There are lots of print()s to help debug, these will be removed eventually.

Request objects updated to consider JWT

  • User request instances already have a prepareForExecution() method that indicates if this request can be sent yet. For example, does the onesignal ID exist to make this request.
  • Piggyback off this existing method to check for auth and block sending this request if auth is required but invalid, or auth is unknown, etc.
  • We still keep the onesignal ID property on all requests as a way to know if the user has actually been created on the server and to check for the post-create cool-off period. When the request is sent, it will use either the onesignal ID or the external ID based on JWT on/off.
  • Requests Updated:
    • OSRequestCreateUser
    • OSRequestFetchUser
    • OSRequestAddAliases
    • OSRequestRemoveAlias
    • OSRequestUpdateProperties
  • Requests Not Allowed with Identity Verification:
    • OSRequestFetchIdentityBySubscription: This is dropped and the push subscription is included in any upcoming UserCreate calls.
    • OSRequestIdentifyUser: This is translated into a UserCreate.

What has been tested in this PR

  • New install with Identity Verification on, and call login with correct JWT, add tag + email, login to another user with correct JWT, and add tag + email
  • New install with Identity Verification turned off
  • Updating a token using [OneSignal updateUserJwt:"externalUserId" withToken:"token"]
  • Migration from a previous version
  • Upgrading from player model
  • Please see Manual Testing below for more details

What is not included in this PR

  • Any subscriptions related changes needed including Push Subscription updates, fetch IAM, Live Activities, or any request outside of the User module
  • Error handling - 401 responses, firing invalid listeners, retrying
  • Logging into new users with valid JWT if the previous user has still not successfully logged in, future logins are blocked
  • Logout
  • There are existing tangled dependencies that are not ideal, and this PR includes more that could be refactored better.

Motivation

Support Identity Verification

Testing

Unit testing

🚧 WIP

Manual Testing

How to Test:

  1. The Dev App is already pointing at staging with the bundle ID of com.onesignal.example.staging. Update the app ID to either 168... to test Identity Verification off, and 013... to test Identity Verification on. This is documented somewhere and please reach out for the keys and IDs.
  2. Use the "Get Tags" button on the Dev App to print information for testing
  3. Fill in an EID and token and use the Login button or Update button to login or update token.
New install - Identity Verification is off - no login is called
  1. New install, see that no requests go through
  2. Remote params returns and mock a response of "off"
  3. Internal listeners fire and the anonymous user is created with the push subscription
  4. Send tags works, update push sub works
  5. Next cold start, Identity Verification is uncached, "off" is the starting state and things proceed as normal. No listeners fire.
New install - Identity Verification is ON - then login is called
  1. New install, see that no requests go through
  2. Remote params returns and mock a response of "on"
  3. Internal listeners fire, no requests go through
  4. Call login with a valid token
  5. Create User is sent with the push subscription, and succeeds
  6. Response is received and hydrated, send tags works
  7. Fetch IAM and update push subscription does not work, it returns 401
  8. Next cold start, Identity Verification is uncached, "on" is the starting state and things proceed as normal. No listeners fire. Fetch user by EID to hydrate works, session count is sent.
Upgrade from main - Identity Verification is ON
  1. On main, turn off connection, do a new install and call:
[OneSignal login:@"nanaug23a"];
[OneSignal.User addTagWithKey:@"from" value:@"main"];
[OneSignal.User addEmail:@"test@domain.com"];
  1. See that no requests go through due to connection
  2. Update app to this branch and open app
  3. The operations are uncached but not requests go through due to unknown auth
  4. Remote params returns and mock a response of "on"
  5. Internal listeners fire, no requests go through due to missing JWT token
  6. Call [OneSignal updateUserJwt:@"nanaug23a" withToken:@"validToken"] for the external ID that was logged into.
  7. Create User is sent with the push subscription, the tags is sent and the create email is sent. They are successful.
Upgrade from player model - Identity Verification is ON

This is the test process I did:

  1. On player-model-main, do a new install and I did not set external ID.
  2. The player is created. Check the player ID and push token in the dashboard. Kill app.
  3. Update app to this branch and open app
  4. The cached data is read but no user requests are made. Get IAM is automatically sent.
  5. Remote params returns and mock a response of "on". The OSRequestFetchIdentityBySubscription that was previously generated is dropped.
  6. Nothing happens.
  7. Then login with a JWT but forget to include the push subscription ID
  8. Create User is sent with the local push subscription read from cache (this was the player) and includes the player ID and push token, but receive a 401 due to missing push subscription ID in claims.
  9. Update the jwt token with the subscription ID
  10. Create User is re-sent successfully. Fetch user happens.

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
  • 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.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

@nan-li nan-li mentioned this pull request Aug 22, 2024
18 tasks
@emawby emawby self-requested a review August 22, 2024 18:22
@emawby emawby self-assigned this Aug 22, 2024
@nan-li nan-li force-pushed the identity_verification_app_startup branch 4 times, most recently from 6fe9b03 to 4827f36 Compare August 22, 2024 19:56
@nan-li nan-li force-pushed the internal_dump_method branch 2 times, most recently from 186c65d to 35677bc Compare August 22, 2024 23:26
@nan-li nan-li force-pushed the identity_verification_app_startup branch from 4827f36 to a012272 Compare August 23, 2024 15:41
* Add text fields and buttons for testing
* Use staging
* remove app clips
* Make Client more verbose with headers
* Make OSUserJwtConfig object to handle auth status etc
* User manager owns an instance of the JWT Config
* Add `OSUD_USE_IDENTITY_VERIFICATION` flag to OneSignalCommonDefines
public updateUserJwt method
* User request instances already have a `prepareForExecution()` method that indicates if this request can be sent yet. For example, does the onesignal ID exist to make this request.
* Use this existing method to check for auth and block sending this request if auth is required but invalid, or auth is unknown, etc.

* OSRequestFetchUser will have onesignalId as a property, regardless of JWT on or off. The aliasLabel and aliasId pair is already expected to be onesignal ID, so make it explicit.
* Keeping the onesignal ID can be used to know if the user has actually been created on the server and to check for the post-create cool-off period. The request will be translated to use the onesignal ID or the external ID based on JWT on/off.

Requests Updated:
* OSRequestCreateUser
* OSRequestFetchUser
* OSRequestAddAliases
* OSRequestRemoveAlias
* OSRequestUpdateProperties

Requests Not Allowed with Identity Verification:
* OSRequestFetchIdentityBySubscription
* OSRequestIdentifyUser
* Identity Model Store Listener will ignore it so no Deltas will be enqueued
* As a refresher, the Identity Model Repo holds all Identity Models present during an app run, which can include past users that have pending updates, while the Identity Model Store contains only the current user's Identity Model.
* Let the OSIdentityModelRepo be the listener for changes to JWT token updates, and it needs to pass on that information to the User Manager who manages the JWT Config and fires other listeners of token changes.
Updates to:
* OSPropertyOperationExecutor
* OSIdentityOperationExecutor
* OSUserExecutor
Updates to OSSubscriptionOperationExecutor is still to be done.
* Refactor from a static shared instance to a instance managed by User Manager
* Operation Repo does not flush while Identity Verification is unknown
* Currently it is not going to process Deltas based on JWT. Executors will.
emawby and others added 30 commits September 11, 2024 11:17
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
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
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
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.
adds handling for pending unauthorized subscription executor requests.
Doesn't yet handle prepare for execution properly
No unit tests yet
Run swiftlint and make a log more helpful
* 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.
* 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.
* 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
* 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
* Remove test on Update Subscription with JWT; it does not use User JWT
* Make some changes to existing tests
* Remote params returns `jwt_required` as the key to use
* If logging into an external ID that already exists in the SDK, re-use that one to keep the same model.
* 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.
* 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.
* Update the API for the listener, add and removal function names, event name
* The listener API is OSUserJwtInvalidatedListener
* The event is OSUserJwtInvalidatedEvent
* 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.
* Revert back to prod servers
* Add app clips back
[JWT] Handle logout when Identity verification is on
…e_users

[JWT] Improve management of multiple users + finalize API
Fire jwt invalidated callback when receiving 401 errors
…ith_prod

[JWT] Handle when jwt_required is not returned in remote params
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