Skip to content

#537588 add options to load some URLs in the webview#397

Merged
jkuester merged 4 commits intomedic:masterfrom
icrc-fdeniger:sso-icrc
Jun 20, 2025
Merged

#537588 add options to load some URLs in the webview#397
jkuester merged 4 commits intomedic:masterfrom
icrc-fdeniger:sso-icrc

Conversation

@icrc-fdeniger
Copy link
Copy Markdown
Contributor

@icrc-fdeniger icrc-fdeniger commented Jun 11, 2025

add options to load some URLs in the webview
should fix #396

@jkuester jkuester self-requested a review June 18, 2025 22:04
Copy link
Copy Markdown
Contributor

@jkuester jkuester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@icrc-fdeniger I have pushed a commit that moves the logic up to the UrlHandler. (See my inline comment for the reason.) Let me know if you see any problems with that and if that functionality still works on your end. If all that is good, I think we can ship this! 👍

return isUrlRelated(appUrl, uriToTest.toString());
String uriToTestString = uriToTest.toString();
return isUrlRelated(appUrl, uriToTestString)
|| (uriToTestString.contains("redirect_uri="+ Uri.encode(appUrl)+Uri.encode("/medic/login/oidc")));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit worried about unintended consequences of including this logic down here in the generic isUrlRelated function. This function also gets called via isValidNavigationUrl when the app is closed (as part of the logic to save the user's current url to open next time). If the app gets closed during the middle of an OIDC login flow, I don't think we want to save that URL.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @jkuester all good for me with your improvements.

@jkuester
Copy link
Copy Markdown
Contributor

@lorerod can you also give these changes a go and see if find any issues? 🙏

@jkuester jkuester requested a review from lorerod June 19, 2025 01:57
@lorerod
Copy link
Copy Markdown

lorerod commented Jun 19, 2025

Hi @jkuester, @icrc-fdeniger ,
Thank you for adding me to this PR and also for adding the unit tests. I’ve tested this branch locally by following the steps outlined in these documentation, and everything is working as expected on my end.

Here are videos of two tests conducted on my Galaxy A01 Core (Android 10):

Login with SSO when already logged in to Keycloak
scenario2.mov
Login with SSO when not logged in to Keycloak
scenario1.mov

This work looks good to me. Approving now.
Let me know if there’s anything else I should try or verify.

Copy link
Copy Markdown

@lorerod lorerod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment with my test findings. This work looks great. Thank you @icrc-fdeniger.

@jkuester jkuester merged commit 72bb65f into medic:master Jun 20, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support CHT SSO

3 participants