Skip to content

minor: refactor session create methods #7827

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.

Sign up for GitHub

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

Merged
david-crespo merged 1 commit into from
Mar 21, 2025
Merged

minor: refactor session create methods #7827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 1 addition & 20 deletions nexus/src/app/login.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ use dropshot::{HttpError, HttpResponseFound, http_response_found};
use nexus_auth::context::OpContext;
use nexus_db_model::{ConsoleSession, Name};
use nexus_db_queries::authn::silos::IdentityProviderType;
use nexus_db_queries::db::identity::Asset;
use nexus_types::external_api::{params::RelativeUri, shared::RelayState};
use omicron_common::api::external::Error;

impl super::Nexus {
pub(crate) async fn login_saml_redirect(
Expand Down Expand Up @@ -77,28 +75,11 @@ impl super::Nexus {
&authenticated_subject,
)
.await?;
let session = self.create_session(opctx, user).await?;
let session = self.session_create(opctx, &user).await?;
let next_url = relay_state
.and_then(|r| r.redirect_uri)
.map(|u| u.to_string())
.unwrap_or_else(|| "/".to_string());
Ok((session, next_url))
}

// TODO: move this logic, it's weird
pub(crate) async fn create_session(
&self,
opctx: &OpContext,
user: Option<nexus_db_queries::db::model::SiloUser>,
) -> Result<ConsoleSession, Error> {
let session = match user {
Some(user) => self.session_create(&opctx, user.id()).await?,
None => Err(Error::Unauthenticated {
internal_message: String::from(
"no matching user found or credentials were not valid",
),
})?,
};
Ok(session)
}
}
32 changes: 3 additions & 29 deletions nexus/src/app/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use nexus_db_queries::authn::Reason;
use nexus_db_queries::authz;
use nexus_db_queries::context::OpContext;
use nexus_db_queries::db;
use nexus_db_queries::db::identity::Asset;
use nexus_db_queries::db::lookup::LookupPath;
use omicron_common::api::external::CreateResult;
use omicron_common::api::external::DeleteResult;
Expand All @@ -34,40 +35,13 @@ fn generate_session_token() -> String {
}

impl super::Nexus {
async fn login_allowed(
&self,
opctx: &OpContext,
silo_user_id: Uuid,
) -> Result<bool, Error> {
// Was this silo user deleted?
let fetch_result = LookupPath::new(opctx, &self.db_datastore)
.silo_user_id(silo_user_id)
.fetch()
.await;

match fetch_result {
// if the silo user was deleted, they're not allowed to log in :)
Err(Error::ObjectNotFound { .. }) => Ok(false),
Err(e) => Err(e),
// they're allowed
Ok(_) => Ok(true),
}
}

Copy link
Contributor Author

@david-crespo david-crespo Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@david-crespo david-crespo Mar 19, 2025

Choose a reason for hiding this comment

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

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

pub async fn spoof_login(
rqctx: Arc<RequestContext<Arc<ServerContext>>>,
params: TypedBody<LoginParams>,
) -> Result<Response<Body>, HttpError> {
let apictx = rqctx.context();
let nexus = &apictx.nexus;
let params = params.into_inner();
let user_id: Option<Uuid> = match params.username.as_str() {
"privileged" => Some(USER_TEST_PRIVILEGED.id),
"unprivileged" => Some(USER_TEST_UNPRIVILEGED.id),
_ => None,
};
if user_id.is_none() {
return Ok(Response::builder()
.status(StatusCode::UNAUTHORIZED)
.header(header::SET_COOKIE, clear_session_cookie_header_value())
.body("".into())?); // TODO: failed login response body?
}
let user_id = user_id.unwrap();
let session = nexus.session_create(user_id).await?;

pub(crate) async fn session_create(
&self,
opctx: &OpContext,
user_id: Uuid,
user: &db::model::SiloUser,
) -> CreateResult<db::model::ConsoleSession> {
if !self.login_allowed(opctx, user_id).await? {
return Err(Error::Unauthenticated {
internal_message: "User not allowed to login".to_string(),
});
}

let session =
db::model::ConsoleSession::new(generate_session_token(), user_id);

db::model::ConsoleSession::new(generate_session_token(), user.id());
self.db_datastore.session_create(opctx, session).await
}

Expand Down
67 changes: 36 additions & 31 deletions nexus/src/app/silo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ impl super::Nexus {
authz_silo: &authz::Silo,
db_silo: &db::model::Silo,
authenticated_subject: &authn::silos::AuthenticatedSubject,
) -> LookupResult<Option<db::model::SiloUser>> {
) -> LookupResult<db::model::SiloUser> {
// XXX create user permission?
opctx.authorize(authz::Action::CreateChild, authz_silo).await?;
opctx.authorize(authz::Action::ListChildren, authz_silo).await?;
Expand All @@ -383,35 +383,38 @@ impl super::Nexus {
)
.await?;

let (authz_silo_user, db_silo_user) =
if let Some(existing_silo_user) = fetch_result {
existing_silo_user
} else {
// In this branch, no user exists for the authenticated subject
// external id. The next action depends on the silo's user
// provision type.
match db_silo.user_provision_type {
// If the user provision type is ApiOnly, do not create a
// new user if one does not exist.
db::model::UserProvisionType::ApiOnly => {
return Ok(None);
}
let (authz_silo_user, db_silo_user) = if let Some(existing_silo_user) =
fetch_result
{
existing_silo_user
} else {
// In this branch, no user exists for the authenticated subject
// external id. The next action depends on the silo's user
// provision type.
match db_silo.user_provision_type {
// If the user provision type is ApiOnly, do not create a
// new user if one does not exist.
db::model::UserProvisionType::ApiOnly => {
return Err(Error::Unauthenticated {
internal_message: "User must exist before login when user provision type is ApiOnly".to_string(),
});
Copy link
Contributor Author

@david-crespo david-crespo Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@david-crespo david-crespo Mar 19, 2025

Choose a reason for hiding this comment

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

I'm pretty confident but I would appreciate a very quick look @jmpesp since you wrote this way back in #1358.

}

// If the user provision type is JIT, then create the user if
// one does not exist
db::model::UserProvisionType::Jit => {
let silo_user = db::model::SiloUser::new(
authz_silo.id(),
Uuid::new_v4(),
authenticated_subject.external_id.clone(),
);
// If the user provision type is JIT, then create the user if
// one does not exist
db::model::UserProvisionType::Jit => {
let silo_user = db::model::SiloUser::new(
authz_silo.id(),
Uuid::new_v4(),
authenticated_subject.external_id.clone(),
);

self.db_datastore
.silo_user_create(&authz_silo, silo_user)
.await?
}
self.db_datastore
.silo_user_create(&authz_silo, silo_user)
.await?
}
};
}
};

// Gather a list of groups that the user is part of based on what the
// IdP sent us. Also, if the silo user provision type is Jit, create
Expand Down Expand Up @@ -460,7 +463,7 @@ impl super::Nexus {
)
.await?;

Ok(Some(db_silo_user))
Ok(db_silo_user)
}

// Silo user passwords
Expand Down Expand Up @@ -587,7 +590,7 @@ impl super::Nexus {
opctx: &OpContext,
silo_lookup: &lookup::Silo<'_>,
credentials: params::UsernamePasswordCredentials,
) -> Result<Option<db::model::SiloUser>, Error> {
) -> Result<db::model::SiloUser, Error> {
let (authz_silo, _) = self.local_idp_fetch_silo(silo_lookup).await?;

// NOTE: It's very important that we not bail out early if we fail to
Expand Down Expand Up @@ -617,9 +620,11 @@ impl super::Nexus {
"passed password verification without a valid user"
);
let db_user = fetch_user.unwrap().1;
Ok(Some(db_user))
Ok(db_user)
} else {
Ok(None)
Err(Error::Unauthenticated {
internal_message: "Failed password verification".to_string(),
})
Copy link
Contributor Author

@david-crespo david-crespo Mar 19, 2025

Choose a reason for hiding this comment

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

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.

}
}

Expand Down
2 changes: 1 addition & 1 deletion nexus/src/external_api/http_entrypoints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7399,7 +7399,7 @@ impl NexusExternalApi for NexusExternalApiImpl {
let user =
nexus.login_local(&opctx, &silo_lookup, credentials).await?;

let session = nexus.create_session(opctx, user).await?;
let session = nexus.session_create(opctx, &user).await?;
let mut response = HttpResponseHeaders::new_unnamed(
HttpResponseUpdatedNoContent(),
);
Expand Down
23 changes: 6 additions & 17 deletions nexus/tests/integration_tests/silos.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,6 @@ async fn test_silo_admin_group(cptestctx: &ControlPlaneTestContext) {
},
)
.await
.unwrap()
.unwrap();

let group_memberships = nexus
Expand Down Expand Up @@ -824,13 +823,12 @@ async fn test_silo_user_provision_types(cptestctx: &ControlPlaneTestContext) {
groups: vec![],
},
)
.await
.unwrap();
.await;

if test_case.expect_user {
assert!(existing_silo_user.is_some());
assert!(existing_silo_user.is_ok());
} else {
assert!(existing_silo_user.is_none());
assert!(existing_silo_user.is_err());
}

NexusRequest::object_delete(&client, &"/v1/system/silos/test-silo")
Expand Down Expand Up @@ -1063,7 +1061,6 @@ async fn test_silo_groups_jit(cptestctx: &ControlPlaneTestContext) {
},
)
.await
.unwrap()
.unwrap();

let group_memberships = nexus
Expand Down Expand Up @@ -1137,7 +1134,6 @@ async fn test_silo_groups_fixed(cptestctx: &ControlPlaneTestContext) {
},
)
.await
.unwrap()
.unwrap();

let group_memberships = nexus
Expand Down Expand Up @@ -1193,7 +1189,6 @@ async fn test_silo_groups_remove_from_one_group(
},
)
.await
.unwrap()
.unwrap();

// Check those groups were created and the user was added
Expand Down Expand Up @@ -1236,7 +1231,6 @@ async fn test_silo_groups_remove_from_one_group(
},
)
.await
.unwrap()
.unwrap();

let group_memberships = nexus
Expand Down Expand Up @@ -1306,7 +1300,6 @@ async fn test_silo_groups_remove_from_both_groups(
},
)
.await
.unwrap()
.unwrap();

// Check those groups were created and the user was added
Expand Down Expand Up @@ -1349,7 +1342,6 @@ async fn test_silo_groups_remove_from_both_groups(
},
)
.await
.unwrap()
.unwrap();

let group_memberships = nexus
Expand Down Expand Up @@ -1418,8 +1410,7 @@ async fn test_silo_delete_clean_up_groups(cptestctx: &ControlPlaneTestContext) {
},
)
.await
.expect("silo_user_from_authenticated_subject")
.unwrap();
.expect("silo_user_from_authenticated_subject");

// Delete the silo
NexusRequest::object_delete(&client, &"/v1/system/silos/test-silo")
Expand Down Expand Up @@ -1501,8 +1492,7 @@ async fn test_ensure_same_silo_group(cptestctx: &ControlPlaneTestContext) {
},
)
.await
.expect("silo_user_from_authenticated_subject 1")
.unwrap();
.expect("silo_user_from_authenticated_subject 1");

// Add the first user with a group membership
let _silo_user_2 = nexus
Expand All @@ -1516,8 +1506,7 @@ async fn test_ensure_same_silo_group(cptestctx: &ControlPlaneTestContext) {
},
)
.await
.expect("silo_user_from_authenticated_subject 2")
.unwrap();
.expect("silo_user_from_authenticated_subject 2");

// TODO-coverage were we intending to verify something here?
}
Expand Down
Loading