-
Notifications
You must be signed in to change notification settings - Fork 12
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
[CA-1065] Update queries for nested roles #481
Conversation
left join ${PolicyRoleTable as policyRole} on ${userResourcePolicy.policyId} = ${policyRole.resourcePolicyId} and ${userResourcePolicy.inherited} = ${policyRole.descendantsOnly} | ||
left join ${ResourceRoleTable as resourceRole} on ${policyRole.resourceRoleId} = ${resourceRole.id} and ${userResourcePolicy.baseResourceTypeId} = ${resourceRole.resourceTypeId} | ||
left join ${PolicyRoleTable as policyRole} on ${userResourcePolicy.policyId} = ${policyRole.resourcePolicyId} | ||
left join ${FlattenedRoleMaterializedView as flattenedRole} on ${policyRole.resourceRoleId} = ${flattenedRole.baseRoleId} and ((${userResourcePolicy.inherited} and (${policyRole.descendantsOnly} or ${flattenedRole.descendantsOnly})) or not (${userResourcePolicy.inherited} or ${policyRole.descendantsOnly} or ${flattenedRole.descendantsOnly})) |
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 don't really care for this long logic statement, but I believe it is right. Would love to shorten/simplify it if anyone has any ideas
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.
first, I might try putting everything after on ${policyRole.resourceRoleId} = ${flattenedRole.baseRoleId}
in the where clause
second, how about
${userResourcePolicy.inherited} = ${policyRole.descendantsOnly} or ${userResourcePolicy.inherited} = ${flattenedRole.descendantsOnly}
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.
well I tried this and it did not quite work so I am missing something
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.
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 couldn't move everything into the where clause because it's just on a left join here and the where clause is too selective, but I did move it into a where clause for listUserResourceActions
and listUserResourceRoles
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.
would it be useful to leave a comment about this ^? (maybe even including the truth table!)
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.
Yeah that's a good call, I think it would be helpful. I'm pulling this check out into a separate query fragment and adding a comment
also enable the pet test that I disabled please |
8d00d72
to
8c4127d
Compare
can jenkins retest if it hasn't finished failing yet? |
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.
👍 LGTM - thanks for adding the nice bit of documentation.
I didn't get too deep into reviewing the unit tests
Ticket: CA-1065
update queries that list roles to check for any nested roles as well
PR checklist