db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Knut Anders Hatlen (JIRA)" <j...@apache.org>
Subject [jira] Commented: (DERBY-3223) SQL roles: make use of privileges granted to roles in actual privilege checking
Date Mon, 14 Jul 2008 12:25:34 GMT

    [ 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


  - mix of tabs and spaces in getPrivName() and toString()


  - 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

  - 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


  - 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?


  - typo in comment: columns are still be unauthorized

  - typo in comment: s/attemped/attempted


  - 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

  - 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

  - 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:
>         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,
> 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.

View raw message