[jira] [Comment Edited] (OAK-8408) UserImporter must not trigger creation of rep:pwd node unless included in xml

["angela (JIRA)" <ji...@apache.org>]

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

angela edited comment on OAK-8408 at 6/19/19 7:07 AM:
------------------------------------------------------

while working on a possible fix, i noticed that _initial-pw-change_ works somewhat contrary to the _pw-expiry_ feature where {{rep:passwordLastModified}} for the former must NOT be set unless the user logs in the first time, whereas the latter should have a baseline date set upon user creation. when it comes to user-import i decided to change the existing behavior (i.e. treating import the same way as regular user-creation and pw-change api calls) as follows:

- _initial-pw-change_: don't set  {{rep:passwordLastModified}} if the user tree has either Status.NEW _or_ if {{UserManagerImpl.setPassword}} is call from an import (i.e. adding the extra import condition. was only checking for NEW before, which resulted in the original issue)
- _pw-expiry_: always set {{rep:passwordLastModified}} for all user mgt api calls; however, for user-import only set upon user creation (i.e. user tree has Status.NEW) in order to comply with {{UserManager.createUser}} but don't update the last-mod date when an import modifies an existing user unless the XML to be imported contains that information. (original behavior: always set  {{rep:passwordLastModified}} when pw-expiry is configured)

note: for the combination of _initial-pw-change_ and _pw-expiry_ the behavior for _initial-pw-change_ applies (i.e. original behavior, i tried to make to code a bit easier to read).

[~stillalex], since the fix changes existing and potentially introduces regressions, i would like you to review the proposed changes and assess the risk that comes with the changes. while there exists documentation about importing the {{rep:pwd}} subtree it is a bit vague about the exact behavior upon user import in general. but it nevertheless states that the importing for an existing user might have undesired effects:

{quote}
On the other hand, if the imported user already exists, potentially existing rep:passwordLastModified properties will be overwritten with the value from the import. If password expiry is enabled, this may cause passwords to expire earlier or later than anticipated, governed by the new value. Also, an import may create such a property where none previously existed, thus effectively cancelling the need to change the password on first login - if the feature is enabled.

Therefore customers using the importer in such fashion should be aware of the potential need to enable password expiry/force initial password change for the imported data to make sense, and/or the effect on already existing/overwritten data.
{{quote}}


was (Author: anchela):
while working on a possible fix, i noticed that _initial-pw-change_ works somewhat contrary to the _pw-expiry_ feature where {{rep:passwordLastModified}} for the former must NOT be set unless the user logs in the first time, whereas the latter should have a baseline date set upon user creation. when it comes to user-import i decided to change the existing behavior (i.e. treating import the same way as regular user-creation and pw-change api calls) as follows:

- _initial-pw-change_: don't set  {{rep:passwordLastModified}} if the user tree has either Status.NEW _or_ if {{UserManagerImpl.setPassword}} is call from an import (i.e. adding the extra import condition. was only checking for NEW before, which resulted in the original issue)
- _pw-expiry_: always set {{rep:passwordLastModified}} for all user mgt api calls; however, for user-import only set upon user creation (i.e. user tree has Status.NEW) in order to comply with {{UserManager.createUser}} but don't update the last-mod date when an import modifies an existing user unless the XML to be imported contains that information. (original behavior: always set  {{rep:passwordLastModified}} when pw-expiry is configured)

note: for the combination of _initial-pw-change_ and _pw-expiry_ the behavior for _initial-pw-change_ applies (i.e. original behavior, i tried to make to code a bit easier to read).

[~stillalex], since the fix changes existing and potentially introduces regressions, i would like you to review the proposed changes and assess the risk that comes with the changes. while there exists documentation about importing the {{rep:pwd}} subtree it is a bit vague about the exact behavior upon user import in general just stating {{Also, an import may create such a property where none previously existed, thus effectively cancelling the need to change the password on first login - if the feature is enabled.}}.

> UserImporter must not trigger creation of rep:pwd node unless included in xml
> -----------------------------------------------------------------------------
>
>                 Key: OAK-8408
>                 URL: https://issues.apache.org/jira/browse/OAK-8408
>             Project: Jackrabbit Oak
>          Issue Type: Bug
>          Components: core, security
>            Reporter: angela
>            Assignee: angela
>            Priority: Minor
>         Attachments: OAK-8408-tests.patch, OAK-8408.patch
>
>
> when xml-importing an existing user (i.e. {{Tree}} doesn't have status NEW upon import) calling {{UserManagerImpl.setPassword}} will force the creation of the {{rep:pwd}} node and {{rep:passwordLastModified}} property contained therein _if_ theinitial-password-change feature is enabled.
> imo the {{rep:pwd}} (and any properties contained therein) must not be auto-created by should only be imported if contained in the XML. 
> proposed fix: {{UserManagerImpl.setPassword}} already contains special treatment for the password hashing triggered upon xml import -> renaming that flag and respect it for the handling of the pw last modified.
> [~stillalex], wdyt?



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)