bug: OnboardingService no longer supports activation

[XavierChanth]

• When calling OnboardingService.onboard with the cramSecret, it just onboards a random atSign from the keychain
• When calling OnboardingService.authenticate with the cramSecret, it tries to pull an atSign from the keychain and throws with an uncaught exception

[gkc]

Walking through these issues with @sitaram-kalluri and @murali-shris & me transcribing our findings

• When calling OnboardingService.onboard with the cramSecret, it just onboards a random atSign from the keychain

Yes - if setAtsign has not been called previously then this will happen. The immediate fix for this is to call setAtsign beforehand in your application code.

However clearly this is a bug and needs to be fixed. In general the use of the _atSign instance variable needs to be reviewed as well as all of the method parameters

• When calling OnboardingService.authenticate with the cramSecret, it tries to pull an atSign from the keychain and throws with an uncaught exception

Yes we can see how that happens if the atSign previously hasn't been onboarded - i.e. no entry for it in the keychain. To avoid the issue, need to ensure you don't call authenticate in this situation (no entry in keychain, no keys supplied, only cram key supplied) but call onboard instead (with the caveat that you must call setAtsign before calling onboard)

@sitaram-kalluri @murali-shris can you verify the above with some demo app please? i.e.

1. Reproduce the problems Xavier encountered
2. Show how taking the steps outlined above successfully mitigate the issue

@XavierChanth I am assuming that your primary need here is to onboard and/or authenticate ... in both cases you need to use the onboard method which, if the atSign has already been onboarded (keychain entry exists) will try to authenticate, and if the atSign has not yet been onboarded, will onboard.

[sitaram-kalluri]

Adding to the above, the only time we wanted to call OnboardingService.authenticate is when we have atKeys file generated for an atSign and wanted to authenticate an atSign by supplying the atKeys file. In this case, set optional parameter jsonData with the atKeys file content.

[gkc]

Thanks @sitaram-kalluri

Once you've verified the mitigation for the issue Xavier is facing right now, please create a ticket to tidy up OnboardingService - first its method documentation, then parameter validations (e.g. in authenticate it looks like cramSecret and jsonData are mutually exclusive). Don't do anything more broad on implementation changes for now please; there need to be a design discussion with Xavier to agree the way forward.

[sitaram-kalluri]

Thanks @sitaram-kalluri

Once you've verified the mitigation for the issue Xavier is facing right now, please create a ticket to tidy up OnboardingService - first its method documentation, then parameter validations (e.g. in authenticate it looks like cramSecret and jsonData are mutually exclusive). Don't do anything more broad on implementation changes for now please; there need to be a design discussion with Xavier to agree the way forward.

Sure, Gary.

[XavierChanth]

Yes - if setAtsign has not been called previously then this will happen. The immediate fix for this is to call setAtsign beforehand in your application code.

The internal _atSign variable is used to store the currently onboarded atSign. If onboard fails, then _atsign should remain unchanged. The expected behavior is that when I call onboard. The atsign stored in the AtOnboardingRequest is the one used for onboarding.

This will cause breakages in changePrimaryAtsign if that fails. It won't affect NoPorts, but it will affect other apps.

[sitaram-kalluri]

Thanks @sitaram-kalluri

Once you've verified the mitigation for the issue Xavier is facing right now, please create a ticket to tidy up OnboardingService - first its method documentation, then parameter validations (e.g. in authenticate it looks like cramSecret and jsonData are mutually exclusive). Don't do anything more broad on implementation changes for now please; there need to be a design discussion with Xavier to agree the way forward.

@gkc and @XavierChanth : Verified the fix suggested above and able to onboard an atSign successfully when setAtSign is invoked before calling the OnboardingService.onboard method. Also, if an atSign is onboarded successfully and keys are stored in the keychain manager, then calling onboard will authenticate the atSign successfully.

[XavierChanth]

Adding to the above, the only time we wanted to call OnboardingService.authenticate is when we have atKeys file generated for an atSign and wanted to authenticate an atSign by supplying the atKeys file. In this case, set optional parameter jsonData with the atKeys file content.

Yes, but this is not what the rest of the code in at_onboarding_flutter was using that method to do. All of the existing activation code is calling authenticate with the cramkey.

[XavierChanth]

Thanks @sitaram-kalluri

Once you've verified the mitigation for the issue Xavier is facing right now, please create a ticket to tidy up OnboardingService - first its method documentation, then parameter validations (e.g. in authenticate it looks like cramSecret and jsonData are mutually exclusive). Don't do anything more broad on implementation changes for now please; there need to be a design discussion with Xavier to agree the way forward.

The only proper way forward is to remove all the layers, including the OnboardingService, and replace it all with a proper utility class that provides a progress stream for the UI. Each one of the layers I found checks atServerStatus on its own before doing anything. The code as it is today, makes 3 or 4 api calls to the same thing before it does any work.

My previous recommendation about adding generic Keychain management to at_auth removes the need for the OnboardingService to exist.

[sitaram-kalluri]

Created a clean ticket : https://github.com/atsign-foundation/at_widgets/issues/946

[gkc]

Yes all of this is a complete mess and has a bad case of code rot

[XavierChanth]

I will uptake the same fixes I made to NoPorts back into this package once NoPorts is released