apiserver/testing.FakeAuthoriser is not a good mock

[David Cheney]

Hello,

This message serves as background to a series of changes coming soon.

The goal of this message, and the changes that follow, is to describe
the problems that I see with the FakeAuthoriser and a describe in
general terms the steps I propose to address those shortcomings.

Background:

The FakeAuthoriser implements the apiserver/common.Authoriser
interface and is used heavily in the apiserver tests. The other
implementation of this interface is the apiserver itself,
specifically, apiserver.srvRoot.

tf. The goal of the FakeAuthoriser is to imitate apiserver.svrRoot.

I assert that this is currently not true and that the tests are
structured to expect the behaviour of the FakeAuthorizer rather than
the contract of apiserver/common.Authoriser as defined by
apiserver.srvRoot.

Issues:

This section describes the ways that the FakeAuthoriser diverges from
the behaviour of the srvRoot authoriser.

1. In srvRoot, all authorisation stems ultimately from the
state.Entity object referenced from the srvRoot instance. This is a
problem as our authorisation system is based on the _tag_ passed over
the api, not the state.Entity, although for practical purposes the tag
of the authenticated entity and the entity itself are interchangeable.

Proposal: remove Authoriser.GetAuthEntity() and replace all calls with
Authoriser.GetAuthTag(). I have a branch for this, it's < 30 lines as
there are only 3 places in the apiserver where we do this and they
usually call the .Tag method on the entity they get back from
GetAuthEntity.

2. This extends from point 1, while the svrRoot derives everything
from svrRoot.entity, the FakeAuthoriser allows the caller to pass a
unique value for the tag and the entity of the authorisee. This leads
to impossible situations where the FakeAuthorizer returns nil for
AuthGetEntity but a non nil value for AuthGetTag. Other permutations
exist in our tests and are expected by the test logic.

Propsal: Once step 1 is fixed the difference between svrRoot and
FakeAuthoriser is the former starts from a state.Entity and derives a
tag from which other values are derived, the latter skips the initial
step and starts from a tag. This work falls out of the solution to
steps 1 and 3.

3. The helper methods, AuthMachineAgent(), AuthUnitAgent() on the
Authoriser interface are implemented differently in svrRoot and
FakeAuthoriser. In tests it is quite common to take the FakeAuthoriser
from the test suite, copy it and change some of these values leading
to impossible situations, ie, the tag or entity of the FakeAuthoriser
pointing to a Unit, but AuthMachineAgent() returning true.

Proposal: The simplest solution is to copy the implementation of these
helper methods from svrRoot to FakeAuthoriser. A more involved
solution would be to factor these methods out to be functions in the
apiserver/common package that take an Authorizer. This second step may
not pay for itself.

These steps resolves the majority of the discontinuity between the two
implementations and will resolve a set of blocking issues I've hit
converting the apiserver and state packages to speak tags natively.

Thanks for your time

Dave