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

Fix Biometric Unlock & Enhancement #625

Merged
merged 17 commits into from
Jul 5, 2020
Merged

Fix Biometric Unlock & Enhancement #625

merged 17 commits into from
Jul 5, 2020

Conversation

neopilou
Copy link
Contributor

- Delete "Lock" folder
- Add Prompt Unlock function directly in WebViewActivity
- Added session timeout in app settings
- App is locked when resumed (after session timedout)
- When the application is locked, the screen is blurred (no quite obnoxious splash screen to look at), which allows the HA frontend to be loaded before or during unlocking
- No button needs to be touched for unlock
- When the unlock prompt is displayed, the back button close the app
- Fix issues #555 and #587
Copy link
Collaborator

@JBassett JBassett left a comment

Choose a reason for hiding this comment

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

I think the blur affect is pretty awesome. Personally I'm not a fan of adding the complexity for the timeout. The logic being added is a running cost anytime we add a new view or menu we need to add timeout logic where the old way was just part of launching the app. Even in your PR you added 2 different ways to set the timeout time. Just my personal take on it so looking for others to chime in.

On another note I know that @balloob also had concerns about adding more preferences and this seems like an excessive one.

app/build.gradle.kts Outdated Show resolved Hide resolved
}

override fun onPause() {
presenter.setSessionExpireMillis((System.currentTimeMillis() + (presenter.sessionTimeOut() * 1000)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this set the lock time in the future onPause even if the app is locked? Couldn't you just put to background in settings then return to app and it would be unlocked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but we can resume the app with WebViewActivity and not with SettingsActivity when user leaving application while settings are displayed

@balloob
Copy link
Member

balloob commented Jun 18, 2020

This option is okay. We'll eventually need some cleanup of the options. Not sure if a slider is easy to manage it here, maybe just a dialog prompting for text when you tap on it is better ?

neopilou and others added 5 commits June 23, 2020 07:53
- Added putInt() an getInt() methods in localStorage & localStorageImpl

- Deleted PREF_UNLOCK constant ED in IntegrationRepositoryImpl.kt

- Moved "com.eightbitlab:blurview:1.6.3" in Config.kt

- The "promptForUnlock()" method is no longer duplicated and is now a "Authenticator" class with "authenticate()" methods

- EditTextPreference is used rather than SeekBarPreference in app settings
Added modified  & forgotten file LocalStorageImpl.kt
@neopilou neopilou requested a review from JBassett June 23, 2020 09:33
@neopilou
Copy link
Contributor Author

EditTextPreference is used rather than SeekBarPreference in app settings :

Screenshot_20200623-125021_Home Assistant

@balloob
Copy link
Member

balloob commented Jun 23, 2020

Can we update the strings for the dialog.

Header update: "Home Assistant is locked"
Description: drop this

Deleted promptDialog.setMessage

Modified promptDialog.setTitle :
   - "Home Assistant is locked" when open  or resume app
   - "Confirm to continue" when activate "Lock app" in app settings
@neopilou
Copy link
Contributor Author

Modified promptDialog.setTitle :

  • "Home Assistant is locked" when open or resume app :
    Screenshot_20200624-094548_Home Assistant

  • "Confirm to continue" when activate "Lock app" in app settings :
    Screenshot_20200624-094301_Home Assistant

Comment on lines 11 to 13
val CANCELED = 2
val SUCCESS = 1
val ERROR = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
val CANCELED = 2
val SUCCESS = 1
val ERROR = 0
companion object {
const val CANCELED = 2
const val SUCCESS = 1
const val ERROR = 0
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change made

@@ -59,6 +59,7 @@ class SettingsPresenterImpl @Inject constructor(
"connection_internal" -> (urlUseCase.getUrl(true) ?: "").toString()
"connection_external" -> (urlUseCase.getUrl(false) ?: "").toString()
"registration_name" -> integrationUseCase.getRegistration().deviceName
"session_timeout" -> integrationUseCase.getSessionTimeOut().toString()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's still necessary if the user is allowed to choose time in second to session timedout

Copy link
Member

Choose a reason for hiding this comment

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

What exactly are we allowing to configure, how long the app can be unused until we ask again for biometrics ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's exactly it

Copy link
Member

Choose a reason for hiding this comment

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

I'm personally a big believer in sane defaults that one cannot override. Having a 1000 options is going to be a pain to maintain. How will we show all the app options to the user while not hiding options?

@@ -69,6 +70,7 @@ class SettingsPresenterImpl @Inject constructor(
when (key) {
"connection_internal" -> urlUseCase.saveUrl(value ?: "", true)
"connection_external" -> urlUseCase.saveUrl(value ?: "", false)
"session_timeout" -> integrationUseCase.sessionTimeOut(value.toString().toInt())
Copy link
Collaborator

Choose a reason for hiding this comment

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

This still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's still necessary if the user is allowed to choose time in second to session timedout

@neopilou neopilou requested review from JBassett and balloob July 3, 2020 13:24
@JBassett JBassett merged commit 7565560 into home-assistant:master Jul 5, 2020
@JBassett JBassett added enhancement New feature or request minor A minor change - Bumps minor build number labels Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed enhancement New feature or request minor A minor change - Bumps minor build number
Projects
None yet
4 participants