Constants, PasswordUtil, builtin-nodetypes.cnd: -------------------------------------------------------------------------------- - move DEFAULT_PASSWORD_MAX_AGE to UserConstants as its not used in PasswordUtil -> you didn't use it consistently - move DEFAULT_PASSWORD_INITIAL_CHANGE to UserConstants as its not used in PasswordUtil -> you didn't use it consistently (see below)... maybe just drop this one? - rename NN_REP_PW to REP_PW for consistency with other constants. - i am not soooooo happy with the rep:pw node name. it's not very meaningful compared to rep:password and look a bit confusing to me... but so far i don't have a brilliant alternative... let's have another brainstorming... maybe we can find a better solution. UserAuthenticationFactory -------------------------------------------------------------------------------- - javadoc of spi-interface refers to UserAuthentication which is a specific implementation of the Authentication interface -> should rather link to the spi-interface as the return value can be any implementation of the Authentication interface, right? - in order to allow for later pluggability i would rather extend the method to also pass the UserConfiguration there. UserAuthenticationFactoryImpl -------------------------------------------------------------------------------- - you don't need the Impl of the UserConfiguration, right? - see above: in order to allow for later pluggability i would make the constructor empty and add the userconfig to the method call. - we need a test class for the factory UserConfigurationImpl -------------------------------------------------------------------------------- - UserConfigurationImpl the default factory field can be declared "UserAuthenticationFactory" - instead of creating a new HashMap with just a single value i would suggest to call ImmutableMap.of(key, value) or Collections.singletonMap(key, value). even better would be to extend the ConfigurationParameters with a new of() method that allows to create a single-valued instance... otherwise the map is just copied over again. - the way you add the authenticationfactory to the parameters will clear all existing config params... this is IMHO a bug; i would rather override getParameters and add the factory there. - do i get it right, that the UserAuthenticationFactory is currently not intended to be pluggable? why? - we need a test for the UserConfigurationImpl changes; specially in the light of what i considered a bug above. UserAuthentication -------------------------------------------------------------------------------- - by moving the UserAuthentication you broke access to the PRE_AUTHENTICATED constant in LoginModuleImpl and just duplicated the constant... IMO that's a bit unfortunate... i would rather move that constant to the PreAuthenticatedLogin class. - please don't change code that is not directly related to this issue. this should be done along with a dedicated separate issue such that we properly track potential regressions; in particular when changing the from returning false to throwing an exception will have an impact on how the loginmodule behaves in the chain -> lets put these parts of the patch to another issue. - you don't need UserConfigurationImpl -> rather use the interface - isPasswordExpired: > you can simplify the test if the user is admin by calling User.isAdmin > for consistency of coding style: in oak we don't declare local fields final unless mandated by the compiler. > you don't use the default value for max-age and initial-change you defined. > i would only read the last-mod value if you are actually interested. > new Calendar vs new Date... ? > not sure about the expiration itself and the comparison of the millis. is it wise to expire the pw based on millis? it would feel more natural to me to expire it based on days. (-> tests needed) > IMO the forceinitialpw-change only makes sense if max-age is set to something else than 0... so, i think you can drop the test for force-initial-pw here altogether (-> see also UserManagementImpl) - getPasswordLastModified: > i would not retrieve the tree again to read the last-mod property. you can obtain it from the user if it's the default UserImpl or otherwise you can use the user-api to get the info. > IMO you have bug when retrieving the long value from the property: it's not multivalued and this method would IMO fail. AbstractSecurityTest -------------------------------------------------------------------------------- - please don't introduce dependencies to *Impl classes (the createUser method) - second i somewhat dislike the method createUser(String, UserManager) as it will cause wired test setup that has users associated with different user managers depending on how you create them. - the proper way to do this would be to override the security setup and make sure you expose your usermanager in the user config. i.e. moving these tests into separate test classes UserAuthenticationTest -------------------------------------------------------------------------------- - i would not mix the userauthentication test with testing if the factory works properly -> that needs an extra test class - same as above: please don't change tests that are directly related to the issue - the testsetup for the custom stuff is a bit hacky (see above). i would rather refactor these test in a separate test class with a proper setup. - you need to remove the users created during the tests... but IMO you can simplify this a lot and just use the test user. - imo root.commit should be added when you change the users password in the test - the tests that verify if changing the pw helps doen't need to perform the test again that verifies if authentication fails without changing it. UserManagerImpl -------------------------------------------------------------------------------- - you don't use the default values for Initial-Change - IMO it's a quite ugly that you set last modified and then remove it again upon creation. i would rather test for the tree being new and make the test in the setPassword method instead. - setPasswordLastModified: > new Calendar vs new Date ... (?) > i would only add the extra pw stuff if MAX_AGE is > 0, which is the default. in other words: this feature is disabled by default and i would not write a lot of content that is by default not needed at all. since this features is just introduced now a given oak-repo instance will most probably have user accounts that don't have the last-mod set. the same is true for those repositories that later on change the max-age value to something else than 0. the proper way IMO is that accounts without the extra pw-content become expired as soon as the feature is turned on... that's what you do in UserAuthentication anyway :-) > you ignore admin in the evaluation -> should do the same here > use NodeUtil.getOrAddChild > why do you set the last-modified property as type DATE? you only read it as long and thus cause extra conversion without any need... just make it Type.LONG - UserManagerImplTest should be extended to contain test-cases for the new functionality covered by setPassword. LoginModuleImpl -------------------------------------------------------------------------------- - possible NPE in getUserAuthenticationFactory. getSecurityProvider may return null - possible NPE when creating the UserAuthentication. getRoot may return null - it would be better to keep the changes minimal... otherwise it's much harder to make the review -> see proposed changes UserImporter -------------------------------------------------------------------------------- - UserImporter will fail for users that have the new rep:pw childnode - we need test cases for this.