automatically purge stale invitations

[stefan0xC]

Fixes part of #2792 by automatically deleting invited users that have not registered before the expiration date.

INVITATION_EXPIRATION_HOURS defaults to 120 (5 days) because that was the default expiration date of the Invite JWT. When set to 0 the expiration date is not checked.

Also no invitation is deleted when mail is disabled.

[stefan0xC]

Note: Emergency access invites might not work if the expiration is set to below 5 days and signup is disabled but I am not sure how we should handle that case. Maybe more descriptive error messages?

[BlackDex]

I'm not sure if i would like this to be enabled. It would prevent me from seeing if i invited that user before or not so that (audit) trail is gone.

Also, we try to keep as close as possible to Bitwarden with most of the way-of-working when we can, and Bitwarden has a 5 day's invite expire for all items ( https://bitwarden.com/help/getting-started-organizations/#invite ).

[stefan0xC]

I'm not sure if i would like this to be enabled. It would prevent me from seeing if i invited that user before or not so that (audit) trail is gone.

Also, we try to keep as close as possible to Bitwarden with most of the way-of-working when we can, and Bitwarden has a 5 day's invite expire for all items ( https://bitwarden.com/help/getting-started-organizations/#invite ).

I'll have to think some more about this because I think there is a flaw with the way vaultwarden handles invitations and automatically removing the record might not be the only way to go about it.

Oh, and at least the self-hosted Bitwarden has an option to change the expiration date of invites (see cksapp's answer on Discourse). But I will probably split this into a separate PR because making vaultwarden more configurable should not be dependent on the automatic purge.

[BlackDex]

I'm not sure if i would like this to be enabled. It would prevent me from seeing if i invited that user before or not so that (audit) trail is gone.
Also, we try to keep as close as possible to Bitwarden with most of the way-of-working when we can, and Bitwarden has a 5 day's invite expire for all items ( https://bitwarden.com/help/getting-started-organizations/#invite ).

I'll have to think some more about this because I think there is a flaw with the way vaultwarden handles invitations and automatically removing the record might not be the only way to go about it.

Oh no, flaws are bad. But the whole invite accept flow is a bit complex with all the different ways to invite people and having mail enabled or not. So carefully check, and you can always poke me on Matrix with some questions :).

Oh, and at least the self-hosted Bitwarden has an option to change the expiration date of invites (see cksapp's answer on Discourse). But I will probably split this into a separate PR because making vaultwarden more configurable should not be dependent on the automatic purge.

AH! That I totally missed! In that case, the variable invite hours is probably something we want to add. But make sure it does this for all invites then, unless that is also different per invite type, that I haven't checked (yet).

[stefan0xC]

In my view the main flaw is that you cannot register an account in vaultwarden if you have already been invited via mail without the link in the invitation. Vaultwarden will tell you "Account with this email already exists".

Joining organizations I understand but why can't I register my mail address without the token?
It's not to make sure that my account is the one registered because I'll need to accept the invitation afterwards by clicking the link again and also the owner/admin of an organization has to confirm my after the acceptance anyway before a user gets access so I am not sure why this step should be neccessary.

I've now registered and tested with the official bitwarden server and the behavior in this regard is different. I can always register a new account even if it has been invited and I don't register with the link (I can also register a different mail address once I click on the link though I can't accept the invitation with a differently registered mailaddress because the token will be invalid).

Update: Sorry, the error message was when signups were disabled, it's the wrong account already exists message, of course.

[stefan0xC]

I will close this PR for now as you are right that it does not address my main concern. Keeping the behavior the same as Bitwarden is more important and I am not sure I have checked for other side effects.

[BlackDex]

In my view the main flaw is that you cannot register an account in vaultwarden if you have already been invited via mail without the link in the invitation. Vaultwarden will tell you "Registration not allowed or user already exists".

This is a complex part of the code. The main reason here is that when SIGNUPS_ALLOWED is set to false, an admin is still able to invite users to the server (and not a specific org) by using this flow.
It could also be seen as an extra security check, but that might be a bit to much when SIGNUPS_ALLOWED is set to true.

We might need to draw a nice picture of a signup flow with the cases mentioned there, and see if we need to make some changes.
It could very well be that there is some redundant check right now which triggers some corner cases.

The only thing i can think of right now, is that you want to prevent someone else from signing up without the correct token if you have all locked down, but that would implicate the SIGNUPS_ALLOWED to be set to false.

Joining organizations I understand but why can't I register my mail address without the token? It's not to make sure that my account is the one registered because I'll need to accept the invitation afterwards by clicking the link again and also the owner/admin of an organization has to confirm my after the acceptance anyway before a user gets access so I am not sure why this step should be neccessary.

I've now registered and tested with the official bitwarden server and the behavior in this regard is different. I can always register a new account even if it has been invited and I don't register with the link (I can also register a different mail address once I click on the link though I can't accept the invitation with a differently registered mailaddress because the token will be invalid).

You have to take into account some of the lock-down settings. But, this is a complex part.

[BlackDex]

I Think this is should be the main overview.
There might be some cases missing here, but this is at least a start.
No checks are done if SMTP is enabled or not from what i see. Maybe we need to add that.

But looking at the flow-chart above, the following line should be returning a user object i think, instead of an error:

vaultwarden/src/api/core/accounts.rs

Line 127 in 6fa6eb1

err!("Account with this email already exists")

[tessus]

Also, we try to keep as close as possible to Bitwarden with most of the way-of-working when we can, and Bitwarden has a 5 day's invite expire for all items

I understand, but using a config option to specify for how long an invitation is valid wouldn't hurt either. But that's just me. I love options, the more the better. But I also understand that others rather have less options.

[BlackDex]

Also, we try to keep as close as possible to Bitwarden with most of the way-of-working when we can, and Bitwarden has a 5 day's invite expire for all items

I understand, but using a config option to specify for how long an invitation is valid wouldn't hurt either. But that's just me. I love options, the more the better. But I also understand that others rather have less options.

If you would have read my later comment, i mentioned that i didn't knew about this option on the Self-hosted Bitwarden also. And that we probably want to at least add that option also.

Having a lot of options sounds nice for users, but could become a pain to maintain, and issues tend to sneak in. Especially if they have connections to other settings or a different way of working upstream.

That said, big useful options are always considered of course.

[tessus]

Yes, I must have missed your other comment. And I agree, the maintenance burden can become overwhelming. However, this specific option does not have side effects and/or dependencies to other workflows or parts of the code. It is pretty much isolated. By isolated I mean, I only have to change this line in the code: https://github.com/dani-garcia/vaultwarden/blob/main/src/auth.rs#L153 (and add parding the option from the config file) and not change many parts of the code with a bunch of if/else clauses....

[stefan0xC]

I'm not sure if it's just that. You have to think about the implications and side effects as well. For example what should happen if I set the expiration hours to 0. Should the tokens be simply invalid (as by your proposed change) or would I need to handle that by the disabling the validation of the expiration check in the decode_invite function. And then you have to think if you want emergency invites and other to behave the same or if they should be configured separately as well and then you might want to write a decode_jwt_no_exp that does not validate expirations instead of customizing each function individually, etc.

But I would rather we continue this discussion in a separate issue or you wait for my new pull request which only has the configuration option and not the scheduled cleanup.

[BlackDex]

@tessus that isn't the only part that uses the 5 days, the same goes for the emergency invite.

@stefan0xC, setting it to 0 would be a no-go, so it at least needs to be 1 or higher.
If the token is expired the decoding will fail and that should be enough.

I might even think that the whole invitations table might be obsolete, since we could just check if the user has a password set or not. Shouldn't matter if SMTP is enabled or not at all in my opinion.

But, as said before. This part is very complex. And a better flow-chart then above would help, including flow-charts regarding inviting or adding users.

[tessus]

that isn't the only part that uses the 5 days, the same goes for the emergency invite.

This is ok. All invites would then be X days.

what should happen if I set the expiration hours to 0

I did not mention this at all, because that would be an edge case that would have to be handled differently in the code. For "never" you can just use 10,000 days. ;-)
But I would probaby not set that anyway.

I only mentioned that I would not have expected it to expire when signup is disabled. But since this is not the case, I did not even think about deactivating expiration. Especially since you can workaround it as I mentioned before.

As a general note, I am just throwing out a few ideas. I am not expecting anything to be changed. Except the error message should be changed to something more useful. ;-)
The other things are nice to have, but as you guys have mentioned will have to weighed against the maintenance costs.