[#APF-1071] LazyLoading filter turned on break the ability to update a password.

AppFuse

LazyLoading filter turned on break the ability to update a password.

Created: 19/May/08 09:13 AM Updated: 01/Oct/08 09:30 AM

Component/s:

Persistence Layer, Security

Affects Version/s:

2.0.1

Fix Version/s:

2.1

Description

« Hide

Hi Matt et.al.,

Found a serious and strange issue.

1. Turn on the LazyLoading filter in web.xml.
2. Log in as admin
3. Update users password.

-> Cleartext password gets saved in database.

When i tried to debug this problem it seems that hibernate (or some other interceptor?) stores the User object to database (with cleartext passwords) even before the UserManagerImpl.saveUser(User user) gets called.

----- SECTION FROM UserManagerImpl.java ------------

    // Get and prepare password management-related artifacts
        boolean passwordChanged = false;
        if (authenticationProvider != null) {
            PasswordEncoder passwordEncoder = authenticationProvider.getPasswordEncoder();

            if (passwordEncoder != null) {
                // Check whether we have to encrypt (or re-encrypt) the password
                if (user.getVersion() == null) {

                    // New user, always encrypt
                    passwordChanged = true;
                } else {
                    // Existing user, check password in DB
                    String currentPassword = dao.getUserPassword(user.getId()); <---- Here is password in database already new cleartext pw.

                    if (currentPassword == null) {
                        passwordChanged = true;
                    } else {
                        if (!currentPassword.equals(user.getPassword())) {
                            passwordChanged = true;
                        }
                    }
                }

                // If password was changed (or new user), encrypt it
                if (passwordChanged) {
                    user.setPassword(passwordEncoder.encodePassword(user.getPassword(), null));
                    log.warn("encodeedd="+user.getPassword());

                }
            } else {
                log.warn("PasswordEncoder not set on AuthenticationProvider, skipping password encryption...");
            }
        } else {
            log.warn("AuthenticationProvider not set, skipping password encryption...");

        }

----------------------------------------------

Note that i had to change the dao.getUserPassword to take user-id instead of username to be able to update peoples login-name (another bug!).

Description

Hi Matt et.al., Found a serious and strange issue. 1. Turn on the LazyLoading filter in web.xml. 2. Log in as admin 3. Update users password. -> Cleartext password gets saved in database. When i tried to debug this problem it seems that hibernate (or some other interceptor?) stores the User object to database (with cleartext passwords) even before the UserManagerImpl.saveUser(User user) gets called. ----- SECTION FROM UserManagerImpl.java ------------ // Get and prepare password management-related artifacts boolean passwordChanged = false; if (authenticationProvider != null) { PasswordEncoder passwordEncoder = authenticationProvider.getPasswordEncoder(); if (passwordEncoder != null) { // Check whether we have to encrypt (or re-encrypt) the password if (user.getVersion() == null) { // New user, always encrypt passwordChanged = true; } else { // Existing user, check password in DB String currentPassword = dao.getUserPassword(user.getId()); <---- Here is password in database already new cleartext pw. if (currentPassword == null) { passwordChanged = true; } else { if (!currentPassword.equals(user.getPassword())) { passwordChanged = true; } } } // If password was changed (or new user), encrypt it if (passwordChanged) { user.setPassword(passwordEncoder.encodePassword(user.getPassword(), null)); log.warn("encodeedd="+user.getPassword()); } } else { log.warn("PasswordEncoder not set on AuthenticationProvider, skipping password encryption..."); } } else { log.warn("AuthenticationProvider not set, skipping password encryption..."); } ---------------------------------------------- Note that i had to change the dao.getUserPassword to take user-id instead of username to be able to update peoples login-name (another bug!).

Show »

All

Comments

Change History

FishEye

Sort Order:

[ Permlink | « Hide ]

Matt Raible - 19/May/08 09:18 AM

Can you please verify this issue exists in 2.0.2?

[ Permlink | « Hide ]

Henrik Ekblad - 19/May/08 09:50 AM

I have just created a new 2.0.2 project fron scratch and activated LazyLoading. Same problem in 2.0.2.

My settings:

mvn archetype:create -DarchetypeGroupId=org.appfuse.archetypes \
-DarchetypeArtifactId=appfuse-basic-struts \
-DremoteRepositories=http://static.appfuse.org/releases \
-DarchetypeVersion=2.0.2 \
-DgroupId=com.its.aims \
-DartifactId=aims

[ Permlink | « Hide ]

Henrik Ekblad - 20/May/08 02:40 AM

I got created an work-around by narrowing down LazyLoading to the servlet that actually need it.

    <filter-mapping>
        <filter-name>lazyLoadingFilter</filter-name>
        <url-pattern>/kml</url-pattern>
    </filter-mapping>

But the problem still exist for everyone enabling this filter for the whole application.

Best regards
Henrik

[ Permlink | « Hide ]

John Mark Suhy - 27/Aug/08 05:56 PM

I ran into the same issue I fixed it by un-commenting the read-only setting in the applicationContext-service.xml
I've got to test the rest of our portal to figure out if that broke anything. I don't have time to test it on an "out of the box" appfuse setup this week.

  <tx:advice id="txAdvice">
        <tx:attributes>
            
            
            <tx:method name="get*" read-only="true"/>
            <tx:method name="*"/>
        </tx:attributes>
    </tx:advice>

Anyone want to comment on how this could effect other areas of the portal?

[ Permlink | « Hide ]

Matthew Jones - 17/Sep/08 02:50 PM

Tracked this down a little further in 2.0.1. The issue is being caused by the following code in the onSubmit method of the UserFormController:

-------- SECTION FROM onSubmit in UserFormController ---------

    if (request.isUserInRole(Constants.ADMIN_ROLE)) {
        String[] userRoles = request.getParameterValues("userRoles");
        if (userRoles != null) {
            user.getRoles().clear();
            for (String roleName : userRoles) {
                user.addRole(roleManager.getRole(roleName));
            }
        }
    }

If the lazy load filter is enabled then the in-view transaction is commited after user.addRole is called persisting the new data before getUserManager().saveUser(user) is called.

If you are not running embedded Appfuse you can remove the above code from the onSubmit method and add the follow to the last else clause in the formBackingObject method:

------------ SECTION FROM formBackingObject method in UserFormController --------------------

   else if (request.getParameter("id") != null && !"".equals(request.getParameter("id"))
&& request.getParameter("cancel") == null) {

// fetch roles here before getting user from database into hibernate cache
        Set<Role> roles = new HashSet<Role>();
        if (request.isUserInRole(Constants.ADMIN_ROLE)) {
            String[] userRoles = request.getParameterValues("userRoles");

            if (userRoles != null) {
                for (String roleName : userRoles) {
                    Role _role = roleManager.getRole(roleName);
                    roles.add(_role);
                }
            }
        }

        // populate user object from database, so all fields don't need to be hidden fields in form
        User user = getUserManager().getUser(request.getParameter("id"));
        if (request.isUserInRole(Constants.ADMIN_ROLE)) {
            user.setRoles(roles);
        }
        return user;
    }

That way you populate the roles before the form object is bound avoiding the commit.

[ Show » ]

Matthew Jones - 17/Sep/08 02:50 PM Tracked this down a little further in 2.0.1. The issue is being caused by the following code in the onSubmit method of the UserFormController: -------- SECTION FROM onSubmit in UserFormController --------- if (request.isUserInRole(Constants.ADMIN_ROLE)) { String[] userRoles = request.getParameterValues("userRoles"); if (userRoles != null) { user.getRoles().clear(); for (String roleName : userRoles) { user.addRole(roleManager.getRole(roleName)); } } } If the lazy load filter is enabled then the in-view transaction is commited after user.addRole is called persisting the new data before getUserManager().saveUser(user) is called. If you are not running embedded Appfuse you can remove the above code from the onSubmit method and add the follow to the last else clause in the formBackingObject method: ------------ SECTION FROM formBackingObject method in UserFormController -------------------- else if (request.getParameter("id") != null && !"".equals(request.getParameter("id")) && request.getParameter("cancel") == null) { // fetch roles here before getting user from database into hibernate cache Set<Role> roles = new HashSet<Role>(); if (request.isUserInRole(Constants.ADMIN_ROLE)) { String[] userRoles = request.getParameterValues("userRoles"); if (userRoles != null) { for (String roleName : userRoles) { Role _role = roleManager.getRole(roleName); roles.add(_role); } } } // populate user object from database, so all fields don't need to be hidden fields in form User user = getUserManager().getUser(request.getParameter("id")); if (request.isUserInRole(Constants.ADMIN_ROLE)) { user.setRoles(roles); } return user; } That way you populate the roles before the form object is bound avoiding the commit.

[ Permlink | « Hide ]

Yaozong Zhu - 01/Oct/08 09:30 AM

I've met the same issue when uncommenting the "Open Session In View" filter in web.xml.

What I did to fix it was to change formBackingObject method described below. I commented the logic to load USER for submission request, so the command object won't be known by persistence context (or hibernate session), thus the sql to lookup roles won't trigger an update sql for user. As to the original comment there, I believe every field of user has been properly binded to UI field.

    protected Object formBackingObject(HttpServletRequest request)
    throws Exception {

        if (!isFormSubmission(request)) {
            String userId = request.getParameter("id");

            User user;
            if (userId == null && !isAdd(request)) {
                user = getUserManager().getUserByUsername(request.getRemoteUser());
            } else if (!StringUtils.isBlank(userId) && !"".equals(request.getParameter("version"))) {
                user = getUserManager().getUser(userId);
            } else {
                user = new User();
                user.addRole(new Role(Constants.USER_ROLE));
            }

            user.setConfirmPassword(user.getPassword());

            return user;
// } else if (request.getParameter("id") != null && !"".equals(request.getParameter("id"))
// && request.getParameter("cancel") == null) {
// // populate user object from database, so all fields don't need to be hidden fields in form
// return getUserManager().getUser(request.getParameter("id"));
        }

        return super.formBackingObject(request);
    }