Skip to content

[authz] Add coverage for more endpoints#9760

Merged
david-crespo merged 3 commits intomainfrom
more-authz
Jan 31, 2026
Merged

[authz] Add coverage for more endpoints#9760
david-crespo merged 3 commits intomainfrom
more-authz

Conversation

@smklein
Copy link
Collaborator

@smklein smklein commented Jan 30, 2026

Fixes #9747

login_local (post "/v1/login/{silo_name}/local")
logout (post "/v1/logout")
rack_membership_add_sleds (post "/v1/system/hardware/racks/{rack_id}/membership/add")
networking_switch_port_lldp_config_update (post "/v1/system/hardware/switch-port/{port}/lldp/config")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is correct but now that these endpoints aren't exempted here, they need audit logging. Which makes me realize the audit log test must only be checking endpoints that not exempted here? Wtf? Looking into that.

Image

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. The audit log coverage test does use the schema for getting operation IDs, but for the actual looping through the endpoints, it doesn't. Let me fix it on this branch and I'll fix the endpoints missing audit logging too.

// Build a map from (method, url_regex) to (operation_id, path_template)
let spec_operations: BTreeMap<(String, String), (String, String)> = spec
.operations()
.map(|(path, method, op)| {
// Convert path template to regex pattern
let re = regex::Regex::new("/\\{[^}]+\\}").unwrap();
let regex_path = re.replace_all(path, "/[^/]+");
let regex = format!("^{}$", regex_path);
let label = op
.operation_id
.clone()
.unwrap_or_else(|| String::from("unknown"));
((method.to_uppercase(), regex), (label, path.to_string()))
})
.collect();
// Set up resources needed by many endpoints
DiskTest::new(&ctx).await;
create_default_ip_pools(client).await;
let _project = create_project(client, "demo-project").await;
let t_start = Utc::now();
let mut missing_audit: BTreeMap<String, (String, String)> = BTreeMap::new();
let mut unexpected_get_audit: BTreeMap<String, (String, String)> =
BTreeMap::new();
for endpoint in &*VERIFY_ENDPOINTS {

Copy link
Contributor

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

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

There are bigger changes to the audit log coverage test I need to make, but after fixing the endpoints I fixed here, I think the only endpoint genuinely missing coverage is logout. Writing up an issue for all that. In the meantime I'm going to merge this so we can get #9671 in.

@david-crespo david-crespo enabled auto-merge (squash) January 31, 2026 14:41
@david-crespo david-crespo merged commit 001cb15 into main Jan 31, 2026
16 checks passed
@david-crespo david-crespo deleted the more-authz branch January 31, 2026 16:16
david-crespo added a commit that referenced this pull request Feb 1, 2026
Depends on #9760
Fixes #9748

---------

Co-authored-by: David Crespo <david.crespo@oxidecomputer.com>
david-crespo added a commit that referenced this pull request Feb 3, 2026
…9773)

This is almost entirely a testing change in the direction of more
comprehensive coverage. The only app code change is to remove audit
logging from the SCIM GET endpoints, which should not have it.

---

Thanks to #9760 I realized that both the authz and audit log coverage
tests (audit log imitates the authz one) take the OpenAPI schema as the
source of the list of API endpoints. This means unpublished endpoints
like the SCIM ones are not checked. This PR changes that by starting
from a full list of all endpoints in the Dropshot server.

The good news is that you can see that we aren't missing any endpoints
(we were missing a few before that I fixed in #9760) except arguably
session `logout`, which would need to be reworked slightly in order to
get logging to work.

* Centralize logic for iterating through all endpoints in
`ApiOperations`, use it in both tests
* Add ability to use SCIM token instead of session auth in the authz
coverage test, gated by the URL starting with `/saml` — this doesn't
feel great, but the alternative would probably be to add something like
an `auth_strategy: AuthStrategy::Saml` field to every single
`VerifyEndpoint` entry. Fine and doable but feels even sillier IMO.
* Fix newly detectable oversights: remove audit logging from SCIM GET
endpoints

### Remaining issues

These are not blockers because they were already the case before this
PR.

* We are still not exercising `login_saml` and `login_local`, which do
have audit logging (rightly so). The test considers them "not verified"
because they don't have entries in the endpoints list because they don't
auth like normal endpoints, and then the audit log test can't test them
for the same reason. One way to fix this would be to directly manually
exercise them in the audit log test, but this feels a little ad hoc. I
will think about it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

many endpoints skipped by authz coverage test that seem like they shouldn't be

2 participants