Skip to content

authz: check project list endpoints #617

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 1 commit into from
Jan 21, 2022
Merged

authz: check project list endpoints #617

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
35 changes: 15 additions & 20 deletions nexus/src/db/datastore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,45 +607,40 @@ impl DataStore {

pub async fn projects_list_by_id(
&self,
organization_id: &Uuid,
opctx: &OpContext,
authz_org: &authz::Organization,
pagparams: &DataPageParams<'_, Uuid>,
) -> ListResultVec<Project> {
use db::schema::project::dsl;

opctx.authorize(authz::Action::ListChildren, authz_org).await?;

paginated(dsl::project, dsl::id, pagparams)
.filter(dsl::organization_id.eq(*organization_id))
.filter(dsl::organization_id.eq(authz_org.id()))
.filter(dsl::time_deleted.is_null())
.select(Project::as_select())
.load_async(self.pool())
.load_async(self.pool_authorized(opctx).await?)
.await
.map_err(|e| {
public_error_from_diesel_pool(
e,
ResourceType::Project,
LookupType::Other("Listing All".to_string()),
)
})
.map_err(public_error_from_diesel_pool_shouldnt_fail)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change to the error-handling function is sort of unrelated but cleans things up a bit. When the previous code was written, the "shouldnt_fail" function did not exist. We used public_error_from_diesel_pool with parameters that would allow it to generate a 404 if something wasn't found. I don't believe this case was possible. There's no reason this query should ever produce a 404 error, or any other error resulting from bad input or a valid state of the database. That's what the "shouldnt_fail" variant is for -- it produces either a 503 (if there was a problem making the query, either at the network or database level) or a 500 (if any other problem was found -- this would reflect some kind of bug in Nexus).

}

pub async fn projects_list_by_name(
&self,
organization_id: &Uuid,
opctx: &OpContext,
authz_org: &authz::Organization,
pagparams: &DataPageParams<'_, Name>,
) -> ListResultVec<Project> {
use db::schema::project::dsl;

opctx.authorize(authz::Action::ListChildren, authz_org).await?;

paginated(dsl::project, dsl::name, &pagparams)
.filter(dsl::organization_id.eq(*organization_id))
.filter(dsl::organization_id.eq(authz_org.id()))
.filter(dsl::time_deleted.is_null())
.select(Project::as_select())
.load_async(self.pool())
.load_async(self.pool_authorized(opctx).await?)
.await
.map_err(|e| {
public_error_from_diesel_pool(
e,
ResourceType::Project,
LookupType::Other("Listing All".to_string()),
)
})
.map_err(public_error_from_diesel_pool_shouldnt_fail)
}

/// Updates a project by name (clobbering update -- no etag)
Expand Down
13 changes: 11 additions & 2 deletions nexus/src/external_api/http_entrypoints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,13 +381,18 @@ async fn organization_projects_get(
let organization_name = &path.organization_name;

let handler = async {
let opctx = OpContext::for_external_api(&rqctx).await?;
let params = ScanByNameOrId::from_query(&query)?;
let field = pagination_field_for_scan_params(params);
let projects = match field {
PagField::Id => {
let page_selector = data_page_params_nameid_id(&rqctx, &query)?;
nexus
.projects_list_by_id(&organization_name, &page_selector)
.projects_list_by_id(
&opctx,
&organization_name,
&page_selector,
)
.await?
}

Expand All @@ -396,7 +401,11 @@ async fn organization_projects_get(
data_page_params_nameid_name(&rqctx, &query)?
.map_name(|n| Name::ref_cast(n));
nexus
.projects_list_by_name(&organization_name, &page_selector)
.projects_list_by_name(
&opctx,
&organization_name,
&page_selector,
)
.await?
}
}
Expand Down
22 changes: 10 additions & 12 deletions nexus/src/nexus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,30 +590,28 @@ impl Nexus {

pub async fn projects_list_by_name(
&self,
opctx: &OpContext,
organization_name: &Name,
pagparams: &DataPageParams<'_, Name>,
) -> ListResultVec<db::model::Project> {
let organization_id = self
.db_datastore
.organization_lookup_id(organization_name)
.await?
.id();
let authz_org =
self.db_datastore.organization_lookup_id(organization_name).await?;
self.db_datastore
.projects_list_by_name(&organization_id, pagparams)
.projects_list_by_name(opctx, &authz_org, pagparams)
.await
}

pub async fn projects_list_by_id(
&self,
opctx: &OpContext,
organization_name: &Name,
pagparams: &DataPageParams<'_, Uuid>,
) -> ListResultVec<db::model::Project> {
let organization_id = self
.db_datastore
.organization_lookup_id(organization_name)
.await?
.id();
self.db_datastore.projects_list_by_id(&organization_id, pagparams).await
let authz_org =
self.db_datastore.organization_lookup_id(organization_name).await?;
self.db_datastore
.projects_list_by_id(opctx, &authz_org, pagparams)
.await
}

pub async fn project_delete(
Expand Down
82 changes: 58 additions & 24 deletions nexus/tests/integration_tests/basic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
* TODO-coverage add test for racks, sleds
*/

use dropshot::test_util::iter_collection;
use dropshot::test_util::object_get;
use dropshot::test_util::objects_list_page;
use dropshot::test_util::read_json;
Expand Down Expand Up @@ -177,11 +176,32 @@ async fn test_projects_basic(cptestctx: &ControlPlaneTestContext) {

let org_name = "test-org";
create_organization(&client, &org_name).await;
let projects_url = "/organizations/test-org/projects";

/* Unauthenticated and unauthorized users cannot list projects. */
NexusRequest::expect_failure(
client,
http::StatusCode::NOT_FOUND,
http::Method::GET,
projects_url,
)
.execute()
.await
.expect("failed to make request");
NexusRequest::expect_failure(
client,
http::StatusCode::NOT_FOUND,
http::Method::GET,
projects_url,
)
.authn_as(AuthnMode::UnprivilegedUser)
.execute()
.await
.expect("failed to make request");

/*
* Verify that there are no projects to begin with.
*/
let projects_url = "/organizations/test-org/projects";
let projects = projects_list(&client, &projects_url).await;
assert_eq!(0, projects.len());

Expand Down Expand Up @@ -564,9 +584,15 @@ async fn test_projects_list(cptestctx: &ControlPlaneTestContext) {
* increasing order of name.
*/
let found_projects_by_name =
iter_collection::<Project>(&client, projects_url, "", projects_subset)
.await
.0;
NexusRequest::iter_collection_authn::<Project>(
&client,
projects_url,
"",
projects_subset,
)
.await
.expect("failed to list projects")
.all_items;
assert_eq!(found_projects_by_name.len(), project_names_by_name.len());
assert_eq!(
project_names_by_name,
Expand All @@ -580,14 +606,16 @@ async fn test_projects_list(cptestctx: &ControlPlaneTestContext) {
* Page through all the projects in ascending order by name, which should be
* the same as above.
*/
let found_projects_by_name = iter_collection::<Project>(
&client,
projects_url,
"sort_by=name-ascending",
projects_subset,
)
.await
.0;
let found_projects_by_name =
NexusRequest::iter_collection_authn::<Project>(
&client,
projects_url,
"sort_by=name-ascending",
projects_subset,
)
.await
.expect("failed to list projects")
.all_items;
assert_eq!(found_projects_by_name.len(), project_names_by_name.len());
assert_eq!(
project_names_by_name,
Expand All @@ -601,14 +629,16 @@ async fn test_projects_list(cptestctx: &ControlPlaneTestContext) {
* Page through all the projects in descending order by name, which should be
* the reverse of the above.
*/
let mut found_projects_by_name = iter_collection::<Project>(
&client,
projects_url,
"sort_by=name-descending",
projects_subset,
)
.await
.0;
let mut found_projects_by_name =
NexusRequest::iter_collection_authn::<Project>(
&client,
projects_url,
"sort_by=name-descending",
projects_subset,
)
.await
.expect("failed to list projects")
.all_items;
assert_eq!(found_projects_by_name.len(), project_names_by_name.len());
found_projects_by_name.reverse();
assert_eq!(
Expand All @@ -622,14 +652,15 @@ async fn test_projects_list(cptestctx: &ControlPlaneTestContext) {
/*
* Page through the projects in ascending order by id.
*/
let found_projects_by_id = iter_collection::<Project>(
let found_projects_by_id = NexusRequest::iter_collection_authn::<Project>(
&client,
projects_url,
"sort_by=id-ascending",
projects_subset,
)
.await
.0;
.expect("failed to list projects")
.all_items;
assert_eq!(found_projects_by_id.len(), project_names_by_id.len());
assert_eq!(
project_names_by_id,
Expand Down Expand Up @@ -679,7 +710,10 @@ async fn projects_list(
client: &ClientTestContext,
projects_url: &str,
) -> Vec<Project> {
objects_list_page::<Project>(client, projects_url).await.items
NexusRequest::iter_collection_authn(client, projects_url, "", 10)
.await
.expect("failed to list projects")
.all_items
}

async fn project_get(client: &ClientTestContext, project_url: &str) -> Project {
Expand Down
49 changes: 41 additions & 8 deletions nexus/tests/integration_tests/projects.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@
// 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 nexus_test_utils::http_testing::AuthnMode;
use nexus_test_utils::http_testing::NexusRequest;
use omicron_nexus::external_api::views::Project;

use dropshot::test_util::object_get;
use dropshot::test_util::objects_list_page;

use nexus_test_utils::resource_helpers::{create_organization, create_project};
use nexus_test_utils::ControlPlaneTestContext;
Expand Down Expand Up @@ -34,12 +35,41 @@ async fn test_projects(cptestctx: &ControlPlaneTestContext) {
let project: Project = object_get(&client, &p2_url).await;
assert_eq!(project.identity.name, p2_name);

let projects = objects_list_page::<Project>(
client,
&format!("/organizations/{}/projects", org_name),
/*
* Unauthenticated and unauthorized users should not be able to list
* Projects.
*/
let projects_url = format!("/organizations/{}/projects", org_name);
NexusRequest::expect_failure(
&client,
http::StatusCode::NOT_FOUND,
http::Method::GET,
&projects_url,
)
.execute()
.await
.items;
.expect("failed to make request");
NexusRequest::expect_failure(
&client,
http::StatusCode::NOT_FOUND,
http::Method::GET,
&projects_url,
)
.authn_as(AuthnMode::UnprivilegedUser)
.execute()
.await
.expect("failed to make request");

/* Verify the list of Projects. */
let projects = NexusRequest::iter_collection_authn::<Project>(
&client,
&projects_url,
"",
10,
)
.await
.expect("failed to list projects")
.all_items;
assert_eq!(projects.len(), 2);
// alphabetical order for now
assert_eq!(projects[0].identity.name, p2_name);
Expand All @@ -54,12 +84,15 @@ async fn test_projects(cptestctx: &ControlPlaneTestContext) {
assert_ne!(org_p1_id, org2_p1_id);

// Make sure the list projects results for the new org make sense
let projects = objects_list_page::<Project>(
client,
let projects = NexusRequest::iter_collection_authn::<Project>(
&client,
&format!("/organizations/{}/projects", org2_name),
"",
10,
)
.await
.items;
.expect("failed to list projects")
.all_items;
assert_eq!(projects.len(), 1);
assert_eq!(projects[0].identity.name, p1_name);
}
2 changes: 1 addition & 1 deletion tools/oxapi_demo
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ function cmd_organization_get
function cmd_projects_list
{
[[ $# != 1 ]] && usage "expected ORGANIZATION_NAME"
do_curl "/organizations/$1/projects"
do_curl_authn "/organizations/$1/projects"
}

function cmd_project_create_demo
Expand Down