element-x-android icon indicating copy to clipboard operation
element-x-android copied to clipboard

Use external app for OIDC fallback instead of webview

Open msusman1 opened this issue 9 months ago • 7 comments

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

access blocked

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

msusman1 avatar Feb 21 '25 15:02 msusman1

CLA assistant check
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.

CLAassistant avatar Feb 21 '25 15:02 CLAassistant

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 screenshots GH 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.

github-actions[bot] avatar Feb 21 '25 15:02 github-actions[bot]

@bmarty please have a look at this Reference Issue

msusman1 avatar Feb 26 '25 09:02 msusman1

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.

iamtalhaasghar avatar Feb 26 '25 10:02 iamtalhaasghar

@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.

bmarty avatar Feb 26 '25 10:02 bmarty

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.

codecov[bot] avatar Feb 26 '25 10:02 codecov[bot]

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

sandhose avatar Feb 26 '25 10:02 sandhose

Replaced by #4808 which contains more cleanup

bmarty avatar Jun 03 '25 09:06 bmarty