shiro-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Les Hazlewood (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (SHIRO-277) JdbcRealm needs to be refactored
Date Fri, 22 Jul 2011 21:04:57 GMT

    [ https://issues.apache.org/jira/browse/SHIRO-277?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13069743#comment-13069743
] 

Les Hazlewood commented on SHIRO-277:
-------------------------------------

Applied and committed the patch.  Quite good unit tests Phil - nicely done! And thanks again
for the patch - it is greatly appreciated!

I have a few comments so far:

1) I think we should add back in the buildAuthenticationInfo method (the original) and add
a new one that takes in the salt.  doGetAuthenticationInfo would call the new method and it
in turn can just call the older one for backwards-compatibility.

2) I see that the salt returned from the JDBC query is a String.  I think that this string
would probably represent a Base64 or Hex-encoded String though and need to be decoded first
before acquiring the bytes. (the current call to ByteSource.Util.bytes(salt) returns the Strings
direct bytes without decoding first).  A theme in Shiro's INI configuration is that if the
value should be decoded, you can assume that a prefix of '0x' (zero x) indicates a hex value
and anything else is assumed to be Base64.

3) I'm wondering if we should forego an Enum to represent how we acquire the salt and instead
provide a JdbcAccountResolver that takes in a Connection and the AuthenticationToken as arguments
(or maybe just a ResultSet) and returns a Map of data that we can check (i.e. map.get("username"),
map.get("password"), map.get("salt")).

This way we can provide the 3 default implementations that replace the SaltStyle.NO_SALT,
CRYPT, and COLUMN cases.  EXTERNAL would just be a custom implementation of the JdbcAccountResolver
interface.  I'm keen on allowing people to plugin in simple implementations instead of having
to override methods where possible.  Also, using the username as a salt is not recommended
for security reasons, which is why I think allowing end-users to plug in a resolver of their
own is better than defaulting to the username.

Thoughts?

Les

> JdbcRealm needs to be refactored
> --------------------------------
>
>                 Key: SHIRO-277
>                 URL: https://issues.apache.org/jira/browse/SHIRO-277
>             Project: Shiro
>          Issue Type: Improvement
>          Components: Realms 
>    Affects Versions: 1.1.0
>            Reporter: Ilya Pyatigorskiy
>             Fix For: 1.2.0
>
>         Attachments: jdbcRealm.patch
>
>
> There are at least 2 obvious problems:
> 1) the javadoc for JdbcRealm.setPermissionsQuery suggests that the query is expected
to have 3 columns ("containing the fully qualified name of the permission class, the permission
name, and the permission actions (in that order)"), but the code actually looks only for 1
- permission actions on index 0
> 2) it doesn't support salt - checks only for password matching

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message