Add notify command for app-lock

[RoboMagus]

Summary

This PR adds notify commands that enable HA to control the companion app lock settings. Related to issues #840, #2797, and an extension of my previous PR #2800.

Usecases include:

• Enfocing security setting from HA for e.g. the companion app on devices of your children, who keep disabling the app lock for their convenience.
• Disabling the app lock on conditions specified in HA automations. E.g. when at home, but not connected to the home WiFi. (My main usecase due to WiFi issues on one specific device)

Example usage (As added to the docs):

automation:
  - alias: Reset App lock to defaults
    trigger:
      ...
    action:
      - service: notify.mobile_app_<your_device_id_here>
        data:
          message: "command_app_lock"
          data:
            app_lock_enabled: true
            app_lock_timeout: 60
            home_bypass_enabled: false

Screenshots

N/a

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#809

Any other notes

To make this added feature more usefull by adding some feedback I'm also working on an app lock sensor with related settings in it's attributes. That's W.I.P. as it proved to be more challenging than just adding the notify command.

[dshokouhi]

As we are adding new command and parameters to the notification we also need a PR created at the FCM side to add the command to the rate limit skip list and also to add the new parameters:

https://github.com/home-assistant/mobile-apps-fcm-push/blob/master/functions/android.js

[RoboMagus]

Thanks for the quick response, I'll add a PR for that as well.

[jpelgrom]

Enabling the lock functionality using app_lock_enabled: true when there is no device credential set will leave the app in an unusable state: it is blurred like when the lock is normally enabled, but with no way to continue or cancel.

When receiving the device command, you should check to make sure a credential is available before enabling it and/or this scenario should be handled when opening the WebViewActivity without compromising security.

[RoboMagus]

Thats a good point @jpelgrom, I had not considered that. I've added a check to see if the app lock has been enabled at least once before. That should ensure the biometrics have been setup and thus prevent the app to become unusable.

Could you check if you agree with these changes? If not, please feel free to provide the changes you'd like to see. I'm not at all familiar with programming for Android so any insights are appreciated :)

[jpelgrom]

I've added a check to see if the app lock has been enabled at least once before. That should ensure the biometrics have been setup and thus prevent the app to become unusable.

Could you check if you agree with these changes?

I think it's easier to use the same check the settings screen uses when you try to enable the option, instead of keeping track of whether it was enabled at some point in the past which may no longer be relevant: https://github.com/home-assistant/android/blob/8713bb9678a967d69932dd0b85e34428ce1385fa/app/src/main/java/io/homeassistant/companion/android/settings/SettingsFragment.kt#L105

(it does look like the function has been deprecated, but this is wat the settings use right now so I'd say it's no problem to use it in this case)

[RoboMagus]

Wouldn't calling BiometricManager.from(requireActivity()).canAuthenticate() fail when the app isn't open, or when the phone is locked?

I figured requiring an activity would imply the requirement of having the app open for it to succeed.

[dshokouhi]

Wouldn't calling BiometricManager.from(requireActivity()).canAuthenticate() fail when the app isn't open, or when the phone is locked?

I figured requiring an activity would imply the requirement of having the app open for it to succeed.

Looks like we only need context here 😃

https://developer.android.com/reference/androidx/biometric/BiometricManager#from(android.content.Context)

[RoboMagus]

It indeed seems to work well, though I can't check this on a device where biometrics are not enabled.

[dshokouhi]

So although the app does not crash I wonder if we should add more sanity checks so we don't have to hit an error if a user entered an invalid value for boolean or integer. Kotlin does offer toIntOrNull() and toBooleanOrNullI() checks so we can avoid some of the errors if a user entered an invalid value.

[jpelgrom]

Is there a specific reason you're using runBlocking { ... } for each preference instead of using them like normal suspending functions (either by making the entire setAppLock function suspending, or using mainScope.launch { ... } once inside the function)?

So although the app does not crash I wonder if we should add more sanity checks so we don't have to hit an error if a user entered an invalid value for boolean or integer.

Also toBooleanStrict() isn't used for other commands, so I propose at least dropping the strict part of it to make sure this command accepts the same inputs for booleans as other commands. Right now it is a lot easier to encounter an 'invalid' boolean because it only accepts "true" or "false", no uppercase.

[RoboMagus]

Thanks for the feedback!

I've made changes that check the provided setting values. I did stick to strict boolean checking, since a non-strict boolean check defaults to false. In case of setting a security setting I feel that defaulting to disabling it would not be the best choice.

@jpelgrom, the main reason for the separate runblocking blocks would be lack of experience in this language :wink:. The runBlocking blocks have now been cleaned up.

Since everything that could previously throw is now properly checked, the try {} catch block has also been removed, but I'd like a second opinion on that.

[dshokouhi]

I did stick to strict boolean checking, since a non-strict boolean check defaults to false. In case of setting a security setting I feel that defaulting to disabling it would not be the best choice.

If you use toBooleanOrNull() you can control the default null value to be true instead. I do agree with @jpelgrom that we need to have consistency between the boolean values on each command.

[JBassett]

I agree we should be consistent with the rest of the commands and use toBooleanOrNull()

[RoboMagus]

To get this merged, I'm happy to make the change for consistency. But even when the default is explicitly set to true in case of fallback, I do not think any defaults should apply when a user calls:

service: notify.mobile_app_<your_device_id_here>
data:
  message: "command_app_lock"
  data:
    app_lock_enabled: asdfa43

Instead this should fail, such that the user is at least made aware of the potential issue they've created in their script / automation.

As it is right now, the boolean conversion is case insensitive and should provide users the flexibility to interpret the boolean options without being too pedantic.

[jpelgrom]

To get this merged, I'm happy to make the change for consistency. But even when the default is explicitly set to true in case of fallback, I do not think any defaults should apply when a user calls: (...)

Instead this should fail, such that the user is at least made aware of the potential issue they've created in their script / automation.

So instead you could choose to only apply it if there is a valid result, for example:

data[APP_LOCK_ENABLED]?.toLowerCase()?.toBooleanStrictOrNull()?.let { enabled ->
    // Check if biometric is available and update preference
}

Right now it will also send a notification if nothing is modified, is this desirable? The data might be correct.

[RoboMagus]

I hadn't seen your comment before my last commit in which I've added a check for data validity and fall back to sending a notification if a parameter was provided, but with an invalid value. I'm not sure if I fully understand your comment, but it looks like the intentions are the same.

Right now it will also send a notification if nothing is modified, is this desirable? The data might be correct.

Sending a command to change the settings but providing no settings to change should be considered invalid in my opinion. And in that case similar to providing a parameter, but with an invalid value.

[dshokouhi]

Sending a command to change the settings but providing no settings to change should be considered invalid in my opinion. And in that case similar to providing a parameter, but with an invalid value.

I agree we should post a notification if the app does not get the minimum amount of information to do something. So the current logic for detecting when the command did not receive enough data is to handle it earlier in the code like the other commands:

https://github.com/home-assistant/android/blob/12997df55673f6bef9016b6c7822a0d79dfaee3d/app/src/main/java/io/homeassistant/companion/android/notifications/MessagingManager.kt#L464-L466

You should be able to do some logic similar to COMMAND_VOLUME_LEVEL:

https://github.com/home-assistant/android/blob/12997df55673f6bef9016b6c7822a0d79dfaee3d/app/src/main/java/io/homeassistant/companion/android/notifications/MessagingManager.kt#L367-L381

[RoboMagus]

Appologies for the delay, I have not been able to work on any side projects last week.

I've pulled argument validity checking up in the process chain. What I don't like about this approach though is the added code duplication, as now both parsing of the argument and checking to call the fallback notification now has to be performed twice.

[dshokouhi]

What I don't like about this approach though is the added code duplication, as now both parsing of the argument and checking to call the fallback notification now has to be performed twice.

I agree but its good to follow the flow of the other commands, this one is a special case as none of the commands are actually required. Its only a requirement that one of the 3 parameters are supplied.