JdbcUserDetailsManager does not implements UserDetailsPasswordService

[hbourada]

Summary

JdbcUserDetailsManager does not implements UserDetailsPasswordService like InMemoryUserDetailsManager

Actual Behavior

We ca not upgrade password encoding when using JdbcUserDetailsManager

Expected Behavior

JdbcUserDetailsManager is not easy to extends because sql fields are private as createUserSql and not getters for them.

Configuration

Version

5.2

Sample

[jzheaux]

Thanks for the report, @hbourada.

It does make sense to implement UserDetailsPasswordService for upgrading passwords in a database; however, this is risky specifically for JdbcUserDetailsManager from a backward compatibility standpoint. If JdbcUserDetailsManager were to implement UserDetailsPasswordService, it would break applications whose password column was too narrow to accept the newly encoded password.

You might consider something like:

@Component
public class MyJdbcPasswordService implements UserDetailsPasswordService {
    private final JdbcUserDetailsManager jdbc;

    public MyJdbcPasswordService(JdbcUserDetailsManager jdbc) {
        this.jdbc = jdbc;
    }

    public UserDetails updatePassword(UserDetails user, String newPassword) {
        UserDetails newUser = new User(user.getUsername(), newPassword, ...
        this.jdbc.updateUser(newUser);
        return newUser;
    }
}

And then exposing your JdbcUserDetailsManager instance as a bean.

Will this approach work for you?

[hbourada]

Thank's for your response
The same problem occurs with the suggested code if password column is too narrow to accept the new encoded password.
We can make a defensive approach if we can't update to the new encoded password and log a warning to tell user that UserDetailsPasswordService was not able to update password and let previous one.
Regards

[jzheaux]

@hbourada Agreed that the provided code doesn't work if the column is too narrow; that's why it's not going to be added into Spring Security.

If your password column is too narrow, I'd recommend widening it in order to accept the encoded value. Either way, it sounds like you've got a solution, but feel free to continue posting to the ticket if you feel like there is more to discuss.

[hbourada]

I didn't saw the retro-compatibility problem, for that we can have an enabled flag which defaulted to false which give chance to consumer to activate this option.
I attach a version of modified version of JdbcUserDetailsManager
JdbcUserDetailsManager.txt

[jzheaux]

Thanks for sharing, @hbourada, though I guess I'm not quite understanding. While a flag may be an option, I don't see this as an improvement over delegation.

For example, a simpler rendition of the earlier snippet is:

@Bean 
UserDetailsPasswordService userDetailsPasswordService(UserDetailsManager manager) {
    return (user, newPassword) -> {
        UserDetails newUser = new User(user.getUsername(), newPassword, ...
        manager.updateUser(newUser);
        return newUser;
    }
}

Though a flag would be fewer lines of code, compare this to the fact that with a flag, the developer would need to know that, even though JdbcUserDetailsService implements UserDetailsPasswordService, they need to also set a flag for it to actually work. This is scary when considering the fact that if the developer forgets to set the flag, passwords will silently not get updated.

[hbourada]

You said:
It does make sense to implement UserDetailsPasswordService for upgrading passwords in a database; however, this is risky specifically for JdbcUserDetailsManager from a backward compatibility standpoint.
and:
Though a flag would be fewer lines of code, compare this to the fact that with a flag, the developer would need to know that, even though JdbcUserDetailsService implements UserDetailsPasswordService, they need to also set a flag for it to actually work
The flag is for retro-compatibility to not break old projects.
AsInMemoryUserDetailsManager implements UserDetailsPasswordService it's more coherent to do it also for JdbcUserDetailsManager IMHO.

[jzheaux]

Agreed, @hbourada, that the flag addresses the compatibility issue. It raises security issues, though, which is why I don't think it should be added. Thank you for the suggestion, though.

[onlinebaba]

Thanks for the report, @hbourada.

It does make sense to implement UserDetailsPasswordService for upgrading passwords in a database; however, this is risky specifically for JdbcUserDetailsManager from a backward compatibility standpoint. If JdbcUserDetailsManager were to implement UserDetailsPasswordService, it would break applications whose password column was too narrow to accept the newly encoded password.

You might consider something like:

@Component
public class MyJdbcPasswordService implements UserDetailsPasswordService {
    private final JdbcUserDetailsManager jdbc;

    public MyJdbcPasswordService(JdbcUserDetailsManager jdbc) {
        this.jdbc = jdbc;
    }

    public UserDetails updatePassword(UserDetails user, String newPassword) {
        UserDetails newUser = new User(user.getUsername(), newPassword, ...
        this.jdbc.updateUser(newUser);
        return newUser;
    }
}

And then exposing your JdbcUserDetailsManager instance as a bean.

Will this approach work for you?

I tried this approach and got a constraint violation:

ERROR 23138 --- [readPoolQueue-2] c.c.a.q.h.AuthEventHandlingService : PreparedStatementCallback; SQL [insert into authorities (username, authority) values (?,?)]; Unique index or primary key violation: "PUBLIC.IDX_USERNAME_AUTHORITY_INDEX_A ON PUBLIC.AUTHORITIES(USERNAME, AUTHORITY) VALUES 11"; SQL statement: insert into authorities (username, authority) values (?,?) [23505-200]; nested exception is org.h2.jdbc.JdbcSQLIntegrityConstraintViolationException: Unique index or primary key violation: "PUBLIC.IDX_USERNAME_AUTHORITY_INDEX_A ON PUBLIC.AUTHORITIES(USERNAME, AUTHORITY) VALUES 11"; SQL statement: insert into authorities (username, authority) values (?,?) [23505-200]

I am now trying to explore a workaround for this.

[jzheaux]

I think something else might be going on with your code, @onlinebaba. updateUser first deletes all authorities associated with a user and then reinserts them. Perhaps the deletes are failing first.

You might consider asking your question on StackOverflow for increased visibility.