[ https://issues.apache.org/jira/browse/OAK-2445?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14591925#comment-14591925 ]
angela edited comment on OAK-2445 at 6/18/15 2:55 PM: ------------------------------------------------------ [~djaeggi], the patch looks great... as always a pleasure to review. there is just one minor thing i am not fully convinced of: why do you think it's necessary to store the password history entries in a separate node each? wouldn't it be sufficient to have a single string property for that (either single or multivalued? i started wondering because i spotted that {{rep:PasswordHistoryEntry}} extends from {{nt:hierarchyNode}} (for the sake of getting a {{jcr:created}} (but you additionally get {{jcr:createdBy}}). is it by intention that you also want to record _who_ did the change? without having thought about it very carefully (in contrast to you), i would have expected this to be just store in a single property as i wouldn't have expected to have thousands of entries in the pw-history. you could argue that updating that property would fail if there was a concurrent pw-change but on the other hand you may also end up with multiple nodes that have the exact same {{jcr:createdBy}} property, which somehow would also leave you with a conflict, wouldn't it? so maybe having those concurrent modifications immediately causing a collision might even be the expected behavior.... wdyt? at least it was less verbose, easier to implement and probably a bit faster to parse than creating and maintaining a tree structure of that. and btw: sorry for the delay... it drop off my TODO list. sorry for that. was (Author: anchela): [~djaeggi], the patch looks great... as always a pleasure to review. there is just one minor thing i am not fully convinced of: why do you think it's necessary to store the password history entries in a separate node each? wouldn't it be sufficient to have a single string property for that (either single or multivalued? i started wondering because i spotted that {{rep:PasswordHistoryEntry}} extends from {{nt:hierarchyNode}} (for the sake of getting a {{jcr:created}} (but you additionally get {{jcr:createdBy}}). is it by intention that you also want to record _who_ did the change? without having thought about it very carefully (in contrast to you), i would have expected this to be just store in a single property as i wouldn't have expected to have thousands of entries in the pw-history. you could argue that updating that property would fail if there was a concurrent pw-change but on the other hand you may also end up with multiple nodes that have the exact same {{jcr:createdBy}} property, which somehow would also leave you with a conflict, wouldn't it? so maybe having those concurrent modifications immediately causing a collision might even be the expected behavior.... wdyt? at least it was less verbose, easier to implement and probably a bit faster to parse than creating a tree structure of that. and btw: sorry for the delay... it drop off my TODO list. sorry for that. > Password History Support > ------------------------ > > Key: OAK-2445 > URL: https://issues.apache.org/jira/browse/OAK-2445 > Project: Jackrabbit Oak > Issue Type: New Feature > Components: core > Affects Versions: 1.1.5 > Reporter: Dominique Jäggi > Assignee: Dominique Jäggi > Fix For: 1.3.1 > > Attachments: OAK-2445_-_Password_History_Support.patch > > > This is a patch that enhances oak security to support user user password > history. > details of the feature as per doc: > {noformat} > Password History > -------------------------------------------------------------------------------- > ### General > Oak provides functionality to remember a configurable number of > passwords after password changes and to prevent a password to > be set during changing a user's password if found in said history. > ### Configuration > An administrator may enable password history via the > _org.apache.jackrabbit.oak.security.user.UserConfigurationImpl_ > OSGi configuration. By default the history is disabled. > The following configuration option is supported: > - Maximum Password History Size (_passwordHistorySize_, number of passwords): > When greater 0 enables password > history and sets feature to remember the specified number of passwords for > a user. > ### How it works > #### Recording of Passwords > If the feature is enabled, during a user changing her password, the old > password > hash is recorded in the password history. > The old password hash is only recorded if a password was set (non-empty). > Therefore setting a password for a user for the first time does not result > in a history record, as there is no old password. > The old password hash is copied to the password history *after* the provided > new > password has been validated (e.g. via the _PasswwordChangeAction_) but > *before* > the new password hash is written to the user's _rep:password_ property. > The history operates as a FIFO list. A new password history record exceeding > the > configured max history size, results in the oldest recorded password from > being > removed from the history. > Also, if the configuration parameter for the history > size is changed to a non-zero but smaller value than before, upon the next > password change the oldest records exceeding the new history size are removed. > History password hashes are recorded as child nodes of a user's > _rep:pwd/rep:pwdHistory_ > node. The _rep:pwdHistory_ node is governed by the _rep:PasswordHistory_ node > type, > its children by the _rep:PasswordHistoryEntry_ node type, allowing setting of > the > _rep:password_ property and its record date via _jcr:created_. > ##### Node Type rep:PasswordHistory and rep:PasswordHistoryEntry > [rep:PasswordHistory] > + * (rep:PasswordHistoryEntry) = rep:PasswordHistoryEntry protected > [rep:PasswordHistoryEntry] > nt:hierarchyNode > - rep:password (STRING) protected > ##### Nodes rep:pwd and rep:pwdHistory > [rep:Password] > + rep:pwdHistory (rep:PasswordHistory) = rep:PasswordHistory protected > ... > [rep:User] > rep:Authorizable, rep:Impersonatable > + rep:pwd (rep:Password) = rep:Password protected > ... > > The _rep:pwdHistory_ and history entry nodes and the _rep:password_ property > are defined protected in order to guard against the user modifying > (overcoming) her > password history limitations. The new sub-nodes also has the advantage of > allowing > repository consumers to e.g. register specific commit hooks / actions on such > a node. > #### Evaluation of Password History > Upon a user changing her password and if the password history feature is > enabled > (configured password history size > 0), the _PasswordChangeAction_ checks > whether > any of the password hashes recorded in the history matches the new password. > If any record is a match, a _ConstraintViolationException_ is thrown and the > user's password is *NOT* changed. > #### Oak JCR XML Import > When users are imported via the Oak JCR XML importer, password history is > currently not supported (ignored). > {noformat} > [~anchela], if you could kindly review the patch. -- This message was sent by Atlassian JIRA (v6.3.4#6332)