Skip to content

Add list of group IDs to /session/me response #1830

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
merged 7 commits into from
Oct 19, 2022
Merged

Add list of group IDs to /session/me response #1830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a0b8735
add group IDs to /session/me response
david-crespo Oct 17, 2022
7479c5d
you win this round, clippy
david-crespo Oct 17, 2022
8acee42
more smarter comment
david-crespo Oct 17, 2022
5f0ae14
address PR comments
david-crespo Oct 18, 2022
2950162
let's not be so casual in the comments
david-crespo Oct 18, 2022
0e61d09
test that session me returns group IDs when it's supposed to
david-crespo Oct 18, 2022
52b9f46
order agnostic vec equality helper
david-crespo Oct 19, 2022
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
7 changes: 7 additions & 0 deletions nexus/src/app/iam.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,13 @@ impl super::Nexus {
Ok(db_silo_user)
}

pub async fn silo_user_fetch_groups_for_self(
&self,
opctx: &OpContext,
) -> ListResultVec<db::model::SiloGroupMembership> {
self.db_datastore.silo_group_membership_for_self(opctx).await
}

// Silo groups

pub async fn silo_groups_list(
Expand Down
22 changes: 22 additions & 0 deletions nexus/src/db/datastore/silo_group.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use omicron_common::api::external::CreateResult;
use omicron_common::api::external::DataPageParams;
use omicron_common::api::external::DeleteResult;
use omicron_common::api::external::Error;
use omicron_common::api::external::InternalContext;
use omicron_common::api::external::ListResultVec;
use omicron_common::api::external::LookupResult;
use omicron_common::api::external::UpdateResult;
Expand Down Expand Up @@ -105,6 +106,27 @@ impl DataStore {
.map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))
}

pub async fn silo_group_membership_for_self(
&self,
opctx: &OpContext,
) -> ListResultVec<SiloGroupMembership> {
// Similar to session_hard_delete (see comment there), we do not do a
// typical authz check, instead effectively encoding the policy here
// that any user is allowed to fetch their own group memberships
let &actor = opctx
.authn
.actor_required()
.internal_context("fetching current user's group memberships")?;

use db::schema::silo_group_membership::dsl;
dsl::silo_group_membership
.filter(dsl::silo_user_id.eq(actor.actor_id()))
.select(SiloGroupMembership::as_returning())
.get_results_async(self.pool_authorized(opctx).await?)
.await
.map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))
}

/// Update a silo user's group membership:
///
/// - add the user to groups they are supposed to be a member of, and
Expand Down
10 changes: 8 additions & 2 deletions nexus/src/external_api/console_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,7 @@ pub async fn login_begin(
}]
pub async fn session_me(
rqctx: Arc<RequestContext<Arc<ServerContext>>>,
) -> Result<HttpResponseOk<views::User>, HttpError> {
) -> Result<HttpResponseOk<views::SessionMe>, HttpError> {
let apictx = rqctx.context();
let nexus = &apictx.nexus;
let handler = async {
Expand All @@ -601,7 +601,13 @@ pub async fn session_me(
// not clear what the advantage would be.
let opctx = OpContext::for_external_api(&rqctx).await?;
let user = nexus.silo_user_fetch_self(&opctx).await?;
Ok(HttpResponseOk(user.into()))
let groups = nexus.silo_user_fetch_groups_for_self(&opctx).await?;
Ok(HttpResponseOk(views::SessionMe {
id: user.id(),
display_name: user.external_id,
silo_id: user.silo_id,
group_ids: groups.iter().map(|g| g.silo_group_id).collect(),
}))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think views::SessionMe::new(user, groups) would make sense here, but I struggled to decide where to define that. The obviously place would be right next to the definition of SessionMe in views.rs, but that would be the first time anything in that file refers to DB models. Could be fine, or maybe it offends our sense of propriety. Generally we seem to prefer doing our impl From for these views next to the DB model, which to be honest is not what I prefer. I like to think of the view as "knowing about" the model but the model not knowing about the view.

Copy link
Contributor Author

@david-crespo david-crespo Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In fact, there does not seem to be a way to import db into nexus/types/src/external_api/views.rs because it's a different crate and that would introduce a circular dependency. So it basically has to be defined here in the app, but I can't impl a constructor for it because it comes from another crate.

Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to point to a fn Nexus::session_me(..) -> views::SessionMe, no?

Copy link
Contributor Author

@david-crespo david-crespo Oct 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I considered that too. The problem is the app layer methods all return DB models, not views, and it's up to the From impls to turn those into views in the entrypoints. This is a pretty rare case where we're making a view out of two models. I don't think we do that very often. So if it can't go in nexus/src/app, it would basically be a helper that sits right next to the route handler right here, at which point I figure we might as well inline it (leave as is) since it's not going to be used anywhere else.

};
apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await
}
Expand Down
11 changes: 6 additions & 5 deletions nexus/tests/integration_tests/console_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,33 +335,34 @@ async fn test_session_me(cptestctx: &ControlPlaneTestContext) {
.execute()
.await
.expect("failed to get current user")
.parsed_body::<views::User>()
.parsed_body::<views::SessionMe>()
.unwrap();

assert_eq!(
priv_user,
views::User {
views::SessionMe {
id: USER_TEST_PRIVILEGED.id(),
display_name: USER_TEST_PRIVILEGED.external_id.clone(),
silo_id: DEFAULT_SILO.id(),
group_ids: vec![],
}
);

// make sure it returns different things for different users
let unpriv_user = NexusRequest::object_get(testctx, "/session/me")
.authn_as(AuthnMode::UnprivilegedUser)
.execute()
.await
.expect("failed to get current user")
.parsed_body::<views::User>()
.parsed_body::<views::SessionMe>()
.unwrap();

assert_eq!(
unpriv_user,
views::User {
views::SessionMe {
id: USER_TEST_UNPRIVILEGED.id(),
display_name: USER_TEST_UNPRIVILEGED.external_id.clone(),
silo_id: DEFAULT_SILO.id(),
group_ids: vec![],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be cool to be able to add the user to a group, then refetch and see the group here. But I see that there is no endpoint for adding a user to a group and modification of group memberships only happens during JIT user create. A next-best thing would be if the built-in privileged and unprivileged users were in groups, so at least this vec would come back non-empty. @jmpesp any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

In nexus/tests/integration_tests/saml.rs, test_correct_saml_response_with_group_attributes tests that groups are JITed correctly, and test_post_saml_response calls /session/me to validate a successful SAML login. I'm not sure about the built-in users being part of some fake groups when a test(s) could be done there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. In 0e61d09 I modified test_post_saml_response to include groups and assert that the groups are there on /session/me. One downside is that test didn't used to be about groups and now it is. Not sure if that is really a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's not a problem, though if the order of groups returned changes then that assert_eq will fire.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a cute helper for that in 52b9f46

}
);
}
Expand Down
46 changes: 39 additions & 7 deletions nexus/tests/integration_tests/saml.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

use std::fmt::Debug;

use nexus_test_utils::http_testing::{AuthnMode, NexusRequest, RequestBuilder};
use omicron_common::api::external::IdentityMetadataCreateParams;
use omicron_nexus::authn::silos::{
Expand All @@ -19,7 +21,9 @@ use nexus_test_utils::resource_helpers::{create_silo, object_create};
use nexus_test_utils::ControlPlaneTestContext;
use nexus_test_utils_macros::nexus_test;

use dropshot::ResultsPage;
use httptest::{matchers::*, responders::*, Expectation, Server};
use uuid::Uuid;

// Valid SAML IdP entity descriptor from https://en.wikipedia.org/wiki/SAML_metadata#Identity_provider_metadata
// note: no signing keys
Expand Down Expand Up @@ -959,7 +963,7 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) {

signing_keypair: None,

group_attribute_name: None,
group_attribute_name: Some("groups".into()),
},
)
.await;
Expand All @@ -984,7 +988,7 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) {
)
.raw_body(Some(
serde_urlencoded::to_string(SamlLoginPost {
saml_response: base64::encode(SAML_RESPONSE),
saml_response: base64::encode(SAML_RESPONSE_WITH_GROUPS),
relay_state: None,
})
.unwrap(),
Expand All @@ -1006,19 +1010,47 @@ async fn test_post_saml_response(cptestctx: &ControlPlaneTestContext) {
.await
.expect("expected success");

let _session_user: views::User = NexusRequest::new(
let session_cookie_value =
result.headers["Set-Cookie"].to_str().unwrap().to_string();

let groups: ResultsPage<views::Group> = NexusRequest::new(
RequestBuilder::new(client, Method::GET, "/groups")
.header(http::header::COOKIE, session_cookie_value.clone())
.expect_status(Some(StatusCode::OK)),
)
.execute()
.await
.expect("expected success")
.parsed_body()
.unwrap();

let silo_group_names: Vec<&str> =
groups.items.iter().map(|g| g.display_name.as_str()).collect();
let silo_group_ids: Vec<Uuid> = groups.items.iter().map(|g| g.id).collect();

assert_same_items(silo_group_names, vec!["SRE", "Admins"]);

let session_me: views::SessionMe = NexusRequest::new(
RequestBuilder::new(client, Method::GET, "/session/me")
.header(
http::header::COOKIE,
result.headers["Set-Cookie"].to_str().unwrap().to_string(),
)
.header(http::header::COOKIE, session_cookie_value)
.expect_status(Some(StatusCode::OK)),
)
.execute()
.await
.expect("expected success")
.parsed_body()
.unwrap();

assert_eq!(session_me.display_name, "[email protected]");
assert_same_items(session_me.group_ids, silo_group_ids);
}

/// Order-agnostic vec equality
fn assert_same_items<T: PartialEq + Debug>(v1: Vec<T>, v2: Vec<T>) {
assert_eq!(v1.len(), v2.len(), "{:?} and {:?} don't match", v1, v2);
for item in v1.iter() {
assert!(v2.contains(item), "{:?} and {:?} don't match", v1, v2);
}
}

// Test correct SAML response with relay state
Expand Down
13 changes: 13 additions & 0 deletions nexus/types/src/external_api/views.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,19 @@ pub struct User {
pub silo_id: Uuid,
}

/// Client view of a [`User`] and their groups
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize, JsonSchema)]
pub struct SessionMe {
pub id: Uuid,
/** Human-readable name that can identify the user */
pub display_name: String,

/** Uuid of the silo to which this user belongs */
pub silo_id: Uuid,

pub group_ids: Vec<Uuid>,
}

// SILO GROUPS

/// Client view of a [`Group`]
Expand Down
34 changes: 33 additions & 1 deletion openapi/nexus.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5202,7 +5202,7 @@
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/User"
"$ref": "#/components/schemas/SessionMe"
}
}
}
Expand Down Expand Up @@ -11002,6 +11002,38 @@
"technical_contact_email"
]
},
"SessionMe": {
"description": "Client view of a [`User`] and their groups",
"type": "object",
"properties": {
"display_name": {
"description": "Human-readable name that can identify the user",
"type": "string"
},
"group_ids": {
"type": "array",
Copy link
Contributor

Choose a reason for hiding this comment

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

is this bounded in some way? in general I think we try to avoid responses that are effectively unbounded in terms of their size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, and bad timing for the auto merge. I might revert it, or I might just make the necessary changes on top. I don't think there is a bound on the size of this list. The problem from the console point of view is that we really do want all the groups for the user.

Our standard approach is to make a separate paginated endpoint. I was trying to avoid that, but it's natural enough: the console can GET /session/me/groups or whatever with a large page size (default is 1000) to try to get all the groups. This would likely cover us for a long time, especially since we can increase the page size up to (I think) 10000 if someone wants to get weird and assign a ton of groups, and it's not too surprising if the console experience degrades somewhat when a user is in 10,000 groups. In this case, the degradation would be that when we determine client-side whether a user has a given permission (by comparing the user's ID and group IDs to a policy response), we will determine wrong if we are missing groups for the user.

On the other hand, this whole rigmarole is a point in favor of moving that logic server-side, letting the console ask for the current user's effective role on any resource.

Another option is to actually limit the number of group memberships and not paginate this.

"items": {
"type": "string",
"format": "uuid"
}
},
"id": {
"type": "string",
"format": "uuid"
},
"silo_id": {
"description": "Uuid of the silo to which this user belongs",
"type": "string",
"format": "uuid"
}
},
"required": [
"display_name",
"group_ids",
"id",
"silo_id"
]
},
"Silo": {
"description": "Client view of a ['Silo']",
"type": "object",
Expand Down