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] Get In-App Messages from Server #1476

Merged

Conversation

nan-li
Copy link
Contributor

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

Description

One Line Summary

Update fetch IAMs to use the new endpoint, support JWT, and include the alias.

Details

Motivation

Fetch IAMs

Scope

Fetch IAMs

Future Work

  • The user and subscription model's relationship should be improved. Should the subscription model own a reference to its identity model that owns it?
  • The retrying fetch IAMs can be improved and less hacky.

Testing

Unit testing

No changes

Manual testing

New install with JWT on and off, misc scenarios tested

  • Works under normal flows with connection.
  • With JWT on, receive a 401 from fetch IAM. Then update the jwt token, and see the fetch that is immediately retried.
  • With JWT off, login to a new user with network turned off, and start a new session. Fetch IAM is blocked by null onesignal ID. Turn data on and the user is created. The fetch IAM is immediately retried.

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

* The client already logs the error response from the server
* The executors don't need to re-log the same response, just adds clutter.
* The client only logged sent requests that contained a body, now also log GETs that do not have a body, such as fetching remote params or IAMs
Motivation:
* The OSMessagingController will be a `OSUserJwtConfigListener` and observe jwt
* Optional bool is not compatible with objective c
* All methods that trigger fetching IAM from server will no longer pass the push subscription ID.
* The method to fetch IAM will itself handle the requirements.
* To get IAMs from the server, the following requirements are necessary:
 - A subscription ID
 - An appropriate alias (depending on Identity Verification enabled) for the subscription
 - A valid JWT token for the user if Identity Verification is enabled
* OSMessagingController will observe user and jwt
@nan-li
Copy link
Contributor Author

nan-li commented Aug 30, 2024

swiftlint file too long

emawby and others added 23 commits September 3, 2024 13:01
This uses the callback in the UserExecutor only
Also adds tests for the property executor and refactors some testing code that can be shared with other executor tests
Includes tests

Currently delete and update requests don't have an identity model attached. This may need to be changed for JWT
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.
@emawby emawby self-requested a review October 1, 2024 21:10
@emawby emawby self-assigned this Oct 1, 2024
* 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
@nan-li nan-li merged commit 1fbddcf into identity_verification_app_startup Oct 4, 2024
1 of 2 checks passed
@nan-li nan-li deleted the identity_verification_get_iams branch October 4, 2024 22:07
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