-
Notifications
You must be signed in to change notification settings - Fork 44
authz: add built-in roles to the database #512
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.
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
Conversation
@@ -26,93 +26,12 @@ | |||
|
|||
pub mod external; | |||
|
|||
use lazy_static::lazy_static; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much of this file got moved over to nexus/src/db/fixed_data/users.rs
.
nexus/src/external_api/views.rs
Outdated
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize, JsonSchema)] | ||
#[serde(rename_all = "camelCase")] | ||
pub struct Role { | ||
pub name: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps use a newtype here (which we could then put a custom schema on to describe the format)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a stab at this in 7799728. I think it's an improvement but it has some warts I'd love your opinion on.
Here's the issue:
- Within the database, the "resource_type" is a string. This refers to some other resource table (e.g., "project") that the role applies to. The short explanation for this being a string is that I didn't want to have to do a schema update in order to add a role to an object that doesn't already have a role. I put much more detail below.
- For the newtype
RoleName
, I wanted it to be the case that an instance ofRoleName
represents a valid role name. And I wanted to store the parsed representation, similar toName
. I figured it makes sense to validate that the resource type part of the name (the "project" in "project.viewer") corresponds to an actualResourceType
variant. - This introduces a failure mode where you can load a
role_builtin
from the database whose name isn't a valid resource type. This should only be possible due to a previous bug or else because we've rolled out a Nexus upgrade that delivers a new resource type with a new built-in role and an old Nexus handles this request. The way this shows up is in the endpoint handler when we construct the view, it can fail. We treat this as a 500. It should be really uncommon unless there's a bug, in which case a 500 is correct.
There are a few other ways to deal with this:
- Don't validate the resource_type of the RoleName when we parse it, which means storing it as a String rather than a ResourceType. That's okay I guess, but it just means that code in Nexus that happens to have a RoleName cannot rely on it corresponding to a valid resource. (Maybe that's okay, since it already can't rely on it being a valid role?) This approach also means that Nexus can return RoleNames for resource types it doesn't know about, which is a little weird, but admittedly seems correct in the upgrade case I mentioned above.
- On the other end of the spectrum, store a ResourceType rather than a String in the
db::model::RoleBuiltin
. The net result is basically the same as what we have today: if there's a resource type in the database that we don't know about, we're going to fail the request with an error when we try to deserialize it. I don't like this because I prefer to let the consuming code decide if this is a problem or not. - Constrain the database values. There are several ways to do this too. I don't like this approach because it means that adding built-in roles to a resource that didn't have them before requires a schema update -- that sucks. Plus, it doesn't solve this problem: some other Nexus might have done such a schema update and now we're basically back in case 2.
As I write this, I've nearly talked myself into alternative 1.
As an aside, I've changed the serialized form of ResourceType
to use underscores instead of spaces between words, so "vpc subnet" becomes "vpc_subnet". I did this mainly because I didn't think it mattered much either way, and this is easier to programmatically serialize and deserialize because we can just use style = "snake_case"
. (I had originally used spaces because it's slightly more user-friendly, but I don't think it's worthwhile now.)
Finally, this isn't super relevant after all but I'd already written it, so here's some more context about the schema representation.
- use a separate table for every possible target. So instead of "roles_builtin", we'd have "roles_builtin_project", "roles_builtin_organization", etc. Cons: lots of tables, lots of boilerplate, and adding roles on a new target (even if that resource already exists) requires a schema update.
- create a custom datatype (an enum) and store that in the table. So the "resource_type" column for "roles_builtin" is a custom enum. Cons: adding roles on a new target (even if that resource already exists) requires a schema update.
- keep it as a string and have Nexus just know that it's a table name. Cons: kind of implicit, and allows the database to become inconsistent or incorrect. But huge pro: no schema change necessary (and often no code change necessary) to add roles on new targets.
I opted for this last option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, after all that, I talked myself into not validating the resource type in RoleName. It wouldn't validate that the name referred to a real role, anyhow. Plus, nobody was using the typed version -- it's read-only at this point -- so it's not like somebody else needs to validate it anywhere else. I made this change in a4524ba.
I kept the change to ResourceType
's serialized form because this issue made me realize that we probably do want to make this change now, while we still can without breaking any consumers. However, it's now unrelated to the rest of this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine. It might be good to put a comment in explaining that choice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 38fccbf.
common/src/api/external/mod.rs
Outdated
)] | ||
#[display("{resource_type}.{role_name}")] | ||
pub struct RoleName { | ||
#[from_str(regex = "[a-z_]+")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use - instead of _ to be consistent with name strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 38fccbf. I changed this in both the resource type and the role name.
nexus/src/external_api/views.rs
Outdated
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize, JsonSchema)] | ||
#[serde(rename_all = "camelCase")] | ||
pub struct Role { | ||
pub name: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine. It might be good to put a comment in explaining that choice
For where this fits into the authz project, see #419. The Omicron Polar file defines a fixed set of roles. This change loads them into the database.
There is currently no way I know of to verify that our database roles match the roles in the Polar files, but there is a feature coming from Oso to get this information.