element-x-android
element-x-android copied to clipboard
Use external app for OIDC fallback instead of webview
Motivation and context
if the default browser is not Chrome we are currently using Webview which google no longer supports, and we should open the auth link in an external browser instead
Tests
- Change the default Browser to other than Chrome
- change the home server
- try login by tapping continue
- google login URL opened in external browser
Tested devices
- [ ] Physical
- [X] Emulator
- OS version(Android 15):
Checklist
- [x] Changes have been tested on an Android device or Android emulator with API 24
- [x] UI change has been tested on both light and dark themes
- [x] Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
- [x] Pull request is based on the develop branch
- [x] Pull request title will be used in the release note, it clearly define what will change for the user
- [x] Pull request includes screenshots or videos if containing UI changes
- [x] You've made a self review of your PR
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.
Thank you for your contribution! Here are a few things to check in the PR to ensure it's reviewed as quickly as possible:
- Your branch should be based on
origin/develop, at least when it was created. - The title of the PR will be used for release notes, so it needs to describe the change visible to the user.
- The test pass locally running
./gradlew test. - The code quality check suite pass locally running
./gradlew runQualityChecks. - If you modified anything related to the UI, including previews, you'll have to run the
Record screenshotsGH action in your forked repo: that will generate compatible new screenshots. However, given Github Actions limitations, it will prevent the CI from running temporarily, until you upload a new commit after that one. To do so, just pull the latest changes and push an empty commit.
@bmarty please have a look at this Reference Issue
Yes embedded web-view based login when OIDC upstream provider is Google doesn't work and it isn't recommended either. This PR fixes this issue. We were having this problem with our community hence we decided to contributed the patch upstream. Please let us know if there is something else that we need to do.
We would love to have this PR merged.
@sandhose do you have any concern with this changes?
On my side I think that some actions that was handled by the OidcView may not be handled correctly, but I haven't tested this patch yet.
Codecov Report
Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
Project coverage is 80.12%. Comparing base (
a3732fe) to head (5e8e4eb). Report is 579 commits behind head on develop.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| ...droid/libraries/oidc/impl/DefaultOidcEntryPoint.kt | 0.00% | 1 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## develop #4293 +/- ##
===========================================
- Coverage 80.13% 80.12% -0.01%
===========================================
Files 2052 2052
Lines 54576 54577 +1
Branches 6672 6672
===========================================
- Hits 43733 43729 -4
- Misses 8560 8562 +2
- Partials 2283 2286 +3
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
I think it's indeed better to fallback to the system browser rather than a WebView, even though that means switching apps. We do need to make sure that the callback to the app works as intended
Replaced by #4808 which contains more cleanup