[
https://issues.apache.org/jira/browse/DERBY-3223?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12613305#action_12613305
]
Knut Anders Hatlen commented on DERBY-3223:
-------------------------------------------
The patch looks good to me (I've only read through it and not actually
tested it, though). The code was well-commented and easy to read. I
have a couple of questions and comments, most of them minor. Feel free
to ignore them, I have no problem with the patch being committed as
is.
StatementRolePermission.java
- mix of tabs and spaces in getPrivName() and toString()
StatementRoutinePermission.java
- typo in comment: s/attemped/attempted
- perhaps the check method would be simpler if the second condition
was inverted (would save one nesting level for a large section of
the code)
if (perms != null && perms.getHasExecutePermission()) {
// The user has execute permission, the check is successful
return;
}
- if the current role is not granted to the user, would it be more
appropriate to raise an exception saying that than to set the role
to null?
- (in old code that is moved, not new code - same in
StatementTablePermission and StatementColumnPermission) error
messages are created with hard-coded strings "routine" and
"schema" that won't be localized
StatementTablePermission.java
- same question as above about setting current role to null
- perhaps the code will be slightly simpler if the role check in
hasPermissionOnTable() is factored out into a separate method?
StatementColumnPermission.java
- typo in comment: columns are still be unauthorized
- typo in comment: s/attemped/attempted
RolesConferredPrivilegesTest:
- typo in class javadoc: s/thats/that
- typo in makeSuite(): s/as lot/a lot
- the first two calls to assertEquals() in assertPrivilegeMetadata()
have mixed up the arguments (actual/expected instead of
expected/actual)
- assertPrivilegeMetadata(): is JDBC.identifierToCNF() needed around
the literal "test_dbo"?
- assertPrivilegeMetadata(): garbage in comment (�)
- assertPrivilegeMetadata(): assertTrue(noFound == X) =>
assertEquals(X, noFound)
- setRole() looks like a useful helper method. Should it be moved to
JDBC?
- the PreparedStatement in setRole() should be closed
- doGrantRevoke(): try { ... } catch (SQLException e) { fail("hmmmm", e); }
Why not just propagate the exception without try/catch?
- doGrantRevoke(): no need to check if (warn == null) since that's
also checked by assertSQLState()
> SQL roles: make use of privileges granted to roles in actual privilege
> checking
> -------------------------------------------------------------------------------
>
> Key: DERBY-3223
> URL: https://issues.apache.org/jira/browse/DERBY-3223
> Project: Derby
> Issue Type: Task
> Components: Security, SQL
> Reporter: Dag H. Wanvik
> Assignee: Dag H. Wanvik
> Fix For: 10.5.0.0
>
> Attachments: derby-3223-1a.diff, derby-3223-1a.stat,
> derby-3223-1b.diff, derby-3223-1b.stat, derby-3223-1c.diff,
> derby-3223-1c.stat, derby-3223-1d.diff, derby-3223-1d.stat,
> derby-3223-activate-roles-1.diff, derby-3223-activate-roles-1.stat,
> derby-3223-revise-iterator-api-b.diff, derby-3223-revise-iterator-api-b.stat,
> derby-3223-revise-iterator-api.diff, derby-3223-revise-iterator-api.stat,
> roles.sql, roles2.sql, roles3.sql
>
>
> Pushing out to 10.5
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
