swaywm / swaylock

Screen locker for Wayland
MIT License
853 stars 201 forks source link

All `finished` events are resulting in an `exit(2)`, when the protocol states otherwise #346

Open mattkae opened 9 months ago

mattkae commented 9 months ago

I noticed that whenever a finished event is sent to ext_session_lock_v1, swaylock exits with 2. According to the spec (source: https://wayland.app/protocols/ext-session-lock-v1#ext_session_lock_v1:event:finished):

If the locked event is sent on creation of this object the finished event may still be sent at some later time in this object's lifetime. This is compositor policy.

Upon receiving this event, the client should make either the destroy request or the unlock_and_destroy request, depending on whether or not the locked event was received on this object.

In this case, I would expect swaylock to send an unlock_and_destroy request and exit gracefully.

Use case: This could happen because the compositor's policy accepts unlocking via logind (or some other 3rd party mechanism) as a valid unlock. In this case, we want the compositor to inform the client that they can exit without any issues.

Let me know if I'm misunderstanding something here. Cheers!

emersion commented 9 months ago

I'm not sure this is a good thing to do. I'm worried about unlocking in other situations that when the user has correctly typed their password. If the compositor decides to unlock, they shouldn't care that the client sends the unlock_and_destroy request.

kennylevinsen commented 9 months ago

I suppose this issue is primarily about the exit code. To sort that part out, we could exit(0) if we have previously received confirmation of lock, and exit(2) otherwise, so scripts that called swaylock can see that it was not a failure.

(Sending destroy before existing is redundant, the compositor will clean up.)

mattkae commented 9 months ago

An exit(0) would solve my particular problem. However, I see why an exit(2) was being used, because this finished event is sent both when the compositor is done with the lock and when the compositor denies the lock altogether. I would expect an exit(0) in the former and an exit(2) in the latter, as the latter means the lockscreen didn't work at all.

emersion commented 9 months ago

I'm a little uneasy exiting with a zero code when the user hasn't explicitly unlocked the screen by typing the correct password.

mattkae commented 9 months ago

Yeah I understand that. The fact that the finished event is coupled with a theoretical denied event isn't helping matters either. The spec says "This might also be sent because the compositor implements some alternative, secure way to authenticate and unlock the session" but how could we know from the finished event whether or not that was sent in response to a failure.

If that's the case, I'd be okay leaving it for the time being. We might eventually require an updated spec if we're serious about the idea that compositor may implement another avenue for authentication (e.g. logind) :wink:

mattkae commented 9 months ago

Although... Perhaps I am wrong! There are two scenarios I see:

  1. The finished event is received before the locked event, meaning that the lockscreen never started.
  2. The finished event is received after the locked event, meaning that the compositor decided that the lockscreen is no longer needed

The first case would be a valid exit(2), while the second would be a valid exit(0)