service IP pool lookup returns wrong authz error

[davepacheco]

I noticed this while reading code debugging #3889. Basically, it's a wrong pattern to map_err() the result of authorize() to replace a Forbidden error with a NotFound. Distinguishing between these two is the responsibility of the authz subsystem because it should be consistent for all callers of authorize(). If we want it to be a NotFound, the authz subsystem supports that via the impl of AuthorizedResource::on_unauthorized() for the resource. Concretely, we could preserve the existing behavior (while still delegating it to the authz subsystem) by customizing the impl for IpPoolList:

omicron/nexus/db-queries/src/authz/api_resources.rs

Lines 433 to 441 in 94cde6d

	fn on_unauthorized(
	&self,
	_: &Authz,
	error: Error,
	_: AnyActor,
	_: Action,
	) -> Error {
	error
	}

to look more like the one for most non-synthetic resources (which do return NotFound when someone's not authorized to read them):

omicron/nexus/db-queries/src/authz/api_resources.rs

Lines 132 to 154 in 94cde6d

	fn on_unauthorized(
	&self,
	authz: &Authz,
	error: Error,
	actor: AnyActor,
	action: Action,
	) -> Error {
	if action == Action::Read {
	return self.not_found();
	}
	// If the user failed an authz check, and they can't even read this
	// resource, then we should produce a 404 rather than a 401/403.
	match authz.is_allowed(&actor, Action::Read, self) {
	Err(error) => Error::internal_error(&format!(
	"failed to compute read authorization to determine visibility: \
	{:#}",
	error
	)),
	Ok(false) => self.not_found(),
	Ok(true) => error,
	}
	}

I started looking at that when I saw that we're doing a ListChildren here, not a Read. The resource itself ("the list of service IP pools") is not really hidden from end users -- the URL is even published in the OpenAPI spec. So I think it makes more sense to return Forbidden here, just like if you try to list IP pools when not authorized to do so.

[david-crespo]

This comment is no longer accurate

[davepacheco]

Good catch.

[david-crespo]

Looks good, reasoning makes sense. I also don't see a reason for special handling.