feat(auth): send from: on connect and reuse its challenge

[gkc]

- What I did
For a new connection, the first command sent should be from: - this is so that the client can communicate successfully via proxies / gateways etc.

- How I did it

• A new AtCommandExecutorContext bundles atSign/keys/config plus the single-use challenge (getAndSet(null)). The builder owns it and closes it over the onReady consumers; the AtCommandExecutor never sees it.

• CLI imperative flows (onboard / complete)

  • onboard: issues from: on connect, then the scan sanity check; then CRAM which reuses the challenge and doesn't issue from, then PKAM issues from because there is no saved challenge (it's been used).
  • complete: authenticates in a pending-retry loop, so it issues its own from: per attempt — reusing a connect-time challenge could in theory go stale before the delayed PKAM. 1 from:.
  • Two factories for clarrity: createConnectionSendingFrom andcreateConnectionDeferringFrom.

• Tests

  • Added unit coverage for the challenge lifecycle:
    • AuthenticationCommandsTest — sendFrom issues from: and retains the
      challenge; PKAM reuses it (one from: on the wire) and falls back to its own
      from: when none is retained; the challenge is consumed at most once.
    • EnrollCommandsTest — onboard's CRAM reuses the connection's from: challenge
      (one from:, not two).
  • Adapted the existing CRAM and onboard unit tests to the context-based auth API
    (no behaviour change). The pre-existing functional packs which drive real onboarding against
    real atServer prove it all hangs together.

- How to verify it
Tests pass

[akafredperry]

I can see what the objective is and what you did clearly works but I wonder if there is a simpler way.

Namely leaving the executor as is. But just adding a default onReady consumer when an atsign is specified (and no onReady consumer has been set).

This default onReady consumer would send the from command and store the challenge.

The changes to the CRAM and PKAM commands would then check for a stored challenge in their implementation.

We would need to decide where the appropriate place to cache the challenge is, I don't think it's the command executor.

One option would be AtKeys as an instance is already provided to the auth command implementations.
AtKeys already supports a transient map for caching keys, using that would be the least amount of changes.
BUT
If we feel that is an abuse of the design we could add an explicit authChallenge field to AtKeys (perhaps still yuck).
OR
Create a new context class that "gathers together" all the current (and future) contexty stuff (e.g. AtSign, AtKeys, connection config and now authChallenge).

The latter also has the advantage of simplifying and homogenizing the existing command method signatures.

This context instance would "live in" AtClientImpl.

The place to wire this default in is AtCommandExecutors.java#createOnReady()

This already adds a default onReady if no onReady is provided to the builder.

At the moment that is a pkamAuthenticator if atSign and keys are set or "do nothing" (talking to root server).

I think we just need a middle case which is atSign is set but keys are not set in which case, it should create this new "send from" onReady.

Happy to discuss further / help if you want

[gkc]

Thanks Fred I will noodle on that today

[gkc]

@akafredperry i made an attempt along the lines you suggested here and in chat ... ptal

[akafredperry]

Since we are using Lombok, I would recommend for this class too (as will remove boiler plate). Something like this...

@Value
public class AtCommandExecutorContext {

   AtSign atSign;
   AtKeys keys;
   Map<String, Object> config;

    @Getter(AccessLevel.NONE)
    @EqualsAndHashCode.Exclude
    @ToString.Exclude
    AtomicReference<String> challenge = new AtomicReference<>();

    public void setChallenge(String challenge)...
    public String consumeChallenge()...
    public void clearChallenge()...
}

[akafredperry]

This change still "lives on" the command executor rather than within the AtClientImpl as I was suggesting.

The intention was that the AtCommandExecutor(s) are solely transport specific implementations that are capable of sending commands but with no protocol knowledge.

The protocol for initial connection is currently the onReady impl.

What we currently do is almost compliant with the requirement (I think), just need to cover the case where AtCommandExecutorBuilder (in AtCommandExecutors.java) has atSign field set but not the AtKeys field.

Having said that we need to ensure that everything is using AtCommandExecutorBuilder.

[gkc]

OK .... maybe I should scrap this branch and start again

[gkc]

@akafredperry Take 3 ... I think it's now in line with your intent

[akafredperry]

perfect (imho)

[gkc]

Thanks for the reviews Fred, great to get expert feedback and guidance