OneSignal-Android-SDK icon indicating copy to clipboard operation
OneSignal-Android-SDK copied to clipboard

Fix onesignalid is local when identity verification off

Open jinliu9508 opened this issue 1 year ago • 4 comments

Description

One Line Summary

Fix a bug that a local onesignal ID is used for logging in when identity verification is turned off.

Details

Motivation

The beta for identity verification has a bug that incorrectly uses a local OneSignal ID in a login request. This may result in a 400 error and cause the login to fail. Some users who unknowingly adopt the beta version may experience a change in behavior. This PR aims to resolve the bug in the beta and ensure stability for the identity verification feature, whether it is enabled or disabled.

Scope

Added a check for the useIdentityVerification status before composing a login request.

Testing

Manual testing

Steps to reproduce the bug with a 400 error:

  1. Install 5.1.26 and login an arbitrary user. Observe that the logging in is successful.
  2. Ensure the identity verification is turned off in the OneSignal dashboard.
  3. Upgrade to 5.2.0-beta. Observe that the logging in is still successful.
  4. Call Logout() and observe the 400 error returned from the server. After the fix, we no longer receive the 400 error and the logout is successful.

Affected code checklist

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

Checklist

Overview

  • [x] I have filled out all REQUIRED sections above
  • [x] 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
  • [x] Any Public API changes are explained in the PR details and conform to existing APIs

Testing

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

Final pass

  • [X] 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.
  • [x] 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

jinliu9508 avatar Jan 10 '25 20:01 jinliu9508

This is tagged WIP, is it still in WIP?

nan-li avatar Jan 16 '25 18:01 nan-li

This is tagged WIP, is it still in WIP?

I just removed the tag and it is now ready for review.

jinliu9508 avatar Jan 16 '25 18:01 jinliu9508

Can you test a new install using this branch with identity verification turned off?

I am getting an immediate 400 on the user creation request - the push subscription is missing. May be reproducible or just me.

Thanks for pointing out. I just pushed a change that is tested for a new install and the steps to reproduce.

jinliu9508 avatar Jan 25 '25 01:01 jinliu9508

I think these changes require more testing and I am not sure the changes are the right solution - I don't think they get to the root issue.

Using the latest changes, If I run the app, and then kill it and reopen to drive a new cold start, I am seeing the same [DefaultDispatcher-worker-8] HttpClient: Got Response = POST - STATUS: 400 - Body: {"errors":[{"code":"user-3","title":"Creating a User requires at least one Alias or Subscription"}]} error.

Look at IdentityVerificationService.kt - it listens to the ConfigModel and in the onModelReplaced for that model, it is enqueueing a LoginUserOperation. This onModelReplaced is triggered on every cold start. So, every cold start, the operation repo enqueues a login user operation that hits the create user endpoint, that could have no external ID if the user is anonymous and no subscription (since the SDK did not enqueue anything related to the push subscription). Hence, Creating a User requires at least one Alias or Subscription in this case.

Additionally, we should consider how to get devices out of this stuck state on upgrade.

nan-li avatar Jan 28 '25 17:01 nan-li