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

Comment From: 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?

Comment From: 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

Comment From: 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.

Comment From: 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

Comment From: 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.

Comment From: 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.

Comment From: 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.

Comment From: 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:

```java @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.

Comment From: 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.

Comment From: geirhe

@jzheaux Thank you for pointing me in this direction.

The concerns here are valid, but having the authorization server do one thing and the core security library quite another is kind of a security issue in itself, or at least bad developer experience. I can do without that kind of surprise.

What about making a JdbcPasswordUpdatingUserDetailsManager (yeah, I suck at naming) which does the above extension, adding some pointers in the documentation that suggests that as the sensible default, and then considering a deprecation of JdbcUserDetailsManager in favour of the new class, with a migration help that says what might break during the migration?

I would be happy to help, so manpower should not be a concern. The inconsistent target behaviour between the different bits here gets on my wick.

Comment From: jzheaux

I'm open to the idea of deprecation, @geirhe. Maybe we call it JdbcUserPasswordDetailsManager. Would you consider submitting a new PR that introduces the new class?

Comment From: geirhe

Sure, just give me a chance to get over the autumn sniffles first. Head doesn't work all that well right now.