FWD » Core Development » Runtime Infrastructure

The point of this task is to implement admin REST APIs for interacting with the tenant authentication tables: sec_authentication_domain and sec_authentication_system.

For the moment, some code resides in SecurityOps$WorkArea, such as creating the default domains, adding a new domain, finding a specific domain. We should provide more customization for these tables (insert/update/delete), and also expose these methods via the admin console.

Greg/Constantin: Is it OK if the work is done in TenantHandler from 6229b? Considering that these tables are tenant-based, I think that grouping these makes sense.

Additional note: if so, then the HTTP request path would have to start with /admin/tenant, but I don't necessarily see a problem with this.

I thought these tables were also used outside of multi-tenant. If so, then we want the API to be independent.

Greg Shah wrote:

I thought these tables were also used outside of multi-tenant. If so, then we want the API to be independent.

I've only seen them be used in multi-tenancy scenarios, but I don't know where to find the exact information. To me, sec_authentication_domain would make sense to be used only in multi-tenant mode since it has a tenant_name column that needs to be assigned when a new domain is added. As for the other table, I really don't know. If you wish, I can provide the definition of the two tables, or we can ask someone who was more knowledge on this.

I thought these tables were also used outside of multi-tenant. If so, then we want the API to be independent.

I've only seen them be used in multi-tenancy scenarios, but I don't know where to find the exact information. To me, sec_authentication_domain would make sense to be used only in multi-tenant mode since it has a tenant_name column that needs to be assigned when a new domain is added. As for the other table, I really don't know. If you wish, I can provide the definition of the two tables, or we can ask someone who was more knowledge on this.

Marian: Please comment.

We are deferring this work for now. I want to complete #6229 first and we will come back later to work this.

Greg Shah wrote:

Marian: Please comment.

I’m sure both are used outside of the ‘multi tenant’ environment, both authentication system and domains pertains to authentication functionality, the tenant there is only a mean to group the domains per tenant (if used), otherwise the tenant field is simply not used.

This is from the top of my head, i can double check when i get to my laptop but I’m pretty sure it is like this.

Marian Edu wrote:

I’m sure both are used outside of the ‘multi tenant’ environment, both authentication system and domains pertains to authentication functionality, the tenant there is only a mean to group the domains per tenant (if used), otherwise the tenant field is simply not used.

Then this would mean that the API needs to be independent, as Greg said. For achieving this, do we need to add another handler (like TenantHandler, RestHandler, SoapHandler), or can it be integrated somehow into one of these?

Also, knowing that these tables can be used outside the multi-tenant environment, then this would mean that the two tables should not be part of the landlord database, right? If so, where should they be stored (or more precisely, where are they stored now, in the current codebase)?

For achieving this, do we need to add another handler (like TenantHandler, RestHandler, SoapHandler), or can it be integrated somehow into one of these?

I think we might rename TenantHander to AdminHandler and make it support the full range of admin APIs. For now it is fine to leave the TenantHandler name as is.

Also, knowing that these tables can be used outside the multi-tenant environment, then this would mean that the two tables should not be part of the landlord database, right? If so, where should they be stored (or more precisely, where are they stored now, in the current codebase)?

Today (which is always single tenant) we import these table into the same logical database instance as the rest of the data. It is my understanding that if you had multiple logical databases, each one would have its own set of auth tables. In other words, this is database-level authentication.

As discussed above, we can't keep these tables in the individual database instances for the mutli-tenant case since the tables store data for all tenants.

If my concept is correct (one set of auth tables per logical database), it does suggest a conflict with the landlord db. But I still think the landlord db is the right place for it, we just would have to add a way to "multipex" it.

• a list of APIs needs to be creates (with the set of parameters and responses) and exposed;
• the implementation will route the calls through persistence framework so that it will handle automatically save/update/cache/locking events. Working on a low SQL level will interfere with normal persistence data flow and it is not desired. Due to minimal execution flow (also, these APIs are only used for provisioning, at a low server load), the performance is not paramount;
• each method will dispatch the calls to specific logical databases;
• a new class will be implemented to receive these calls. We will need to decide it will have an all methods static (which will receive the database for lookup at each call) or a mapping of one object for each persistence (early database lookup);
• with the proper Persistence object for each logical database the parameters will be converted to DMO implementation of a minimal interface (in a similar manner to LockTableUpdater$MinimalLock).
• after processing, each of these primitive will return a specific message, which will be returned to client, along with an optional error code.

Most likely, Radu will handle the first 3 and last bullet and Ovidiu those related to persistence. Depending on the work flow and task assignments, this might change.

I re-started working on this issue for a couple of days now.

I am implementing the support for keeping in sync the 'landlord' database with the meta tables, mainly the _Tenant. This means, for each call from tenant administration REST API, after performing the changes into the persistent landlord database, the changes (create/delete/update) must be updated into FWD so that the users can see the changes in-real-time using 4GL code.

The solution proposed initially was not optimal because the code was dependent on the conversion changes. The runtime names of tables/properties had to be known in advance at compile time, making this a bit of chicken-and-the-egg issue. Because of this, the code might stop working if the conversion changes these names in the slightly way. Instead I decided to go for a fully dynamic solution, in which the operations of _meta database to be executed using the dereferenciation of original 4GL names.

But things prove to be a bit more complicated than expected. In both cases, the operations require a persistence and a user context. The Persistence object is available, but the context is not. That is because the REST API are not executed in a true context. Without one, usual operations are not available or will end with error. Until now. Currently, I am in progress of deciding what is the best course of action here.

Status after additional work on this issue:
I have been successful with my attempts. I created a special buffer object (proxy) that is used only for _meta tables. I was able to emulate a user context and a micro-transaction. The records are correctly written to back-end.

A special case when a record (for example, a _Tenant) is updated (state flag changes). The record is located using its name (it should be unique according to the index). Maybe a better solution here would be to use the PK. For sure, in 4GL that would give direct access to the record, but in FWD searching for the name of OK is pretty much identical effort, both of these are uniquely indexed.

However, there I am still facing a bit of a problem: these special buffers do not release the LOCK when the buffer is released and neither when the transaction ends. The former happens because there is an ongoing transaction (it downgrades pinned lock type, instead). I still do not know why the latter does not free the locks.

I can try to release the lock manually, emulating an find-current(no-lock), but this must happen during the flush. Since for the moment I use a release(), after it is executed there is no record in the buffer to release the lock.

API design feedback:

1. The admin API here should not be specific to tenants. It is a generic API for editing the _sec-auth* tables which are involved with all of the newer OE authentication features including things like SSO and CLIENT-PRINCIPAL.

• The endpoint route should not be /admin/tenant/... but instead /admin/legacy_auth/....
• All columns and both tables should be supported and documented.
• Documentation should be in Legacy Security Authentication Administration API

2. The GET should allow retrieval of a single entry in the table (in addition to the read all rows mode).

Greg, thank you for the valuable feedback. I was focused more on the backstage of the implementation, more specifically, on the issues needed to overcome.

1. The admin API here should not be specific to tenants. It is a generic API for editing the _sec-auth* tables which are involved with all of the newer OE authentication features including things like SSO and CLIENT-PRINCIPAL.

• The endpoint route should not be /admin/tenant/... but instead /admin/legacy_auth/...

This can be easily adjusted. Also I am thinking of renaming of TenantHander as noted in #8976-8 / #8976-9.

• All columns and both tables should be supported and documented.

The attribute names from both received/sent JSON request/response are kept the very same from meta table. I considered this will make clear what is sent/received and there is no ambiguity whether the name is in camelCase (ex: domainDescription), snake_case (ex: domain_description), or other (ex, some abbreviations: domain_desc). It's also the information quickly available, without using an intermediary map lookup.

• Documentation should be in Legacy Security Authentication Administration API

I will copy/move it there after the implementation gets stable.

2. The GET should allow retrieval of a single entry in the table (in addition to the read all rows mode).

OK. It makes sense that without additional parameter the GET will return only a list of the domains, without their details and return the details only if a specific domain is requested from the database.

I am thinking of some changes in the existing Tenant API. I will come back with details as I get a more clear image.

Ovidiu Maxiniuc wrote:

I am thinking of some changes in the existing Tenant API. I will come back with details as I get a more clear image.

Greg, I created #9602 and exposed my thoughts.

I was able to pinpoint the cause of my instability. As I said, the (write) accesses to (meta)_sec_auth tables are performed in small transactions which are performed just above the global scope. When such a block is processed, some of the resources are stored one level up (like BufferManager, SavepointManager) so that they are available immediately after the block has ended. In my case the are added to globalBlock,finalizables. The problem is that the above resources are actually not deleted. Things works correctly for the first time, but in subsequent calls, while doing the clean-up, the finalize for these resources is called a second time, causing internal imbalance.

Constantin,
I was able to fix the issue with a single line:

--- old/p2j/src/com/goldencode/p2j/util/TransactionManager.java
+++ new/p2j/src/com/goldencode/p2j/util/TransactionManager.java
@@ -4839,6 +4839,7 @@

             if (globalTerm)
             {
                processFinalizables(wa, wa.globalBlock, false);
+               wa.globalBlock.finalizables = new Set[WeightFactor.WEIGHTS_ARRAY_SIZE];
             }

My question is: will this interfere with normal execution of a client? Otherwise, I will have to implement some kind of awareness inside each resources to avoid duplicate finalization.
OTOH, I think that the clean-up of this list should always happen. At the moment, we rely on the fact that the blocks will be removed naturally from the stack, but in this case the global level is involved and it is never removed, unless the context is completely dropped.

• enclose all calls from TenantTableUpdater or others which currently have doBlock in an externalProcedure
• this externalProcedure also needs to be wrapped in push/popScope calls for a 'startup' block - look at StandardServer.invoke how it does it

You may also need to reset the context after each call - see what Agent.prepare and Agent$ResetContextCommand does for appservers.

Well, that was also one of my ideas but I put it aside because of the nested blocks. It seemed a bit of an overkill.

Nevertheless, I changes the code as you suggested and it works. I created a 'virtual block' where the access to _sec-auth tables is performed. All my local tests are successful. #9595 is no more blocking this task since the locks are released with the inner block.

I must thank you, otherwise I would not have gone this way.

I have rebased it and, with the latest changes now the branch is at revision 15689.

I'm OK with the changes in runInGlobalBlock.

These tests should be encoded using the harness so that we can easily automate the execution in our CI/CD pipelines.

Review for 8976a rev. 15687-15690

• StandardServer and TemporaryBuffer are missing history entries.
• In MetadataManager.populateTenant(), there are a few lines over 110 characters. The formatting might suffer a bit if we are to keep the maximum of 110 characters per line, but it was worth mentioning.
• TenantTableUpdater is missing the header, history entry, javadocs.
• There are 9 System.out.println() left in the code, in TenantTableUpdater.
• I also see some TODOs in TenantTableUpdater. Are these something for the future?
• In TenantManager, at line 1385, there is this TODO: // TODO: delete tenant from _Tenant and some commented code afterwards. Is this something we need?
• TenantMetaChangeListener is missing the header and history entry.
• I know we spoke about this yesterday, but can we do something to have more meaningful error messages? For example, when I tried to add a new domain type, I got a NPE logged instead of a message saying "there is already a domain type with the same name".

Currently, I'm still having some issues while testing. If I figure out what's happening, I'll let you know.

Radu Apetrii wrote:

Review for 8976a rev. 15687-15690

• StandardServer and TemporaryBuffer are missing history entries.

Done for StandardServer. Reverted changes from TemporaryBuffer since they were only whitespaces (minor code formatting fixes);

• In MetadataManager.populateTenant(), there are a few lines over 110 characters. The formatting might suffer a bit if we are to keep the maximum of 110 characters per line, but it was worth mentioning.

Fixed.

• TenantTableUpdater is missing the header, history entry, javadocs.

Fixed.

• There are 9 System.out.println() left in the code, in TenantTableUpdater.

Removed.

• I also see some TODOs in TenantTableUpdater. Are these something for the future?

Maybe future optimisations, if we will have support for faster record lookup using the rowid. I improved the TODO messages.

• In TenantManager, at line 1385, there is this TODO: // TODO: delete tenant from _Tenant and some commented code afterwards. Is this something we need?

The logical name of the database was not always available. The CM was not the appropriate place since the database might have not been connected at that moment. Instead, the local TenantDatabaseDescriptor provided it.
Fixed.

• TenantMetaChangeListener is missing the header and history entry.

Added.

• I know we spoke about this yesterday, but can we do something to have more meaningful error messages? For example, when I tried to add a new domain type, I got a NPE logged instead of a message saying "there is already a domain type with the same name".

I think addressed this issue. I added a sink for collecting and reporting errors occurred during REST invocations. However, the changes are pretty extensive since all called methods are requested to report the errors they encountered during execution instead of just returning null, false, or void.

Currently, I'm still having some issues while testing. If I figure out what's happening, I'll let you know.

I hope after removing of the duplicate _sec-authentication tables. Things went smooth for you.
Indeed, there is a problem is a customer will declare tables with same name as those from meta database. I think we have some protection for _User, although I am not sure if the dump-name is also handled correctly. For _User maybe there is a chance for a collision, but for _sec-authentication tables, I think chances are really slim, if any.

Note that I have rebased the branch to latest trunk and pushed it up to revision 15701.

Ovidiu Maxiniuc wrote:

I hope after removing of the duplicate _sec-authentication tables. Things went smooth for you.

Yes, testing went really smoothly after we solved that naming conflict. Thank you for the help!

• The paths in the ACL nodes need to have REST-ADMIN instead of TENANT.
• The ACL nodes with the domain need to be removed.

Thank you for quick review. There were lots of changes in last commit.

I will do the adjustments in documentation after the branch reaches the trunk (and xfer) so that the customer won't adjust their environments without having the code.

Branch 8976a passed the MT harness tests of a customer application.
I also did a bit of navigation in the hotel_gui project (creating, modifying and deleting rooms, rates guests). No regressions in sight.

Code review 8976a/15712-15716:

I'm not the best person to review the REST and admin interface changes, but the persistence changes look good to me.

In TenantManager.listTenants(List<String> errorSink), errorSink is used without a null check. I thought I saw places in the code that possibly pass null for this parameter, but I didn't do an exhaustive search to determine if all code paths were safe. Please ensure NPE can't be raised with this API.

You have many TODOs in the new TenantTableUpdater class. What's the plan for those?

Thank you for cleaning up the code formatting and javadoc across many files.

One minor typo I noticed: MetadataManager file header entry 028: "lamdlord".

Sorry it's taken me some days to get to this review. The branch will need a rebase from trunk.

Eric Faulhaber wrote:

In TenantManager.listTenants(List<String> errorSink), errorSink is used without a null check. I thought I saw places in the code that possibly pass null for this parameter, but I didn't do an exhaustive search to determine if all code paths were safe. Please ensure NPE can't be raised with this API.

Done.

You have many TODOs in the new TenantTableUpdater class. What's the plan for those?

They are just hints for a possible future optimisation, if we will have a faster access to records. Their content is identical. I chose to keep them for the moment.

One minor typo I noticed: MetadataManager file header entry 028: "lamdlord".

Fixed.

Committed the changes and rebased the branch to latest trunk. The current revision is 15724.

Ovidiu, renaming TenantHandler to AdminHandler and making this the authorative 'handler' for admin purposes is not that easy - there is also SessionHandler.

More, how a handle works is that you provide a context path., and all targets will be relative to that path. You provide the /admin context(base) path, thus it will handle all request; also,

      if ((TENANT_PREFIX + "login").equals(target) ||
          (DOMAIN_PREFIX + "login").equals(target) ||
          (DOMAIN_TYPE_PREFIX + "login").equals(target))
      {
         target = "/login"; // for convenience 
      }

I don't think this is OK, as 'prefixes' grow, this will grow.

My point with the above is: we need to have a discussion in #6377 how to structure the handlers for admin REST APIs to be manageable. I don't want to end up with a huge ThinClient-like class, and with target.startsWith(<somePrefix>) to handle all the rest APIs.

Constantin Asofiei wrote:

Ovidiu, renaming TenantHandler to AdminHandler and making this the authorative 'handler' for admin purposes is not that easy - there is also SessionHandler.

I understand your point. The AdminHandler is a provides access to meta tables (_Tenant, _sec-authentication*), whilst SessionHandler manages the user sessions (if I am not mistaken). Indeed, the implementations are similar, but the data type they work with is a bit different.

OTOH, I tried to implement the REST API for interacting with tenant authentication tables in another class letting the old TenantHandler mainly untouched, but I hit some difficulties. The way these classes register themselves (for receiving the requests) makes that difficult. This is the reason why I preferred the current solution.

More, how a handle works is that you provide a context path., and all targets will be relative to that path. You provide the /admin context(base) path, thus it will handle all request;

also,
[...]
I don't think this is OK, as 'prefixes' grow, this will grow.

This is only for convenience, everything would work without this, except the login URL is no more common (each sub-feature set will have a personal login, even if it will work for all of them). Since this class is supposed to handle only requests for managing the 'regular' _meta tables, I do not expect to grow too much.

My point with the above is: we need to have a discussion in #6377 how to structure the handlers for admin REST APIs to be manageable. I don't want to end up with a huge ThinClient-like class, and with target.startsWith(<somePrefix>) to handle all the rest APIs.

Indeed, the code is developing as a big switch handling different levels of branches. I think we can address this by using a new level of handler/listener with a common interface/abstract class which expose methods for GET/PUT/... and each implementing class would override only the desired (web-)methods, for example SessionHandler will implement only handleGet() and handleDelete.

I am happy to discuss fine-tuning of the current implementation and shall do the refactoring after-all.

Ovidiu, I see that 8976a has latest changes discussed in #7080. Please make sure the code compiles with Java 8, I want to run it but it has errors.

Moved review to #7080

Branch 8976a passed hotel GUI tests, dockered harness smoke tests for a customer application and the unit-tests of another customer.

Also, it was rebased to latest trunk. Current revision is 15839.