Block insecure connection on the WebView #6103
Conversation
Test Results 89 files + 2 89 suites +2 7m 31s ⏱️ -15s For more details on these failures, see this check. Results for commit fe0b2a9. ± Comparison against base commit bfecbf7. ♻️ This comment has been updated with latest results. |
| * @param url The URL being used for the connection | ||
| * @return [SecurityStatus.Secure] if the connection is secure, [SecurityStatus.Insecure] otherwise | ||
| */ | ||
| fun currentSecurityStatusForUrl(context: Context, url: String): SecurityStatus { |
There was a problem hiding this comment.
"for a given url" where you provide the url shouldn't exist when it is an extension function on the class that holds the URLs. It should not be an extension function here, determine the url to check automatically, or allow you to pass an enum for internal/external/cloud.
There was a problem hiding this comment.
You sent me a DM on Discord about creating a new UrlHelper class for this one. Feel free to experiment but please comment here once you decided whether to introduce that or keep this.
...src/main/kotlin/io/homeassistant/companion/android/webview/insecure/BlockInsecureFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/kotlin/io/homeassistant/companion/android/webview/insecure/BlockInsecureScreen.kt
Outdated
Show resolved
Hide resolved
| modifier = Modifier | ||
| .padding(all = 20.dp) | ||
| .size(120.dp), | ||
| imageVector = Icons.Default.Lock, |
There was a problem hiding this comment.
We had some discussions about this on Discord. My vote is for mdi:lock-open-alert as you're talking about http(s) in the screen and users have been trained for open lock = http, closed lock = https in web browsers, so showing a closed lock because they are using http feels wrong.
build-logic/convention/src/main/kotlin/AndroidComposeConventionPlugin.kt
Show resolved
Hide resolved
app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewActivity.kt
Show resolved
Hide resolved
| val allowInsecureConnection = serverManager.integrationRepository( | ||
| presenter.getActiveServer(), | ||
| ).getAllowInsecureConnection() |
There was a problem hiding this comment.
I think this should be exposed via the presenter instead, the activity should try to avoid using repositories directly
There was a problem hiding this comment.
I had the same feelings but we are already calling getServer removeServer that we shouldn't.
I'm going to move this to the presenter in any case.
TimoPtr
force-pushed
the
feature/onboarding_set_security_level_migration
branch
from
bf162f7 to
91753f8
Compare
TimoPtr
force-pushed
the
feature/block_insecure_connection
branch
from
423e8ff to
7919d50
Compare
TimoPtr
force-pushed
the
feature/block_insecure_connection
branch
from
7919d50 to
fe0b2a9
Compare
Summary
Add a new screen on top of the WebViewActivity that prevents loading the URL if we are in an insecure state and the user picked the
Most securedoption. This screen change based on the current state and can display banner with button to help the user 'fix' the situation.Checklist
Screenshots
Any other notes
The screen doesn't refresh on its own.