-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[ZEPPELIN-6186] fix searching user using JdbcRealm #4926
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
Conversation
77287ab
to
706653a
Compare
|
||
userquery = "SELECT ? FROM ?"; | ||
userquery = String.format("SELECT %s FROM %s WHERE %s LIKE ?", username, tablename, username); |
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 think it would be better to use PreparedStatement
instead of making a query using String.format
.
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.
That's the last PR's issue for this, you can use parameter replacement in where clause, not the table name or projections. That's why I make this PR.
SELECT ? FROM ?
makes query syntax error because it create invalid query by the PreparedStatement because you can not use '?' in select clause or from clause.
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.
Can you please explain more about it? If PreparedStatement disallows it, it shouldn't be passed as a query. The test case doesn't cover it as well.
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 think we could set SELECT ? FROM ? WHERE ? LIKE ?
, and pass it as ps.setString(4, "%" + username + "%")
.
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.
https://stackoverflow.com/a/2917509/1395707 this explains it better than me.
I use PreparedStatement
only set WHERE username LIKE ?
but remove from select clause and from clause.
If I use old implementation, it creates an error in test case that I added here.
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.
SELECT ? FROM ? WHERE ? LIKE ?
that's the point, previous PR use this form rather than String.format but it misuse PreparedStatement
, so that it creates error.
So I changed and I added some validation for username and tablename for the possible SQL Injection that previous PR tried to implement.
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.
Ah .. I got the point. You meant we shouldn't use an identifier as a literal in the preparedStatement.
Thank you for your contribution! I left one comment. Please check it. |
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. By the way, we can refator the whole logic to parse a tablename and username, and get them from separate variables and pass them directly. Can you consider refactoring the logic as well?
@sh1nj1 Can you please create a new ticket so that I can give you credit as the assignee for this issue? |
@jongyoul updated the ticket ID
I can do in a separate PR |
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.
Looks good to me too. Only the indentation looks strange. Maybe it's a GitHub display issue. Can you please make sure you don't use tabs for the indentation.
@Reamer fixed indentation |
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.
The indentation looks much better. I have two minor points to mention.
zeppelin-server/src/main/java/org/apache/zeppelin/service/ShiroAuthenticationService.java
Outdated
Show resolved
Hide resolved
zeppelin-server/src/main/java/org/apache/zeppelin/service/ShiroAuthenticationService.java
Outdated
Show resolved
Hide resolved
zeppelin-server/src/main/java/org/apache/zeppelin/service/ShiroAuthenticationService.java
Outdated
Show resolved
Hide resolved
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
If no further comments are received, I will merge the whole thing next Monday.
Sorry for the delay. For some unknown reason the pipeline |
@sh1nj1 Please perform a rebase to the current master. There have been corrections to the CI pipeline. |
…oAuthenticationService.java Co-authored-by: Philipp Dallig <philipp.dallig@gmail.com>
…oAuthenticationService.java Co-authored-by: Philipp Dallig <philipp.dallig@gmail.com>
3980642
to
f1923ba
Compare
@Reamer pushed rebased codes |
### What is this PR for? This PR fixes a broken feature where getUserList fails when using JdbcRealm for Shiro authentication. The previous PR addressing SQL injection introduced an error that prevents getUserList from working correctly with JdbcRealm. As a result, users are unable to set notebook permissions. Ref: #4676 ### What type of PR is it? Bug Fix ### Todos ### What is the Jira issue? * https://issues.apache.org/jira/browse/ZEPPELIN-3725 * https://issues.apache.org/jira/browse/ZEPPELIN-6186 ### How should this be tested? Added new unit test should be passed * `./mvnw clean package -pl zeppelin-server -am` Verify in UI * Go to notebook permission settings * Type username and see if username listed ### Screenshots (if appropriate) ### Questions: * Does the license files need to update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Closes #4926 from sh1nj1/fix/user-list-in-jdbc. Signed-off-by: Philipp Dallig <philipp.dallig@gmail.com> (cherry picked from commit 1221444) Signed-off-by: Philipp Dallig <philipp.dallig@gmail.com>
Merged into master and branch-0.12 |
What is this PR for?
This PR fixes a broken feature where getUserList fails when using JdbcRealm for Shiro authentication.
The previous PR addressing SQL injection introduced an error that prevents getUserList from working correctly with JdbcRealm. As a result, users are unable to set notebook permissions.
Ref: #4676
What type of PR is it?
Bug Fix
Todos
What is the Jira issue?
How should this be tested?
Added new unit test should be passed
./mvnw clean package -pl zeppelin-server -am
Verify in UI
Screenshots (if appropriate)
Questions: