[Feature] Identity verification implementation #2030

jinliu9508 · 2024-03-15T04:31:42Z

READ AND DELETE THIS SECTION BEFORE SUBMITTING PR

Fill out each REQUIRED section
Fill out OPTIONAL sections, remove section if it doesn't apply to your PR
Read and fill out each of the checklists below
Remove this section after reading

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).

Scope

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.

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

!!! Note that we are unable to manual test this feature at the moment due to the backend service is not completed. However, I have tested other normal functionalities to ensure the code does not break for existing users.

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

nan-li

It's not testable yet, but have you thought about how to handle the anonymous user that would have been automatically created in initWithContext since I didn't see any changes for anonymous users, and unsubscribing the user on logout (if JWT-enabled)?

nan-li · 2024-04-01T18:48:32Z

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt

@@ -359,12 +363,23 @@ internal class OneSignalImp : IOneSignal, IServiceProvider {
            currentIdentityOneSignalId = identityModelStore!!.model.onesignalId

            if (currentIdentityExternalId == externalId) {
+                // login is for same user that is already logged in, fetch (refresh)
+                // the current user.
+                identityModelStore!!.model.jwtToken = jwtBearerToken


main has changed a bit since some PRs have been merged; this refresh on login was removed in a previous PR

nan-li · 2024-04-01T18:53:23Z

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt

        }
    }

+    override fun updateUserJwt(externalId: String, token: String) {
+        if (!identityModelStore!!.model.externalId.equals(externalId))


This returns early but the SDK may need to update JWT for previous users?

jkasten2 · 2024-04-04T20:03:16Z

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

+ *
+ */
+class UserJwtInvalidatedEvent(
+        val external_id: String


Remove underscore, rename to externalId.

This was a mistake in the spec, and just corrected it there.

nan-li · 2024-05-16T09:07:41Z

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

-)
+) {
+    fun invalidateJwt() {
+        model.setStringProperty(


This will add jwt_token as an alias on the Identity Model and get enqueued as a set-alias operation. If you look at IdentityModelStoreListener, any changes are detected as aliases.

nan-li · 2024-05-16T09:09:01Z

...nal/core/src/main/java/com/onesignal/user/internal/operations/UpdateSubscriptionOperation.kt

+    var jwt: String?
+        get() = getStringProperty(::jwt.name)
+        private set(value) {
+            setStringProperty(::jwt.name, value!!)


This !! is dangerous, since the constructor for this class just below says jwt passed in can be null, and it will call this set method directly within it.

jkasten2 · 2024-06-06T21:08:54Z

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt

+                        // remove identification required operations in the queue
+                        operationRepo!!.removeIdentificationRequiredOperations()


I don't think this is the right high level concept here. We want to remove any operations called before OneSignal.login(). If addTags() are call right after login we want to keep it, but if it was call before we want to discard.

An edge case that comes to mind which is tricky is subscription updates, such as the user accepting the notification permission.

jkasten2 · 2024-06-06T21:12:04Z

...gnal/core/src/main/java/com/onesignal/core/internal/backend/IParamsBackendServiceObserver.kt

+ * in order to receive control when the params has fetched on the current device.
+ */
+interface IParamsBackendServiceObserver {
+    fun onParamsFetched(params: ParamsObject)


I notice we have a ConfigModel that has subscribe and unsubscribe methods. I see some of the remote params are on ConfigModel. I am not sure why all remote level params didn't go through the ConfigModel to begin with, but now sounds like a good time to consolidate, if it is a good fit.

jkasten2 · 2024-06-06T21:14:08Z

...DK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt

@@ -101,6 +103,13 @@ internal class OperationRepo(
        }
    }

+    fun <T : Any> containsParameterOfType(


(ignore if this is just a test function we delete later) Shouldn't this be called getParametersOfType and return a list?

…tions that have no valid JWT

jinliu9508 · 2024-09-23T16:12:06Z

Close and re-create another PR for this feature.

jinliu9508 force-pushed the identity-verification-implementation branch from 268d1a7 to 97e4b8a Compare March 15, 2024 05:42

jinliu9508 force-pushed the identity-verification-implementation branch 3 times, most recently from 81ebe0b to ee6ff69 Compare March 28, 2024 18:23

jinliu9508 changed the title ~~WIP: Identity verification implementation~~ Identity verification implementation Apr 1, 2024

jinliu9508 requested review from jkasten2, shepherd-l, emawby, nan-li and jennantilla April 1, 2024 15:23

nan-li reviewed Apr 2, 2024

View reviewed changes

jkasten2 reviewed Apr 4, 2024

View reviewed changes

jinliu9508 force-pushed the identity-verification-implementation branch from cb7a790 to 3238d9a Compare April 9, 2024 18:10

jinliu9508 changed the title ~~Identity verification implementation~~ [Feature] Identity verification implementation May 2, 2024

jkasten2 added the WIP Work In Progress label May 3, 2024

jinliu9508 force-pushed the identity-verification-implementation branch from 22fe20f to 4d1a0b4 Compare May 15, 2024 17:56

nan-li reviewed May 16, 2024

View reviewed changes

jinliu9508 force-pushed the identity-verification-implementation branch from 868d37b to 95ec9c2 Compare May 17, 2024 17:28

jinliu9508 force-pushed the identity-verification-implementation branch 2 times, most recently from 52edda4 to 108b627 Compare June 3, 2024 17:25

jkasten2 reviewed Jun 6, 2024

View reviewed changes

jinliu9508 force-pushed the identity-verification-implementation branch from bcdc0f3 to c8388e6 Compare July 30, 2024 14:22

jinliu9508 force-pushed the identity-verification-implementation branch from c8388e6 to 0bf69c9 Compare August 13, 2024 19:20

jinliu9508 force-pushed the identity-verification-implementation branch from 0bf69c9 to 42dced1 Compare August 26, 2024 16:56

jinliu9508 added 6 commits August 26, 2024 12:59

Infrastructure change around JWT: models, model stores

024bf16

Public API impl for JWT, User Manager, and callbacks

5713fe7

Add JWT to HttpClient, backend services, and operations

1066393

Add UNAUTHORIZED handling to OperationRepo and operation executors

f86dddf

Add test units for JWT related change

7df4a95

Add UI element to DemoApp to manually test JWT impl

71b06ae

jinliu9508 added 4 commits August 26, 2024 12:59

Manually test with a specific app in staging

c18a0de

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

5fd9330

…tions that have no valid JWT

Disable operations that are not allowed when identity verification is on

8508962

use new name from remote params

fe688e3

jinliu9508 force-pushed the identity-verification-implementation branch from 42dced1 to fe688e3 Compare August 26, 2024 17:00

jinliu9508 added 2 commits September 9, 2024 11:41

Init behavior change based on identity verification

26f36c2

Add deviceAuthToken to subscription requests

c4590fa

jinliu9508 force-pushed the identity-verification-implementation branch from a3d47a7 to c4590fa Compare September 19, 2024 17:17

jinliu9508 closed this Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Identity verification implementation #2030

[Feature] Identity verification implementation #2030

jinliu9508 commented Mar 15, 2024 •

edited

Loading

nan-li left a comment •

edited

Loading

nan-li Apr 1, 2024

nan-li Apr 1, 2024

jkasten2 Apr 4, 2024

nan-li May 16, 2024

nan-li May 16, 2024

jkasten2 Jun 6, 2024

jkasten2 Jun 6, 2024 •

edited

Loading

jkasten2 Jun 6, 2024

jinliu9508 commented Sep 23, 2024

		// remove identification required operations in the queue
		operationRepo!!.removeIdentificationRequiredOperations()

[Feature] Identity verification implementation #2030

[Feature] Identity verification implementation #2030

Conversation

jinliu9508 commented Mar 15, 2024 • edited Loading

READ AND DELETE THIS SECTION BEFORE SUBMITTING PR

Description

One Line Summary

Details

Motivation

Scope

Testing

Unit testing

Manual testing

Affected code checklist

Checklist

Overview

Testing

Final pass

nan-li left a comment • edited Loading

Choose a reason for hiding this comment

nan-li Apr 1, 2024

Choose a reason for hiding this comment

nan-li Apr 1, 2024

Choose a reason for hiding this comment

jkasten2 Apr 4, 2024

Choose a reason for hiding this comment

nan-li May 16, 2024

Choose a reason for hiding this comment

nan-li May 16, 2024

Choose a reason for hiding this comment

jkasten2 Jun 6, 2024

Choose a reason for hiding this comment

jkasten2 Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

jkasten2 Jun 6, 2024

Choose a reason for hiding this comment

jinliu9508 commented Sep 23, 2024

jinliu9508 commented Mar 15, 2024 •

edited

Loading

nan-li left a comment •

edited

Loading

jkasten2 Jun 6, 2024 •

edited

Loading