-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Adjusting SqlStatementResource
and SqlTaskResource
to set request attribute via a new method.
#14878
Conversation
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.
Few nits, PR looks good
return; | ||
} | ||
if (request.getAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED) != null) { | ||
throw new ISE("Request already had authorization check."); |
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.
throw new ISE("Request already had authorization check."); | |
throw DruidException.defensive("Request already had authorization check."); |
public static void setRequestAuthorizationAttributeIfNeeded(final HttpServletRequest request) | ||
{ | ||
if (request.getAttribute(AuthConfig.DRUID_ALLOW_UNSECURED_PATH) != null) { | ||
// do nothing |
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.
Seems shallow, can you please expand on this or remove the comment?
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.
Thanks for the PR! 🚀
Adjusting
SqlStatementResource
andSqlTaskResource
to set request attribute via a new method in AuthUtils which sets the request attribute since we donot have anyResources
to validate against.Please note that this PR is a code readability change and does not effect the functionality of both these API's.
This was one of the feedback for : #14416