ENG-1933: add support for has_role bindings #23
Conversation
vrama628
left a comment
vrama628
left a comment
There was a problem hiding this comment.
Awesome! I like this approach; more general than trying to go through SQLAlchemy's many-to-many relationship, since it seems likely that people may have role-assignment tables that they don't explicitly use a many-to-many relationship for. This looks mostly good to me; one small blocking comment and a bunch of other considerations that you can take or leave as you see fit.
we would also lose the type entry for sql_types
Could we just get the sql type from the column type? Doesn't seem like we'd need to the user to specify the sql type since we already have that on-hand.
We can fix the positional arg issues as a separate PR. Do we have a linear ticket for that? I wonder if we can use / in the call signature to indicate that our arguments should only be accepted as kwargs and shouldn't conflict with positional args.
| organization: Mapped["Organization"] = relationship() | ||
| role: Mapped["Role"] = relationship() | ||
|
|
||
| class Agent(Base, Resource): |
There was a problem hiding this comment.
thought (non-blocking): I wonder if we should add a class Actor(Resource) mixin for better symmetry with Oso types here, so that you could declare the Agent model as an Actor instead of a Resource
tests/models.py
Outdated
| id: Mapped[str] = mapped_column(String(50), primary_key=True) | ||
| description: Mapped[str] | ||
|
|
||
| class AgentOrganizationRole(Base, Resource, RoleMapping): |
There was a problem hiding this comment.
issue (blocking): to me it doesn't make sense for AgentOrganizationRole to be a Resource; it seems to correspond to facts but not to resources. There is no corresponding resource AgentOrganizationRole {...} in the policy, nor should there be.
It seems like the if not issubclass(mapper.class_, Resource): continue line in oso.py would need to be changed given that we now need to process models that aren't resources.
src/sqlalchemy_oso_cloud/orm.py
Outdated
| """ | ||
| pass | ||
|
|
||
| class RoleMapping: |
There was a problem hiding this comment.
nit: is it unnecessary to have Mapping in this name? It seems redundant or inconsistent. Does "mapping" mean a mapping from ORM models to role assignments (in which case it would seem like Resource would have to be named ResourceMapping for the naming to be consistent) or does it mean mapping from users to resources via roles? In which case I would think "Assignment" may be more fitting than "Mapping", but still not really necessary. Another idea would be to name it HasRole, to make it clearer that it maps to has_role facts
src/sqlalchemy_oso_cloud/orm.py
Outdated
| col.column.info[_REMOTE_RELATION_INFO_KEY] = (remote_resource_name, remote_relation_key) | ||
| return col | ||
|
|
||
| def actor(*args, actor_type: Optional[str] = None, **kwargs) -> MappedColumn: |
There was a problem hiding this comment.
thought: I wonder if it would make sense for these functions to be class methods on the Role[Mapping] class, to make it clearer what they're doing. E.g. agent_id = Role.actor(String(50), ...). A function named actor on its own seems a bit general/ambiguous to a reader who doesn't know it's connected to the Role[Mapping] mixin
| return Value("Agent", "1") | ||
|
|
||
| @pytest.fixture | ||
| def bellerophon(): |
src/sqlalchemy_oso_cloud/oso.py
Outdated
| elif _ROLE_MAPPING_ROLE_INFO_KEY in attr.columns[0].info: | ||
| role_column = attr.columns[0] |
There was a problem hiding this comment.
thought (non-blocking): Hmm, I wonder if the role value should also get the same treatment wrt possibly having a different type. Sometimes roles are declares as a Role resource rather than strings, particularly when doing custom roles. I think it may make sense to do that IFF the role model is a Resource, and use String if not. With custom roles it is common to have an app-side table with information about the roles, like you have in the test example, since they're dynamically created by users. While we're at it, it may make sense to sanity-check that the actor/resource foreign keys are subclasses of Resource as well, especially if that allows for code that can be applied homogeneously across all three args.
src/sqlalchemy_oso_cloud/oso.py
Outdated
| if len(foreign_keys) == 0: | ||
| if column.info[info_key] is None: | ||
| raise ValueError(f"Oso role mapping {role_mapping_type} type must be provided if it is not a foreign key") | ||
| oso_type = column.info[info_key] | ||
| else: | ||
| fk = foreign_keys[0].column | ||
| oso_type = _get_classname_by_table(fk.table, registry) |
There was a problem hiding this comment.
nit: this seems like it allows for a confusing case where the user supplies a custom actor_type="Something" on a column that has a foreign key (perhaps to a non-oso-resource table/model), and we just silently ignore their configuration. Would it make sense to either prefer the user-supplied type or loudly error if there's both a foreign key and configured type?
There was a problem hiding this comment.
i think it makes sense to loudly error so the behavior is explicit. also kind of a workaround for the positional arguments issue - users can't use both a foreign key and a user-supplied type (kwarg).
included the option to have roles for remote actors/resources (aka no foreign key) - not sure how robust this approach is with letting users specify the type name. we would also lose the type entry for sql_types, which we could add a custom field for as well, but that starts to get a little harder to validate with types and all.
also didn't do function wrapping (signature forwarding) for the
actorandresourceorm functions due to the issue i discovered previously with the additional arguments not playing well with positional arguments.