Make JdbcUserDetailsManager to be able to handle UserDetails'es: nonLocked, nonExpired, credentialsNonExpired

paul-ovchinnikov commented 6 years ago

Summary

JdbcUserDetailsManager has excellent native implementation CRUD and utility methods to handle User state (groups, authorities, roles and so on)

org.springframework.security.core.userdetails.User and org.springframework.security.core.userdetails.UserDetails also determines additional states:

accountNonExpired
accountNonLocked
credentialsNonExpired

Actual Behavior

At the moment org.springframework.security.provisioning.JdbcUserDetailsManager and org.springframework.security.core.userdetails.jdbc.JdbcDaoImpl only can handle enabled state of the User

On the other hand oauth2 Spring implementation is able to handle this states already. But security framework returns default hardcoded values for accountNonExpired, accountNonLocked, credentialsNonExpired (true, true, true)

Expected Behavior

Please extend org.springframework.security.provisioning.JdbcUserDetailsManager and org.springframework.security.core.userdetails.jdbc.JdbcDaoImpl to support all UserDetails and User states. And make private methods to be public:

private void insertUserAuthorities(UserDetails user)
private void deleteUserAuthorities(String username)

Version

Spring Framework: 4.3.8.RELEASE Spring Security Framework: 4.2.3.RELEASE

avaud commented 6 years ago

We run into this issue at work, it's problematic for us and it seems quite simple to fix, is there any reason it was never addressed ? Would a pull request fixing it be accepted ?

Thanks !

restourgie commented 5 years ago

Same issue here. I went around it by extending the JdbcUserDetailsManager. However then I run into issues with private validation functions: private void validateUserDetails(UserDetails user) and private void validateAuthorities(Collection<? extends GrantedAuthority> authorities). I would rather see them protected.

rwinch commented 5 years ago

Thanks for the ping @123raoul123 If someone wants to send a PR (the changes must be passive) for this, we'd gladly accept it. If anyone wants to work on it, please claim it via the comments

pvliss commented 5 years ago

Hello @rwinch, I would like to take this one

rwinch commented 5 years ago

Thank you for volunteering @pvliss! The issue is all yours. Please let us know if you have any questions!

pvliss commented 5 years ago

Hello @rwinch,

Thanks!!! I have already actually created a first draft commit at my fork although I do have some questions that I need your help with before I submit the PR:

So since the changes need to be passive I took the simplest approach and that is to add a boolean flag(enableLockingAttributes) to JdbcUserDetailsManager similar to the flags used by JdbcDaoImpl. Is that ok with you or do you think we should take another approach (e.g. add a subclass)? Also no changes have been made to the default DB schema as I guess that client code is expected to do that manually?
Following the above I have also added a new method to JdbcUserDetailsManagerConfigurer and a new attribute to the XSD. Are these ok? My main concern is that I had to generate a new XSD.
At SecuirtyNamespaceHandler.matchesVersionInternal() I made a change to allow for usage of 5.x version. Also the InMemoryXmlApplicationContext is fixed to version 4.2, is that by purpose?
Finally I have also made the API access modifier changes requested. I also think that there is no need for the methods to be private as someone might want to simply add/remove user authorities instead of having to update the whole user entity and for the validation methods they make it easier to extend. What do you think?

Thanks for all your hard work!!!

Regards,

rwinch commented 5 years ago

Thanks for putting this together @pvliss!

I think it might be easier if we specify the contract as if the column count is larger than 3, then it will get the accountNonExpired, credentialsNonExpired, accountNonLocked attributes. Then in the RowMapper we can check the column count and if it is bigger than three use the additional properties.

pvliss commented 5 years ago

Hello @rwinch,

Thanks for taking a look.

While your suggestion/solution requires no additional configuration I think that it is not safe as there might be cases where it fails at run-time as an app might already have more columns on the user table and at the same time not fully customize JdbcUserDetailsManager. At least I did have some extra columns at that table at my last application (e.g. user prefs stored as JSON text). So in case we rely on checking the schema instead of using a flag then I think it would be best to check for actual columns instead of a simple column count. What do you think?

What about other users? @paul-ovchinnikov, @avaud , @123raoul123 any comments

Regards,

avaud commented 5 years ago

I don't understand why you would retrieve the user prefs in that context since it won't be mapped anyway. For the sake of simplicity I would have gone with @rwinch solution, but I'm not really an expert in Spring so I might miss a point here.

pvliss commented 5 years ago

Hello @avaud

You misunderstood me or maybe I was not clear enough.

I did not say I would retrieve the user prefs in that context, its obviously not needed. What I meant was that I had more columns added in the users DB table for other custom logic of my app related to the user entity and I did not want to have an extra table. Hence, in such a scenario, doing a simple column count on the table would assume that the extra columns needed by Spring Security would be available while in reality they would not be there and result in an error at runtime.

This could of-course be caught in your test run but that would be yet another assumption.

I think that checking the schema with the help of DatabaseMetaData#getColumns() would be a safer solution. Ofcourse will be checking just the existence of a single column for simplicity and once at startup to avoid possible performance issues.

Will create a new POC soon

Regards,

avaud commented 5 years ago

Hello,

I get that but if you use ResultSetMetaData: ` ResultSetMetaData rsmd = rs.getMetaData();

int columnsNumber = rsmd.getColumnCount(); ` Then you would get the number of columns selected in your query, not the number in the table. It could fail if someone has a "select * " as a query but I think it's acceptable since it's not really a good practice to do that.

pvliss commented 5 years ago

Hey, thanks for helping out.

Yes, you are right but in order to use ResultSetMetaData I would need to execute the proper select query first. So if there's no flag then I would have to use a select * from users in order to find out the column count. Using a query with predefined columns e.g. select username,password,enabled, acc_locked, acc_expired, creds_expired from users where username = ? would cause an error and never reach the mapping phase anyways.

Am I missing something obvious here?

avaud commented 5 years ago

I don't get why it would cause an error since it's written by a dev who should know the columns in the DB. But since @rwinch seemed to have an idea on how to do that, I suggest you wait for his advice. He will probably know better than me.

pvliss commented 5 years ago

So I am guessing you are talking about the case where someone also overrides the default queries. In that case, yes it would not be such a problem.

I guess you are right, I will wait for @rwinch's advice.

Nevertheless, I have taken another shot at it using DatabaseMetaData: https://github.com/pvliss/spring-security/commit/3f23fb54a1c90d0eab7fd28b52d3748bfbc096ea

avaud commented 5 years ago

Well I did not know there was default queries... That explains the confusion.

rwinch commented 5 years ago

I'm not sure I understand the concern. Spring Security does not have select * by default. It specifically lists out the columns it wants in the order it wants them.

The JdbcUserDetailsManager and JdbcDaoImpl both already have a requirement on the order the columns are returned, so users who customize it with select * are already asking for trouble. Optionally processing additional columns being returned in the query seems to fit in with how it works already quite nicely.

pvliss commented 5 years ago

Hello Rob. Thanks for your advice. So if I understood correctly we should not care about the default query and only have the user define custom queries in order to enable these attributes in a pre-defined order. Is that right?

Please see my latest commit using that approach and let me know if that is what you had in mind.

rwinch commented 5 years ago

@pvliss Thanks for the fast turnaround. Sorry I wasn't clear about leaving the default query the same. We need to do this for passivity. Only user's needing the new functionality would leverage this by changing the query.

Yes this is what I had in mind. A few pieces of feedback:

Please leave existing tests the same. This demonstrates we haven't broken anything. Instead, please add some new tests to cover the new use cases.
We want to ensure that the order of the properties stays consistent. If they query returns more results, then we just add column parameters. So for example, rather than moving the username to column of 6, we would always use column of 3 and shift all the boolean properties to the end.
Make sure to squash your commits
In general, I expect the only changes in the main classes is to have a check for the column count. If it is bigger that the current column count supported it will get the boolean properties from the result and use those, otherwise it will behave as it did.

pvliss commented 5 years ago

@rwinch No problem. Glad we are on the same page now. Should have asked for more clarifications before going on to implementing things.

Now to answer your points:

Do you mean I should add a new class or just new methods? If the latter then I have not changed any existing tests and I confirmed by comparing to the current master. If you are referring to the renaming of method addAccLockingColumns to setUpAccLockingColumns then I have added this method in my second commit and is shared by both createUserInsertsCorrectDataWithLocking and updateUserChangesDataCorrectlyAndClearsCacheWithLocking which are also new test methods added by me. Maybe when I squash the commits this will become easier to see.
The example you are referring to is an update query where the last parameter is the user name ,e.g update users set password = ?, enabled = ?, acc_locked=?, acc_expired=?, creds_expired=? where username = ? so I am forced to set username as the 6th parameter. Again , I am missing something here?

pvliss commented 5 years ago

@rwinch So I squashed the commits, hopefully the changes are more easier to spot now.

Regarding your last point

In general, I expect the only changes in the main classes is to have a check for the column count.

Does that mean I should also revert my change from anonymous classes to lambdas as well or are you fine with them?

rwinch commented 5 years ago

So I squashed the commits, hopefully the changes are more easier to spot now.

Thanks!

Does that mean I should also revert my change from anonymous classes to lambdas as well or are you fine with them?

I think they are ok changes, but not as a part of this issue. We want to keep changes isolated to the ticket in the commit so that we can track the changes. By keeping changes isolated we make thinks like backporting and tracking down issues a lot easier.

Do you mind submitting this as a PR so it is easier to provide feedback? Commenting on the commit's isn't ideal since the feedback is lost if your branch is removed, forced push occurs, etc.

pvliss commented 5 years ago

We want to keep changes isolated to the ticket in the commit so that we can track the changes. By keeping changes isolated we make thinks like backporting and tracking down issues a lot easier. Do you mind submitting this as a PR so it is easier to provide feedback?

Yes, I understand. I have reverted all lambda changes, squashed again and submitted the PR.

I think they are ok changes, but not as a part of this issue.

Should I create an issue just for these JDBC related classes or a more generic one for the whole project? IDEA reports 54 such cases and 90 incl. tests.

rwinch commented 5 years ago

Yes, I understand. I have reverted all lambda changes, squashed again and submitted the PR.

Thank you.

Should I create an issue just for these JDBC related classes or a more generic one for the whole project? IDEA reports 54 such cases and 90 incl. tests.

Thank you. Yes that would be good. It might help (so we don't need to rebase) to wait till these changes get merged first.

pvliss commented 5 years ago

Thank you. Yes that would be good. It might help (so we don't need to rebase) to wait till these changes get merged first.

Just to avoid any misunderstandings I will ask again.

A generic one(i.e. convert anonymous classes to lambdas throughout the whole project) or a JDBC code specific one?

spring-projects / spring-security