minor: refactor session create methods #7827
Conversation
| Ok(_) => Ok(true), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
looking up the user by ID when either have the DB model on hand or we don't at the call site makes no sense unless we're trying to rule out the user being deleted in the 1ms between the retrieval and now
There was a problem hiding this comment.
I traced this waaaay back, and when this was added, there was only spoof login and the user_id was hard-coded. I am confident it is no longer applicable. The methods used to authenticate users for local and SAML login both do the user lookup themselves.
omicron/nexus/src/external_api/console_api.rs
Lines 48 to 70 in 9d54805
| db::model::UserProvisionType::ApiOnly => { | ||
| return Err(Error::Unauthenticated { | ||
| internal_message: "User must exist before login when user provision type is ApiOnly".to_string(), | ||
| }); |
There was a problem hiding this comment.
This diff is annoying due to indentation changes, but the only actual change here was changing return Ok(None); to this. The end result is the same as before because this None was being converted to the error in the now-deleted create_session method. Now the logic is a lot clearer.
| Ok(None) | ||
| Err(Error::Unauthenticated { | ||
| internal_message: "Failed password verification".to_string(), | ||
| }) |
There was a problem hiding this comment.
This was the other helper function. Instead of returning Ok(None) on auth failure and converting that to the error in the calling code, we return the appropriate error directly.
There should be no functional changes here, though the internal error messages are now slightly different between saml login and local login, where before they were the same. Ran into this while working #7339. This logic (which I wrote originally and shuffled around in #7374) never really made sense, and I figured it's good prep for #7818 and friends to clean it up.
The core of the change here is updating two existing functions that returned
Result<Option<User>, Error>to just returnResult<User, Error>and move the logic about what we do when the user wasNoneinside each function. In both cases, when the user wasNonewe ended up with anError::Unauthenticatedanyway, so we can just do that a moment earlier and eliminate a lot of misdirection.