[#9583] improvement(authz): loadTable should indicate if it's for writing #9585
Conversation
jerqi
changed the title
| @ResponseMetered(name = "load-table", absolute = true) | ||
| @AuthorizationExpression( | ||
| expression = AuthorizationExpressionConstants.loadTableAuthorizationExpression, | ||
| expression = AuthorizationExpressionConstants.LOAD_TABLE_AUTHORIZATION_EXPRESSION, |
There was a problem hiding this comment.
After using LoadTableAuthorizationExecutor, do we still need to use the metadata authorization expression here?
There was a problem hiding this comment.
If privileges is null, we will still use the expression. So I think we should keep this expresssion.
There was a problem hiding this comment.
Since LoadTableAuthorizationExecutor replaces the authorization expression, we should add explanatory comments to clarify this behavior and prevent users from being confused by apparent inconsistencies between the declared privileges and the actual expressions.
Alternatively, we could make the behavior more transparent by encoding the logic directly into the expression itself—for example:
PRIVILEGE == 'MODIFY_TABLE' && MODIFY_TABLE_AUTHORIZATION_EXPRESSION
|| PRIVILEGE == null && LOAD_TABLE_AUTHORIZATION_EXPRESSIONThere was a problem hiding this comment.
I modified the AuthorizationExpression. Could u take a look again?
There was a problem hiding this comment.
I'm a bit unsure—could you please take a look? @FANNG1
There was a problem hiding this comment.
What‘s your concern? What do u need to confirm?
| Object[] args) { | ||
| Object[] args, | ||
| String secondaryExpression, | ||
| String secondaryExpressionCondition) { |
There was a problem hiding this comment.
I'm wondering if modifying the annotation structure for this particular scenario might not be a good idea—mainly because it's unclear whether other interfaces will have similar scenarios in the future, which could lead to things getting messy later on.
Changing the input parameter here to AuthorizationExpression might be slightly better.
There was a problem hiding this comment.
This is a proper common ability. We can choose the expression according to the parameters. If we need more parameters, we can add more parameters.
|
Merged. Thanks. |
2 participants
What changes were proposed in this pull request?
loadTable should indicate if it's for writing
Why are the changes needed?
Fix: #9583
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Add new test cases.