FWD » Core Development » Database

• there must be exactly one database connected at compile time to use the userid and setuserid without specifying ldbname.
• if it is disconnected programatically, the functions will return empty string respectivelly false, but no error will break the procedure. Additionally, setuserid will issue 1072 warning in the message bar.
• if some other database will be connected during the execution of the procedure, the forms of the functions without ldbname will continue to attempt to query/set the database that was connected at the startup of the program, more likely the one that was connected at compile (I believe the name of the database is inlined in the compiled code). In P2J terms, we need to have an "default" database defined in the directory, that will be automatically connected at startup, I am not sure if we have such feature.

• if there are any records in _user table of the designated connected database and the user/pwd combination is wrong, then the userid remains unchanged and false is returned. No error or warning is generated.
• if there are no records in _user table, any attempt to set the userid will fail (return false), and set the userid to the user id extracted from the underlying OS.

• when _User table security is configured, the class that I believe will be in charge with storing the userid should be ConnectionManager. It already holds database-connection related information so I think the userid should also be stored as a Database field or a private mapping (another solution would be a contextual mapping(database - userid) in the newly implemented class - alternate security handler)
• the userid will simply return this cached value
• the setuserid will do all the hard work, including treating the exceptions case, querying the _user metatable and updating the current userid inside ConnectionManager.

I already started implementing much of the above but at this moment the missing piece is the access to _User metatable.

If the _user table of "default" database is populated (is not empty) and the -U -P command line parameters are not specified, the P4GL will automatically prompt the user with the following dialog:

 ┌─────────────────── Login ───────────────────┐
 │                                             │
 │ Please enter a User Id and Password for     │
 │ database: p2j_test                          │
 │                                             │
 │    User Id:                                 │
 │   Password:                                 │
 │                                             │
 │              <OK>   <Cancel>                │
 └─────────────────────────────────────────────┘

On cancel, the userid will be the empty string. Of course, if users enters a valid uid/pwd combination, the userid will be the one specified.

Ovidiu Maxiniuc wrote:

In P2J terms, we need to have an "default" database defined in the directory, that will be automatically connected at startup, I am not sure if we have such feature.

Well, sort of. Any non-trivial application would have the "load_at_startup" flag in the directory set to TRUE for its primary database. However, so far we have not set up an application with more than one database that is automatically connected at startup, so we haven't had to specify which one is the "default".

Code review 20130917a:

• I am a bit confused by the use of the Database class to hold the userid. This is not a threadsafe approach, since the same Database instance stored in a Persistence instance is available to all user sessions. You need a ContextLocal variable for this somewhere (or to add userid to some existing, context-local object); I don't think Database is the right place for it. Database as a class was meant to hold information describing a database. Instances of this class are used as tokens in various places throughout the code, but it wasn't intended as a session-level, operational class.
• When you need the unformatted String value from within a character object, please use toStringMessage rather than toJavaType.
• In the query within MetadataSecurityOps.setUserId, you have a comment about using the dialect for the query. I think what you're looking for is the use of P2JDialect.getProcessedCharacterColumnName for the column names in the WHERE clause, so they can be adjusted to allow the appropriate index(es) to be used by the query.
• What is the plan for default database? I realize using "p2j_test" is a placeholder.
• I assume the individual class-level import statements were added by your IDE during cut and paste, and you will consolidate them to the package level for your final update, in accordance with the coding standards.
• Although there is already heavy use of the Apache logging infrastructure in the persistence framework, for new code we are trying to move away from that dependency (and eventually we will remove it from old code, too). Please use com.goldencode.util.LogHelper instead. I'll add a separate history entry with a simple use example.

Eliminating the Apache logging dependency:

1. Replace:

/** Logger */
private static final Log LOG = LogFactory.getLog(MyClass.class);

/** Logger */
private static final Logger LOG = LogHelper.getLogger(MyClass.class);

if (LOG.isErrorEnabled())
{
   LOG.error("Error message", exc);
}

if (LOG.isLoggable(Level.SEVERE))
{
   LOG.log(Level.SEVERE, "Error message", exc);
}

The logging level names don't match up exactly, but Level has plenty of granularity.

• I am a bit confused by the use of the Database class to hold the userid.

Indeed, I was a little confused about the Contextmanager local-context, but it's clear that the Database objects are common. I created a new map for userids that is maintained in synch with the connected map of databases.

• In the query within MetadataSecurityOps.setUserId, you have a comment about using the dialect for the query. I think what you're looking for is the use of P2JDialect.getProcessedCharacterColumnName for the column names in the WHERE clause, so they can be adjusted to allow the appropriate index(es) to be used by the query.

Yes, this is 'in progress', I added a method in P2JDialect that returns directly the query needed for auth checking that takes the two strings as parameters and applies any needed processing in each implementation H2/PSQL.

• What is the plan for default database? I realize using "p2j_test" is a placeholder.

My first idea was to have a string entry somewhere in the directory for specifying this. This should reside somewhere in the database branch. On the other side, will it be possible to dynamically load a database like 4GL does by specifying the physical filename ? I searched into the customer code ASTs and I saw about ten or more CONNECT s that do that. I think we don't need to implement this as in the customer's code, the forms of the functions with default ldbname are not present. Unless for the sake of completeness.

• I assume the individual class-level import statements were added by your IDE during cut and paste, and you will consolidate them to the package level for your final update, in accordance with the coding standards.

This is true, I will fix this just before entering the testing stage,

• Although there is already heavy use of the Apache logging infrastructure in the persistence framework, for new code we are trying to move away from that dependency (and eventually we will remove it from old code, too).

I will do this as I update affected files.

I really needed some hints here. Thank you for your input.

• to manually add the AST of _User from standard schema to each databases' p2o ?
• more likely, start the PatternEngine with both p2o (p2j_test and standard) - or somehow standard.p2o automatically added in schema/import.xml to processing list and ignored everything else except for _User table ?
• the same should happen at conversion time in order to obtain a DDL file that contains the description for _User metatable.

Anyway, this is another issue here, the _User table has changed; the .d /.df pair I extracted today have a different number of fields. The good part is that we are only interested in the first few of them, the other that were incrementally added can be dropped from the import. Here is the table as it is now on testcases:

TABLE "_User" 
   FIELD "_Userid" AS character 
   FIELD "_Password" AS character 
   FIELD "_User-Name" AS character 
   FIELD "_U-misc1" AS integer 
   FIELD "_U-misc2" AS character 
   FIELD "_User-Misc" AS character 
   INDEX "_Userid" ON "_User" UNIQUE PRIMARY INDEX-FIELD "_Userid" ASCENDING 

      FIELD "_User_number" AS integer 
      FIELD "_Group_number" AS integer 
      FIELD "_Given_name" AS character 
      FIELD "_Middle_initial" AS character 
      FIELD "_Surname" AS character 
      FIELD "_Telephone" AS character 
      FIELD "_Email" AS character 
      FIELD "_Description" AS character 
      FIELD "_Disabled" AS logical 
      FIELD "_Create_date" AS datetime-tz 
      FIELD "_Account_expires" AS datetime-tz 
      FIELD "_Pwd_expires" AS datetime-tz 
      FIELD "_Pwd_duration" AS integer 
      FIELD "_Last_login" AS datetime-tz 
      FIELD "_Logins" AS integer 
      FIELD "_Max_logins" AS integer 
      FIELD "_Max_tries" AS integer 
      FIELD "_Login_failures" AS integer 
      FIELD "_Spare1" AS integer 
      FIELD "_Spare2" AS integer 
      FIELD "_Spare3" AS character 
      FIELD "_Spare4" AS character 

Ovidiu Maxiniuc wrote:

• I am a bit confused by the use of the Database class to hold the userid.

Indeed, I was a little confused about the Contextmanager local-context, but it's clear that the Database objects are common. I created a new map for userids that is maintained in synch with the connected map of databases.

I guess you mean ConnectionManager. Is the idea that the map tracks current userid by Database key?

• In the query within MetadataSecurityOps.setUserId, you have a comment about using the dialect for the query. I think what you're looking for is the use of P2JDialect.getProcessedCharacterColumnName for the column names in the WHERE clause, so they can be adjusted to allow the appropriate index(es) to be used by the query.

Yes, this is 'in progress', I added a method in P2JDialect that returns directly the query needed for auth checking that takes the two strings as parameters and applies any needed processing in each implementation H2/PSQL.

Any method you add to P2JDialect will have to be implemented for every dialect we support, so these must be added sparingly, only when necessary to implement fundamental differences between database flavors. These should not be helper methods for specific features if that work can be done elsewhere, using the building blocks provided by P2JDialect. Can't you build the query in MetadataSecurityOps, leveraging P2JDialect.getProcessedCharacterColumnName?

• What is the plan for default database? I realize using "p2j_test" is a placeholder.

My first idea was to have a string entry somewhere in the directory for specifying this. This should reside somewhere in the database branch.

Yes, this is a good idea.

On the other side, will it be possible to dynamically load a database like 4GL does by specifying the physical filename? I searched into the customer code ASTs and I saw about ten or more CONNECT s that do that. I think we don't need to implement this as in the customer's code, the forms of the functions with default ldbname are not present. Unless for the sake of completeness.

Any physical database name specified with the CONNECT statement will have to map to a database configuration in the directory. We may need some input from the customer here in order to extract the meaningful portion of the provided pdbname (i.e., which parts of the path are relevant and which can be ignored).

I really needed some hints here. Thank you for your input.

No problem. Good work so far.

Ovidiu Maxiniuc wrote:

I also start adding import support for _User meta-table.
I understand that this is a table like others from the database. I don't know what would be the best approach at import:

• to manually add the AST of _User from standard schema to each databases' p2o ?
• more likely, start the PatternEngine with both p2o (p2j_test and standard) - or somehow standard.p2o automatically added in schema/import.xml to processing list and ignored everything else except for _User table ?
• the same should happen at conversion time in order to obtain a DDL file that contains the description for _User metatable.

I was just making some changes to schema/fixups.xml today to ensure the metadata tables we know we always need for runtime support are always converted (i.e., _file, _field, _index, _index-field).

I think this also is the rule-set where we have to deal with the special requirements of the _user table. If the p2j.cfg.xml file specifies the _user table is in use, we should graft the _user table into each primary schema during conversion, though I am not sure of the best way to do this yet, as it will require loading the metadata .dict file and copying/grafting one sub-branch of it while already walking a primary database .dict file. I don't recall that we have that capability today.

We can use a schema hint (see Conversion Hints chapter of the P2J Conversion Reference) to drop (in p2o.xml) the fields which are not in use by a particular customer, so long as the fields dropped do not participate in any index (in this case, it will be safe to drop all of the unused fields, since only _Userid is referenced by an index). We currently only support the drop hint at the table level, so you will need to add support for a field-level drop hint. Assuming field-level drop hint support is implemented, this approach should ensure that all downstream conversion (DDL, DMOs, Hibernate mappings, etc.) and data import work naturally with the reduced table.

So, it looks like this task will involve some conversion tool enhancements.

Ovidiu Maxiniuc wrote:

...the _User table has changed; the .d /.df pair I extracted today have a different number of fields.

It appears you are using an old version of standard.df (probably v9.1e, from the testcases project) and expecting it to match data export from a newer installation (v10.2b) of Progress. As you have noticed, the metaschema can change between versions, though I think to avoid breakage in their customer's apps, they only add things, not take things away (note that I have not confirmed this). We have the correct .df file with the current customer project. Make sure any code and rule changes work with both. If you limit conversion to the fields in use as discussed above, it should be fine.

• the line numbers in (p2j_test - as sample db) ASTs of the _user table are those from where the were initially. Maybe an annotation srcfile should be added to this table to clarify them.
• a new dmo will be generated (interface/implementation/hbm) in the dmo/p2j_test with the name User. This was expected, but there could be a name conflict with the one from _meta. Forcing using p2j_test/_User is permitted by java, but not recommended. I think the last one (from _meta) should not exist any more, I am not aware if it is useful anymore.
• the DDLs are also not valid. Again, they try to create the user table; however user is a PSQL keyword si the DDLs are bad. Again, a solution would be to use the underscore. I have tested and H2 allows a table to be named User.

I tested P2J with a user-defined table named user. As expected, it created a countered dmo to differentiate between them.
I wonder if all above issues could be fixed by some conversion hints or hardcoding: by replacing the underscore with some other prefix when merging the table into database schema.

Ovidiu Maxiniuc wrote:

I altered the conversion so that the _user AST is copied from meta to each processed database if configured in p2j.cfg.xml (just at the moment of saving .schema file). The conversion finishes without errors. But at this moment I encountered some issues inspecting the generated output:

• the line numbers in (p2j_test - as sample db) ASTs of the _user table are those from where the were initially. Maybe an annotation srcfile should be added to this table to clarify them.

I like this idea, please add this.

• a new dmo will be generated (interface/implementation/hbm) in the dmo/p2j_test with the name User. This was expected, but there could be a name conflict with the one from _meta. Forcing using p2j_test/_User is permitted by java, but not recommended. I think the last one (from _meta) should not exist any more, I am not aware if it is useful anymore.

Right, this should not exist in _meta anymore.

• the DDLs are also not valid. Again, they try to create the user table; however user is a PSQL keyword si the DDLs are bad. Again, a solution would be to use the underscore. I have tested and H2 allows a table to be named User.

I tested P2J with a user-defined table named user. As expected, it created a countered dmo to differentiate between them.
I wonder if all above issues could be fixed by some conversion hints or hardcoding: by replacing the underscore with some other prefix when merging the table into database schema.

Let's hard-code the prefix "meta" for the _user table during name conversion. I was about to make this change also for the remaining metadata tables.

Are these results based on using a single-user (pro) or multi-user (mpro) database process?

I will let Eric reply in regard to the code. I will try to answer some questions.

_Should we emulate these parameters by adding equivalent items in the directory?

Yes, this is probably a good idea. BUT this would be something that could be different for each user session. So we must honor the multi-tiered lookup that is account, then group, then server, then default. The DirectoryManager already provides this capability so it should be easy to implement.

Does it make sense to quit the server application if values fails the validation against _user table ?

No.

if -U and -P command-line params are not used, the dialog from note 7 is displayed.
Should I implement in P2J too ? I did not notice something like it, I believe it does not exist yet.

Yes. Actually, we do have something similar. Please look at the following for an approach that will be close to what you need:

com/goldencode/p2j/security/DefaultLoginPanel.java
com/goldencode/p2j/security/SecurityManager.authenticateSingle()

If this is really a feature of the multi-user version (mpro) and not just pro, then we will need a more advanced approach than authenticateSingle(), but the same Authenticator approach should still work. I'm not sure if we will need the server-side auth hook but the client side will be needed for sure.

In addition, the security manager would need to know to process this session in _user mode instead of with the normal directory-backed authentication.

Look through this code and come up with some ideas. Document the approach here and we will discuss it.

I don't see any difference between the behaviors of pro and mpro executables.
I looked into launch scripts and there is extra the -1 parameter.

In mpro mode (without -1) there can be multiple client instances authenticated with the same userid. So the userid is not exclusive, it's just used for enforcing access-rights to data from respective database.

In pro mode (-1 supplied), only one client can access the database so exclusion is implicit.

Greg Shah wrote:

_Should we emulate these parameters by adding equivalent items in the directory?

Yes, this is probably a good idea. BUT this would be something that could be different for each user session. So we must honor the multi-tiered lookup that is account, then group, then server, then default. The DirectoryManager already provides this capability so it should be easy to implement.

So each user/group defined in directory.xml can have a [user/password] combination that will be used like -U -P params ? I'm not sure I got this right. This way we should have the possibility to connect separate databases for each group/user. In Progress, the -U/-P are related to a database to which the client is connecting.
Anyway, I think this gets a little too far from our target.

if -U and -P command-line params are not used, the dialog from note 7 is displayed.
Should I implement in P2J too ? I did not notice something like it, I believe it does not exist yet.

Yes. Actually, we do have something similar. Please look at the following for an approach that will be close to what you need:

com/goldencode/p2j/security/DefaultLoginPanel.java
com/goldencode/p2j/security/SecurityManager.authenticateSingle()

If this is really a feature of the multi-user version (mpro) and not just pro, then we will need a more advanced approach than authenticateSingle(), but the same Authenticator approach should still work. I'm not sure if we will need the server-side auth hook but the client side will be needed for sure.

In addition, the security manager would need to know to process this session in _user mode instead of with the normal directory-backed authentication.

Look through this code and come up with some ideas. Document the approach here and we will discuss it.

I started working on this but at this moment I feel like there is a missing link. The DefaultLoginPanel gets executed if the client share the same process with the server so it has in-process access to any data from server.
My approach was to add this piece of authentication to be on server-side. Something like a hook-dialog that gets executed before the client-entry point (execute()) if user/pwd is needed for authentication to database (if _user authentication is configured and _user is not empty). Is it so simple or I am missing something here ?

So each user/group defined in directory.xml can have a [user/password] combination that will be used like -U -P params ?

Yes. See Utils.getDirectoryNode*() which is used by the Directory interface with ID_RELATIVE for searching.

This way we should have the possibility to connect separate databases for each group/user. In Progress, the -U/-P are related to a database to which the client is connecting.

Yes, this is tricky. Can you pass multiple -U and -P options in Progress and somehow tell Progress which DBs they match?

Anyway, I think this gets a little too far from our target.

Basic directory support for one -U and one -P per account/group/server/default is easy. Multiple -U/-P is the hard part.

The DefaultLoginPanel gets executed if the client share the same process with the server so it has in-process access to any data from server.

Yes and no. Yes, this only gets used when the client and server share the same process. No, the DefaultLoginPanel does not access any server state directly. This could be run (without changes) as the Authenticator for a remote client. The trick (that is done for you without any extra effort on your part) is that the server will serialize this class and send it as a stream of bytes to the client at the proper point in the login processing. The client has a special class loader to take those bytes and create the class. It creates an instance and invokes the hook.

Yes, you probably need to provide a server-side hook too. That is the part could handle the _user integration.

The Authenticator interface is used for both the server and client side hooks.

My approach was to add this piece of authentication to be on server-side. Something like a hook-dialog that gets executed before the client-entry point (execute()) if user/pwd is needed for authentication to database (if _user authentication is configured and _user is not empty). Is it so simple or I am missing something here ?

Please use the hooks that already exist. They handle this properly today. The P2J Developer Guide has some more details on this.

Progress client allows multiple database connection at startup, the following command line is correct:

pro db1 -db db2 -db db3 -1

pro db1.db -U x -P y -db db2.db -U a -P b

So we need some matrix here written as a tree:
/each-database/each-user/(userid + password)
or
/each-user/each-database/(userid + password)

I don't know at this moment which one of them is best.

/each-user/each-database/(userid + password)

Closer to this way is best. But we already have a structure for user accounts and it is probably best to extend this with the database credentials.

Yes, the above directory paths are relative, not absolute. The question was: should I add p2j-user collections with (userid + password) to database structure or a database collections to existing p2j-user accounts. I will use the second choice.

I'm moving this discussion here from email, because it is important to record it as part of the task.

-------- Original Message --------
Subject: Re: status report
Date: Tue, 01 Oct 2013 13:10:11 +0300
From: Ovidiu Maxiniuc <om@goldencode.com>
To: ges@goldencode.com
CC: Eric Faulhaber <ecf@goldencode.com>

Yes, it does.
If for one of the databases the authentication fails all three times,
the client process is closed, even if for a previous database the user
entered the correct combination of credentials.

Regards,
Ovidiu

On 30.09.2013 21:39, Greg Shah wrote:

Ovidiu,

as the hook should be run for each permanent database that requires
authentication

Does the 4GL prompt you multiple times if you have multiple databases
and no -U/-P input for them on the command line?

Thanks,
Greg

If for one of the databases the authentication fails all three times, the client process is closed, even if for a previous database the user entered the correct combination of credentials.

Investigate if you can duplicate this behavior by reading configuration from the directory (probably in the server hook) and passing that configuration to the client hook. Then the client hook (which would only ever be loaded once) can duplicate the multi-prompting behavior (if needed) in its own internal loop.

If the authentication hook interface(s) need enhancement to enable this, I can accept that. BUT please try to make it work without such changes, since that will require changes in Majic as well.

• I added a new type of authentication (AUTH_MODE_METADATA = 5) in SecurityConstants and configured the directory to it.
• also set the auth-plugin to com.goldencode.p2j.security.DatabaseAuthenticationHook and set the retry = 2
• when user connects, the meta_user is checked whether is empty for permanent databases, otherwise I attempt to read each usr/pwd combination from directory.
• if they are not present the DatabaseAuthenticationHook client-side should prompt the user for each one for a user/password combination that will be checked against the meta_user table.
• on the server-side, the combination is validated, and on fail, the user is prompted again if the number of retries is less than 3.

Because I am using other implementation of Authenticator and a new AUTH_MODE I guess Majic should not be affected by these changes.

When importing the _User table, the implementation searches for the _User.d and pwds.txt files at the same location as the rest of the tables from database dump. For example, uast/data/dump/p2j_test/pwds.txt for the p2j_test database.
The content of the password file is rather simple, each line contains the userid followed by the password in plain text. The later will be encoded and stored with the userid replacing the old value (encoded using 4GL algorithm) when the respective record is processed.
This is a critical time because this file contains the passwords in plain text. I don't know if we can find a solution for solving this issue.

Code Review 1004a

I'm only reviewing the util/ and security/ changes. Eric will handle the persistence stuff (runtime and conversion).

1. Your "implements UserSecurityOps" needs to be on its own line (in 2 files).

2. Why is there a <node class="container" name="bogus"> container in your example directory structure?

3. In MetadataSecurityOps.setUserId(String, String, String), don't you need to uppercase(rtrim()) the userid field and the passed userid before comparing them?

4. In MetadataSecurityOps.setUserId(String, String, String), do you need to uppercase(rtrim()) the password field and the passed password before comparing them? This should be tested in the 4GL. I suspect that Progress may have a security hole here, since they would be greatly weakening their hashing algorithm (that is already quite bad) if that field is not marked CASE-SENSITIVE.

5. The System.err.println() usage needs to be changed to real error processing.

6. In SecurityManager, this line is concerning:

if (authModeReq != AUTH_REQ_USER) // AUTH_RESULT_SUCCESS - semanthic types are different

Is this change safe? I don't understand it.

7. Add javadoc for DatabaseAuthenticationHook.createFrameDefinition().

Greg Shah wrote:

1. Your "implements UserSecurityOps" needs to be on its own line (in 2 files).

I believe this is helping hand from IDE. Fixing it manually.

2. Why is there a <node class="container" name="bogus"> container in your example directory structure?

Yes, this is a problem I thought of during the weekend. The user/password are stored in directory for each p2j-user (see notes 24-27 above). How should I know whose p2j-user database structure to load? The answer is to let the p2j client authenticate as before (using one of the AUTH_MODE_ modes). Then I can search the directory for the appropriate -U/-P combinations stored relative to p2j-user: /server/<server>/runtime/<p2juser>/databases-auth/<database>/userid and equivalent for password.

3. In MetadataSecurityOps.setUserId(String, String, String), don't you need to uppercase(rtrim()) the userid field and the passed userid before comparing them?

userid is toUpperCase()-d just when passed as parameter to query. I just tested on 4GL, and I see that spaces to the right of username are ignored. You can either authenticate as "user1" and "user1 " as same user, using the same password. I will add the trimming.

4. In MetadataSecurityOps.setUserId(String, String, String), do you need to uppercase(rtrim()) the password field and the passed password before comparing them? This should be tested in the 4GL. I suspect that Progress may have a security hole here, since they would be greatly weakening their hashing algorithm (that is already quite bad) if that field is not marked CASE-SENSITIVE.

Indeed, the _password is not sensitive so one can search for QFBHICBKGBDKKDQF and obtain password encoded as QFbhicbkGbdkkdqf. However, the SETUSERID is case-sensitive. This means that if you have a password stored as QFbhicbkGbdkkdqf and manually change your password to QFBHICBKGBDKKDQF, you will not be able to authenticate unless you find the password that ENCODES to QFBHICBKGBDKKDQF.

6. In SecurityManager, this line is concerning:
[...]
Is this change safe? I don't understand it.

There is a little mess in there as authModeReq variable is used to store both AUTH_MODE_ and AUTH_RESULT_ enumeration items, which have overlapping values. The code might be running fine, but if the constants are re-ordered the code could broke.
Syntactically, AUTH_MODE_NONE, AUTH_REQ_USER and AUTH_RESULT_SUCCESS are all 0, but semantically they represent somewhat incomparable things. Anyway, my "strong-typed" sense lets me know this might not be correct.

2. Why is there a <node class="container" name="bogus"> container in your example directory structure?

Yes, this is a problem I thought of during the weekend. The user/password are stored in directory for each p2j-user (see notes 24-27 above). How should I know whose p2j-user database structure to load? The answer is to let the p2j client authenticate as before (using one of the AUTH_MODE_ modes). Then I can search the directory for the appropriate -U/-P combinations stored relative to p2j-user: /server/<server>/runtime/<p2juser>/databases-auth/<database>/userid and equivalent for password.

This seems reasonable. My only concern is that there is an extra, unneeded container named "bogus":

        <node class="container" name="runtime">
          <node class="container" name="bogus">
            <node class="container" name="databases-auth">

Can't it be removed?

6. In SecurityManager, this line is concerning:
[...]
Is this change safe? I don't understand it.

There is a little mess in there as authModeReq variable is used to store both AUTH_MODE_ and AUTH_RESULT_ enumeration items, which have overlapping values. The code might be running fine, but if the constants are re-ordered the code could broke.
Syntactically, AUTH_MODE_NONE, AUTH_REQ_USER and AUTH_RESULT_SUCCESS are all 0, but semantically they represent somewhat incomparable things. Anyway, my "strong-typed" sense lets me know this might not be correct.

Please add comments to that location explaining these concerns and your perspective. At least it will be easier for the next developer to build upon your findings.

The customer is using simultaneous connection to databases "common" and "<db_name>".
I have done today some research on multiple database connection but looks like Hibernate does not properly handle this. Once a connection to first database is in place the subsequent connections to other databases will be routed through the same connection so any access to tables from second databases that are not defined in the first one will end up with an exception:

Hibernate: select ((select count(id) from appuser where id >= ? and id <= ?) + ... + (select count(id) from vehicle_owner where id >= ? and id <= ?)) as cnt
[10/09/2013 21:17:55 EEST] (org.hibernate.engine.jdbc.spi.SqlExceptionHelper:WARN) SQL Error: 0, SQLState: 42P01
[10/09/2013 21:17:55 EEST] (org.hibernate.engine.jdbc.spi.SqlExceptionHelper:ERROR) ERROR: relation "appuser" does not exist
  Position: 32
[10/09/2013 21:17:55 EEST] (com.goldencode.p2j.persist.id.IdentityPool$KeyScanner:SEVERE) Identity pool has been stopped because of an exception:
org.hibernate.exception.SQLGrammarException: ERROR: relation "appuser" does not exist
  Position: 32
        at org.hibernate.exception.internal.SQLStateConversionDelegate.convert(SQLStateConversionDelegate.java:122)
        at org.hibernate.exception.internal.StandardSQLExceptionConverter.convert(StandardSQLExceptionConverter.java:49)
        at org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:125)
        at org.hibernate.engine.jdbc.spi.SqlExceptionHelper.convert(SqlExceptionHelper.java:110)
        at org.hibernate.engine.jdbc.internal.proxy.AbstractStatementProxyHandler.continueInvocation(AbstractStatementProxyHandler.java:129)
        at org.hibernate.engine.jdbc.internal.proxy.AbstractProxyHandler.invoke(AbstractProxyHandler.java:81)
        at $Proxy5.executeQuery(Unknown Source)
        at org.hibernate.loader.Loader.getResultSet(Loader.java:1926)
        at org.hibernate.loader.Loader.executeQueryStatement(Loader.java:1727)
        at org.hibernate.loader.Loader.doQuery(Loader.java:852)
        at org.hibernate.loader.Loader.doQueryAndInitializeNonLazyCollections(Loader.java:293)
        at org.hibernate.loader.Loader.doList(Loader.java:2411)
        at org.hibernate.loader.Loader.doList(Loader.java:2397)
        at org.hibernate.loader.Loader.listIgnoreQueryCache(Loader.java:2227)
        at org.hibernate.loader.Loader.list(Loader.java:2222)
        at org.hibernate.loader.custom.CustomLoader.list(CustomLoader.java:331)
        at org.hibernate.internal.SessionImpl.listCustomQuery(SessionImpl.java:1783)
        at org.hibernate.internal.AbstractSessionImpl.list(AbstractSessionImpl.java:231)
        at org.hibernate.internal.SQLQueryImpl.list(SQLQueryImpl.java:156)
        at org.hibernate.internal.AbstractQueryImpl.uniqueResult(AbstractQueryImpl.java:905)
        at com.goldencode.p2j.persist.id.JdbcKeyRetriever.retrieveFreeKeys(JdbcKeyRetriever.java:125)
        at com.goldencode.p2j.persist.id.IdentityPool$KeyScanner.run(IdentityPool.java:599)
        at java.lang.Thread.run(Thread.java:662)
Caused by: org.postgresql.util.PSQLException: ERROR: relation "appuser" does not exist
  Position: 32
        at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2157)
        at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:1886)
        at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:255)
        at org.postgresql.jdbc2.AbstractJdbc2Statement.execute(AbstractJdbc2Statement.java:555)
        at org.postgresql.jdbc2.AbstractJdbc2Statement.executeWithFlags(AbstractJdbc2Statement.java:417)
        at org.postgresql.jdbc2.AbstractJdbc2Statement.executeQuery(AbstractJdbc2Statement.java:302)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
        at java.lang.reflect.Method.invoke(Method.java:597)
        at org.hibernate.engine.jdbc.internal.proxy.AbstractStatementProxyHandler.continueInvocation(AbstractStatementProxyHandler.java:122)
        at org.hibernate.engine.jdbc.internal.proxy.AbstractProxyHandler.invoke(AbstractProxyHandler.java:81)
        at $Proxy5.executeQuery(Unknown Source)
        at org.hibernate.loader.Loader.getResultSet(Loader.java:1926)
        at org.hibernate.loader.Loader.executeQueryStatement(Loader.java:1727)
        at org.hibernate.loader.Loader.doQuery(Loader.java:852)
        at org.hibernate.loader.Loader.doQueryAndInitializeNonLazyCollections(Loader.java:293)
        at org.hibernate.loader.Loader.doList(Loader.java:2411)
        at org.hibernate.loader.Loader.doList(Loader.java:2397)
        at org.hibernate.loader.Loader.listIgnoreQueryCache(Loader.java:2227)
        at org.hibernate.loader.Loader.list(Loader.java:2222)
        at org.hibernate.loader.custom.CustomLoader.list(CustomLoader.java:331)
        at org.hibernate.internal.SessionImpl.listCustomQuery(SessionImpl.java:1783)
        at org.hibernate.internal.AbstractSessionImpl.list(AbstractSessionImpl.java:231)
        at org.hibernate.internal.SQLQueryImpl.list(SQLQueryImpl.java:156)
        at org.hibernate.internal.AbstractQueryImpl.uniqueResult(AbstractQueryImpl.java:905)
        at com.goldencode.p2j.persist.id.JdbcKeyRetriever.retrieveFreeKeys(JdbcKeyRetriever.java:125)
        at com.goldencode.p2j.persist.id.IdentityPool$KeyScanner.run(IdentityPool.java:599)
        at java.lang.Thread.run(Thread.java:662)

There is no difference if the databases are loaded at server-startup load_at_startup = true or later, at client connection time.

This query doesn't look correct. Why are we accessing tables from different databases in the same query? Perhaps a bug in DatabaseManager.getDatabaseTables(Database) or an incorrect assumption in JdbcKeyRetriever?

I will investigate the JdbcKeyRetriever, I did not see any flaw in DatabaseManager.getDatabaseTables(Database).
However, the query looks fine to me. I shortened it (I replaced the subqueries for the rest of tables with elipsis). Probably you were confused by the name of the appuser table. It is part of the p2j_test database/schema. I added a new table user to test name collisions with meta-table _user, and used some hints at conversion time.
The new db2 database has a sole table t1 with only one field f1.
The only thing that appear incorrect is the connection to database server, in spite of explicit setting of values "jdbc:postgresql:db2" / "jdbc:postgresql:p2j_test" in hibernate.connection.url. I guess hibernate is trying to optimize database access by reusing the connection in a wrong way.

Thanks for the clarification. So, are you saying the above stack trace was generated from the query running against db2, but it should have been running against p2j_test?

We use multiple connection URLs which specify different database instances in the same PostgreSQL cluster from the same P2J server successfully today. In fact, every time we run the regression test harness, we connect to multiple databases. The only difference I see with what you have reported is that we specify host (and optionally port) in the URL (e.g., jdbc:postgresql//localhost:5434/p2j_test).

At a glance, IdentityPool and JdbcKeyRetriever look ok to me. My main concern during my quick review was that we were not somehow sharing a Hibernate Session for two different databases.

Another difference I thought of is that when we connect to multiple databases in regression testing, both have the same schema, whereas they differ in your use case. Could there be a problem with our SessionFactory setup?

Meanwhile I tried to connect to different database servers using different dialects (one PSQL, one H2).
I was expected it (although not fully confident), the queries went fine and clients were successful ran.
From my researches, the SessionImpl is created and destroyed as soon as it is not needed any more. I though it relies on some even lower-level.
Then I re-put hibernate into debug mode and trace the sql query.
My investigations lead me to TempTableConnectionProvider. It delegates the whole job to another com.goldencode.p2j.persist.UnpooledConnectionProvider object which is ... static.
The problem is that once created an initialized it will not accept to be re-configured for other databases (see the first line in configure(Map props)). So each TempTableConnectionProvider of the connected databases will use the same delegate (which is configured only once - for the first database). The rest is obvious, the subsequent database queries will be executed against the first connected database.

Wait, why are you using TempTableConnectionProvider for those other connected databases? This sounds like a configuration problem, probably the result of inadequate documentation.

TempTableConnectionProvider backed by UnpooledConnectionProvider is intended to be a combination only used by the _temp database and no other. You should never have an external database configured like this. External databases should always be configured to use c3p0 as the connection pool.

There can be only one _temp database per P2J server process, no matter how many other databases are connected. So, it should not be a problem that the UnpooledConnectionProvider is stored in a static field within TempTableConnectionProvider. In fact, this is by design.

The flexibility in configuration of the _temp database is historical. We used to allow a PostgreSQL database to be used for temp-tables, but we've long since switched over to an embedded, H2 database. The configuration was left in the directory, however. We probably should change this to be hard-coded in the runtime, but we want to leave ourselves the option of using different modes, so we at least want to leave the URL available for configuration.

How are your non-temp-table databases configured, specifically, what does the connection provider configuration look like?

Indeed, the database/p2j_test from my directory.xml was wrong, it contained the following entries:

<node class="string" name="provider_class">
    <node-attribute name="value" value="com.goldencode.p2j.persist.TempTableConnectionProvider"/>
</node>
<node class="string" name="delegate_provider_class">
    <node-attribute name="value" value="com.goldencode.p2j.persist.UnpooledConnectionProvider"/>
</node>

Now I removed the extra config nodes and the database accesses are fine.

Code Review 1014a

Eric will handle all the database/persistence portions (conversion and runtime).

In regard to the non-database changes, I generally like them. I like that the changes are not as mixed in to the SecurityManager now.

However, I do have a serious concern about the idea of having the database-specific authentication features to be tightly coded into both the security package and the net package. I really should have noted the tight coupling of the security package as an issue before. Perhaps it is unavoidable in that case because it is so intrinsic to the authentication process.

But the idea of the both the security and net packages is that they should be usable for non-P2J projects too. The security package probably already fails in this regard, but the net package is actually pretty well encapsulated away from 4GL-specific behavior. So: we can leave the security package changes as is. But I really want to remove the net package changes and find a different solution. Please explain the requirement you are trying to solve and we can brainstorm a solution.

I would also prefer to move the direct database access out of the MetadataSecurityOps. It is too tight of a linkage between the database and our more general util code.

Greg Shah wrote:

Code Review 1014a

Eric will handle all the database/persistence portions (conversion and runtime).

In regard to the non-database changes, I generally like them. I like that the changes are not as mixed in to the SecurityManager now.

Indeed, there are two steps involved handled by different methods (both on server and client). However I kept the low-level communication protocol. I believe that this protocol could be further abstracted but I am not sure if the effort worths (the time and the aspect of the code with additional if s and switch case s).

However, I do have a serious concern about the idea of having the database-specific authentication features to be tightly coded into both the security package and the net package. I really should have noted the tight coupling of the security package as an issue before. Perhaps it is unavoidable in that case because it is so intrinsic to the authentication process.
But the idea of the both the security and net packages is that they should be usable for non-P2J projects too. The security package probably already fails in this regard, but the net package is actually pretty well encapsulated away from 4GL-specific behavior. So: we can leave the security package changes as is. But I really want to remove the net package changes and find a different solution. Please explain the requirement you are trying to solve and we can brainstorm a solution.

The problem is that I need the authenticated userids for each database at runtime, in the context of the client. They have been obtained using an intermediary context that is dropped anyway. The new valid context is created and actived only after the communication queue/protocol is started and before entering the method declared by p2j-entry. This is particularly difficult because we need to move this map from one thread to another and the only commonly available objects is the protocol queue.

I would also prefer to move the direct database access out of the MetadataSecurityOps. It is too tight of a linkage between the database and our more general util code.

I think the setUserId and getAuthLevel bodies can be moved to static methods in ConnectionManager. The direct access to database persistence would be removed, the linkage will remain only at method calls to ConnectionManager.

The problem is that I need the authenticated userids for each database at runtime, in the context of the client. They have been obtained using an intermediary context that is dropped anyway. The new valid context is created and actived only after the communication queue/protocol is started and before entering the method declared by p2j-entry. This is particularly difficult because we need to move this map from one thread to another and the only commonly available objects is the protocol queue.

Let's back up a bit.

Although I like the database authentication to be isolated in its own methods as much as possible, I do think that making the authenticateDatabases() into a process that runs after the user is already authenticated is not the right way to go. In particular, in the most common case (where the initial "authentication" is to just allow "guest" access to everyone), we don't have any additional context to use for help anyway.

Why can't we use the authentication hook the way it was intended? When database authentication is in effect, this is just a different version of our AUTH_MODE_IDPW user/password authentication approach. The user provides the userid and we lookup the hashed password in _user table to authenticate. If we must loop for multiple databases AND if our security manager infrastructure needs to be enhanced to enable that, then we enhance it to enable the proper flow. Perhaps that is the core issues here (that the Authenticator hook interfaces may need to be enhanced).

Note also that we have previously provided a way for the client to non-interactively pass a userid value from the command line/bootstrap configuration. Then we have used that value to lookup user-specific values in the directory, in a password-less login scenario.

In the case where we are deliberately trying to use -U/-P values from the directory, it seems to me that the interactive login panel is not going to be used. How does Progress mix these two concepts together?

I think the setUserId and getAuthLevel bodies can be moved to static methods in ConnectionManager. The direct access to database persistence would be removed, the linkage will remain only at method calls to ConnectionManager.

Great!

Greg Shah wrote:

Although I like the database authentication to be isolated in its own methods as much as possible, I do think that making the authenticateDatabases() into a process that runs after the user is already authenticated is not the right way to go. In particular, in the most common case (where the initial "authentication" is to just allow "guest" access to everyone), we don't have any additional context to use for help anyway.

Why can't we use the authentication hook the way it was intended? When database authentication is in effect, this is just a different version of our AUTH_MODE_IDPW user/password authentication approach. The user provides the userid and we lookup the hashed password in _user table to authenticate. If we must loop for multiple databases AND if our security manager infrastructure needs to be enhanced to enable that, then we enhance it to enable the proper flow. Perhaps that is the core issues here (that the Authenticator hook interfaces may need to be enhanced).

Note also that we have previously provided a way for the client to non-interactively pass a userid value from the command line/bootstrap configuration. Then we have used that value to lookup user-specific values in the directory, in a password-less login scenario.

• P2J identifies each of its users even if they will authenticate using another protocol to databases. From p4gl, there are different entities for each database, they can even be changed using the setuserid function and connect statement, but for p2j they are unique - I think this is an advantage for security and statistics.
• in order to store database-level userid/password a per-user structure need to be added in directory. In a non-scoped search in directory, all p2j users will use the same p4gl credentials for connecting to databases. Because the p2j user is already known (from previous authentication step) we can do a scoped search in directory that allows a per-p2j user or group individual storage of credentials.
• all p2j authentication modes (including loopback, non-interactive etc.) are available just as before
• management of p2j users can be performed using the current 'admin' implementation
• in the multiple-database case with different userid, which one will be chosen to be set into context for access scoped settings ?
• allows the X509 certification to add layer to authentication security.

There is the alternative of enriching client-side command-line parameters to support the equivalent of p4gl's -db / -P / -U in order to match the p4gl architecture, but this will only remove the need for storing these values in the directory.

In the case where we are deliberately trying to use -U/-P values from the directory, it seems to me that the interactive login panel is not going to be used. How does Progress mix these two concepts together?

In Progress ach database access can be configured independently. In the case there are multiple databases connected using client command-line Progress will take them in the given order and:
- if check userid/password are provided they are checked. On fail, the client is disconnected.
- if metadata is enable for the database, the user is prompted for max. three times for userid/password. On fail the client is disconnected. The user can choose "cancel" and will be authenticated as blank user.
- otherwise (metadata not enabled, _user is empty), the user is authenticated using OS username.

in order to store database-level userid/password a per-user structure need to be added in directory. In a non-scoped search in directory, all p2j users will use the same p4gl credentials for connecting to databases. Because the p2j user is already known (from previous authentication step) we can do a scoped search in directory that allows a per-p2j user or group individual storage of credentials.

We can still do a scoped search in the case where we use the database authentication as the main P2J authentication. We just would use the userid that is entered by the person responding to the login prompt. Once we know that, we can search the directory with that account as the relative "scope".

Please note that being able to configure the -U/-P in the directory should not be the most important part of the design. And later in the project, we will be providing more sophisticated ways to override the -U/-P values from the command line.

all p2j authentication modes (including loopback, non-interactive etc.) are available just as before

This doesn't change in my scenario either. I am not suggesting removing the other modes. The issue here is that apps that use _user for authentication are not going to ALSO use the other P2J modes. So it is best to make this an alternative rather than an extra step.

management of p2j users can be performed using the current 'admin' implementation

This doesn't change in my scenario. But I will note that any app using _user won't really be using the P2J user environment, so they likely won't be using the admin console anyway.

in the multiple-database case with different userid, which one will be chosen to be set into context for access scoped settings ?

We can try each of them to see if there is a -U/-P combo stored that should be used.

allows the X509 certification to add layer to authentication security.

The _user authentication method wouldn't be used in conjunction with this.

if metadata is enable for the database, the user is prompted for max. three times for userid/password. On fail the client is disconnected. The user can choose "cancel" and will be authenticated as blank user.

We can still do this, but the user will be allowed/rejected into P2J based on the _user table for the matching database. The whole point to the login dialog in this case is that there is no -U/-P combo and the user's given input should determine access. That will be fine to use it for P2J access too, since this is no worse than what they do today in the 4GL.

The only difference here is that I expect that the maximum 3-tries thing should be driven by the Authenticator interface from the server, where the first input is passed back to the server for authentication, it fails and the server knows to try again and so it calls back down to the client for the next try...

otherwise (metadata not enabled, _user is empty), the user is authenticated using OS username

No sure what you mean by "authenticated". In the 4GL, there is no authentication in this case. It is just that the USERID builtin will return the OS username. We should do the same thing.

This is the same thing as our "guest" access mode that we use in "simple server". And this will be the default setup for P2J in the future, because it is how most Progress apps work.

In other words, we initially thought that we would improve app security as part of converting to Java. So we made a nice P2J security model that was significantly better than anything Progress had. The problem: it is really hard (and huge work) to remap and manually rewrite everything during conversion such that the app uses the P2J security model. We did it for TIMCO and it probably was not worth it. On top of that, there is the admin overhead/effort and much more infrastructure/code to maintain on our side. The bottom line: for the customer's project we decided that adding that extra work into an already complex/big project was too much risk. I expect many future customers to also do this: we are essentially duplicating enough of the 4GL functionality so that their current "roll your own" security implementation will work as is.

• in the case that metadata authentication is successful for n databases, is there a clean way for transferring the map of authenticated userids from the authentication intermediary context to the correct context in which the client executes without touching the Queue and Conversation ?

If I understand the issue properly, you need to do some processing at the end of Conversation.run() when the session's context is fully initialized and the session is "real".

Why not used an interface that can be used as a callback at that time? Please look at com/goldencode/p2j/net/SessionListener. Right now this only gets called back for terminate() processing, but originally it also provided an initialize() callback. If that can be made to work cleanly, then it is the preferred approach. A second choice would be to add a new interface with the callback method. The session listener seems well matched to this though.

Once called back at the right time (and on the right thread with the context set and ready), the code can store any data there as needed.

The key is to design a solution that is not hard coded to this one use case AND which is not P2J-specific.

Sorry, I fail to understand some of the notes above.
I'm thinking that the SecurityManager should be completely disabled if database authorization is in charge. The _user table is rather dynamic, users can be added by simply create-ing records in the meta table. A middle solution would be to mirror the imported _user table into directory.

Greg Shah wrote:

in order to store database-level userid/password a per-user structure need to be added in directory. In a non-scoped search in directory, all p2j users will use the same p4gl credentials for connecting to databases. Because the p2j user is already known (from previous authentication step) we can do a scoped search in directory that allows a per-p2j user or group individual storage of credentials.

We can still do a scoped search in the case where we use the database authentication as the main P2J authentication. We just would use the userid that is entered by the person responding to the login prompt. Once we know that, we can search the directory with that account as the relative "scope".

If the user already entered the userid what's the purpose of searching the directory for that scope. Unless it's a 2 level authentication: user enters the p2j userid and we search the directory for his saved p4gl -U -P combination that will used for checking against _user table.

in the multiple-database case with different userid, which one will be chosen to be set into context for access scoped settings ?

We can try each of them to see if there is a -U/-P combo stored that should be used.

what if user chooses to cancel authentication or the databases' _user is not populated so it won't require authentication ?

if metadata is enable for the database, the user is prompted for max. three times for userid/password. On fail the client is disconnected. The user can choose "cancel" and will be authenticated as blank user.

We can still do this, but the user will be allowed/rejected into P2J based on the _user table for the matching database. The whole point to the login dialog in this case is that there is no -U/-P combo and the user's given input should determine access. That will be fine to use it for P2J access too, since this is no worse than what they do today in the 4GL.

This was my initial solution. Because the credentials matched in database authentication, we allow him to enter the system. However, because the P2J security model from SecurityManager is so deep (at least before getting to "real" context - you cannot build an intermediary context without having a valid p2j user) all access was performed using a p2j "bogus" user.

The only difference here is that I expect that the maximum 3-tries thing should be driven by the Authenticator interface from the server, where the first input is passed back to the server for authentication, it fails and the server knows to try again and so it calls back down to the client for the next try...

This can be added to Authenticator and configured dynamically with n "entities" (?) one for each database when using metadata and one "entity" for other others. At this moment this is done on the old protocol using AUTH_MODE_METADATA for resetting the retry counter and resending the Authenticator to client with next database.

otherwise (metadata not enabled, _user is empty), the user is authenticated using OS username

No sure what you mean by "authenticated". In the 4GL, there is no authentication in this case. It is just that the USERID builtin will return the OS username. We should do the same thing.

Maybe "authenticated" is not the right term, but this is what userid function returns and any client is granted full access the database.

This is the same thing as our "guest" access mode that we use in "simple server". And this will be the default setup for P2J in the future, because it is how most Progress apps work.
In other words, we initially thought that we would improve app security as part of converting to Java. So we made a nice P2J security model that was significantly better than anything Progress had. The problem: it is really hard (and huge work) to remap and manually rewrite everything during conversion such that the app uses the P2J security model. We did it for TIMCO and it probably was not worth it. On top of that, there is the admin overhead/effort and much more infrastructure/code to maintain on our side. The bottom line: for the customer's project we decided that adding that extra work into an already complex/big project was too much risk. I expect many future customers to also do this: we are essentially duplicating enough of the 4GL functionality so that their current "roll your own" security implementation will work as is.

So we need to duplicate Progress' behavior. If 4GL was enough - P2j will be too.

Greg Shah wrote:

If I understand the issue properly, you need to do some processing at the end of Conversation.run() when the session's context is fully initialized and the session is "real".

Why not used an interface that can be used as a callback at that time? Please look at com/goldencode/p2j/net/SessionListener. Right now this only gets called back for terminate() processing, but originally it also provided an initialize() callback. If that can be made to work cleanly, then it is the preferred approach. A second choice would be to add a new interface with the callback method. The session listener seems well matched to this though.

Once called back at the right time (and on the right thread with the context set and ready), the code can store any data there as needed.

The key is to design a solution that is not hard coded to this one use case AND which is not P2J-specific.

Yes, something like that, but I'm afraid is not so simple mostly because of last idea to which I totally subscribe.
First, I think the listener should be on Queue, not Session, but this is not a big issue.
I wrote an (anonymous) class that sets the userids into current context that should be triggered when context is in place. My issue is to pass it from authenticateLocal to a place where the Queue is created (in RouterSessionManager.run()). The easier solution is to simply abuse of the returned Object and in run() just test the rtti and add the listener to queue. This is rather ugly, but the problem is visible only at runtime because before the Conversation can be started a valid context is needed (with a valid p2j userid).
Then I thought - at connection time there is only one session per queue so let's try enriching the SessionListener with init method and added it directly into the Session. This was a dead-end because there is a name collision (from p2j.security and p2j.net) - my mistake.

• returning an 2-Object array from SecurityManager.authenticateLocal() with both Context and Listener ? again rtti and casts are needed.
• add a container parameter to SecurityManager.authenticateLocal() to serve as output for the Lisnener ?
• store the Listener in the Context or vice-versa ? this is really ugly as they are rather unrelated.
• temporary storing the listener in SecurityManager and obtaining the listener is a second call ? Access to SecurityManager must be synchronized.

I mean, there are solutions, but none I can see are clean enough and not to mix the p2j.net with special metadata authentication.

I'm thinking that the SecurityManager should be completely disabled if database authorization is in charge.

I don't think this is a possibility. We have too many dependencies on the SecurityManager.

The _user table is rather dynamic, users can be added by simply create-ing records in the meta table.

I understand this. I expect the server-side Authenticator hook to just call the equivalent of SETUSERID.

A middle solution would be to mirror the imported _user table into directory.

This seems like much more work than is needed.

Unless it's a 2 level authentication: user enters the p2j userid and we search the directory for his saved p4gl -U -P combination that will used for checking against _user table.

Yes, this is basically it.

what if user chooses to cancel authentication or the databases' _user is not populated so it won't require authentication ?

Why can't we respond like Progress in these cases?

This was my initial solution. Because the credentials matched in database authentication, we allow him to enter the system. However, because the P2J security model from SecurityManager is so deep (at least before getting to "real" context - you cannot build an intermediary context without having a valid p2j user) all access was performed using a p2j "bogus" user.

Yes, I do understand that we must map a P2J account to the incoming session. But the idea is that in this mode, there would generally be a single P2J account that is shared by everyone. This is equivalent to the "guest" access in "simple server".

So we need to duplicate Progress' behavior. If 4GL was enough - P2j will be too.

Exactly.

Also: can't we just use the AUTH_MODE_CUSTOM to do everything in our hook? It is OK if we must enhance the hook interface to provide more features. But the whole idea of the hooks was that they would be a flexible way to implement a completely customer-specific authentication mechanism and then map the result to a P2J account. The Authenticator design has both client-side and server-side portions, with this objective in mind.

I'll think about the SessionListener problem and post something later.

Then I thought - at connection time there is only one session per queue so let's try enriching the SessionListener with init method and added it directly into the Session.

Yes and no. Yes, there is only one session per queue (in the DirectSession case, which is the case we care about). But we don't need to set it into the Session object manually.

If we set it into RouterSessionManager.Incoming.notify, it would be automatically registered during createDirectSession().

This was a dead-end because there is a name collision (from p2j.security and p2j.net) - my mistake.

I don't understand what you mean here. There is no p2j/security/SessionListener or references to any SessionListener class from that package.

I think the answer is this:

1. Please do add a SessionListener.initialize() method. You will have to change all of the current instances of that interface, but there are not too many.
2. Enhance the NetSocket class to have a SessionListener data member (and getter/setter).
3. Enhance the Authenticator interface to provide a getSessionListener() method that can be used to return a SessionListener. If any authentication hooks (in AUTH_MODE_CUSTOM) need to register a listener, they just return the instance using that method, otherwise they can return null.
4. SecurityManager.authenticateLocal() would call getSessionListener() in AUTH_MODE_CUSTOM and if a non-null value is returned, that instance is set into the NetSocket instance.
5. In RouterSessionManager.Incoming.run(), before creating the Queue instance, any SessionListener that is in the NetSocket will be gotten and stored in the notify member. Actually, we may need an ArrayList<SessionListener> as the notify member, so that we can handle this additional case.
6. In RouterSessionManager.createDirectSession() the code would be enhanced to add multiple listeners.
7. Conversation.run() would be an optimal place to trigger the initialize() notification for CONVERSATION_MODE. We have to find a good place to trigger it in non-CONVERSATION_MODE.

Code review 20131014a (database-related):

• Other than to avoid collisions with Java and SQL reserved keywords, let's not hard-code any specific phrases directly into the NameConverter class. If we need a "meta" prefix, we should feed it in as a separate word on input, as in "meta _user". This will convert to "MetaUser" for a class name, "metaUser" for a field name, "meta_user" for a table, column, or index name, etc. Also, we should use the STANDARD constant, to make sure we are not getting any unexpected expansions, for example: name.convert(originalName, name.class, name.standard, true, reportText) where originalName contains "meta _user".
• Was it necessary to hard code the _User table schema move in SchemaLoader? I would prefer to leave SchemaLoader unaware of particular tables and perform this move in TRPL instead (maybe in schema/fixups?). This will require, however, a bit of heretofore missing functionality to load an arbitrary AST and register it with the AstResolver. I have added a loadTree user function to CommonAstSupport for my current task, and I will post this update to #2115 shortly. The idea is to load the metadata AST into the AstResolver when you are processing a (non-metadata) schema, manually walk to the _User table part of the metadata AST, and graft a copy of it into the current schema. After all non-metadata schemas have been processed, you would remove the _User AST from the metadata schema (in a different, later rule-set).
• In the ConnectionManager c'tor, you have a TODO about needing to query the _User table. Look at DatabaseManager.checkOutConnection. Will this help (perhaps with some slight modification)?
• Is it necessary to prefix all the _User table's fields with "meta" as well? In all the other metadata tables, I just changed the table names (again, see update in #2115).
• Nice work with the import changes.
• Note that (unless I have misunderstood your changes) the approach we have taken with the _User table (and even if you change to my proposed approach) results in one MetaUser DMO class per application schema. These classes will reside in different packages, but will be structurally identical and thus redundant, which is not ideal. I don't want to address this point now, but we need to open a separate issue to track this as a future conversion improvement.

• when importing. The importer needs to know if _user meta-table is present in order to further process the password merging. Of course, the field names must be known apriori or a fixed algorithm should be used to detect the new converted names of properties _userid and _password in order to lookup userids and set the associated (encoded) password.
• at runtime, when using setuserid function. The converted name of the table and both the _userid and _password fields must be known for direct access.

In the ConnectionManager c'tor, you have a TODO about needing to query the _User table. ...

This in not an issue anymore, the initial userid detection was moved to makeConnection method.

Ovidiu Maxiniuc wrote:

Hardcoding the table name, _userid and _password fields are necessary because they need to be accessed in a special way later, at two times:
[...]

Since you know what the input always will be for these names (it's not application specific), and you know what the output needs to be, then either don't call the NameConverterWorker at all for these special cases, or call it the way I suggest above. But please don't hard code the values inside NameConverter, rather deal with them in the TRPL rules.

Code review 20131021a (database-related only):

• Based on a comment in ConnectionManager.authenticate, it appears you aren't satisfied with the query part, though it looks like it should work.
• Please undo all the formatting changes to CommonAstSupport.
• Also one minor formatting aberration in ImportWorker, line 1132 (opening curly brace).
• No need to include NameConverter in the update, since there are no longer any changes to it.

Code Review 1021a

Overall, this is really good. It is very close to what we need.

1. It seems that we can drop the changes from Queue. The RouterSessionManager.createDirectSession() can call session.addSessionListener() directly since it also has access to the Incoming instance from which it can get the NetSocket instance and thus call getSessionListeners(). This makes the result cleaner.

2. Please change the history entry text to match the newer implementation for these classes: Conversation, RouterSessionManager, SecurityConstants.

3. You put this comment in SecurityManager.authenticateClient(): // why would it stop the server ?. As far as I understand it, authenticateClient() runs on the client side so this use of System.exit() does not stop the server.

4. What do you mean by the command // TODO: this is only temporary: in SecurityManager.authenticateLocal()?

5. "ptorocol" misspelling in SecurityManager.

6. I think AUTH_RESULT_SKIP_ENTITY/PKT_SIZE_SKIP_ENTITY should be renamed AUTH_RESULT_SKIP_TO_NEXT/PKT_SIZE_SKIP_TO_NEXT because we have a UI class named SkipEntity and it initially made me think this constant somehow was related to notifying the client login UI to add a skip...

7. The following classes need history entries added: NetSocket, BaseSession, SessionManager, ExportTracker, AdminLogon, ServerImpl, LowLevelSocketImpl, NumberType, AppServerManager, LowLevelSocketListenerImpl, Agent, SessionListener.

8. SessionManager.notifyInitialization() needs javadoc.

9. The following classes should have the initialize() method changed slightly. I think you can remove the TODO since I don't think there is any need to do a real implementation there. And each initialize() method should have javadoc. See AdminLogon, ServerImpl, LowLevelSocketImpl, AppServerManager, LowLevelSocketListenerImpl, Agent.

10. In NumberType.setNumericFormat(), in what case will we be running without a SessionManager? Is this just a timing case during login?

11. In non-conversation mode, where should we call sessMgr.notifyInitialization()? This mode should honor the SessionListener too.

12. For virtual sessions, where should we call sessMgr.notifyInitialization()?

Code Review 1022a

The changes are fine. The only thing I see is that it seems the SessionListener notification in Conversation should be removed (assuming the changes to LeafSessionManager and RouterSessionManager are sufficient). Otherwise I think you may get a double initialize() call.

Good work!

I have discovered an issue yesterday. Having two databases connected that have common table names (as particular case the _user from metadata) will lead to an incorrect conversion.
Ex:

FIND FIRST db2.customer.
FIND FIRST p2j_test.customer.

import com.goldencode.testcases.dmo.p2j_test.*;
import com.goldencode.testcases.dmo.db2.*;
...
   Customer.Buf customer = RecordBuffer.define(Customer.Buf.class, "p2j_test", "customer");
   Customer.Buf customerBuf2 = RecordBuffer.define(Customer.Buf.class, "db2", "customerBuf2");

   com.goldencode.testcases.dmo.p2j_test.Customer.Buf customer = RecordBuffer.define(com.goldencode.testcases.dmo.p2j_test.Customer.Buf.class, "p2j_test", "customer");
   com.goldencode.testcases.dmo.db2.Customer.Buf customerBuf2 = RecordBuffer.define(com.goldencode.testcases.dmo.db2.Customer.Buf.class, "db2", "customerBuf2");

I encountered another issue. The following code:

FIND FIRST db2.customer.
DISPLAY name.
FIND FIRST p2j_test.customer.
DISPLAY name.

   new FindQuery(customerBuf2, (String) null, null, "customerBuf2.id asc").first();
   message((character) new FieldReference(customerBuf2, "name").getValue());
   new FindQuery(customer, (String) null, null, "customer.name asc, customer.address desc").first();
   message((character) new FieldReference(customerBuf2, "name").getValue());

Ovidiu Maxiniuc wrote:

I have fixed it (by analyzing in record_scoping_post.rules if there are buffer conflicts and then in buffer_definitions.rules use the full package name for classes)...

I agree this is a good solution, since it is limited only to cases where the names actually conflict.

P4gl does not compile the code (Error 1762: name is ambiguous with p2j_test.customer.Name and db2.Customer.name) so I guess we should not care about this issue.

Yes. A base prerequisite of our conversion is that the code we convert is valid 4GL code.

P4gl does not compile the code (Error 1762: name is ambiguous with p2j_test.customer.Name and db2.Customer.name) so I guess we should not care about this issue.

Yes, we don't have to worry about this right now.

But even so, please do create a new redmine bug task for this case. We have disabled ambiguity warnings too aggressively and our parser/SchemaDictionary should be fixed.

I merged my sources with the latest from bzr (especially 2115, with other metadata implementation)

I have the following issue when connecting a second database that was not marked as load-at-startup: when connecting (ConnectionManager.makeConnection()), I am attempting to register all three databases: meta/primary/dirty. My attempt to MetadataManager.prepareDatabase(); with the new metaDB will fail because this method works correctly only if early connections are available (at server startup) otherwise PersistenceFactory.getInstance() will not be able to identify the dialect of the database.
I wonder if it wouldn't be better if this registrations of metaDb-s to be done once for all permanent databases, at server startup ?
I think it would be best to register once and never unregister the metadatas because they are rather resource-consuming and the information does not change so it doesn't make sense to reload it for each new session. The only disadvantages would be if the meta database is prepared/registered and never used and the blocked memory between two separate sessions.

I was continuing work on #2115 and noticed this same issue. I don't have a use case like yours readily available; please try this patch:

=== modified file 'src/com/goldencode/p2j/persist/DatabaseManager.java'
--- src/com/goldencode/p2j/persist/DatabaseManager.java    2013-10-28 04:11:29 +0000
+++ src/com/goldencode/p2j/persist/DatabaseManager.java    2013-10-28 21:03:00 +0000
@@ -177,6 +177,9 @@
 ** 054 CA  20131015          Added support for registration/deregistration of legacy table/field 
 **                           names.
 ** 055 ECF 20130806          Added metadata support; moved logging to J2SE logging framework.
+** 056 ECF 20131028          Ensure metadata databases are pre-loaded, even if not auto-connect.
+**                           TODO: this may not be optimal for databases which are never or seldom
+**                           connected, since this will consume memory for no good reason.
 */

 package com.goldencode.p2j.persist;
@@ -1147,8 +1150,9 @@
                            managed.add(database);
                         }

-                        // if database must be auto-connected, register it now
-                        if (perm != null && perm.booleanValue())
+                        // if database must be auto-connected or is a meta database, register it
+                        // now
+                        if ((perm != null && perm.booleanValue()) || database.isMeta())
                         {
                            // register database
                            registerDatabase(cfg, database, true);

Sorry for the delay in review, lost track of this one. Please merge up to latest code level, make my minor changes below, and regression test. If it passes, check it in (no need for me to review again unless you make further, substantive changes).

Code review 20131031a (database-related only):

Overall, looks good. Some minor adjustments:

• buffer_definitions.rules: the rule at line 508 used to be an action on="false" element. The new rule construct looks more complicated, but appears to do the same thing, plus the indent is no longer aligned properly.
• P2OAccessWorker: minor typo in the schema param javadoc of the new method isDMOInterface(String, String).
• ConnectionManager: please use package wildcards in imports, unless the specific classes listed there resolve conflicts.
• ConnectionManager: at line 398, you commented out the old recordOrThrowError call, but you can remove it. The "not a fatal error" comment is useful, though.
• ConnectionManager: the javadoc description for getCurrentUserid is a bit hard to understand. Can you rephrase it, please?
• multiple files: there are a number of places where you log messages at a higher level of severity than previously, in your switch from Apache Commons Logging: trace became FINE (normally would map to FINEST) and debug became INFO (normally would map to FINE). Was this intentional? If not, to avoid production log file bloat, please remap trace to FINEST and debug to FINE.

Ovidiu Maxiniuc wrote:

...the big problem is registering dirty databases and doing the cleanup of DatabaseManager by updateTransientRefCount()-ing.
When a database is registered, the updateTransientRefCount() is called to increment the transientDatabases counter for it. Of course, it is incremented only once per context. And, when the last buffer is finished or either the Queue stops and the SecurityContext is cleanup()-ed the counter should be decremented.

Aparently, the increments and decrements are done on (different) threads from the same context (Thread[Conversation [00000001:bogus],5,conversation] and Thread[Reader [00000001:bogus],5,protocol]).

This is the issue, in fact. The last localTransients.get() from updateTransientRefCount is called from
endContext(cts, ctx);
after
SecurityContextStack cts = SecurityContextStack.getContext();
SecurityContext ctx = cts.pop();
So practically it is not the same context any more, so the localTransients.get() will return a newly initial value.
So when clients connects and disconnects the localTransients counter will continue to increase and never decrement.
I am not sure how this issue should be fixed.

If I understand you correctly, this appears to be a bug in the security (or net?) package in the way the cleanup method is called for ContextLocal variables. We have to rely on the ContextLocal.cleanup method, in case the converted 4GL code does not properly DISCONNECT a transient database, or we have a forced session end while a transient database is connected. But if the security context already has been popped by the time we get the cleanup call, we're going to have problems, as you've discovered.

I don't think this is a new defect, but rather you've tested the transient database cases more thoroughly than we've had to do for past use cases.

Greg, Constantin: any thoughts? Should we open a new issue for this, or are we already tracking it somewhere? It seems a very familiar issue, but I can't remember where we've hit it before, or what the solution/workaround was...

I already have talked to Contstantin and he helped me with this issue; the last update contains the fix for it. See the DatabaseManager.updateTransientRefCount(Database database, boolean increment) in the update.

I saw that change, but it wasn't clear to me how this fixed the problem. I understand now: the problem was not that the wrong information was being passed to the cleanup(Set<Database>) method when called on the Reader thread, but simply that we can't call localTransients.get() inside updateTransientRefCount when it is called by ContextLocal.cleanup(Set<Database>).

I think we should update ContextLocal to throw an exception in this case (sort of like collections throw ConcurrentModificationException); that is a subtle problem to detect. I'll open a separate issue for that; no need to change it for this update.

BTW, please fix the name updateTransientRecCountWorker --> ("Ref" not "Rec").

I have the following issue:
I have the meta_user table created in the schema_table_<dbname>_<dialect>.sql and the server attempts to access it. However, the database instance is using a copy of the existing database instead of running the generated ddl to create the database schema so. As conclusion, the (primary) database lacks the newly added meta_user table so the following error is reported into log file:

[11/19/2013 06:39:44 EST] (com.goldencode.p2j.persist.id.IdentityPool$KeyScanner:SEVERE) Identity pool has been stopped because of an exception:
org.hibernate.exception.SQLGrammarException: ERROR: relation "meta_user" does not exist

The _User table should only be involved in conversion and at runtime if it was configured to be a metadata table in p2j.cfg.xml. If not (like in the regression test environment), the project should not even know about it, and should convert without it, like it did before. So, it should not appear in schema_table_<dbname>_<dialect>.sql, and should not be reported by DatabaseManager.getDatabaseTables(Database). It is this method that is used by the identity pool scanner, leading to the error you report above.

So, regression testing for this issue should really only be about making sure that your changes did not break conversion or runtime for the regression test project, but the new functionality will not really be exercised by it.

Code review 20131125d (database-related):

The database portions of this most recent drop look fine for check-in. The remaining code looks OK to me as well, but I'll let Greg confirm.

Code Review 1125d and 1121b

CustomSecurityOps has had the public access modifier removed from all the methods. It is safe to put it back without regression testing. After that change you can check in and distribute the update.

Both pdates were committed to bzr rev 10417 and resp. git.
They were also distributed to team by mail.

Please post generic instructions here (or point to them, if they already exist -- I did not find them) on how to produce the special userid/password file, and how to perform the data import using this additional data. We will have to do this very shortly for the current project.

1. Conversion time¶

To be generated with _user metadata support, the p2j.cfg.xml has to declare the <table name="_user" /> in the <metadata name="standard"> and a <namespace name="standard" ... must be defined in the <schema> part.

2. Importing the _User metadata table¶

Because we don't have an ENCODE function that does not generate the same results as P4GL's implementation, we cannot use the password column from dump data from the _user.d. Instead, a simple text file is used. It should contain n lines, having one userid and its plain password separated by a space character on each line. At import time, each userid found in the _user.d is looked up in the text file and, if found, the second token (the password) is ENCODE -d and will replace in the new database the old value exported from P4GL. If there is no password declared for a specific user, the original password is used, and the respective user will most likely be un-authenticable.
The plain text file must be named pwds.txt and must to be placed in the same location with other .d files from the dump. This file can be maintained in an office spreadsheet application and easily exported as plain text file.

3. Configuring P2J to use the _User metadata runtime authentication¶

By default, P2J will use its 'native' authentication, driven by the SecurityManager and SecurityOps and not enforce _User metadata authentication so that the existing implementation will run normally.
In order to activate the P4GL type of authentication (using the userid and password stored in the netadata table _User) a few adjustments need to be applied to directory.xml:
1. to enforce checking of userid in P4GL functions setuserid/userid with _User metadata: the relative, non-scoped userSecurityOpsClass must be set to com.goldencode.p2j.util.MetadataSecurityOps. (the default is com.goldencode.p2j.util.SecurityManagerSecurityOps which instead will make the setuserid a no-op returning yes and userid will always return the P2J user initially authenticated)
2. to enforce the logon authentication the following fragment should be set in the directory:

<node class="authMode" name="auth-mode">
    <node-attribute name="mode" value="4"/>
    <node-attribute name="retries" value="2"/>
    <node-attribute name="plugin" value="com.goldencode.p2j.security.DatabaseAuthenticationHook"/>
</node>
<node class="container" name="auth-plugins">
    <node class="container" name="user_metadata_auth">
       <node class="string" name="classname">
           <node-attribute name="value" value="com.goldencode.p2j.security.DatabaseAuthenticationHook"/>
       </node>
       <node class="string" name="description">
          <node-attribute name="value" value="Database authentication using _User metadata table"/>
       </node>
       <node class="string" name="option">
           <node-attribute name="value" value="<p2juser>"/>
       </node>
    </node>
</node>

Ovidiu, when you were working this task, did you create any test cases that explored the contents of the _User record itself, and whether/how these contents changed with invocations of SETUSERID? Specifically, I'm trying to figure out how _User._User_number is used. When a user locks/unlocks records, I need to update the _Lock-Usr field in the _Lock table. This field is an integer, but all we have available in P2J is a String for userid.

As I understand it, I need to put an integer into _Lock._Lock-Usr, which uniquely identifies the user who has changed the lock status. The code I originally wrote to do this was wrong. Now I'm looking at _User._User_number as a possible source of this information, but I can't find anything written about it. Any insight you can provide from your work on this task?

I also should note that the data export of the _User table has unknown value for the _User_number field in all records. So, this does not appear to be a permanent value. I'm guessing it may be something that is updated and only active at runtime, when a user has a live session.

Indeed, this is true (values are ?) for the rest of fields of _User exports except for _Userid/_Password and _User-Name.
I just re-tested the exports with and without the user being authenticated and there is no difference.
In the hope that the respective fields are internally altered but not exported, I have run some queries during the user's session, but again, there is no difference, they are again ?.

I an not able to tell at this moment what's the exact purpose of each fields. I focused my implementation on the fields that are used by the customer (#2177).

Going back to your problem. A user can be authenticated in two different sessions. This cannot be represented in the _User table (having two or more user numbers, one for each session the used has). I am thinking of some other table that might be storing the user-number related to a session ? Maybe of other _userxx : _UserIndexStat, _UserIO, _UserLock, _UserStatus, _UserTableStat ?