nextcloud / talk-android

📱😀 Video & audio calls through Nextcloud on Android
Other
548 stars 246 forks source link

Screen lock does not behave predictably #3815

Open ASerbinski opened 6 months ago

ASerbinski commented 6 months ago

Steps to reproduce

Enable Settings --> Screen lock, and set an inactivity timeout of 30 seconds.

When going in and out of the application, it does not consistently prompt for authentication, even if the timeout period has definitely elapsed.

The issue appears to be tied to SecurityUtils.checkIfWeAreAuthenticated(timeout)

The comment in the function says

            // Try encrypting something, it will only work if the user authenticated within
            // the last AUTHENTICATION_DURATION_SECONDS seconds.

I do not believe that this works as expected.

Expected behaviour

The user should be consistently prompted for authentication if the timeout has expired.

Actual behaviour

The user may or may not be prompted for authentication even though the timeout has expired.

Device brand and model

Any

Android version

14

Nextcloud Talk app version

master

Nextcloud server version

No response

Talk version

No response

Custom Signaling server configured

None

Custom TURN server configured

None

Custom STUN server configured

None

Android logs

No response

Server log

No response

Additional information

No response

mahibi commented 6 months ago

I believe the implementation of this feature can be improved, but for me this is working as expected. The unlock can also be done on system level, so whenever you unlock your phone, this also counts for the talk app. This might explain why you expect the lock screen to be shown when opening the talk app, but it's not shown because you recently unlocked your phone?

ASerbinski commented 6 months ago

No, the screen lock does not engage in the interim, screen remains on.

But since you mention it, having the application unlocked by the screen lock sounds like a terrible idea that would defeat the whole purpose of having the application lock separately. Consider this scenario; you're driving somewhere and have kids in the back seat and you're letting them use the phone for playing a game or watching a video. You unlock the phone and hand it to them --> now they have access since the application was automatically unlocked by the screen lock.

I can't think of any scenario where it would make sense to have the application unlocked by the screen lock. The only purpose of the application having a lock would be to hand the phone to someone you don't want to have access to that application, but having to unlock the screen first would unlock it anyway, thus it isn't protected.

mahibi commented 6 months ago

I agree that in the scenario with "handing the phone to kids" it would make sense to have to app locked while the phone is unlocked.

On the other hand people may expect there is no app lockscreen when the phone was just unlocked. If there is always a lockscreen which they don't expect they might get annoyed of the feature and disable it completely.

So #3816 would make sense so people can decide which behavior they want.

ASerbinski commented 6 months ago

On the other hand people may expect there is no app lockscreen when the phone was just unlocked. If there is always a lockscreen which they don't expect they might get annoyed of the feature and disable it completely.

I'm not understanding why anyone would expect that behavior, its different than every other lockable application that I've ever come across.

In fact, my experience has been the complete opposite, that in fact the inconsistency of being prompted to unlock causes people to believe that the feature is unreliable and broken (I have several users, one of whom is a victim of domestic abuse and needs communications to be secured, who have expressed this issue to me). Why use something at all if you can't trust that it will work when you need it to?

Now here is the problem as I see it; it you're passing off application unlocking to the lockscreen, wouldn't it be just as effective to just use the lockscreen? Just tap the power button and its locked. The purpose of locking the application is to guarantee that the application won't open even though the phone is unlocked.

mahibi commented 2 weeks ago

The purpose of locking the application is to guarantee that the application won't open even though the phone is unlocked.

I now agree this should be the expected behavior. So the implementation has to be changed/rewritten. I suggest we implement the same as for the nextcloud files app in https://github.com/nextcloud/android/blob/0f17882e3c240db64426fc126d3493e5345a0a2c/app/src/main/java/com/owncloud/android/authentication/PassCodeManager.kt

Best will be to check if the implementation/usage of PassCodeManager.kt can be used for both apps and if yes, then extract it to https://github.com/nextcloud/android-common so it will be available for nextcloud android files and nextcloud android talk. Any opinions on this @ASerbinski @parneet-guraya @alperozturk96 @sowjanyakch @tobiasKaminsky @AndyScherzinger @nickvergessen ?

alperozturk96 commented 2 weeks ago

The PassCodeManager is tightly coupled to the following imports:

import com.nextcloud.client.core.Clock
import com.nextcloud.client.preferences.AppPreferences
import com.owncloud.android.MainApp
import com.owncloud.android.ui.activity.PassCodeActivity
import com.owncloud.android.ui.activity.RequestCredentialsActivity
import com.owncloud.android.ui.activity.SettingsActivity
import com.owncloud.android.utils.DeviceCredentialUtils

To improve modularity, we can inject correct values and extract the PassCodeManager and PassCodeActivity (along with its XML UI) into the android-common library.

First, we should implement this and test in the Files app. After that, we can move on to the Talk app and repeat the process.

Note: The modular implementation should not depend on the Files or Talk app. It must be reusable for any Android app. We will simply inject the necessary values from the Files and Talk apps.