FWD » Core Development » Base Language

From #3810-406:

• AUDIT-POLICY resource
  • AUDIT-POLICY system handle
  • methods
    • ENCRYPT-AUDIT-MAC-KEY()
• AUDIT-CONTROL resource
  • AUDIT-CONTROL system handle
  • methods
    • LOG-AUDIT-EVENT()
    • CLEAR-APPL-CONTEXT()
    • SET-APPL-CONTEXT()
  • attributes
    • APPL-CONTEXT-ID

Please do the following:

• Review #6422 and the 4GL facilities it is intended to implement.
• Write testcases and use any that already exist (if any) in Testcases. Make sure you understand the facility deeply enough to implement it.
• Review the SecurityManager Auditing facilities and code. This will require you to understand the security model and implementation.

You can post your findings and thoughts and we will discuss approaches.

Hello Marian, I am trying to find the exact usage of the commands LOG-AUDIT-EVENT, CLEAR-APPL-CONTEXT and SET-APPL-CONTEXT. I was able to audit-enable a database in OE by using the following code:

File: add.st

d "AuditData":20,64;512 . f 2000000
d "AuditData":20,64;512 .
#
d "AuditIndex":21,1;64 . f 1000000
d "AuditIndex":21,1;64 .

Executed in Proenv:

prodb db empty
prostrct add db add.st
prostrct list db db.st
proutil db -C enableauditing area AuditData 

To fully enable the auditing, I found out that I had to use the Audit Policy Maintenance tool and import the audit events from C:\Progress\OpenEdge\auditing\policies.xml. I also added some audit events manually using the Events Maintenance from the tool.

I found also some hidden tables regarding auditing: _aud-*. I tried to use LOG-APPL-CONTEXT but I couldn't find where does it record the data.

DISPLAY AUDIT-ENABLED.

AUDIT-CONTROL:SET-APPL-CONTEXT("foo").

MESSAGE AUDIT-CONTROL:APPL-CONTEXT-ID.

AUDIT-CONTROL:LOG-AUDIT-EVENT(32010, "foo").

for each _aud-event NO-LOCK WHERE _aud-event._Event-id >= 32000:
   display _aud-event.
end.

The above code shows the events I added manually using the tool, but not the events I am trying to log programmatically.

Theodoros Theodorou wrote:

I found also some hidden tables regarding auditing: _aud-*. I tried to use LOG-APPL-CONTEXT but I couldn't find where does it record the data.
The above code shows the events I added manually using the tool, but not the events I am trying to log programmatically.

That seems to hold only the audit events not the actual audit data, check out this mata-data diagram - [[https://docs.progress.com/bundle/openedge-security-auditing-introduction-117/page/Audit-data-table-schema.html]]

I would say you need to look at the `_aud-audit-data` table instead.

I think that the table _aud-audit-data holds the records only for the SET-APPL-CONTEXT command. In progress documentation [[https://docs.progress.com/bundle/openedge-abl-reference-122/page/LOG-AUDIT-EVENT-method.html]] it says that the event id must be greater than 32000. So I executed the following code:

for each _aud-audit-data NO-LOCK WHERE _aud-audit-data._Event-id >= 32000:
   display _aud-audit-data.
end.

It did not return anything. Although the code:

for each _aud-event NO-LOCK WHERE _aud-event._Event-id >= 32000:
   display _aud-event.
end.

returns the events with _Event-id >= 32000 which is what we expect. I can see the entries that I added manually using the Audit Policy Maintenance tool but I cannot see the events I tried to add with LOG-AUDIT-EVENT.

Theodoros Theodorou wrote:

I think that the table _aud-audit-data holds the records only for the SET-APPL-CONTEXT command.

No, that table holds audit data for both database and application level events - it's really all explained in the diagram I've referenced in the previous email.

So I executed the following code:

[...]

It did not return anything.

That is most probably because you didn't enable that application level event through the audit policy.

Although the code:

[...]

returns the events with _Event-id >= 32000 which is what we expect.

Yes, this is only the event definition - most probably the events with id lower than 32000 are reserved for database events.

I can see the entries that I added manually using the Audit Policy Maintenance tool but I cannot see the events I tried to add with LOG-AUDIT-EVENT.

LOG-AUDIT-EVENT creates an audit record for that event - this should really go into _aud-audit-data proven the specified event is enabled in the audit policy.

I was finally able to understand how it works. So custom events (eventId >= 32000) should be added manually using the File -> Events Maintenance window in the Audit Policy Maintenance tool. After that, the custom event ids should be added into audit policies which are enabled. After these steps, the command LOG-AUDIT-EVENT(eventId, eventContext) records data if the eventId exists in an enabled audit policy.

Code Review Task Branch 6422a Revision 14547

The initial changes seem OK. There may be some slight deviations from coding standards like using specific imports instead of wildcard imports but it is close.

Most of the work expected for this task is still incomplete. For example, there is no actual support for any of the runtime auditing functionality. But before you implement that, I really expect that you will have testcases checked in and a documented/described set of functional requirements here in this task so that we can discuss how to do the implementation. There will also need to be discussions about whether or not we integrate with the auditing in FWD which is implemented in the SecurityManager.

Sorry for the mistake in the email, this task is not ready for review yet.

As I wrote in 7027, the commands AUDIT-POLICY:SET-APPL-CONTEXT and AUDIT-POLICY:LOG-AUDIT-EVENT create entries in the table _aud-audit-data so I suppose this makes this table a core part of the audit support and also part of the standard 4GL code. Should I crate this table? If yes, what are the steps for creating it correctly?

Eric: Please review this task in regard to _aud-audit-data and provide guidance.

There are tests under testcases/security/audit_control.

The command APPL-CONTEXT-ID is ? by default. The command SET-APPL-CONTEXT generates and returns a base64 string uuid (22 characters), which is assigned to the attribute APPL-CONTEXT-ID. This is also used by LOG-AUDIT-EVENT.

Good. Please keep going with tests and documentation for the behavior of the rest of the features. I'd like all of that in one place before we discuss/approve the approach.

Greg Shah wrote:

Eric: Please review this task in regard to _aud-audit-data and provide guidance.

I am not familiar with the practical/business uses of the _aud-audit-data table, nor the meaning of its metadata, but I can comment on the implementation of metadata tables in general in FWD.

Currently, all metadata tables which we support, except one, are implemented in an embedded, in-memory, shared H2 database, named _meta. The one exception currently is the _User table (meta_user in FWD). This table resides in the primary database with which it is associated, because it must be persistent, and its data is imported from a *.d file at the time of initial data migration, like that of any other primary database table.

Of the remaining tables implemented in the embedded H2 database, most metadata tables FWD supports are static; that is, they are populated at server startup time and they can be accessed (read-only) at runtime by converted application logic.

A few metadata tables are dynamic; that is, they are updated by the runtime, based on events which occur at runtime. Some examples are _Lock (meta_lock), _Connect (meta_connect), and _Usertablestat (meta_usertablestat). Early, naive implementations of the dynamic metadata tables updated the corresponding table in the _meta database with each event, but this proved much too costly. Now, the runtime manages the state represented by these tables in an efficient, in-memory data structure. That state only is updated/reflected into the corresponding database table when a query against that table is performed.

For further information, see, for example, UserTableStatUpdater, LockTableUpdater, ConnectTableUpdater in the persist.meta package. See also MetadataManager.

If the _aud-audit-data metadata needs to be persistent across server restarts and for auditing purposes, then we may need to consider implementing FWD's analog for this table in the primary database, like the meta_user table. If this is the case, we need to consider how to provide reliable persistence without sacrificing performance of normal operations. That is, as we found with the other dynamic metadata tables mentioned above, updating an audit table for every audit event is likely to create a significant performance bottleneck. OTOH, if we store state in a more efficient, in-memory data structure, we have to be certain that it is persisted and not lost in the event of a server shutdown (planned or otherwise).

The auditing is about recording important events. I found a wiki page regarding auditing Auditing. It looks like there is already an implementation regarding auditing which writes these events in a file.

The table _aud-audit-data is used to record these audit events and it should be persistent.

As I understand, this table should be dynamic (non read-only) because e.g. the command LOG-AUDIT-EVENT adds a record in this table.

One good solution is to make it work like meta_user. We can create the table at the initial data migration which I believe will not have a serious impact on performance. During runtime, if audit is enabled and e.g. LOG-AUDIT-EVENT is called then we create a record in _aud-audit-data which I believe will be quite fast.

Do you want to proceed with creating this db table or do you prefer to stay with the already implemented mechanism with files?

Greg/Eric, there is this documentation in OpenEdge: https://docs.progress.com/bundle/openedge-security-auditing-introduction-117/page/Using-the-preconfigured-OpenEdge-audit-policies.html

There are a lot of events being audited by default.

There is also this: https://docs.progress.com/bundle/openedge-security-auditing-introduction-117/page/Audit-data-table-schema.html

To what extent do we want to implement the default audit events? This includes (among many others) create/update/delete record events.

Yes, the features in Auditing are what I was referring to above in #6422-5 as the "SecurityManager Auditing". This is not the same thing as the 4GL-compatible auditing support which you are implementing and I don't think they should be linked together since one of them will need to be 4GL compatible and the other is not.

To what extent do we want to implement the default audit events?

It will depend on what the customer uses. We have not yet gotten the list from them.

Theodoros: Is there any 4GL code which can be used to enable these default audit events? I don't see anything that can do so. If that is correct, then only the external tools can be used to enable/disable audit events. Is that right?

I will say that storing database events in the database itself seems insecure, but that is something OE already does so we aren't making it less secure.

Greg Shah wrote:

Theodoros: Is there any 4GL code which can be used to enable these default audit events? I don't see anything that can do so. If that is correct, then only the external tools can be used to enable/disable audit events. Is that right?

Audit Policy Maintenance tools is supposed to be used for defining the audit policies - [[https://docs.progress.com/bundle/openedge-developer-studio-olh-117/page/Audit-Policy-Maintenance-Tool-Windows-only.html]]. However, like any other 4GL tooling this is built with 4GL so one can write/update audit tables directly and the code is available [[https://github.com/progress/ADE/tree/v12.7.0.0/auditing]]

I will say that storing database events in the database itself seems insecure, but that is something OE already does so we aren't making it less secure.

The idea is to store the audit data in OLTP database only for short time and have everything archived in a separate audit database with a more restrictive access... [[https://docs.progress.com/bundle/openedge-security-auditing-introduction-117/page/OpenEdge-auditing-recommendations.html]]

Audit Policy Maintenance tools is supposed to be used for defining the audit policies - [[https://docs.progress.com/bundle/openedge-developer-studio-olh-117/page/Audit-Policy-Maintenance-Tool-Windows-only.html]]. However, like any other 4GL tooling this is built with 4GL so one can write/update audit tables directly and the code is available [[https://github.com/progress/ADE/tree/v12.7.0.0/auditing]]

A quick look at the tools suggests that:

• They are editing the database table directly.
• They exclude any processing of event ids less than 32000, at least in the few l=places that I checked.

Is this the core idea: you edit the _aud-event to add/change events that should be audited? In other words, you just use normal 4GL database access code to modify that table. As long as your user has the rights to edit the table, then you have modified the auditing configuration.

Is that right?

Greg Shah wrote:

Is this the core idea: you edit the _aud-event to add/change events that should be audited? In other words, you just use normal 4GL database access code to modify that table. As long as your user has the rights to edit the table, then you have modified the auditing configuration.

Is that right?

Pretty much yes, just like the data administration/dictionary there isn't much one can do from 4GL beside updating (and later using) tables in a database :)

Theodoros wrote:
Will I need to implement the database functionality?

Yes. Since the 4GL just exposes auditing (configuration and the actual events) as a persistent database table, I don't see how this can be translated into our existing audit facility. Otherwise any 4GL code that depends on it would not work.

The details of how to implement this will need to be guided by Eric or someone else on the persistence team.

I am thinking of creating a persistent metadata table similar to meta_user for the _aud-audit-data. Something like meta_aud_audit_data. Should I proceed?

Yes

Hello Eric and Ovidiu. Greg asked me to ask for a review regarding my implementation of meta_aud_audit_data. To test my implementation, please checkout p2j branch 6422a and rebuild the application with something like:

message audit-control:set-appl-context("a", "b", "c").
message audit-control:appl-context-id.

As 6422a currently is implemented, we are taking a round trip to the database to insert a record into the meta_aud_audit_data for every audited event. If, as Constantin notes, default auditing includes very frequent operations, such as every create/update/delete record event, this is likely to have a very adverse affect on performance. We have seen this time and again with other live/dynamic metadata tables.

Igor, with other implementations of live metadata tables, we have implemented much faster alternatives to the "hit the database for every event" approach. Can you give some guidance on how to improve this? I touched on this in #6422-17, though I am not sure how appropriate the approach we are using for other live tables is for an audit mechanism.

Eric Faulhaber wrote:

As 6422a currently is implemented, we are taking a round trip to the database to insert a record into the meta_aud_audit_data for every audited event. If, as Constantin notes, default auditing includes very frequent operations, such as every create/update/delete record event, this is likely to have a very adverse affect on performance. We have seen this time and again with other live/dynamic metadata tables.

Igor, with other implementations of live metadata tables, we have implemented much faster alternatives to the "hit the database for every event" approach. Can you give some guidance on how to improve this? I touched on this in #6422-17, though I am not sure how appropriate the approach we are using for other live tables is for an audit mechanism.

Eric,
Please give some time to recall the details.
Thank you.

The idea of the optimization of the _Lock and _Usertablestat VST support is to postpone updating the corresponding tables until the application accesses them. See LockTableUpdater and UserTableStatUpdate.
We keep the tables' data in memory and maintain two AtomicLong monotonic counters containing the 'version' of the in-memory data (which is increased on any update) and the 'version' of the persisted data in the database VST.
There is a persist method which compares these two versions, and, if the in-memory data version is greater than the database one, the changes are applied to the database.
The persist is called from the 'hook' in the Query.createQuery when access to the corresponding VST is detected.

Due to a different semantics of the _Lock and _Usertablestat VST, the details are different.

• _Usertablestat records are updated many times. To avoid locking the in-memory data for the whole duration of the persist call, each in-memory record also contains two counters - version and persisted with the same semantics as the 'global' version described before.
  The in-memory data are persisted in a scope of a single transaction.
• _Lock records are updated only once (when deleted). This makes the logic simple, but we still maintain the version and peristed counters for each in-memory record. In addition, the LockTableUpdater exposes the locks' statistics to JMX, which in particular was very useful in the debugging of the code.

The common peculiarity of the _Usertablestat and _Lock is that both VST contain a limited number of records and contain transient data which is not supposed to survive the server restart. This allows us to keep all data in memory and flush the state to the database on demand only. In particular, if the application never explicitly accesses these VSTs, the persis method will never be executed.

1. The audit records are never updated.
2. The number of records can grow w/o any reasonable limit.
3. The data should survive the server restart.

Since the records are effectively immutable, we can keep them in an in-memory Queue instead of a Map. This is a good news, but, the only one good news.
We cannot work only in memory, both because of the size of the data and its non-transient nature. This means that we must flush the data to the database time to time regardless of the application access to the audit tables.
I've faced this situation before and found that it is convenient to do in a single thread based on two thresholds: max number of pended changes and max time before persist. The timed version of the CountDownLatch.await can be used to wake up the thread when one of the thresholds is crossed.
Of course, the persist should be called as a part of the normal server shutdown.

Please note that some of the events can still be lost in case of server crash and I cannot suggest a simple way to deal with this. Based on my experience, the reliable solution should use some clustered queue with persistent storage. This can be e.g. a cluster based on ActiveMQ or Kafka. The FWD server can start the cluster on startup and check if some audit records from the previous 'life' should be persisted.

I agree with Igor.

I investigated the classes a bit and as I understand they keep some info in memory instead of updating the db every time. In my scenario, I do not have massive updates, only insertions.

For my scenario I think it is not a good idea to keep data in memory because we might loose audit events if the server crashes. By the way I did a quick measurement and each insertion in postgres takes around 10ms (FWD and postgres are running on the same machine).

Some solutions I want to propose:

1. Leave it as it is for now because just 2 commands are adding in this table (set-appl-context and log-audit-event)
2. Perform the insertions asynchronously (add tasks to a threadpool)
3. Perform the insertions asynchronously using java nio (use the java Non-blocking IO capabilities which needs fewer actual threads). This is more advanced, harder to develop and maintain but a bit more efficient.
4. Write each event into a file and when the file gets n records (or t seconds have passed), we flush them into the DB as a bulk insert.
5. As Igor stated, we can use Kafka to store these messages and use something like kafka connect to write them in our database. If I am not mistaken, ActiveMQ cannot be used for storing messages, it deletes the messages if there are no active consumers or all the consumers have received the message. This option is the most complex one.

Igor/Theodoros: These are good analyses.

Leave it as it is for now because just 2 commands are adding in this table (set-appl-context and log-audit-event)

I am hoping we are going to defer implementation of the full predefined audit events support (event_id < 32000).

That would mean that this direct approach is probably OK for an immediate/first implementation. Its javadoc should make it clear that this is an early/provisional implementation that is definitely going to be reworked when the full predefined audit events support is implemented.

As Igor stated, we can use Kafka to store these messages and use something like kafka connect to write them in our database. If I am not mistaken, ActiveMQ cannot be used for storing messages, it deletes the messages if there are no active consumers or all the consumers have received the message. This option is the most complex one.

I think this will probably be our best choice. Because of the nature of auditing, we cannot accept lost data nor allow any appreciable in-memory queue approach. On a positive note, the database insert portion could be asynchronous and independent of the persistence layer which is a big win. This assumes there is no special behavior that requires us to implement using the persistence layer. This last part worries me.

Theodoros Theodorou wrote:

5. As Igor stated, we can use Kafka to store these messages and use something like kafka connect to write them in our database. If I am not mistaken, ActiveMQ cannot be used for storing messages, it deletes the messages if there are no active consumers or all the consumers have received the message. This option is the most complex one.

Actually I think that persisting events only via Kafka server is not a very good idea. With a fault-tolerant cluster it requires a pretty complicated logic. W/o a cluster the data can be lost because of the Kafka server abend. What I have in mind is to use Kafkab(or similar) cluster for maintaning a fault-tolerant persistent journal which will be used only for the recovery on the FWD server startup after crash.

I was not working with ActiveMQ for several years. AFAIR it has multiple deployment options including persistent queue/topic. However, I may be wrong. Anyway, Kafka seems to be a better option.

I've confirmed with the customer that they do not need the predefined events support. At this time, let's just get the features of this task finished and we will defer the work on predefined events (and on a better approach for persistence to that table).

Code review 6422a/14552-14554:

I reviewed the code from the perspective of persistence and coding standards, but TBH, I do not have a good grasp on the legacy audit behavior being represented here. Someone else with that knowledge needs to do a review from that perspective as well.

Persistence

In AuditDbOps.writeAuditDataInDb, we have this:

      String sql = "INSERT INTO meta_aud_audit_data (recid, audit_data_guid, database_connection_id," +
         " client_session_uuid, user_id, audit_date_time, audit_date_time_offset, audit_event_group, " +
         "db_guid, transaction_id, transaction_sequence, event_id, event_context, application_context_id, " +
         "event_detail, audit_custom_detail, audit_data_security_level, data_seal) VALUES ((select count(*)" +
         " from meta_aud_audit_data), ?, ?, ?, ?, ?, ?, ?, ?, (select count(*) from meta_aud_audit_data), " +
         "?, ?, ?, ?, ?, ?, ?, ?)";

There are a few problems here:

• While the primary key name defaults to recid and this default is most often used, it is configurable. The primary key name a project has configured is accessible at runtime via the public, static variable Session.PK (in the com.goldencode.p2j.persist.orm package). Since there is string concatenation involved in creating the INSERT statement, it therefore is probably best to generate the sql string for this insert once and store it in a private, static variable.
• The primary key of any record in any legacy table must be unique across the database. Instead of generating the primary key value with the subselect select count(*) from meta_aud_audit_data, please use one of the following alternatives:
  • replace the subselect with ? and pass the primary key as a substitution parameter, getting the value from Persistence.nextPrimaryKey(); or
  • replace the subselect with nextval('p2j_id_generator_sequence');
  • the former is preferred because if we ever need to change the primary key generation strategy, it is more abstract than directly referencing the sequence.
• transaction_id presents a similar problem. Using the subselect select count(*) from meta_aud_audit_data could give you the same transaction_id value across concurrent sessions. I don't know whether this is allowed for this feature or not, but I would suggest using a dedicated sequence for this. Since is no such sequence currently, the DDL generation related to the legacy audit table(s) would have to be modified to include a CREATE SEQUENCE statement.

General / Coding Standards / Formatting

I'm not sure if this is still necessary (Greg or anyone, please correct me if this idiom is outdated)...normally, when I write code to read a directory setting, it looks something like this:

         DirectoryService ds = DirectoryService.getInstance();
         if (!ds.bind())
         {
            // throw an exception (unchecked if in a static initializer)
            throw new RuntimeException("Directory bind failed");
         }

         try
         {
            // read from the directory
            ...
         }
         finally
         {
            ds.unbind();
         }

Are the bind/unbind still necessary? Are we leaking resource if we don't explicitly unbind?

In AuditDbOps, please change getdatabasesAuditEnabledMap to getDatabasesAuditEnabledMap.

The javadoc for the return in the implemented methods in AuditPolicyManager is left as "n/a". Please replace this with an actual description of the return values.

Please ensure implements <list of interfaces> follows the coding standards w.r.t. newlines and indents.

Please ensure unnecessary/unused import statements are removed.

Please ensure opening curly braces (e.g., AuditPolicyManager.java:298) are on a separate line.

Some files had their file headers and license text indented by 1 space, as if they represented javadoc. This was most likely done by the IDE, but please set it back.

When cut-and-pasting a file header from another source file, please ensure the "Module" and "Abstract" text is updated (e.g., see AuditDbOps.java).

• Added "Session.PK" and made sql string as private static.
• I also wasn't sure about this and I made an easy solution to just work. Added Persistence.nextPrimaryKey();.
• Used the same id as above for the transaction_id. I do not think that this number matters a lot to make extra effort for a new sequence.

• I think the bind/unbind is not needed because I think there was not present from the place I copied the code. Please let me know if I should add it.
• Changed to getDatabasesAuditEnabledMap.
• Updated the javadoc.
• I think I haven't added any implements in my code. Please let me know if I need to change anything.
• Removed unused imports.
• Put the curly brace on a separate line.
• Removed one space from the license text.
• Updated the "Module" and "Abstract" text in AuditDbOps.

Eric: This needs your code review.

Eric: Please review.

Kind reminder for a review

Code review 6422a/14649-14653:

Please note again that I am not familiar with the legacy audit feature set, so I am just looking at this from a persistence, coding standards, and general programming POV. I have to assume the functionality of the auditing is correct and has been / will be verified through testing. If someone better understands the functional requirements, it would be worth a separate review.

Theodoros Theodorou wrote:

• Added "Session.PK" and made sql string as private static.

Good.

• I also wasn't sure about this and I made an easy solution to just work. Added Persistence.nextPrimaryKey();.

Good.

• Used the same id as above for the transaction_id. I do not think that this number matters a lot to make extra effort for a new sequence.

This seems reasonable to me.

• I think the bind/unbind is not needed because I think there was not present from the place I copied the code. Please let me know if I should add it.

Please add it. I've reviewed many places where DirectoryService.bind() is used, and while there certainly are some cases which do not unbind, there seem to be many more cases which do. I don't know the reason some do not, or whether it represents a bug in those cases. I guess any resources are cleaned up upon the end of the session, but I think an explicit unbind should be done.

Among those areas of the code which do the unbind, some do not do it in a finally block, which is ok if an exception can not be thrown between the bind and the unbind. If there is a chance that an exception can be thrown after a successful bind, we should wrap the work after binding in a try block and the corresponding unbind in a finally block, as in the example I posted in my earlier review.

• Changed to getDatabasesAuditEnabledMap.

Good.

• Updated the javadoc.

Good.

• I think I haven't added any implements in my code. Please let me know if I need to change anything.

Sorry, I must have meant the extends clause (e.g., see AuditControlManager, AuditPolicyManager). Same rule applies.

• Removed unused imports.

Good.

• Put the curly brace on a separate line.

Good.

• Removed one space from the license text.

AuditDbOps was fixed, but the space is still there in the AuditControlManager license text (also in the file header).

• Updated the "Module" and "Abstract" text in AuditDbOps.

Good.

Eric Faulhaber wrote:

Code review 6422a/14649-14653:

• I think the bind/unbind is not needed because I think there was not present from the place I copied the code. Please let me know if I should add it.

Please add it. I've reviewed many places where DirectoryService.bind() is used, and while there certainly are some cases which do not unbind, there seem to be many more cases which do. I don't know the reason some do not, or whether it represents a bug in those cases. I guess any resources are cleaned up upon the end of the session, but I think an explicit unbind should be done.

Among those areas of the code which do the unbind, some do not do it in a finally block, which is ok if an exception can not be thrown between the bind and the unbind. If there is a chance that an exception can be thrown after a successful bind, we should wrap the work after binding in a try block and the corresponding unbind in a finally block, as in the example I posted in my earlier review.

I have added bind/unbind in a new class named DirectoryServiceHelper which I believe will be helpful in the future. Please take a look.

• I think I haven't added any implements in my code. Please let me know if I need to change anything.

Sorry, I must have meant the extends clause (e.g., see AuditControlManager, AuditPolicyManager). Same rule applies.

Done

• Removed one space from the license text.

AuditDbOps was fixed, but the space is still there in the AuditControlManager license text (also in the file header).

Done

Please review 6422a rev no 14906

Code review 6422a/14902-14906 (after rebase):

The changes look good to me.

Some minor coding standards or format issues in DirectoryServiceHelper:

• Lines 7-8 should be a single line.
• Move private static method after public static.

If these are the only changes you make (i.e., no further logic changes), I don't need another review and you can move to internal test status.

Theodoros: Please rebase this to the latest trunk revision 14903.
Constantin: Please review after this is rebased.

Code Review Task Branch 6422a Revisions 14889 through 14906

I'm OK with the changes.

Constantin Asofiei wrote:

Review of 6422a rev 14906 (the entire code):

• AuditControlManager
  • we don't/can't use volatile for context-local state. All this state must be kept in ContextLocal variables (with a WorkArea as a container for them). This applies for both DATABASE_CONNECTION_ID and APPL_CONTEXT_ID.

Ok, done

• getApplContextId - BDT variables are mutable - a getter must always return a new instance, never expose the instance outside of the class, unless you are doing this explicitly for performance/other reasons.

Ok, done

• is APPL_CONTEXT_ID meant to be client-specific, database-specific, something else? Same for DATABASE_CONNECTION_ID - what happens if there are multiple databases connected - is this value the same for all databases?

I think APPL_CONTEXT_ID is client specific and it is the same for all databases. When the program starts, it has null value, setApplContext assigns a value to it and clearApplContext makes it null again. DATABASE_CONNECTION_ID is assigned when a database is connected. DATABASE_CONNECTION_ID changes when the database is disconnected and connected again. This value is different for each database. Where should I store the databaseConnectionId? In Persistence.java?

• why use System.getProperty("user.name")? We need to identify the FWD/DB user, and not OS user, AFAIK.

Ok. There is /security/accounts/osusers/osuser and webClient/defaultOsUser in hotel_gui. Which one should I use?

Changes were made to branch 6422a rev no 14924

• AuditDbOps
  • is meta_aud_audit_data hard-coded SQL name valid for all supported dialects? Eric: here I mean also lenient MariaDB mode...

I am not aware of the lenient MariaDB mode. I tested it with h2 and postgres.

• in 4GL, when calling the LOG-AUDIT-EVENT method, is this dependent on a tx being currently open at 4GL-level, for all databases with audit enabled?

What is tx? In 4GL, LOG-AUDIT-EVENT logs to all connected audit enabled databases. If there are no connected audit enabled databases, the method works as before, but it does not record anything to any database.

• is LOG-AUDIT-EVENT logging to all connected databases in 4GL?

It logs to all connected audit enabled databases in 4GL.

• what happens if the 4GL tx is rolled back? Are the audit events lost in 4GL? Or do we need to create a micro-tx for this insert, and always ensure is committed to the db?

I haven't checked this scenario. How can I perform a rollback in 4GL?

• DATABASE_AUDIT_ENABLED - why is this list hard-coded? At runtime dbs can get connected/disconnected 'at will'.

I assumed that in FWD, all the databases are connected when the server starts (read from directory.xml) and no database can be connected or disconnected at runtime. If my assumption is wrong, please explain how can I connect/disconnect databases in FWD 'at will' during runtime and I will fix it.

• AuditPolicyManager
  • isAuditEnabled(seqNo) - what is 'seqNo' here in 4GL terms? Is this an index into 'all connected databases'? What happens if a db gets connected/disconnected at runtime?

Yes, it is an index to all connected databases. I assumed that databases cannot be connected/disconnected at runtime in FWD.

• isAuditEnabled(alias) - here an alias or a ldb name can be used. The code does not consider aliases, only ldb names.

How can I read the alias of the database? Is alias already supported or should I add something like "database/" + database + "/alias" in the directory.xml?

• for both audit resources, the id() method is incorrect - don't hard-code this, look into SessionUtils$WorkArea.id how is computed

Ok, I made them null as in SessionUtils$WorkArea.id.

Ok. There is /security/accounts/osusers/osuser and webClient/defaultOsUser in hotel_gui. Which one should I use?

Neither. This is a runtime property of the current security context. You can't look it up in the database. See the USERID builtin function, which in FWD is implemented in SecurityOps.getUserIdFromDB().

I assumed that databases cannot be connected/disconnected at runtime in FWD.

We support the CONNECT statement just like the 4GL does, except we don't support every option. Use that for runtime connection. It is commonly used.

Theodoros Theodorou wrote:

• AuditDbOps
  • is meta_aud_audit_data hard-coded SQL name valid for all supported dialects? Eric: here I mean also lenient MariaDB mode...

I am not aware of the lenient MariaDB mode. I tested it with h2 and postgres.

Add mariadb,mariadblenient to ddl-dialects in p2j.cfg.xml, run conversion again, and check the table/field names in all dialects that they match.

• in 4GL, when calling the LOG-AUDIT-EVENT method, is this dependent on a tx being currently open at 4GL-level, for all databases with audit enabled?

What is tx? In 4GL, LOG-AUDIT-EVENT logs to all connected audit enabled databases. If there are no connected audit enabled databases, the method works as before, but it does not record anything to any database.

By tx I meant a transaction.

• what happens if the 4GL tx is rolled back? Are the audit events lost in 4GL? Or do we need to create a micro-tx for this insert, and always ensure is committed to the db?

I haven't checked this scenario. How can I perform a rollback in 4GL?

Use UNDO, LEAVE.

• isAuditEnabled(alias) - here an alias or a ldb name can be used. The code does not consider aliases, only ldb names.

How can I read the alias of the database? Is alias already supported or should I add something like "database/" + database + "/alias" in the directory.xml?

By alias I mean create alias statement.

Greg Shah wrote:

Ok. There is /security/accounts/osusers/osuser and webClient/defaultOsUser in hotel_gui. Which one should I use?

Neither. This is a runtime property of the current security context. You can't look it up in the database. See the USERID builtin function, which in FWD is implemented in SecurityOps.getUserIdFromDB().

Ok, I used SecurityOps.getUserIdFromDB(). In 4gl, MESSAGE USERID('hotel') returns fwd which is our windows username, but in FWD, it returns an empty string. Is it expected?

Ovidiu: Do you recall the status of our userid implementation? What should be happening here?

I remember we have full support here. IIRC, there were some differences between Linux and Windows and whether the _User table is populated. Or maybe these only happens with SETUSERID function.

On my workstation MESSAGE USERID('fwd'). prints om which is my Ubuntu username (fwd is my working database). OTOH, if the parameter is unknown/empty or not a valid/connected database name/alias the SecurityOps.BLANK_USER; (that is, the empty string) is returned by SecurityOps.getUserIdFromDB().

Please check whether the hotel database is connected at the moment USERID() function is invoked.

Ovidiu Maxiniuc wrote:

Please check whether the hotel database is connected at the moment USERID() function is invoked.

I have executed the following code with the latest versions of p2j and hotel_gui on my linux machine:

def var i as int no-undo.

message num-dbs. // prints: 1

do i = 1 to num-dbs:
  message ldbname(i). // prints: hotel
end.

message userid('hotel') eq ''. // prints: yes

I did some debugging and I found that the lookup is happening inside ConnectionManager.java at Map<String, String> userIds = new LinkedHashMap<>(). This map contains "hotel" -> "". The map is populated in the ConnectionManager.java:

if (!pendingDisconn.remove(database) && !pendingTxDisconn.remove(database))
{
   putConnected(ldbName, database, userId = null);
}
else
{
   putConnected(ldbName, database, userId = SecurityOps.getDefaultUserid());
}

For me, the first case is executed (userId = null). Inside the SecurityOps.getDefaultUserid(), it reads System.getProperty("user.name") but it is not executed.

It seems like you code is a bit different than the trunk. But it still should work:

In putConnected(), there is a null userId detection:

      if (userId == null)
      {
         // userId will be null for a local database; this call has to occur after adding the LDB
         // name to the connected map
         userId = SecurityOps.getDefaultUserid(ldbName);
      }

As result, when the method is called with userId = null as last parameter, the OS username should be used instead. However, this happens inside the putConnected() method. The caller, makeConnection() is not directly aware that the userId was adjusted.

Rebased 6422a from trunk revision 14971. The latest revision in this branch is now 14996.

Ovidiu Maxiniuc wrote:

It seems like you code is a bit different than the trunk. But it still should work:

In putConnected(), there is a null userId detection:
[...]

As result, when the method is called with userId = null as last parameter, the OS username should be used instead. However, this happens inside the putConnected() method. The caller, makeConnection() is not directly aware that the userId was adjusted.

The branch has been rebased to trunk recently. In my case with the default hotel_gui configuration, the if (customSecurityOps.getAuthLevel(ldbname) == SecurityOps.FREE_ACCESS) returns false, so the return BLANK_USER; is executed (BLANK_USER = ""). On my machine, customSecurityOps.getAuthLevel(ldbname) is equal to 1 and SecurityOps.FREE_ACCESS is equal to 0.

Constantin Asofiei wrote:

• what happens if the 4GL tx is rolled back? Are the audit events lost in 4GL? Or do we need to create a micro-tx for this insert, and always ensure is committed to the db?

The code below commits to the database in both 4GL and FWD.

message AUDIT-CONTROL:LOG-AUDIT-EVENT(32001, 'test').
UNDO, LEAVE.

I checked the code and the method Persistence.executeSQL() calls local.commit() which commits to the database if there was no started transaction. Is there something I am missing regarding transactions?

Theodoros Theodorou wrote:

Constantin Asofiei wrote:

• what happens if the 4GL tx is rolled back? Are the audit events lost in 4GL? Or do we need to create a micro-tx for this insert, and always ensure is committed to the db?

The code below commits to the database in both 4GL and FWD.

Did you execute this in a DO TRANSACTION block in 4GL?

Theodoros Theodorou wrote:

I checked the code and the method Persistence.executeSQL() calls local.commit() which commits to the database if there was no started transaction. Is there something I am missing regarding transactions?

Maybe. I am not sure what you are asking. Persistence.executeSQL will try to begin a (micro) transaction to do its work. If it succeeds, it will commit this transaction immediately. If beginning a new transaction does not succeed, that indicates we already are operating within the scope of a larger transaction begun previously by the application. We do not commit in executeSQL in this case, because that would interfere with the semantics of the existing transaction, which must commit or rollback sometime later. When that commit occurs is governed by the application, so we cannot cause a premature commit for auditing (or for whatever executeSQL is doing).

The following code does not write into the db in 4GL, but in FWD it does.

DO TRANSACTION:
    MESSAGE AUDIT-CONTROL:LOG-AUDIT-EVENT(32001, 'test').
    UNDO, LEAVE.    
END.

I tried to debug the code to see why this is happening. As I understand, the doBlock(TransactionType ...) does not call Session.beginTransaction() to make inTx = true. When persistence.executeSQL() is executed, there is no started transaction found, so it immediately commits the audit event to the db. Am I doing something wrong?

Can someone please have a look?

Eric: This is for you.

Theodoros Theodorou wrote:

The following code does not write into the db in 4GL, but in FWD it does.

[...]

I tried to debug the code to see why this is happening. As I understand, the doBlock(TransactionType ...) does not call Session.beginTransaction() to make inTx = true. When persistence.executeSQL() is executed, there is no started transaction found, so it immediately commits the audit event to the db. Am I doing something wrong?

I don't think so. There is probably a bug in the persistence runtime, in that we assume there has to be a record buffer with an open scope in order to begin a database transaction after the business logic requests one.

Database transactions are created lazily, not immediately when the DO TRANSACTION (in this case) statement is executed. What's happening instead is that we create a TxWrapper at the DO TRANSACTION block. This is an object that is ready to begin a transaction, if something happens that requires it (e.g., a record buffer gets initialized). This avoids the overhead of creating a database transaction when one is actually not used.

Look at how the TxWrapper$WorkArea.inactiveTxWrappers map is used, especially when TxWrapper$WorkArea.createTxWrapper is called. For a TxWrapper to be activated and have it begin a transaction, maybeActivateTxWrapper must be invoked. There is probably a bug here, based on the assumption that we have to have an open buffer to activate an inactive TxWrapper (I should have named this map pendingTxWrappers; inactiveTxWrappers suggests they were active and are no longer, but it really means "not yet activated" transaction wrappers).

In your example, there is no open record buffer. In fact, there is no record buffer at all; the MESSAGE ... statement causes a database write (enclosed in its own micro-transaction) without one.

We probably need to invoke TxWrapper$TransactionHelper.maybeActivateTxWrapper(Database) inside the auditing code that is run when MESSAGE AUDIT-CONTROL:LOG-AUDIT-EVENT(32001, 'test') is run (before calling persistence.executeSQL). That would activate the TxWrapper created at the DO TRANSACTION block scope, creating a database transaction scoped to that block. When persistence.executeSQL is then invoked, it would not create a micro-transaction, and the write would rollback when we hit the UNDO, LEAVE.

How can I access TxWrapper$TransactionHelper.maybeActivateTxWrapper? I currently have the following code (branch 6422a AuditDbOps.java):

for (Database database : ConnectionManager.getConnectedDatabases()) {
  ...
  Persistence persistence = ConnectionManager.getPersistence(database.getName());
  ...
  persistence.executeSQL("INSERT INTO...");

Is this the correct way to write into the databases considering transactions and rollback or there are other ways?

The only thing pending for this ticket is #6422-65