From 714374276d1c6364247ab3d18fded88781dad03f Mon Sep 17 00:00:00 2001 From: Joseph Phillips Date: Wed, 27 Nov 2024 17:24:19 +0100 Subject: [PATCH 1/2] chore: skip apiserver auth tests requiring the test factory Some redundant use of the factory has also been removed for retained tests. --- apiserver/authentication/agent_test.go | 180 ++----------------------- apiserver/authentication/user_test.go | 75 ++--------- 2 files changed, 22 insertions(+), 233 deletions(-) diff --git a/apiserver/authentication/agent_test.go b/apiserver/authentication/agent_test.go index 5ebdecccaf8..49f396f7657 100644 --- a/apiserver/authentication/agent_test.go +++ b/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. +`) } diff --git a/apiserver/authentication/user_test.go b/apiserver/authentication/user_test.go index afc01ff2bfe..713c40744c0 100644 --- a/apiserver/authentication/user_test.go +++ b/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") } From e4f021dd420b138947db4ec2a3e9ddd24720c795 Mon Sep 17 00:00:00 2001 From: Joseph Phillips Date: Thu, 28 Nov 2024 11:41:08 +0100 Subject: [PATCH 2/2] fix: remove possibility of zero-effect Stop from api watcher test This handles a situation where we may be entrant into the eventCh case in the call to Next when Stop is called. In that case, the default clause will be invoked and we effectively ignore the stop call. This means the watcher will never stop, causing test failure. --- api/watcher/watcher_test.go | 41 +++++++++++++++++++++---------------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/api/watcher/watcher_test.go b/api/watcher/watcher_test.go index 11862e71868..1a84fd4e079 100644 --- a/api/watcher/watcher_test.go +++ b/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 ¶ms.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 ¶ms.Error{Code: params.CodeStopped} + }, + ).AnyTimes() return "id-666", eventCh }