fix email as 2fa for sso

[stefan0xC]

only validate master password hash if an email is provided and improve the find_by_device lookup by sorting by the most recently used device (i.e. the one last updated, which we make sure by saving the device on each login attempt).

(I think I got rid of the issue by updating the device on each login, if not I can restore the alternative version again.)

[Timshel]

I think using the twofactor_incomplete table is a better idea.
Something like:

    pub async fn find_by_incomplete2fa(device_uuid: &DeviceId, conn: &DbConn) -> Option<Self> {
        db_run! { conn: {
            users::table
                .inner_join(twofactor_incomplete::table.on(twofactor_incomplete::user_uuid.eq(users::uuid)))
                .filter(twofactor_incomplete::device_uuid.eq(device_uuid))
                .order_by(twofactor_incomplete::login_time.desc())
                .select(users::all_columns)
                .first::<Self>(conn)
                .ok()
        }}
    }

I wanted to additionally filter with incomplete_2fa_time_limit but:

vaultwarden/src/db/models/two_factor_incomplete.rs

Line 44 in c9d527d

// Don't update the data for an existing user/device pair, since that

Might prevent a user from login in after an incomplete 2FA.

[stefan0xC]

I think you're right that this would be better to narrow down the correct user.

[Timshel]

Client appears to send empty string :(:

{
    "email": "",
    "masterPasswordHash": "",
    "ssoEmail2FaSessionToken": "",
    "deviceIdentifier": "5564adca-55dd-47dc-b8ea-f46d9eed225e",
    "authRequestAccessCode": "",
    "authRequestId": ""
}

Cleanest is probably to use something like:

use serde_with::{serde_as, NoneAsEmptyString};
...
#[serde_as]
#[derive(Deserialize)]
#[serde(rename_all = "camelCase")]
struct SendEmailLoginData {
    ...
    
    #[serde(alias = "Email")]
    #[serde_as(as = "NoneAsEmptyString")]
    email: Option<String>,

I believe serde_with is already a transitive dependency.

[Timshel]

Hey,
Not sure it's clear why one should update or not.
I modified the is_new to use the create and update time because for the SSO 2FA email recovery the device had to be created with the first call to token (it used to be created only after the 2FA).

I though it was brittle but at the time I didn't want to add more complexity to the SSO PR.
Might be worth it to just use an additional field now, something like last login could work and be useful ?

[BlackDex]

Maybe adding a nice comment there with /// will help at least in Graphical editor's i think.
That will be shown upon hovering over the function call.

Other than that, i think this looks ok too me.
So, either way, i think it is fine :)