[ZEPPELIN-6186] fix searching user using JdbcRealm #4926
Conversation
sh1nj1
force-pushed
the
fix/user-list-in-jdbc
branch
from
77287ab to
706653a
Compare
sh1nj1
changed the title
|
|
||
| userquery = "SELECT ? FROM ?"; | ||
| userquery = String.format("SELECT %s FROM %s WHERE %s LIKE ?", username, tablename, username); |
There was a problem hiding this comment.
I think it would be better to use PreparedStatement instead of making a query using String.format.
There was a problem hiding this comment.
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.
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.
I think we could set SELECT ? FROM ? WHERE ? LIKE ?, and pass it as ps.setString(4, "%" + username + "%").
There was a problem hiding this comment.
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.
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.
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. |
|
@sh1nj1 Can you please create a new ticket so that I can give you credit as the assignee for this issue? |
sh1nj1
changed the title
|
@jongyoul updated the ticket ID
I can do in a separate PR |
|
@Reamer fixed indentation |
| rs = ps.executeQuery(); | ||
| while (rs.next()) { | ||
| int count = 0; |
There was a problem hiding this comment.
is it naming issue, no need issue or anything else?
BTW, I removed the variable.
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
…oAuthenticationService.java Co-authored-by: Philipp Dallig <[email protected]>
…oAuthenticationService.java Co-authored-by: Philipp Dallig <[email protected]>
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 -amVerify in UI
Screenshots (if appropriate)
Questions: