[WIP] Feature - Identity verification implementation #2196

jinliu9508 · 2024-09-26T17:44:43Z

Description

One Line Summary

Implement identity verification functionality to our Android SDK with JWT (JSON Web Token) that manage a specific User, their Subscriptions, and Identities, if enabled using OneSignal dashboard.

Details

Motivation

OneSignal Identity Verification feature only exists today for the Player model and we want to bring this feature to the User Model and gives us an opportunity to switch to a more standard client security model, JWT (JSON Web Token).

For Developers

If identity verification is enabled, all operations requiring identity or authorization must be accompanied by a correct JWT. Developers can use OneSignal.login(external_id, jwt) to set a JWT for a specific user identified by their external ID, or OneSignal.updateUserJwt(external_id, jwt) to update the JWT for that user. Additionally, developers can add a JWTInvalidatedListener by calling OneSignal.addUserJwtInvalidatedListener, allowing them to listen for a JWTInvalidatedEvent triggered when a JWT is invalidated by any operation, and subsequently update the JWT as necessary.

To Help With Review

This PR contains a some major structural changes working around identity verification.

New public API methods addUserJwtInvalidatedListener and updateUserJwt in UserManager.
Certain backend services and operations are now able to make request with JWT or deviceAuthPushToken depending on the operation type and purpose. They can also handle the UNAUTHORIZED http response and fire onUserJwtInvalidated callback accordingly.
Optional headers like JWT and deviceAuthPushToken can now be passed to HttpClient to be included in each request.
OperationRepo behavior change depends on the status of identity verification
initWithContext behavior change: switching user is now placed after the remote params fetch. Creating anonymous user or subscription when identity verification is on will suppress backend operations.
Login behavior change: update JWT for all future requests from the user.
Logout behavior change: suspend the push subscription

Testing

Unit testing

I have include a testing case that simulate a UNAUTHROIZED error (401, 403) and ensure that the jwt for the specific user is invalidated and the JWTInvalidatedEvent is fired.

Manual testing

Fresh Install: An anonymous user is created upon a fresh install and will not create a login request until Login with a JWT is called.
Upgrade from User Model:
- If a user is logged prior the upgrade and does not have JWT, app will fire the callback to retrieve a JWT and then attempt to send the request.
- If the user has JWT cached, app will directly send a login request.
- If no user has previously logged in successfully, the app will work the same as fresh install.
When an UNAUTHORIZED http error is caught: onJwtInvalidatedEvent will be fired and the code that retrieves a new JWT in UserJwtInvalidatedListener will be called to update user JWT. The request will then retry with the new JWT.

Affected code checklist

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

fixup

…tions that have no valid JWT

jkasten2

Most of my feedback was added inline, but I have one additional comment that wasn't clear if it is TODO yet or not. Is the new JWT endpoint to remove email and SMS by token still TODO in this PR, or did I miss it?

jkasten2 · 2024-10-24T22:02:53Z

Examples/OneSignalDemo/app/src/main/java/com/onesignal/sdktest/application/MainApplication.java

-            OneSignal.getNotifications().requestPermission(true, Continue.none());
+            //OneSignal.getNotifications().requestPermission(true, Continue.none());


Revert before merging.

jkasten2 · 2024-10-24T22:03:22Z

Examples/OneSignalDemo/app/src/main/java/com/onesignal/sdktest/application/MainApplication.java

+                // !!! For manual testing only
+                String jwt = "eyJhbGciOiJFUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiIwMTM5YmQ2Zi00NTFmLTQzOGMtODg4Ni00ZTBmMGZlM2EwODUiLCJleHAiOjE3MjczNjkyMjIsImlkZW50aXR5Ijp7ImV4dGVybmFsX2lkIjoiamluIn0sInN1YnNjcmlwdGlvbnMiOlt7InR5cGUiOiJFbWFpbCIsInRva2VuIjoidGVzdEBkb21haW4uY29tIn0seyJ0eXBlIjoiU01TIiwidG9rZW4iOiIrMTIzNDU2NzgifSx7InR5cGUiOiJBbmRyb2lkUHVzaCIsImlkIjoiMTIzZTQ1NjctZTg5Yi0xMmQzLWE0NTYtNDI2NjE0MTc0MDAwIn1dfQ.6XF7wRF4lLOvKr5Gd3MHv9j7U151hcBjmqSyk6nI6JVYUgt6q0YRp2j1aSJcg8VmaejzP1DouN1DpWUT_JTRXA";


Remove manual testing before merging.

jkasten2 · 2024-10-24T22:11:17Z

Examples/OneSignalDemo/app/src/main/java/com/onesignal/sdktest/model/MainActivityViewModel.java

+                    if (update != null && !update.isEmpty()) {
+                        OneSignal.updateUserJwt(OneSignal.getUser().getExternalId(), update);
+                        //String jwt = "eyJhbGciOiJFUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiIxNjg4ZDhmMi1kYTdmLTQ4MTUtOGVlMy05ZDEzNzg4NDgyYzgiLCJpYXQiOjE3MTQwODA4MTMsImlkZW50aXR5Ijp7ImV4dGVybmFsX2lkIjoiMjAyNDA0MjUtYWxleDQyIn0sInN1YnNjcmlwdGlvbiI6W3sidHlwZSI6IiIsImlkIjoiMmRlZGU3MzItMTEyNi00MTlkLTk5M2UtNDIzYWQyYTZiZGU5In1dfQ.rzZ-HaDm1EwxbMxd8937bWzPhPSQDDSqSzaASgZZc5A5v8g6ZQHizN9CljOmoQ4lTLm7noD6rOmR07tlZdrI5w";
+                        //OneSignal.login(update, jwt);


Remove commented out code

jkasten2 · 2024-10-24T22:12:17Z

Examples/OneSignalDemo/app/src/main/java/com/onesignal/sdktest/util/SharedPreferenceUtil.java

-        return getSharedPreference(context).getString(OS_APP_ID_SHARED_PREF, "77e32082-ea27-42e3-a898-c72e141824ef");
+        return getSharedPreference(context).getString(OS_APP_ID_SHARED_PREF, "0139bd6f-451f-438c-8886-4e0f0fe3a085");


Revert app_id change

jkasten2 · 2024-10-24T22:23:37Z

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/IUserJwtInvalidatedListener.kt

@@ -0,0 +1,16 @@
+package com.onesignal
+
+/** TODO: complete the comment part for this listener


Address TODO

jkasten2 · 2024-10-25T02:34:59Z

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/identity/IdentityModel.kt

+    var jwtToken: String?
+        get() = getOptStringProperty(IdentityConstants.JWT_TOKEN)
+        set(value) {
+            setOptStringProperty(IdentityConstants.JWT_TOKEN, value, ModelChangeTags.NO_PROPOGATE)


Why don't we want to propagate?

jkasten2 · 2024-10-25T02:37:48Z

...lSDK/onesignal/core/src/main/java/com/onesignal/user/internal/identity/IdentityModelStore.kt

-)
+) {
+    fun invalidateJwt() {
+        model.jwtToken = ""


The jwtToken has two invalidate states; blank string and null. Can we set this to null here so we don't have to worry about a blank string state?

jkasten2 · 2024-10-25T02:54:54Z

...signal/user/internal/operations/impl/executors/LoginUserFromSubscriptionOperationExecutor.kt

@@ -42,10 +42,12 @@ internal class LoginUserFromSubscriptionOperationExecutor(

    private suspend fun loginUser(loginUserOp: LoginUserFromSubscriptionOperation): ExecutionResponse {
        try {
+            // Not allowed when identity verification is on


I believe this whole class is not used when identity verification is on, so we should move and rework it to fit that.

jkasten2 · 2024-10-25T03:00:18Z

...signal/user/internal/operations/impl/executors/LoginUserFromSubscriptionOperationExecutor.kt

@@ -82,8 +84,10 @@ internal class LoginUserFromSubscriptionOperationExecutor(
            return when (responseType) {
                NetworkUtils.ResponseStatusType.RETRYABLE ->
                    ExecutionResponse(ExecutionResult.FAIL_RETRY)
-                NetworkUtils.ResponseStatusType.UNAUTHORIZED ->
+                NetworkUtils.ResponseStatusType.UNAUTHORIZED -> {
+                    _identityModelStore.invalidateJwt()


I see we call this _identityModelStore.invalidateJwt() in every executor, do you think it would work out to have the OperationRepo process ExecutionResult.FAIL_UNAUTHORIZED so it is the only one calling invalidateJwt()?

jkasten2 · 2024-10-25T03:07:45Z

...ain/java/com/onesignal/user/internal/operations/impl/executors/LoginUserOperationExecutor.kt

@@ -160,7 +164,18 @@ internal class LoginUserOperationExecutor(

        try {
            val subscriptionList = subscriptions.toList()
-            val response = _userBackend.createUser(createUserOperation.appId, identities, subscriptionList.map { it.second }, properties)
+            val pushSubscription = subscriptions.values.find { it.type == SubscriptionObjectType.ANDROID_PUSH }


This SDK supports fireOS and Huawei push so you will need a more complete check. However, since this looks like it is for Device Auth then we can remove it.

That is, once we have 100% committed to that path, so hold on addressing this comment until then.

jinliu9508 requested review from jkasten2, nan-li and emawby September 26, 2024 17:45

jinliu9508 changed the title ~~Identity verification implementation~~ [WIP] Identity verification implementation Sep 26, 2024

jinliu9508 changed the title ~~[WIP] Identity verification implementation~~ [WIP] Feature - Identity verification implementation Sep 26, 2024

jinliu9508 force-pushed the identity-verification-implementation branch 5 times, most recently from 358d254 to 253221f Compare October 8, 2024 18:16

Infrastructure change around JWT: models, model stores

a3b34e6

jinliu9508 force-pushed the identity-verification-implementation branch 5 times, most recently from ff50c7d to 29690e4 Compare October 11, 2024 17:46

Public API impl for JWT, User Manager, and callbacks

c41a852

jinliu9508 force-pushed the identity-verification-implementation branch from 29690e4 to 95c6162 Compare October 11, 2024 17:55

jinliu9508 added 11 commits October 15, 2024 13:26

Add JWT to HttpClient, backend services, and operations

23825c1

fixup

Add UNAUTHORIZED handling to OperationRepo and operation executors

86ef193

Add and adjust some test units for JWT related change

e75647d

Add UI element to DemoApp to manually test JWT impl

f564054

Manually test with a specific app in staging

ba6ad41

OperationRepo: when identity verification is on, do not process opera…

3fed182

…tions that have no valid JWT

Disable operations that are not allowed when identity verification is on

aab187b

use new name from remote params

9da1a9d

Init behavior change based on identity verification

22c6448

Add deviceAuthToken to subscription requests

5b84764

calling logout disable push subscription

8931712

jinliu9508 force-pushed the identity-verification-implementation branch from 95c6162 to 8931712 Compare October 15, 2024 17:27

jkasten2 requested changes Oct 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Feature - Identity verification implementation #2196

[WIP] Feature - Identity verification implementation #2196

jinliu9508 commented Sep 26, 2024 •

edited by niels-s

Loading

jkasten2 left a comment •

edited

Loading

jkasten2 Oct 24, 2024

jkasten2 Oct 24, 2024

jkasten2 Oct 24, 2024

jkasten2 Oct 24, 2024

jkasten2 Oct 24, 2024

jkasten2 Oct 25, 2024

jkasten2 Oct 25, 2024

jkasten2 Oct 25, 2024

jkasten2 Oct 25, 2024

jkasten2 Oct 25, 2024

		OneSignal.getNotifications().requestPermission(true, Continue.none());
		//OneSignal.getNotifications().requestPermission(true, Continue.none());

		// !!! For manual testing only
		String jwt = "eyJhbGciOiJFUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiIwMTM5YmQ2Zi00NTFmLTQzOGMtODg4Ni00ZTBmMGZlM2EwODUiLCJleHAiOjE3MjczNjkyMjIsImlkZW50aXR5Ijp7ImV4dGVybmFsX2lkIjoiamluIn0sInN1YnNjcmlwdGlvbnMiOlt7InR5cGUiOiJFbWFpbCIsInRva2VuIjoidGVzdEBkb21haW4uY29tIn0seyJ0eXBlIjoiU01TIiwidG9rZW4iOiIrMTIzNDU2NzgifSx7InR5cGUiOiJBbmRyb2lkUHVzaCIsImlkIjoiMTIzZTQ1NjctZTg5Yi0xMmQzLWE0NTYtNDI2NjE0MTc0MDAwIn1dfQ.6XF7wRF4lLOvKr5Gd3MHv9j7U151hcBjmqSyk6nI6JVYUgt6q0YRp2j1aSJcg8VmaejzP1DouN1DpWUT_JTRXA";

		return getSharedPreference(context).getString(OS_APP_ID_SHARED_PREF, "77e32082-ea27-42e3-a898-c72e141824ef");
		return getSharedPreference(context).getString(OS_APP_ID_SHARED_PREF, "0139bd6f-451f-438c-8886-4e0f0fe3a085");

		@@ -0,0 +1,16 @@
		package com.onesignal

		/** TODO: complete the comment part for this listener

[WIP] Feature - Identity verification implementation #2196

Are you sure you want to change the base?

[WIP] Feature - Identity verification implementation #2196

Conversation

jinliu9508 commented Sep 26, 2024 • edited by niels-s Loading

Description

One Line Summary

Details

Motivation

For Developers

To Help With Review

Testing

Unit testing

Manual testing

Affected code checklist

Checklist

Overview

Testing

Final pass

jkasten2 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinliu9508 commented Sep 26, 2024 •

edited by niels-s

Loading

jkasten2 left a comment •

edited

Loading