fix(#401): update OIDC handling to use custom tabs#405
Conversation
jkuester
changed the title
| tools:ignore="DiscouragedApi"/> | ||
| android:screenOrientation="portrait" | ||
| android:configChanges="orientation|screenSize" | ||
| android:launchMode="singleTask" |
There was a problem hiding this comment.
One subtle change here is switching the launchMode from singleInstance to singleTask. As far as I could tell, we always had singleInstace set for the AppUrlIntentActivity, but just reading the documentation, it feels like that is not really the right choice. With the way I currently understand things, singleTask will allow us to always re-use the existing instance of EmbeddedBrowserActivity and continue the task chain as normal. The singleInstance docs note that any additional activities launched off of the root are not part of the same task chain.
TLDR, as far as I can tell, singleTask is "more normal" than singleInstance and we get the desired behavior.
There was a problem hiding this comment.
Hm, this was a good read. I remember we had one version of the App that ended up launching multiple instances. Of course the background instances could not be reached or removed, and the only way they disappeared was when the whole app was killed.
:(
singleTask sounds like would not create a new activity for our use case, but we should be careful none the less.
| } | ||
|
|
||
| @Override | ||
| protected void onNewIntent (Intent intent) { |
There was a problem hiding this comment.
FTR, this is now the re-entry point during the OIDC flow. The instance of EmbeddedBrowserActivity is created when displaying the initial login page. Then we pop to the Custom Tab to do the OIDC auth. Finally, the redirect_uri brings us back here via the validated app url.
There was a problem hiding this comment.
Sadly, I have not found any convenient way of testing the code in the onCreate method yet. Given that we don't have any existing tests, I do not think we should block on this, though... 😓
|
@dianabarsan could you have a look at this when you get a chance? 🙏 For convenient manual testing, I have configured gamma for SSO login (via Google OIDC). I provisioned a CHT user associated with your medic.org email that you should be able to use. 👍 @icrc-fdeniger can you test this out from your end too? I am hoping this fix resolves the issues with Microsoft Entra and using Google accounts for guest logins. |
dianabarsan
left a comment
dianabarsan
left a comment
There was a problem hiding this comment.
CustomTabsIntent sounds like a good choice! How do they behave in reality? OH OH just watched a video. I believe apps like FB and Insta use custom tabs to open adds.
Cool stuff, I asked some questions for clarity.
| tools:ignore="DiscouragedApi"/> | ||
| android:screenOrientation="portrait" | ||
| android:configChanges="orientation|screenSize" | ||
| android:launchMode="singleTask" |
There was a problem hiding this comment.
Hm, this was a good read. I remember we had one version of the App that ended up launching multiple instances. Of course the background instances could not be reached or removed, and the only way they disappeared was when the whole app was killed.
:(
singleTask sounds like would not create a new activity for our use case, but we should be careful none the less.
| <action android:name="android.intent.action.VIEW" /> | ||
| <category android:name="android.intent.category.DEFAULT" /> | ||
| <category android:name="android.intent.category.BROWSABLE" /> | ||
| <data android:scheme="@string/scheme" android:host="@string/app_host" android:pathPattern=".*"/> |
There was a problem hiding this comment.
I believe that this section was added for token login. Does this still open the existent activity when an intent is triggered from SMS?
There was a problem hiding this comment.
I believe that this section was added for token login.
Correct. This is how token login links (and really any other kind of deep-linking) works.
Does this still open the existent activity when an intent is triggered from SMS?
Yes, this is key and is the whole point of the launchMode="singleTask" configuration. With that launchMode, (and with the previous singleInstance), if we have an existing task instance of EmbeddedBrowserActivity, then incoming intents will be directed into that task (instead of creating a new one). If we did not set the launchMode, then by default Android I think Android would launch a separate instance of cht-android each time a deep-link is tapped.
| @Override | ||
| protected void onNewIntent(Intent intent) { | ||
| Uri appLinkData = intent.getData(); | ||
| browseTo(appLinkData); |
There was a problem hiding this comment.
is this what makes token login not launch a new instance?
There was a problem hiding this comment.
Technically, it is the launchMode configuration that "makes" a new instance not get launched. But, yes, this is the code that receives the incoming intent for an existing instance of EmbeddedBrowserActivity when a token login link is tapped.
One thing to note is that based on my testing, there is a bit of a difference between the code flows for launching an app link when no cht-android instance is currently running, vs when you already have an instance running in the background or something. In the first case, the onCreate method above is called and the app-url intent is handled by the code at line 128. onNewIntent is not called when onCreate is called. In the second case (when an instance of cht-android is already running in the background), the incoming app-url intent triggers the onNewIntent method, but the onCreate is not called (since we are not creating a new instance of EmbeddedBrowserActivity. Since both methods ultimately end up called browseTo, the functionality/UX of what happens remains the same either way.
@jkuester I confirm it's working well with our setup. Thanks a lot ! |
Can you describe more about this? (Or is there a separate ticket filed somewhere for it?) Is this a cht-core issue we are looking at or is it something else related to the SSO flow? |
|
@jkuester don't think it's an issue on cht. It's related to ICRC's way to accept request from the mobile App. |
3 participants
Closes #401
The PR updates the OIDC handling added in #397 to open the OIDC authentication links in an Android Custom Tab instead of keeping the OIDC authentication flow contained in the root webview.
This approach requires that the app be running with a verified app link (can either be manually verified or configured for auto-verification).
Unlike behavior previously observed in child webviews, the Custom Tabs seem to resolve these app links even when they come as a re-direct at the end of the OIDC authentication flow. Using a Custom Tab (from the default browser) to execute the login flow not only allows for Google logins, but also should allow for true "single" sign-on in the sense that if the user already has a session in their browser with the OIDC provider, they will not have to re-authenticate when logging into the CHT.
As noted out on the issue thread, the existing app link implementation via
AppUrlIntentActivitywas causing issues with a weird race condition where the url with the OIDC auth_code was not evaluated in the cht-android webview fast enough, but instead somehow it was getting evaluated in the default browser first. This meant that by the time the cht-android webview got around to submitting the auth_code it was invalid (since it had already been used...). To fix this, I have simplified the app link handling to just launch directly into theEmbeddedBrowserActivityinstead of launching theAppUrlIntentActivitywhich then launched theEmbeddedBrowserActivity. Overall this seems like an acceptable refactor because it reduces some complexity/redundancy without really sacrificing anything.Tangential to the original issue, but my changes here also apparently fix deep linking via app links. It seems that at some point the ability to deep link (e.g. to a specific person/place/etc) was broken by updates to our "lastUrl" logic. That should be fixed now.