-
Notifications
You must be signed in to change notification settings - Fork 136
[HACK] Add auto-login universal link for debug builds #15062
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
base: trunk
Are you sure you want to change the base?
Conversation
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a debug-only auto-login feature via universal link to streamline developer and testing workflows. The feature allows automatic login using application password credentials through a specially formatted URL.
Key Changes:
- Adds
AutoLoginHandlerto process auto-login requests and validate credentials - Adds
AutoLoginActivityto handle the universal link intent and coordinate the login flow - Registers the auto-login deep link in the debug manifest with intent filter
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
AutoLoginHandler.kt |
Core logic for parsing credentials from URI, validating site/user eligibility, and managing login state |
AutoLoginActivity.kt |
Activity that handles the auto-login universal link intent and navigates to appropriate screens based on success/failure |
AndroidManifest.xml |
Registers the auto-login activity with intent filter for woocommerce.com/mobile/auto-login path (debug builds only) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| onFailure = { exception -> | ||
| WooLog.e(WooLog.T.LOGIN, "AutoLoginHandler: Failed to check user eligibility", exception) | ||
| AutoLoginResult.Error("Failed to verify user: ${exception.message}") |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fold operation on eligibilityResult doesn't return a value in the onFailure branch. This will cause a compilation error because the outer fold expects a return value.
Add return@withContext before the AutoLoginResult.Error:
onFailure = { exception ->
WooLog.e(WooLog.T.LOGIN, "AutoLoginHandler: Failed to check user eligibility", exception)
return@withContext AutoLoginResult.Error("Failed to verify user: ${exception.message}")
}| AutoLoginResult.Error("Failed to verify user: ${exception.message}") | |
| return@withContext AutoLoginResult.Error("Failed to verify user: ${exception.message}") |
| }, | ||
| onFailure = { exception -> | ||
| WooLog.e(WooLog.T.LOGIN, "AutoLoginHandler: Failed to fetch site", exception) | ||
| AutoLoginResult.Error("Failed to connect to site: ${exception.message}") |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fold operation on fetchSiteResult doesn't return a value in the onFailure branch. This will cause a compilation error.
Add return@withContext before the AutoLoginResult.Error:
onFailure = { exception ->
WooLog.e(WooLog.T.LOGIN, "AutoLoginHandler: Failed to fetch site", exception)
return@withContext AutoLoginResult.Error("Failed to connect to site: ${exception.message}")
}| AutoLoginResult.Error("Failed to connect to site: ${exception.message}") | |
| return@withContext AutoLoginResult.Error("Failed to connect to site: ${exception.message}") |
| val fetchSiteResult = wpApiSiteRepository.fetchSite( | ||
| url = siteUrl, | ||
| username = username, | ||
| password = password | ||
| ) |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Credentials (username and password) are being logged or could be exposed in error messages. Even though this is a debug-only feature, credentials should not be logged. The current implementation doesn't directly log credentials, but if any of the underlying repository methods log the parameters, sensitive information could be exposed.
Consider adding a comment or documentation noting that the underlying repository methods should not log credentials, or sanitize error messages to ensure they don't inadvertently include sensitive data.
| android:name=".ui.login.auto.AutoLoginActivity" | ||
| android:exported="true" | ||
| android:theme="@style/Theme.Woo.Splash"> | ||
| <intent-filter android:autoVerify="true"> |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The autoVerify="true" attribute on the intent filter is unnecessary for this debug-only feature and could cause confusion. Auto-verification is used for App Links to verify domain ownership via a Digital Asset Links file, which is not needed for a debug testing tool.
Consider removing android:autoVerify="true" from the intent filter:
<intent-filter>| <intent-filter android:autoVerify="true"> | |
| <intent-filter> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kidinov This seems like a valid point, wdyt?
| selectedSite.set(site) | ||
|
|
||
| WooLog.d(WooLog.T.LOGIN, "AutoLoginHandler: Login successful") | ||
| AutoLoginResult.Success |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The login function doesn't return a value in all code paths. When eligibilityResult.fold succeeds and the user is eligible, AutoLoginResult.Success is created but not returned. This will cause a compilation error.
Add return@withContext before AutoLoginResult.Success to fix this:
return@withContext AutoLoginResult.Success| AutoLoginResult.Success | |
| return@withContext AutoLoginResult.Success |
malinajirka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me overall, however, I'm not sure whether it's a good idea to have it. Especially, if we decide to stop doing smoke tests before each release, I think there is a value in going through the login flow during development. If we had to login after each change, that would be too much hassle, but we need to login just when we are testing something specific. Having said all that, I'm good with merging it.
| android:name=".ui.login.auto.AutoLoginActivity" | ||
| android:exported="true" | ||
| android:theme="@style/Theme.Woo.Splash"> | ||
| <intent-filter android:autoVerify="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kidinov This seems like a valid point, wdyt?
Description
Adds support for a universal link that allows automatic login using application password credentials in debug builds. This is useful for developer and testing workflows.
URL Format:
Example (with placeholder credentials):
Behavior:
Do not merge, as I am not sure how valuable this is for anyone so I firstly would like to ask
Test Steps
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.