Fix/system assignments issue 143#408
Fix/system assignments issue 143#408konac-hamza wants to merge 4 commits intoopenstack-experimental:mainfrom
Conversation
…ith list method logic (openstack-experimental#143)" This reverts commit e0b4520.
gtema
left a comment
gtema
left a comment
There was a problem hiding this comment.
Thanks for the update. Few smaller issues.
| let mut select = DbAssignment::find(); | ||
| let mut select_system = DbSystemAssignment::find(); | ||
| // flags of actors and role ID matches as boolean expressions | ||
| let include_system = params.targets.is_empty(); // Track if we should query system table |
There was a problem hiding this comment.
On the database level we still have target_id also for system assignments (in the system_role_assignment table there is target_id). On practice it is always "system" currently. On the API level it is also expected that the user passes scope.system=XXX to query system assignments. So the part of your previous attempt with adding system scope into the target parameters was partially correct. We need to extend the RoleAssignmentTarget with the enum specifying what the target_id is pointing to (project, domain, system). Here we would need to iterate over the targets and when there is a system target - do the querying. I would suggest - prepare the query and only decide whether to execute it or not based on the logic (i.e. during iterating over the targets also check if there are system assignments and raise a flag). This will help to keep less of the if guards
There was a problem hiding this comment.
I am adding the target type in the #416
I will update the PR by the new types. Thank you for the updates.
| .collect::<Result<Vec<_>, _>>()? | ||
| } else { | ||
| Vec::new() // Don't query system table if not needed | ||
| }; |
There was a problem hiding this comment.
We can query both tables in parallel just like it is done for users https://github.com/openstack-experimental/keystone/blob/main/src/identity/backends/sql/user/list.rs#L58. If you don't feel confident in that leave it like that and I will add it later.
konac-hamza
force-pushed
the
fix/system-assignments-issue-143
branch
from
86249ca to
420df3f
Compare
Integrate RoleAssignmentTargetType and simplify queries.
konac-hamza
force-pushed
the
fix/system-assignments-issue-143
branch
from
420df3f to
f6bd709
Compare
3 participants
@gtema Following your review, I did not modify any SeaORM tables or models.
I updated the implementation to follow the same logic used in the list method.
Looking forward to your feedback. Thank you!