Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use external app for OIDC fallback instead of webview #4293

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

msusman1
Copy link

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
  • Emulator
  • OS version(Android 15):

Checklist

  • Changes have been tested on an Android device or Android emulator with API 24
  • UI change has been tested on both light and dark themes
  • Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
  • Pull request is based on the develop branch
  • Pull request title will be used in the release note, it clearly define what will change for the user
  • Pull request includes screenshots or videos if containing UI changes
  • You've made a self review of your PR

@msusman1 msusman1 requested a review from a team as a code owner February 21, 2025 15:08
@msusman1 msusman1 requested review from bmarty and removed request for a team February 21, 2025 15:08
@CLAassistant
Copy link

CLAassistant commented Feb 21, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

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.

@msusman1
Copy link
Author

@bmarty please have a look at this
Reference Issue

@iamtalhaasghar

This comment was marked as duplicate.

Copy link

@iamtalhaasghar iamtalhaasghar left a comment

Choose a reason for hiding this comment

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

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.

@bmarty
Copy link
Member

bmarty commented Feb 26, 2025

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

Copy link

codecov bot commented Feb 26, 2025

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 30 commits behind head on develop.

Files with missing lines Patch % Lines
...droid/libraries/oidc/impl/DefaultOidcEntryPoint.kt 0.00% 1 Missing ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sandhose
Copy link
Member

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

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.

5 participants