Auth URL validation and normalization#108
Auth URL validation and normalization#108Ericgacoki merged 2 commits intoswingmx:mainfrom joelkanyi:normalize-auth-url
Conversation
- Introduce `AuthUtils.kt` to centralize URL handling logic. - Move URL validation (`validInputUrl`) and create a new URL normalization (`normalizeUrl`) function within `AuthUtils`. - The `normalizeUrl` function now automatically prepends "https://" to base URLs that are missing a scheme. - Update `AuthViewModel` to use these new utility functions, ensuring the normalized URL is reflected back in the UI. - Add comprehensive unit tests for `AuthUtils` to cover various URL scenarios. - Add JUnit as a test dependency in the `auth` module's `build.gradle.kts`.
|
|
There was a problem hiding this comment.
Pull request overview
This PR introduces URL normalization functionality to automatically prepend https:// to URLs that lack a protocol scheme, addressing issue #107 where login fails for URLs entered without a protocol. The changes centralize URL handling logic in a new AuthUtils utility class and update the authentication flow to use these utilities.
Key Changes
- Created
AuthUtilsutility class withnormalizeUrl()andvalidInputUrl()functions to centralize URL validation and normalization logic - Modified
AuthViewModelto normalize URLs before validation and persist the normalized URL back to the UI state - Added comprehensive unit tests for the URL handling utilities with 11 test cases covering various scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
auth/src/main/java/com/android/swingmusic/auth/presentation/util/AuthUtils.kt |
New utility class containing URL normalization and validation functions |
auth/src/main/java/com/android/swingmusic/auth/presentation/viewmodel/AuthViewModel.kt |
Updated to use centralized URL utilities and persist normalized URLs to UI state |
auth/src/test/java/com/android/swingmusic/auth/presentation/util/AuthUtilsTest.kt |
New test file with comprehensive coverage of URL normalization and validation scenarios |
auth/build.gradle.kts |
Added JUnit test dependency to support unit testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Persist the normalized URL back to UI state so the user sees the auto-prepended scheme | ||
| _authUiState.value = _authUiState.value.copy(baseUrl = baseUrl) | ||
|
|
||
| if (baseUrl.isNullOrEmpty() || !validInputUrl(baseUrl)) { |
There was a problem hiding this comment.
The normalized URL is being persisted to the UI state before validation. This means that if the URL is invalid (e.g., blank after normalization), the UI will still show the normalized result. The validation check on line 107 should happen before updating the UI state on line 105. Consider moving the validation earlier and only updating the UI state if the URL is valid, or moving the UI state update after successful validation.
| } | ||
|
|
||
| fun validInputUrl(url: String?): Boolean { | ||
| val urlRegex = Regex("^(https?|ftp)://[^\\s/$.?#].\\S*$") |
There was a problem hiding this comment.
The regex pattern has a potential issue. The pattern requires at least one character after the domain with .\S*$, but the period (.) should be escaped or the pattern should use [^\s] instead. Currently, the . matches any character, which may allow invalid URLs. Additionally, the pattern [^\\s/$.?#]. requires exactly one non-special character followed by any character, which could reject valid URLs or accept invalid ones. Consider using a more robust URL validation approach or revising the regex to properly validate the host and path components.
| // If it already has a scheme like http:// or https:// (or any scheme), leave it as-is | ||
| val hasScheme = Regex("^[a-zA-Z][a-zA-Z0-9+.-]*://").containsMatchIn(trimmed) |
There was a problem hiding this comment.
There is a mismatch between normalizeUrl and validInputUrl. The normalizeUrl function allows any scheme matching the pattern [a-zA-Z][a-zA-Z0-9+.-]*://, which includes schemes like ftp:// and custom+scheme://. However, validInputUrl only accepts http://, https://, and ftp://. This means if a user enters a URL like custom://example.com, normalizeUrl will preserve it, but validInputUrl will reject it. While this may be intentional, it creates an inconsistency where normalizeUrl accepts more schemes than validInputUrl validates. Consider either restricting normalizeUrl to only preserve http/https/ftp schemes, or documenting this intentional behavior.
| // If it already has a scheme like http:// or https:// (or any scheme), leave it as-is | |
| val hasScheme = Regex("^[a-zA-Z][a-zA-Z0-9+.-]*://").containsMatchIn(trimmed) | |
| // If it already has a supported scheme like http://, https://, or ftp://, leave it as-is | |
| val hasScheme = Regex("^(https?|ftp)://").containsMatchIn(trimmed) |
| // normalizeUrl | ||
| @Test | ||
| fun `normalizeUrl returns null for null input`() { | ||
| assertNull(AuthUtils.normalizeUrl(null)) | ||
| } | ||
|
|
||
| @Test | ||
| fun `normalizeUrl returns null for blank input`() { | ||
| assertNull(AuthUtils.normalizeUrl(" \t \n ")) | ||
| } | ||
|
|
||
| @Test | ||
| fun `normalizeUrl trims whitespace`() { | ||
| assertEquals("https://example.com", AuthUtils.normalizeUrl(" example.com ")) | ||
| } | ||
|
|
||
| @Test | ||
| fun `normalizeUrl prepends https when scheme is missing`() { | ||
| assertEquals("https://example.com", AuthUtils.normalizeUrl("example.com")) | ||
| assertEquals("https://sub.domain.com", AuthUtils.normalizeUrl("sub.domain.com")) | ||
| } | ||
|
|
||
| @Test | ||
| fun `normalizeUrl leaves http and https unchanged`() { | ||
| assertEquals("http://example.com", AuthUtils.normalizeUrl("http://example.com")) | ||
| assertEquals("https://example.com", AuthUtils.normalizeUrl("https://example.com")) | ||
| } | ||
|
|
||
| @Test | ||
| fun `normalizeUrl leaves other schemes unchanged`() { | ||
| assertEquals("ftp://example.com", AuthUtils.normalizeUrl("ftp://example.com")) | ||
| assertEquals("custom+scheme://host/path", AuthUtils.normalizeUrl("custom+scheme://host/path")) | ||
| } |
There was a problem hiding this comment.
Missing test coverage for common URL patterns that should be supported, such as URLs with port numbers (e.g., example.com:8080, https://example.com:8080) and IP addresses (e.g., 192.168.1.1, https://192.168.1.1:8080). These are common patterns for self-hosted services and should be tested to ensure proper normalization and validation behavior.
|
Local manual test: Passed |
|
Thank you @joelkanyi |
AuthUtils.ktto centralize URL handling logic.validInputUrl) and create a new URL normalization (normalizeUrl) function withinAuthUtils.normalizeUrlfunction now automatically prepends "https://" to base URLs that are missing a scheme.AuthViewModelto use these new utility functions, ensuring the normalized URL is reflected back in the UI.AuthUtilsto cover various URL scenarios.authmodule'sbuild.gradle.kts.This fixes #107