Authentication Dialog Enhancement #633

neopilou · 2020-06-17T14:23:10Z

Add "Remember" checkbox
Add button for password visibility

- Add "Remember" checkbox - Add button for password visibility

JBassett · 2020-06-18T19:46:29Z

.../java/io/homeassistant/companion/android/data/authentication/AuthenticationRepositoryImpl.kt

+        val httpAuthList: Set<String>
+        if (localStorage.getStringSet(PREF_HTTP_AUTH_LIST) != null)
+            httpAuthList = localStorage.getStringSet(PREF_HTTP_AUTH_LIST)!!
+        else
+            httpAuthList = emptySet<String>()
+        return httpAuthList


return localStorage.getStringSet(PREF_HTTP_AUTH_LIST) ?: emptySet()

HttpAuthList functions have been removed from shared preference since use of a full DB

JBassett · 2020-06-18T19:56:52Z

app/src/main/res/layout/dialog_authentication.xml

@@ -1,20 +1,67 @@
 <?xml version="1.0" encoding="utf-8"?>


We should try to stick to the material guidelines for the dialog (https://material.io/components/dialogs).

Left justify checkbox

Use alert dialog title so its consistent

Padding around user and password fields

Left justified checkbox

Used alert dialog title

Added padding around user and password

JBassett · 2020-06-18T20:18:36Z

app/src/main/java/io/homeassistant/companion/android/webview/WebViewActivity.kt

+                .setView(dialogLayout)
+                .setPositiveButton(android.R.string.ok) { _, _ ->
+                    if (username.text.toString() != "" && password.text.toString() != "") {
+                        val httpAuth = host + "|" + username.text.toString() + "|" + password.text.toString()


This feels like a gigantic hack for persisting this information. We should consider persisting this in a full DB rather than
a Shared Preference.

Persisting data is now save in SQLite database, and no longer in Shared Preference.

- Added a full DB rather than a Shared Preference - Modified dialog_authentication layout to try to stick to the material guidelines for the dialog

balloob · 2020-06-19T16:02:11Z

app/src/main/java/io/homeassistant/companion/android/webview/WebViewActivity.kt

-            .setView(dialogLayout)
-            .setPositiveButton(android.R.string.ok) { _, _ ->
-                handler.proceed(username.text.toString(), password.text.toString())
+        val remember = dialogLayout.findViewById<CheckBox>(R.id.checkBox)


Why is there a remember me checkbox? Isn't that the same as disabling auth completely?

I assume this is not the same thing, a user can access a site or a resource in different ways, this application for example or directly from a browser without going through HASS. So he will not necessarily want to disable authentication. On the other hand, if the application is already locked (by credential or biometric), he can use this "Remember" function so as not to have to re-authenticate himself on his different sites. It's a bit like a SSO.

That makes no sense to me. You either have biometric authentication turned on, and so you are prompted. If you don't want it, you disable it.

Let's remove this option.

I am sorry for intervention, but as we have now confirmed that the authentication dialog is for Basic HTTP auth, shouldn't we keep remember me button? To me it really does make sense to have it. You wouldn't want to login to iFrame every time you open the app.

@balloob this is for saving external authentication a panel or iFrame that requires basic auth. This shouldn't have any bearing on the authentication to Home Assistant itself.

balloob · 2020-06-20T04:38:43Z

I'm actually completely confused here why we ever have username/password? What is going on… I understand fingerprint, maybe pin as fallback. We should not add a user/pass system ON TOP of Home Assistant which has its own. That is confusing.

neopilou · 2020-06-20T06:09:40Z

We not add a user/pass system on top of Home Assistant which has its own. It's for basic HTTP Authentication Request, for embedded sites in IFrame Card, like Blue Iris, Tautulli, Kodi and others

balloob · 2020-06-20T06:48:18Z

Oh my bad. Ok, that is fine I guess. Can we make the dialog header mention that it is HTTP Auth ? We should also make it look like material design https://material.io/components/dialogs

- Replaced title "Authentication Requested" by "Basic HTTP Authentication Requested" - Fix padding around user and password

neopilou · 2020-06-22T09:29:02Z

Replaced title "Authentication Requested" by "Basic HTTP Authentication Requested" :

balloob · 2020-06-22T21:27:22Z

Let's drop the : from the header.

And our primary color is blue, not purple. #03a9f4

balloob · 2020-06-22T21:31:25Z

Let's also make it less wide.

Preferably we make it look exactly like the Chrome on Android dialog. Test it out on Chrome on http://httpbin.org/basic-auth/user/passwd

The "Connection not private" string is added if not over https.

JBassett · 2020-06-22T22:59:17Z

@balloob Do we want to be consistent with chrome or the OS? The Alert Dialog we are using is the standard for the OS and handles the title and action buttons. I don't know if we want to implement a completely custom dialog to try to mirror another app.

balloob · 2020-06-22T23:01:23Z

Oh, yes, you're right. Let's use OS level dialog.

- Added Dialog .setMessage() (like Chrome) with base URL and "Connection not private" string if not over https.

neopilou · 2020-06-23T09:10:28Z

Added Dialog .setMessage() (like Chrome) with base URL and "Connection not private" string if not over https
Removed the : from the header

balloob · 2020-06-23T16:00:17Z

Nice! Can we reduce the width? This seems very wide. Maybe a max width of 300px?

- Seted Dialog maximum width at 300dp

neopilou · 2020-06-24T06:54:00Z

Seted Dialog maximum width at 300dp :

balloob · 2020-06-24T16:38:00Z

Can we drop the : from the header.

neopilou · 2020-06-24T18:11:58Z

Look in String.xml, the : is deleted, but seems not updated in app...

app/src/main/java/io/homeassistant/companion/android/database/DataBaseHelper.kt

app/src/main/java/io/homeassistant/companion/android/webview/WebViewActivity.kt

JBassett · 2020-07-03T11:36:04Z

app/src/main/java/io/homeassistant/companion/android/webview/WebViewActivity.kt

-            .setView(dialogLayout)
-            .setPositiveButton(android.R.string.ok) { _, _ ->
-                handler.proceed(username.text.toString(), password.text.toString())
+        val remember = dialogLayout.findViewById<CheckBox>(R.id.checkBox)


@balloob this is for saving external authentication a panel or iFrame that requires basic auth. This shouldn't have any bearing on the authentication to Home Assistant itself.

- Replaced SQLite by Room

Authentication Dialog Enhancement

bb37645

- Add "Remember" checkbox - Add button for password visibility

homeassistant added the cla-signed label Jun 17, 2020

JBassett requested changes Jun 18, 2020

View reviewed changes

Added full DB & Modified dialog_authentication

49293ee

- Added a full DB rather than a Shared Preference - Modified dialog_authentication layout to try to stick to the material guidelines for the dialog

neopilou requested a review from JBassett June 19, 2020 15:51

Deleted unused constant

78c5b48

balloob reviewed Jun 19, 2020

View reviewed changes

neopilou and others added 3 commits June 22, 2020 07:57

Merge branch 'master' into auhtentication-prompt

03235fb

Modified Dialog

2107ba2

- Replaced title "Authentication Requested" by "Basic HTTP Authentication Requested" - Fix padding around user and password

Fix

fd14945

Fix Strings name

7769ef6

Added message in Dialog

05d7e3e

- Added Dialog .setMessage() (like Chrome) with base URL and "Connection not private" string if not over https.

neopilou requested a review from balloob June 23, 2020 09:12

Seted Dialog maximum width

61614b1

- Seted Dialog maximum width at 300dp

JBassett requested changes Jul 3, 2020

View reviewed changes

Replaced SQLite by Room

c3d1380

- Replaced SQLite by Room

neopilou requested a review from JBassett July 3, 2020 17:01

JBassett added minor A minor change - Bumps minor build number enhancement New feature or request labels Jul 6, 2020

neopilou and others added 3 commits July 6, 2020 08:06

Merge branch 'master' into auhtentication-prompt

60739d6

Merge branch 'master' into auhtentication-prompt

3ba3fa1

Fix bad merge.

361a2d8

JBassett approved these changes Jul 7, 2020

View reviewed changes

JBassett merged commit 50d8041 into home-assistant:master Jul 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authentication Dialog Enhancement #633

Authentication Dialog Enhancement #633

neopilou commented Jun 17, 2020

JBassett Jun 18, 2020

neopilou Jun 19, 2020

JBassett Jun 18, 2020

neopilou Jun 19, 2020

JBassett Jun 18, 2020

neopilou Jun 19, 2020

balloob Jun 19, 2020

neopilou Jun 19, 2020 •

edited

balloob Jun 20, 2020

klejejs Jun 20, 2020

JBassett Jul 3, 2020

balloob commented Jun 20, 2020

neopilou commented Jun 20, 2020

balloob commented Jun 20, 2020

neopilou commented Jun 22, 2020

balloob commented Jun 22, 2020

balloob commented Jun 22, 2020

JBassett commented Jun 22, 2020

balloob commented Jun 22, 2020

neopilou commented Jun 23, 2020 •

edited

balloob commented Jun 23, 2020

neopilou commented Jun 24, 2020

balloob commented Jun 24, 2020

neopilou commented Jun 24, 2020

JBassett Jul 3, 2020

Authentication Dialog Enhancement #633

Authentication Dialog Enhancement #633

Conversation

neopilou commented Jun 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neopilou Jun 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob commented Jun 20, 2020

neopilou commented Jun 20, 2020

balloob commented Jun 20, 2020

neopilou commented Jun 22, 2020

balloob commented Jun 22, 2020

balloob commented Jun 22, 2020

JBassett commented Jun 22, 2020

balloob commented Jun 22, 2020

neopilou commented Jun 23, 2020 • edited

balloob commented Jun 23, 2020

neopilou commented Jun 24, 2020

balloob commented Jun 24, 2020

neopilou commented Jun 24, 2020

Choose a reason for hiding this comment

neopilou Jun 19, 2020 •

edited

neopilou commented Jun 23, 2020 •

edited