Merge pull request juju#18441 from manadart/dqlite-skip-factory-tests…

…-auth

juju#18441

This patch demonstrates a strategy we will pursue to remove use of the testing factory.

This is a strategic approach to avoid being bound to convolutions of the factory for making tests pass as we move large concerns over to Dqlite.

The reasons for it are:
- The tests are integration (not unit) tests anyway, and most of their usage is in the API server, which should increasingly rely on mocked domain services.
- The approach so far to the factory has been piece-meal double-writing that often turns out not to be fit for purpose anyway.
- The factory is already of sufficient complexity that it makes tests based on it hard to understand. 

Here, we apply the approach to API server authentication tests.

**This technique should be recruited consistently in every application of this strategy henceforth.**

- Add a new test (`TestStub` in this case) with a `c.Skip` call.
- The text of the skip message should include a bullet list of required coverage to fill in.
- Each list item must contain as much prose as necessary to describe the scenarios requiring implementation.

By doing this we ensure that test reports will emit a reminder of missing coverage.

The coverage will be re-established as we move on the the relevant domain concerns. Backlogs will include both activities like the one performed here to remove/document the coverage, and to add the coverage back in.

api/watcher/watcher_test.go

@@ -100,26 +100,31 @@ func setupWatcher[T any](c *gc.C, caller *apimocks.MockAPICaller, facadeName str
 	eventCh := make(chan T)
 
 	stopped := make(chan bool)
-	caller.EXPECT().APICall(gomock.Any(), facadeName, 666, "id-666", "Stop", nil, gomock.Any()).DoAndReturn(func(_ context.Context, _ string, _ int, _ string, _ string, _ any, _ any) error {
-		select {
-		case stopped <- true:
-		default:
-		}
-		return nil
-	}).Return(nil).AnyTimes()
-
-	caller.EXPECT().APICall(gomock.Any(), facadeName, 666, "id-666", "Next", nil, gomock.Any()).DoAndReturn(func(_ context.Context, _ string, _ int, _ string, _ string, _ any, r any) error {
-		select {
-		case ev, ok := <-eventCh:
-			if !ok {
-				c.FailNow()
+	caller.EXPECT().APICall(gomock.Any(), facadeName, 666, "id-666", "Stop", nil, gomock.Any()).DoAndReturn(
+		func(context.Context, string, int, string, string, any, any) error {
+			select {
+			case stopped <- true:
+			case <-time.After(testing.LongWait):
+				c.Fatalf("timed out waiting for stop call")
 			}
-			*(*r.(*any)).(*any) = ev
 			return nil
-		case <-stopped:
-		}
-		return &params.Error{Code: params.CodeStopped}
-	}).AnyTimes()
+		},
+	).Return(nil).AnyTimes()
+
+	caller.EXPECT().APICall(gomock.Any(), facadeName, 666, "id-666", "Next", nil, gomock.Any()).DoAndReturn(
+		func(_ context.Context, _ string, _ int, _ string, _ string, _ any, r any) error {
+			select {
+			case ev, ok := <-eventCh:
+				if !ok {
+					c.Fatalf("next channel closed")
+				}
+				*(*r.(*any)).(*any) = ev
+				return nil
+			case <-stopped:
+			}
+			return &params.Error{Code: params.CodeStopped}
+		},
+	).AnyTimes()
 	return "id-666", eventCh
 }
 

apiserver/authentication/agent_test.go

@@ -4,182 +4,22 @@
 package authentication_test
 
 import (
-	"context"
-
-	"github.com/juju/names/v5"
-	jc "github.com/juju/testing/checkers"
+	"github.com/juju/testing"
 	gc "gopkg.in/check.v1"
-
-	"github.com/juju/juju/apiserver/authentication"
-	"github.com/juju/juju/core/permission"
-	usertesting "github.com/juju/juju/core/user/testing"
-	"github.com/juju/juju/domain/access/service"
-	"github.com/juju/juju/internal/auth"
-	loggertesting "github.com/juju/juju/internal/logger/testing"
-	internalpassword "github.com/juju/juju/internal/password"
-	"github.com/juju/juju/internal/testing/factory"
-	"github.com/juju/juju/juju/testing"
-	"github.com/juju/juju/state"
 )
 
 type agentAuthenticatorSuite struct {
-	testing.ApiServerSuite
-	machinePassword string
-	machineNonce    string
-	unitPassword    string
-	machine         *state.Machine
-	user            state.Entity
-	unit            *state.Unit
-	relation        *state.Relation
+	testing.IsolationSuite
 }
 
 var _ = gc.Suite(&agentAuthenticatorSuite{})
 
-func (s *agentAuthenticatorSuite) SetUpTest(c *gc.C) {
-	s.ApiServerSuite.SetUpTest(c)
-
-	userService := s.ControllerDomainServices(c).Access()
-	userUUID, _, err := userService.AddUser(context.Background(), service.AddUserArg{
-		Name:        usertesting.GenNewName(c, "bobbrown"),
-		DisplayName: "Bob Brown",
-		Password:    ptr(auth.NewPassword("password")),
-		CreatorUUID: s.AdminUserUUID,
-		Permission: permission.AccessSpec{
-			Access: permission.LoginAccess,
-			Target: permission.ID{
-				ObjectType: permission.Controller,
-				Key:        s.ControllerUUID,
-			},
-		},
-	})
-	c.Assert(err, jc.ErrorIsNil)
-	user, err := userService.GetUser(context.Background(), userUUID)
-	c.Assert(err, jc.ErrorIsNil)
-	s.user = authentication.TaggedUser(user, names.NewUserTag("bobbrown"))
-
-	f, release := s.NewFactory(c, s.ControllerModelUUID())
-	defer release()
-
-	// add machine for testing machine agent authentication
-	st := s.ControllerModel(c).State()
-	machine, err := st.AddMachine(state.UbuntuBase("12.10"), state.JobHostUnits)
-	c.Assert(err, jc.ErrorIsNil)
-	nonce, err := internalpassword.RandomPassword()
-	c.Assert(err, jc.ErrorIsNil)
-	err = machine.SetProvisioned("foo", "", nonce, nil)
-	c.Assert(err, jc.ErrorIsNil)
-	password, err := internalpassword.RandomPassword()
-	c.Assert(err, jc.ErrorIsNil)
-	err = machine.SetPassword(password)
-	c.Assert(err, jc.ErrorIsNil)
-	s.machine = machine
-	s.machinePassword = password
-	s.machineNonce = nonce
-
-	// add a unit for testing unit agent authentication
-	wordpress := f.MakeApplication(c, &factory.ApplicationParams{
-		Name:  "wordpress",
-		Charm: f.MakeCharm(c, &factory.CharmParams{Name: "wordpress"}),
-	})
-	unit, err := wordpress.AddUnit(state.AddUnitParams{})
-	c.Assert(err, jc.ErrorIsNil)
-	s.unit = unit
-	password, err = internalpassword.RandomPassword()
-	c.Assert(err, jc.ErrorIsNil)
-	err = unit.SetPassword(password)
-	c.Assert(err, jc.ErrorIsNil)
-	s.unitPassword = password
-
-	// add relation
-	wordpressEP, err := wordpress.Endpoint("db")
-	c.Assert(err, jc.ErrorIsNil)
-	mysql := f.MakeApplication(c, nil)
-	mysqlEP, err := mysql.Endpoint("server")
-	c.Assert(err, jc.ErrorIsNil)
-	s.relation, err = s.ControllerModel(c).State().AddRelation(wordpressEP, mysqlEP)
-	c.Assert(err, jc.ErrorIsNil)
-}
-
-// testCase is used for structured table based tests
-type testCase struct {
-	entity       state.Entity
-	credentials  string
-	nonce        string
-	about        string
-	errorMessage string
-}
-
-func (s *agentAuthenticatorSuite) TestValidLogins(c *gc.C) {
-	testCases := []testCase{{
-		entity:      s.machine,
-		credentials: s.machinePassword,
-		nonce:       s.machineNonce,
-		about:       "machine login",
-	}, {
-		entity:      s.unit,
-		credentials: s.unitPassword,
-		about:       "unit login",
-	}}
-
-	st := s.ControllerModel(c).State()
-	for i, t := range testCases {
-		c.Logf("test %d: %s", i, t.about)
-		factory := authentication.NewAgentAuthenticatorFactory(
-			st,
-			loggertesting.WrapCheckLog(c),
-		)
-		entity, err := factory.Authenticator().Authenticate(context.Background(), authentication.AuthParams{
-			AuthTag:     t.entity.Tag(),
-			Credentials: t.credentials,
-			Nonce:       t.nonce,
-		})
-		c.Assert(err, jc.ErrorIsNil)
-		c.Assert(entity.Tag(), gc.DeepEquals, t.entity.Tag())
-	}
-}
-
-func (s *agentAuthenticatorSuite) TestInvalidLogins(c *gc.C) {
-	testCases := []testCase{{
-		entity:       s.user,
-		credentials:  "password",
-		about:        "user login",
-		errorMessage: "invalid request",
-	}, {
-		entity:       s.relation,
-		credentials:  "dummy-secret",
-		about:        "relation login",
-		errorMessage: "invalid request",
-	}, {
-		entity:       s.user,
-		credentials:  "wrongpassword",
-		about:        "user login for nonexistant user",
-		errorMessage: "invalid request",
-	}, {
-		entity:       s.machine,
-		credentials:  s.machinePassword,
-		nonce:        "123",
-		about:        "machine login",
-		errorMessage: "machine 0 not provisioned",
-	}, {
-		entity:       s.user,
-		credentials:  "wrong-secret",
-		about:        "user login for nonexistant user",
-		errorMessage: "invalid request",
-	}}
-
-	st := s.ControllerModel(c).State()
-	for i, t := range testCases {
-		c.Logf("test %d: %s", i, t.about)
-		factory := authentication.NewAgentAuthenticatorFactory(
-			st,
-			loggertesting.WrapCheckLog(c),
-		)
-		entity, err := factory.Authenticator().Authenticate(context.Background(), authentication.AuthParams{
-			AuthTag:     t.entity.Tag(),
-			Credentials: t.credentials,
-			Nonce:       t.nonce,
-		})
-		c.Assert(err, gc.ErrorMatches, t.errorMessage)
-		c.Assert(entity, gc.IsNil)
-	}
+func (s *agentAuthenticatorSuite) TestStub(c *gc.C) {
+	c.Skip(`This suite is missing tests for the following scenarios:
+- Valid unit login.
+- Valid machine login.
+- Invalid agent login as user.
+- Invalid login for machine not provisioned.
+- Login for invalid relation entity.
+`)
 }

apiserver/authentication/user_test.go

@@ -26,10 +26,7 @@ import (
 	usertesting "github.com/juju/juju/core/user/testing"
 	"github.com/juju/juju/domain/access/service"
 	"github.com/juju/juju/internal/auth"
-	"github.com/juju/juju/internal/password"
-	"github.com/juju/juju/internal/testing/factory"
 	jujutesting "github.com/juju/juju/juju/testing"
-	"github.com/juju/juju/state"
 )
 
 type userAuthenticatorSuite struct {
@@ -39,51 +36,22 @@ type userAuthenticatorSuite struct {
 var _ = gc.Suite(&userAuthenticatorSuite{})
 
 func (s *userAuthenticatorSuite) TestMachineLoginFails(c *gc.C) {
-	// add machine for testing machine agent authentication
-	machine, err := s.ControllerModel(c).State().AddMachine(state.UbuntuBase("12.10"), state.JobHostUnits)
-	c.Assert(err, jc.ErrorIsNil)
-	nonce, err := password.RandomPassword()
-	c.Assert(err, jc.ErrorIsNil)
-	err = machine.SetProvisioned("foo", "", nonce, nil)
-	c.Assert(err, jc.ErrorIsNil)
-	password, err := password.RandomPassword()
-	c.Assert(err, jc.ErrorIsNil)
-	err = machine.SetPassword(password)
-	c.Assert(err, jc.ErrorIsNil)
-	machinePassword := password
 
-	// attempt machine login
 	authenticator := &authentication.LocalUserAuthenticator{}
-	_, err = authenticator.Authenticate(context.Background(), authentication.AuthParams{
-		AuthTag:     machine.Tag(),
-		Credentials: machinePassword,
-		Nonce:       nonce,
+	_, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{
+		AuthTag:     names.NewMachineTag("0"),
+		Credentials: "I am a machine",
+		Nonce:       "Ya nonce!",
 	})
 	c.Assert(err, gc.ErrorMatches, "invalid request")
 }
 
 func (s *userAuthenticatorSuite) TestUnitLoginFails(c *gc.C) {
-	f, release := s.NewFactory(c, s.ControllerModelUUID())
-	defer release()
-
-	// add a unit for testing unit agent authentication
-	wordpress := f.MakeApplication(c, &factory.ApplicationParams{
-		Name:  "wordpress",
-		Charm: f.MakeCharm(c, &factory.CharmParams{Name: "wordpress"}),
-	})
-	unit, err := wordpress.AddUnit(state.AddUnitParams{})
-	c.Assert(err, jc.ErrorIsNil)
-	password, err := password.RandomPassword()
-	c.Assert(err, jc.ErrorIsNil)
-	err = unit.SetPassword(password)
-	c.Assert(err, jc.ErrorIsNil)
-	unitPassword := password
-
-	// Attempt unit login
+	// Attempt unit login,
 	authenticator := &authentication.LocalUserAuthenticator{}
-	_, err = authenticator.Authenticate(context.Background(), authentication.AuthParams{
-		AuthTag:     unit.UnitTag(),
-		Credentials: unitPassword,
+	_, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{
+		AuthTag:     names.NewUnitTag("vault/0"),
+		Credentials: "I am a unit",
 	})
 	c.Assert(err, gc.ErrorMatches, "invalid request")
 }
@@ -105,7 +73,6 @@ func (s *userAuthenticatorSuite) TestValidUserLogin(c *gc.C) {
 	})
 	c.Assert(err, jc.ErrorIsNil)
 
-	// User login
 	authenticator := &authentication.LocalUserAuthenticator{
 		UserService: s.ControllerDomainServices(c).Access(),
 	}
@@ -137,7 +104,6 @@ func (s *userAuthenticatorSuite) TestDisabledUserLogin(c *gc.C) {
 	err = userService.DisableUserAuthentication(context.Background(), name)
 	c.Assert(err, jc.ErrorIsNil)
 
-	// User login
 	authenticator := &authentication.LocalUserAuthenticator{
 		UserService: s.ControllerDomainServices(c).Access(),
 	}
@@ -352,7 +318,7 @@ func (s *userAuthenticatorSuite) TestRemovedMacaroonUserLogin(c *gc.C) {
 	macaroons := []macaroon.Slice{{mac}}
 	bakeryService := mockBakeryService{}
 
-	// User login
+	// User login.
 	authenticator := &authentication.LocalUserAuthenticator{
 		UserService: s.ControllerDomainServices(c).Access(),
 		Bakery:      &bakeryService,
@@ -366,27 +332,10 @@ func (s *userAuthenticatorSuite) TestRemovedMacaroonUserLogin(c *gc.C) {
 }
 
 func (s *userAuthenticatorSuite) TestInvalidRelationLogin(c *gc.C) {
-	f, release := s.NewFactory(c, s.ControllerModelUUID())
-	defer release()
-
-	// add relation
-	wordpress := f.MakeApplication(c, &factory.ApplicationParams{
-		Name:  "wordpress",
-		Charm: f.MakeCharm(c, &factory.CharmParams{Name: "wordpress"}),
-	})
-	wordpressEP, err := wordpress.Endpoint("db")
-	c.Assert(err, jc.ErrorIsNil)
-	mysql := f.MakeApplication(c, nil)
-	mysqlEP, err := mysql.Endpoint("server")
-	c.Assert(err, jc.ErrorIsNil)
-	relation, err := s.ControllerModel(c).State().AddRelation(wordpressEP, mysqlEP)
-	c.Assert(err, jc.ErrorIsNil)
-
-	// Attempt relation login
 	authenticator := &authentication.LocalUserAuthenticator{}
-	_, err = authenticator.Authenticate(context.Background(), authentication.AuthParams{
-		AuthTag:     relation.Tag(),
-		Credentials: "dummy-secret",
+	_, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{
+		AuthTag:     names.NewRelationTag("this-app:rel that-app:rel"),
+		Credentials: "I am a relation",
 	})
 	c.Assert(err, gc.ErrorMatches, "invalid request")
 }