[ELY-515] ServerAuthenticationContext rework

[dmlloyd]

No description provided.

[fjuma]

(Wasn't able to add a comment directly on SAC since the diff for that file is too large to be shown, so am commenting here.)

It looks like the following two lines in SAC#assignName are in the wrong order - the post-realm NameRewriters should get applied after the realm has been selected.

https://github.com/dmlloyd/wildfly-elytron/blob/ely-515/src/main/java/org/wildfly/security/auth/server/ServerAuthenticationContext.java#L583-L584

[dmlloyd]

Ah you're right, good catch. I copy-pasted that bit in pieces, must have scrambled it.

[fjuma]

Just noticed the order also needs to be fixed in ActiveState#authorize:

https://github.com/dmlloyd/wildfly-elytron/blob/ely-515/src/main/java/org/wildfly/security/auth/server/ServerAuthenticationContext.java#L718-L719

[dmlloyd]

Hmm I forgot to change that one to create an intermedate NameAssignedState. I better look at that more closely...

[dmlloyd]

I'm also going to make changes so that realm identities are disposed in finally blocks where appropriate.

[dmlloyd]

Added one missing commit: UnassignedState.authorize() didn't check for anonymous.

[dmlloyd]

Actually I'm going to update it though, to support non-required login permission checking for anonymous identities.

[dmlloyd]

Added a commit which shrinks the IdentityPropagationTestCase by using the importIdentity method directly.

[dmlloyd]

The checks disappeared so I fired off a new run.

[darranl]

Is 'Locator' really a good name here? To me locator gives the impression that it does something but from what I can see it is more a container for three optional values that can be used to obtain the realm identity.

[dmlloyd]

Just so named because it "locates" the RealmIdentity. I'm open to suggestions though.

[darranl]

The one point really leaving me with questions is the IdentityLocator - I have mentioned some notes in the PR but thinking - are we sure it needs a name and a principal?

We only seem to set the Principal in two locations, verifyEvidence where it is extracted from the evidence - but the evidence is contained within the IdentityLocator anyway.

Secondly in assignName but now we end up with two different values to locate the RealmIdentity.

Two other comments about SAC.
I don't know if it needs to live in the javadoc or elsewhere but I think we need some very detailed documentation of this class and the states so when supported it can be clearly understood.

Secondly we need a strategy around trace logging to diagnose issues, either we need a lot more in the code or if we don't want to do this possibly ensure something like ByteMan can capture enough information.

[fjuma]

+1, additional documentation of SAC and its states would be good (especially a state diagram).

[dmlloyd]

I'm working on documentation. I want to have some visual diagrams if possible; what I'm working out right now is whether there is a way that the diagrams can be generated, so that we can source-revision them without dealing in binary blobs, and so that we can update them seamlessly without worrying too much about tools going out of date.

Trace logging is a good idea. I can add a good deal around that.