millross / pac4j-async Goto Github PK

DefaultSecurityLogic and DefaultAsyncSecurityLogic have some code in common - basically everything outside the perform() method should be in a common superclass, as should the extension of ProfileManagerFactoryAware().

I suggest initially we call this SecurityLogicBase but again we can review this naming later, along with code organisation.

Since the existing Clients class in pac4j is tightly coupled by typing to the existing sync Client implementation, extend it so it takes a type parameter for the client base class (Client or AsyncClient). The same basic implementation of this class can then be shared for either sync or async client, which in turn enables the ClientFinder implementation to do the same. This helps minimise the impact of going asynchronous across the pac4j API.

Once the AsyncBaseClient strawman implementation is in place, implement tests against its getProfile method. Obvious candidates:-

(1) Happy path all the way through, no authorisation generators - issue #32
(2) Happy path all the way through, authorisation generators - issue #33
(3) Retrieve profile future throws an exception - issue #34
(4) Exception during authorisation generator processing
(5) Exception during authorisation generator modification of profile

Enable sync session store to async session store conversion so that existing SessionStore implementations can be used out of the box.

To be implemented by any client implementation (sync or async) which can be provided to a Clients instance and might be configured by that Clients instance.

Instead of having specific type conditions at runtime in the Clients class to initialise the clients in the list provided, which is very brittle and makes the async implementation over-complicated and messy, instead offer a ConfiguredByClientsObject interface which must be implemented at some level by a client (whether sync or async). This decouples the Clients class from specific knowledge about the types of client with which it is dealing, and instead allows the clients to simply know what they need to get from the Clients object doing the configuring. It may be that we should break the supporting logic out from the Clients class altogether and offer a new class capable of doing this work (it feels like it's config-related to me).

Since we don't know anything about session stores which might be in use, and since some might involve significant blocking i/o which isn't helpful in an async environment, we need to define an AsyncSessionStore base interface which returns future-based versions of the session store methods.

Once we've introduced async Authorizers, consider whether the use of the sync Authorizer in AsyncProfileManager is appropriate. Since it's a private member, I suspect it's fine, as it won't do any blocking i/o, but we should review when we get there.

Identify and apply appropriate automated tests for AsyncProfileManager

At present the work done by AsyncAuthorizationGenerator to a profile's state is not thread-safe - in an async implementation we have to synchronise the changes to a profile's stage.

Any wrapper for converting blocking sync into async will have to synchronise on the profile - this is unavoidable, since the sync generator acts directly on the profile as well as performing operations to determine what it should do to the profile, and therefore we have no guarantee as to what thread will be running the sync generator. However we are defining the async interface, and a thread-cleaner approach (avoiding synchronisation) is as follows:

Use the list of generators to determine a list of operations to perform on the profile, which is ultimately converted to a CompletableFuture. Then, on the async framework's context apply them all serially, on the same thread. This has two benefits: firstly it enables us to work with the async framework's threading/context model (the context management should be easy to create framework-specific implementations for), and secondly it avoids unnecessary synchronisation.

Because the async API can't distinguish synchronous thread-blocking and non-blocking computations, add a class (or interface) which takes a piece of non-thread-blocking synchronous code and wraps it in a CompletableFuture which will complete immediately, then returning it.

This makes it possible to keep code which we can guarantee is non-blocking, although synchronous, on the same thread and avoid unnecessary use of the blocking wrapper. It's mainly to be used to convert existing pac4j code into async versions which we will make available as specific libraries.

Retrieve profile future throws an exception - see #30

Following on from issue #16 add an interface defining the wrapper for converting a blocking computation into a non-blocking computation.

We may need to add a special case if we have a void piece of code to wrap but this can be identified subsequently.

Mimic the behaviour of what is done for a BaseClient in the existing sync type-driven Clients class

Examine existing AuthorizationGenerator implementations and look for any blocking i/o. If not, then for now we'll assume there's no great demand for an async version.

If we identify blocking i/o then create an AsyncAuthorizationGenerator interface based on the existing AuthorizationGenerator interface

Identify common behaviours with BaseClient and extract into a superclass.
Implement ConfigurableByClientsObject as per sync BaseClient equivalent.

Add test for failure behaviour of an AsynchronousComputation

This will be used to inform testing of the AsyncBaseClient implementation failure scenarios

Also consider that if we can put a marker interface on for whether a client is indirect or direct (or even define it as an implementor of HasUpdatableCallbackUrl) we don't need to check its type, making the Clients implementation less wedded to whether it is dealing with Sync or Async clients.

Similarly an AuthorizationGeneratorAcceptor for the second part of initialisation could even remove the need for the cast altogether if we define the generic as having to extend AuthorizationGeneratorAcceptor.

Once we have an async-capable ClientFinder definition we can make use of it in DefaultSecurityLogicBase via a type parameter for the client base type, and therefore keep more of the implementation common between sync and async pac4j APIs

Define AsyncWebContext to allow for blocking/non-blocking session store implementations

Some async frameworks (e.g. vert.x) have the concept of a context on which completion code (e.g. callbacks) will run, and this context provides features such as correct exception handling, ensuring correct behaviour on event loop threads etc.

This task is to define a simple ContextRunner interface for running a block of code (expressed as a Runnable) on the correct context.

My current thinking of how best to write the async clients is to have them either return or accept CompletableFutures (which choice is likely to evolve as I see the logic evolution) which they complete when they have the results of their various computations.

I need to ensure this strategy is valid in an async framework for the following three cases:-

(1) The computation doesn't block but takes some time, and is managed by the framework (for example use of a vert.x http client running on the event loop to validate credentials)
(2) The computation blocks and runs off the main thread, but its result must be processed on the correct thread (a vert.x example is run some blocking code in executeBlocking, but control has to return to the event loop thread when the computation is complete)
(3) The computation completes almost immediately and can be run on the event loop thread, but the future will complete instantly.

It may be that (3) and (2) have to be combined, but maximum efficiency occurs if (3) is left on the event loop thread entirely, in the vert.x scenario.

It's likely that we will need to handle these three situations for any async framework, so create a simple test project which validates all of these for the CompletableFuture-based strategy in a simplistic way, before proceeding further.

Create an async version of the existing DirectClient class

We need to remove the tight coupling between Client type and behaviour if we want to be able to cleanly reuse code between sync and async pac4j worlds. This would remove specific type checking in, for example, DefaultSecurityLogic::loadProfilesFromSession.

We should consider even cleaner methods (injection by client perhaps?).

Happy path all the way through, authorisation generators - see issue #30

Implement test for exception being thrown during auth generator modification of profile following successful profile retrieval and auth generation

Happy path all the way through, no authorisation generators - see issue #30

When a CompletableFuture completes exceptionally then the exception which triggered the failure can be referenced directly in its whenComplete method (which enables easy handling of both success and failure in one method).

However, where the failure occurs in an upstream CompletionStage (for example CompletableFuture A is transformed to CompletableFuture B) then the derived CompletableFuture (in the example above B) will fail with a CompletionException wrapping the actual exception which occurred.

Given that pac4j's strategy is to wrap any triggering unexpected exception in a TechnicalException then throw it outwards, we shouldn't need this difference in behaviour within pac4j-async. Therefore it makes sense to provide a class which can automatically unwrap a CompletionException-wrapped exception and then handle it with a custom handler (or if the exception provided was not a CompletionException just handle the exception directly(, so that any code we write is agnostic of the CompletionException.

We expect to need to inject the exception handler, however, as it is perfectly possible that some code will throw, for example, HttpActions which need to be handled differently (they are treated as expected results) or CredentialsException (which are treated as authentication/authorisation failures).

Since not retrieving a profile means "not authenticated", (and therefore has a real meaning) these methods should return Optional

rather than a nullable P.

We can do this legitimately in pac4j-async since we're essentially defining a new API which will not bleed into the main pac4j API.

The vast majority of code used by these two classes will be the same, so express as a common superclass

Use returned CompletableFuture rather than a callback parameter.

Create an async version of the existing IndirectClient class

The initial cut of AsyncClient accepted CompletableFutures for its async methods, with the intent that the underlying logic would complete them when the async computation results became available.

However, testing of CompletableFutures has shown that an already completed future will trigger subsequent computations immediately they are attached to it, so this is unnecessary and the API will be cleaner if the async methods return CompletableFuture where T is the type the method would normally return.

Exception during authorisation generator processing - specifically the processing which occurs and will return a profile modifier which will subsequently be applied to the profile.

Offer a ContextFreeRunner implementation of ContextRunner which doesn't execute on any specific context, for use by async frameworks which don't have any "context" concept.

#22 Define base interface for clients to update their configuration from Clients object, instead of using magic type-checks within the Clients class.

Create async-capable ClientFinder definition and default implementation by using a type param as the base class for the type of client it is to find. This then lets the same default implementation be reused regardless of whether we are writing sync or async code and therefore minimises the friction between sync and async pac4j APIs

Rename AsynchronousComputation to AsynchronousComputationFactory
Define AsyncAuthorizationGenerator base class including generate method
Define both static methods on AsyncAuthorizationGenerator class
Simple tests for all three should be provided.

Initial work for async base client testing suggests that non-blocking exceptional future completion may give different results to blocking wrapped in future completion. It's not clear why this occurs, so create a simplest scenario test and get this passing first