okta / okta-devices-kotlin

okta-devices-kotlin
https://github.com/okta/okta-devices-kotlin
Apache License 2.0
5 stars 3 forks source link

User can still see Push notification enrollment method on the web, after deleting enrollment with SDK #56

Closed pkrawczykObj closed 1 year ago

pkrawczykObj commented 1 year ago

Describe the bug?

User can still see Push notification enrollment method on the web, after deleting enrollment with SDK 1.0.0.

What is expected to happen?

User no longer sees "Get push notification" as one of the verify options, when logging in via website, after deleting enrollment.

What is the actual behavior?

User can see "Get push notification" as one of the verify options, after deleting enrollment. image

Reproduction Steps?

  1. Log in with Okta to retrieve accessToken.
  2. Enroll for Okta Push SDK with com.okta.devices.push.api.PushAuthenticator.enroll(accessToken,config,params)
  3. User can see "Get push notification" as one of the verify options.
  4. Delete enrollement using com.okta.devices.push.api.PushAuthenticator.delete(accessToken,config,params) - with the same access token.

Additional Information?

No response

SDK Version and Artifact(s) used.

OKTA Devices 1.0.0.

Build Information

Not related to building.

FeiChen-okta commented 1 year ago

Hi @pkrawczykObj This is working in the sample application. Can you reproduce the issue with the sample by toggling off Sign in with push notification?

pkrawczykObj commented 1 year ago

I was not able to test it in the sample app, because of this issue: https://github.com/okta/okta-devices-kotlin/issues/57.

Is there any way to check on the backend side if my user is enrolled for Push SDK?

I suspect that during development I enrolled to the SDK numerous times and then after couple of days I started to delete my enrollements. So the question is if the single deleteEnrollment will delete all enrollments for given user or it might be the case that I enrolled a lot of times?

In our app I use almost the same code as in Sample App in the AutheticatorClient: suspend fun delete(userId: String, authToken: String): Result<Boolean> = runCatching { getEnrollment(userId).getOrNull()?.run { pushAuthenticator.delete(AuthToken.Bearer(authToken), this) } ?: Result.success(true) }.getOrElse { Result.failure(it) }

And it so invoked with following method, with the same accessToken, used previously for Enrollement: private fun sendDeleteEnrollmentRequest(accessToken: String) { externalScope.ioScope.launch { runCatching { val result = oktaAuthenticatorClient.delete(stateManager.getEnrollmentId(), accessToken) if (result.isSuccess) { DebugLog.d(TAG, "Enrollment deleted: ${stateManager.getEnrollmentId()}") stateManager.deleteEnrollment() } else { DebugLog.e(TAG, "Okta enrollment deletion failed") } }.getOrElse { DebugLog.e(TAG, "Enrollment deletion failed, cannot get result") } } } where: stateManager.getEnrollmentId() is returning value saved during Enroll Success response:

                    val pushEnrollment: PushEnrollment = result.getOrThrow()
                    DebugLog.d(TAG, "Enroll success: ${pushEnrollment.enrollmentId()}")
                    stateManager.enroll(pushEnrollment.enrollmentId())
And it returns `result.isSuccess`
FeiChen-okta commented 1 year ago

Ah it sounds like you have multiple enrollments. During development this can happen since we forget to delete the enrollment. We don't have admin access to your companies' okta admin panel. You will have to ask your administrator to sign in to okta admin panel and reset the enrollment.

pkrawczykObj commented 1 year ago

Hi @FeiChen-okta
OK we can try it, but to be honest I think that SDK should be bulletproof in this case and automatically remove all old enrollments when the new one is registered. My arguments:

  1. User can change to the new device and as he enrolls on the new device, the old one should stop to get notifications, without any user actions.
  2. There might be a bug in the client app and it can try to enroll multiple times, since this is async operation and the app needs to wait for a response for couple of seconds.
  3. User can delete apps data / cache in system settings and the app will forgot enrollment and try it again.
  4. User can uninstall the app (so the delete enrollment will be not called) and install it back (so the user will enroll again).
FeiChen-okta commented 1 year ago

Hi @pkrawczykObj

  1. This will present issues for users that have multiple devices
  2. The SDK will use the same app generated device identifier and enrollment will be replaced. If you see issues with this, then it is a bug.

3 & 4 are similar since clear app data is similar to uninstalling. If your sign on policy requires MFA, the user can't enroll again because the user does not have a valid access/refresh token. When they try to sign in to get a token, they will be challenged with a push MFA that they can't meet since the enrollment was lost. The user will need to perform account recovery to reset the lost enrollment.

pkrawczykObj commented 1 year ago

So after removing all enrollments by the Admin it worked fine for a while, but after some more tests, some enrollment was not deleted and I still have this issue.

The issue that I am reporting is exactly what you mention in point 2: "The SDK will use the same app generated device identifier and enrollment will be replaced. If you see issues with this, then it is a bug." - the SDK generates the same device identifier, but the enrollment is not replaced, but duplicated. See screenshot from admin console (all are from my user, and there were 4-5x times more): image

About point 1, my idea is to remove all old enrollments with the same ID (for current device), not all for given user.

FeiChen-okta commented 1 year ago

Hi @pkrawczykObj this is a bug with the backend. I've filed a bug for them to look at. Device ID is generated by the app so it does not persist when users clear data or uninstall. If it is the same device Id then fixing the bug will resolve the issue since they cant have multiple enrollments on the same device id.

pkrawczykObj commented 1 year ago

Hi @FeiChen-okta , I am sorry for slow response, but I was on vacations. Do you have any feedback from the backend team?

FeiChen-okta commented 1 year ago

@pkrawczykObj team is looking into it. Will update once they have a finding.

pkrawczykObj commented 1 year ago

Hi @FeiChen-okta , the issue is quite critical for us, as we are planning release at the beginning of June. Adding "enhancement" label sounds like lowering the severity of this bug. Could you please provide an estimated time for fix?

FeiChen-okta commented 1 year ago

@pkrawczykObj I can check in with team that is looking into preventing duplicate enrollments from the same device.

FeiChen-okta commented 1 year ago

@pkrawczykObj The implementation to prevent duplicate enrollments is in progress on the backend. When the same device identifier is used, the enrollment will be replaced on the backend. the client handling shouldn't change as the SDK will replace the existing enrollment.

pkrawczykObj commented 1 year ago

Hi @FeiChen-okta any updates?

FeiChen-okta commented 1 year ago

@pkrawczykObj Waiting on server release. I don't have a definite timetable on server releases but once new server changes are deployed, a new version of SDK 1.1.0 will be released. The release will add an extra parameter "applicationInstallationId" in AppConfig. This new parameter will be used to identify enrollments from same application installed. Developers is responsible for maintaining the applicationInstallationId.

FeiChen-okta commented 1 year ago

@pkrawczykObj Version 1.1.0 is released. This release introduced applicationInstallationId which will prevent multiple enrollment if enroll is called multiple times. Note that this ID is maintained by the application.

ntopaloglu commented 1 year ago

Hi @FeiChen-okta , I checked with the new release version 1.1.0 by passing applicationInstallationId. Unfortunately the issue persists. Steps that I followed;

  1. All existing enrollments deleted on okta admin panel
  2. Clean install app
  3. Enrolled to okta push once
  4. And then Deleted enrollment successfully
  5. Checked okta admin panel, enrollment is still there & Get push notification is still one of verify option.
Screenshot 2023-07-20 at 10 39 25
Screenshot 2023-07-20 at 10 31 07 Screenshot 2023-07-19 at 23 32 29
FeiChen-okta commented 1 year ago

Hi @nurullahtopaloglu Tested on my environment and the steps you've provided is working with the sample application. One thing you can do is look at app inspection and the Network Inspector. It should show the network call and a DELETE call. If this call is successful then the enrollment is deleted.

Like the following: Screenshot 2023-07-20 at 11 15 41 AM

ntopaloglu commented 1 year ago

Hİ @FeiChen-okta , delete method returns error, could you please look at it?

Screenshot 2023-07-21 at 11 36 59 Screenshot 2023-07-21 at 12 02 42
FeiChen-okta commented 1 year ago

@ntopaloglu Is the user an admin?

orlandina commented 1 year ago

@FeiChen-okta why the user should be an admin user? This is a regular test user, same as all our future app push users (not admins) - they will need to un-enroll from push when they log out from the app, or decide to turn the app push notifications off.

FeiChen-okta commented 1 year ago

Hi @orlandina, the user does not need to be an admin. if the user is an admin then some restrictions on delete are in place. If you can send me your preview domain and network logs I can take a look at where the failure. You can send them through your okta customer rep

FeiChen-okta commented 1 year ago

Hi @orlandina @ntopaloglu , Can you check the url you are using to sign in? For example in the sample app it is set int he local.properties https://github.com/okta/okta-devices-kotlin/tree/master/push-sample-app#configuration That url should be your company and not contain okta in it.

FeiChen-okta commented 1 year ago

@orlandina @ntopaloglu Check your client_id for the OIDC app oidc.client.id="{yourOrgClientId}" It maybe that you are using the wrong client_id

ntopaloglu commented 1 year ago

Hi @FeiChen-okta we don't have any issue with sign in and we didn't change anything on sign in processes during okta push implementation. It is in production for a long time. The issue is; we can enroll the device for okta push notifications and it is working. But deleting enrollment is returning error[delete method returns 501 as above]. So enrollment stays in okta admin panel and user still can see push notification as an option.