Bug 63505

Summary: enhancement - support of stored procedures for DataSourceRealm authentication
Product: Tomcat 9 Reporter: Eugène Adell <eugene.adell>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: NEW ---    
Severity: enhancement CC: eugene.adell
Priority: P2 Keywords: PatchAvailable
Version: 9.0.x   
Target Milestone: -----   
Hardware: All   
OS: All   
Attachments: a new class implementing this enhancement
refactoring as suggested in comment 6

Description Eugène Adell 2019-06-14 21:12:41 UTC
hello,

I would like to add a new way to authenticate on a DataSourceRealm, using stored procedures instead of identifying tables and column names.

I don't want to reopen a new debate about pros/cons on statements vs StoPro but for any service user being confined to only use StoPro, it's a fact it won't be able to use the DataSourceRealm as it is implemented now.
Comment 1 Eugène Adell 2019-06-14 21:22:27 UTC
Created attachment 36628 [details]
a new class implementing this enhancement

This class is based on DataSourceRealm source code (copy/paste of this file, and adding/removing what seemed to be added/removed to me). It uses 2 StoPro (obviously one for the credentials, one for the roles).

The StoPro names are given in the context file, for example :

<Realm className="org.apache.catalina.realm.DataSourceViaSPRealm"
  digest="SHA"
  dataSourceName="jdbc/dbdemo"
  userProcStoc="findPasswordForUser"
  roleProcStoc="findRolesForUser"
  localDataSource="true" />

The 2 procedures must of course be implemented on the DB, and respectively return the password and the roles associated to the username sent as an argument.

My tests are OK (Tomcat 9.0.21, MySQL 5.7.18, mysql-connector 5.1.40). Once the SELECT grant revoked, the users cannot login anymore when using the DataSourceRealm, and this new class DataSourceViaSPRealm allows logging in.
Comment 2 Christopher Schultz 2019-06-17 21:47:49 UTC
What are userProcStoc and roleProcStoc abbreviations for? Those names look weird.
Comment 3 Eugène Adell 2019-06-18 05:32:15 UTC
The first one retrieves the user's password, the second the user's roles.

Better ideas to names these attributes are of course welcome.
Comment 4 Christopher Schultz 2020-06-24 20:34:39 UTC
There might be some refactoring that could help, here. It seems that mostly you are overriding the getPassword(Connection,String) and getRoles(Connection,String) methods from DataSourceRealm, right?
Comment 5 Eugène Adell 2020-07-25 16:49:42 UTC
Exactly, both files are the same except I needed to override these 2 functions and also add variables for handling the procedures names (and what comes with them : getters/setters of course). Maybe making it a child class of DataSourceRealm, but would it be really clean this way ?
Comment 6 Christopher Schultz 2020-07-27 16:17:40 UTC
(In reply to Eugène Adell from comment #5)
> Exactly, both files are the same except I needed to override these 2
> functions and also add variables for handling the procedures names (and what
> comes with them : getters/setters of course). Maybe making it a child class
> of DataSourceRealm, but would it be really clean this way ?

I was thinking maybe creating DataSourceRealmBase and pulling all the shared capabilities between DataSourceRealm and your class up into that class. In fact, I might even make DataSourceRealm a trivial subclass of a new DataSourceViaPSRealm class which extends DataSourceRealmBase and contains the code to perform the authentication via PreparedStatements.

Like this:

DataSourceRealmBase
    /       \
DSViaPS    DSViaSP
   |
DataSourceRealm

I'd do this because DataSourceRealm isn't a good name for what it does since your realm is also a "DataSourceRealm". But we can't just remove classes from Tomcat since they could be used as base classes by other code.

There is probably scope to refactor this set of classes and also the JDBCRealm (because you can authenticate a DS realm using either PS or SP), although JDBCRealm should probably just die so it's not really worth it.
Comment 7 Mark Thomas 2020-08-10 15:13:00 UTC
I agree we need to deprecate the JDBCREalm although that is a little off-topic here.

I think we need:
- a (non-abstract?) base class that uses PreparedStatements
- a concrete implementation as per the current DataSourceRealm that extends the base class and builds the PreparedStatements from table and column names
- a concrete implementation that extends the base class and builds the PreparedStatements from stored procedure names (since CallableStatement extends PreparedStatement)

Having all three classes as concrete realm implementations would allow maximum flexibility (and maximum opportunity to shoot yourself in the foot).
Comment 8 Eugène Adell 2020-08-27 15:16:43 UTC
Created attachment 37407 [details]
refactoring as suggested in comment 6

This zip contains 4 Java files corresponding to the suggestion made by Christopher in Comment 6

Source code from 9.0.37

Some hard-coded logging was changed with this.getClass().getSimpleName() as it's in fact the child classes which are logging and not the intermediary class which is introduced here.

I tested both realms with good results.