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

Don't no-op logouts #439

Closed
wants to merge 14 commits into from
Closed

Conversation

AndrewFerr
Copy link
Contributor

Disconnect bridge users from their accounts on logout. This does not
actually delete the Signal device being logged out of, though.

commands.go Outdated
Name: "logout",
Help: commands.HelpMeta{
Section: commands.HelpSectionAuth,
Description: "Unlink the bridge from your Signal account.",
Copy link
Member

Choose a reason for hiding this comment

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

There shouldn't be a logout command before logging out is actually supported, but delete-session should probably actually delete the session, not sure why it wasn't made to do that in the first place

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps the name delete-session isn't aligned with other mautrix bridges, but the intent behind that command is to delete the Signal encryption session (by wiping out all the keys) but leave the rest of the state intact to allow relinking a new Signal device to the bridge and get all new encryption sessions without losing group keys, profile data, etc.

Resetting encryption sessions is often necessary with Signal due to bugs in the Signal mobile apps.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, keeping those things sounds unnecessary since they're resynced from the phone anyway

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe, but the phone sync is best effort. I figured why not have a command that retains this data if the user's intent is to immediately reconnect the same account. But if you don't want this command to exist I don't feel strongly about it, a user can achieve the same behaviour by deleting the device in the Signal app (the bridge will wipe encryption keys and keep everything else)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So is the main purpose of the current delete-session to fix occasional issues with encryption keys, and to typically be followed up immediately with login?

In any case, would it be appropriate for (*User).Logout to also delete the encryption session like delete-session does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expressed differently: when would a user want to log out of the bridge without deleting their Signal encryption session? Intuitively, I'd expect a bridge logout to also delete the session (but now I understand why delete-session should not also trigger a logout, for the reasons you explained).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup, you're correct about delete-session. The main reason I didn't make logout actually logout is because I didn't need it to at the time. I was implementing the provisioning API, trying to make it behave identical to the old Signal bridge (to avoid making changes to the Signal provisioning flow in Beeper), and during the provisioning flow Beeper called logout a bunch of times before logging in, and I didn't want to break the reconnection logic, so I just made it no-op.

The ideal would be to change the logout endpoint in the provisioning API to be a more obvious no-op (and eventually fix Beeper to be more sane during Signal provisioning so we don't need the no-op at all), then properly implement the logout command (delete keys and session in bridge, and delete device from Signal). But I don't currently have bandwidth to be a lot of help with this at the moment ;)

Disconnect bridge users from their accounts on logout. This does not
actually delete the Signal device being logged out of, though.
Pass it a context, and call it in fnDeleteSession
Treat a failure to delete the device from the store as fatal, because
otherwise credentials would remain in the store & might be reused
accidentally on the next read from the store (like on bridge restart).
as the condition for this response has false positives
Make logouts delete the device from the database (not just clear
sessions and keys) and update the users store & user-to-uuid map.
@AndrewFerr
Copy link
Contributor Author

The latest batch of commits tries to delete devices on "hard" logout situations (like logout, or the bridge being unlinked remotely), and to clear only session keys & connections on "soft" logouts (like delete-session). I hope this makes logouts work the way they ought to.

Logging out from the bridge still doesn't unlink the bridge from the primary Signal device, though -- I did some testing and it seems like it's still the case that only the primary device can unlink other devices, and a non-primary device can't unlink itself.

This still has the provisioning API's logout command not as a no-op, but I've given it a delete-session equivalent. If its logout command should still be a no-op, I can move the non-no-op (op?) version to a new endpoint.

@smweber
Copy link
Collaborator

smweber commented Feb 14, 2024

I don't know about the bridge being unlinked in the Signal app being a "hard-logout" situation. If a user accidentally unlinks (perhaps not knowing what they're doing) it's nice for them to be able to relink with few consequences. I've also told users to unlink in the app then relink their bridge to fix situations where the Signal apps encryption gets jammed up (happens shockingly often).

@smweber
Copy link
Collaborator

smweber commented Feb 14, 2024

This still has the provisioning API's logout command not as a no-op, but I've given it a delete-session equivalent. If its logout command should still be a no-op, I can move the non-no-op (op?) version to a new endpoint.

I think this should be fine :)

@tulir
Copy link
Member

tulir commented Feb 14, 2024

Ah right, Signal doesn't allow real logouts, then it should be fine to have logout just delete the session. Should maybe also rename delete-session to something more accurate because it's deleting less than logout

@AndrewFerr
Copy link
Contributor Author

I don't know about the bridge being unlinked in the Signal app being a "hard-logout" situation. If a user accidentally unlinks (perhaps not knowing what they're doing) it's nice for them to be able to relink with few consequences.

The reason I went for a "hard-logout" on remote unlinking is that changing it to do a "soft-logout" makes future login attempts kick me out right away. i.e. this happens:

  • run login, which succeeds
  • unlink the bridge device from Signal
  • bridge says it got logged out
  • run login again, which succeeds
  • immediately get 401 responses in logs & bridge says it got logged out

@smweber
Copy link
Collaborator

smweber commented Feb 14, 2024

The reason I went for a "hard-logout" on remote unlinking is that changing it to do a "soft-logout" makes future login attempts kick me out right away. i.e. this happens:

* run `login`, which succeeds
* unlink the bridge device from Signal
* bridge says it got logged out
* run `login` again, which succeeds
* immediately get 401 responses in logs & bridge says it got logged out

That's really strange, perhaps there's some difference in how login does provisioning vs the provisioning API? I definitely delete the bridge device in Signal then log back in all the time, it's how I was testing the new prekey stuff I was working on a few weeks ago - but I use the provisioning API S:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants