From 9a8b0f8f4463af272bd3ccb8d868fb2728b6bbc6 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Fri, 16 Feb 2024 16:22:58 +0000 Subject: [PATCH 01/11] Use user service for local user authenticator We know directly if the auth tag is a user, we don't need to jump through hoops to find out if it's a tag from state. Instead, we'll directly speak to the user service. This removes the need for find entity (one down, many to go!). The code was re-arranged to make it a bit easier to grok. I'm of the opinion, that we should probably just yank all the auth out into one location. That might be a task for another day. More work is require, probably requiring #16935 to land first, as the user domain state package has bugs. --- apiserver/authentication/user.go | 376 +++++++++++------- apiserver/authentication/user_test.go | 219 +++++++--- apiserver/export_test.go | 14 +- apiserver/server_test.go | 12 +- apiserver/stateauthenticator/auth.go | 21 +- .../stateauthenticator/authenticator_test.go | 40 +- apiserver/stateauthenticator/context.go | 34 +- apiserver/stateauthenticator/context_test.go | 15 +- .../controller_config_mock_test.go | 56 --- apiserver/stateauthenticator/export_test.go | 5 +- apiserver/stateauthenticator/package_test.go | 2 +- .../stateauthenticator/services_mock_test.go | 110 +++++ .../worker/httpserverargs/authenticator.go | 22 +- .../controller_config_mock_test.go | 56 --- internal/worker/httpserverargs/manifold.go | 3 +- .../worker/httpserverargs/manifold_test.go | 18 +- .../worker/httpserverargs/package_test.go | 2 +- .../httpserverargs/services_mock_test.go | 110 +++++ internal/worker/httpserverargs/worker.go | 72 ++-- internal/worker/httpserverargs/worker_test.go | 36 +- juju/testing/apiserver.go | 20 +- 21 files changed, 812 insertions(+), 431 deletions(-) delete mode 100644 apiserver/stateauthenticator/controller_config_mock_test.go create mode 100644 apiserver/stateauthenticator/services_mock_test.go delete mode 100644 internal/worker/httpserverargs/controller_config_mock_test.go create mode 100644 internal/worker/httpserverargs/services_mock_test.go diff --git a/apiserver/authentication/user.go b/apiserver/authentication/user.go index 7cc0f5a1986..4d48cabab1b 100644 --- a/apiserver/authentication/user.go +++ b/apiserver/authentication/user.go @@ -21,15 +21,77 @@ import ( apiservererrors "github.com/juju/juju/apiserver/errors" coremacaroon "github.com/juju/juju/core/macaroon" + coreuser "github.com/juju/juju/core/user" + usererrors "github.com/juju/juju/domain/user/errors" "github.com/juju/juju/state" ) +const ( + // ErrInvalidLoginMacaroon is returned when a macaroon is not valid for + // a local login request. + ErrInvalidLoginMacaroon = errors.ConstError("invalid login macaroon") +) + var logger = loggo.GetLogger("juju.apiserver.authentication") +// UserService is the interface that wraps the methods required to +// authenticate a user. +type UserService interface { + // GetUserByAuth returns the user with the given name and password. + GetUserByAuth(ctx context.Context, name, password string) (coreuser.User, error) + // GetUserByName returns the user with the given name. + GetUserByName(ctx context.Context, name string) (coreuser.User, error) +} + +// Bakery defines the subset of bakery.Bakery that we require for authentication. +type Bakery interface { + MacaroonMinter + MacaroonChecker +} + +// MacaroonChecker exposes the methods needed from bakery.Checker. +type MacaroonChecker interface { + Auth(ctx context.Context, mss ...macaroon.Slice) *bakery.AuthChecker +} + +// MacaroonMinter exposes the methods needed from bakery.Oven. +type MacaroonMinter interface { + NewMacaroon(ctx context.Context, version bakery.Version, caveats []checkers.Caveat, ops ...bakery.Op) (*bakery.Macaroon, error) +} + +// ExpirableStorageBakery extends Bakery +// with the ExpireStorageAfter method so that root keys are +// removed from storage at that time. +type ExpirableStorageBakery interface { + Bakery + + // ExpireStorageAfter returns a new ExpirableStorageBakery with + // a store that will expire items added to it at the specified time. + ExpireStorageAfter(time.Duration) (ExpirableStorageBakery, error) +} + +// TaggedUser is a user that has been tagged with a names.Tag. +type taggedUser struct { + coreuser.User + tag names.Tag +} + +// Tag returns the tag of the user. +func (u taggedUser) Tag() names.Tag { + return u.tag +} + +type externalUser struct { + tag names.Tag +} + +func (e externalUser) Tag() names.Tag { + return e.tag +} + // LocalUserAuthenticator performs authentication for local users. If a password type LocalUserAuthenticator struct { - AgentAuthenticator - + UserService UserService // Bakery holds the bakery that is used to mint and verify macaroons. Bakery ExpirableStorageBakery @@ -70,6 +132,8 @@ var _ EntityAuthenticator = (*LocalUserAuthenticator)(nil) func (u *LocalUserAuthenticator) Authenticate( ctx context.Context, entityFinder EntityFinder, authParams AuthParams, ) (state.Entity, error) { + // We know this is a user tag and can be nothing but. With those assumptions + // made, we don't need a full AgentAuthenticator. userTag, ok := authParams.AuthTag.(names.UserTag) if !ok { return nil, errors.Errorf("invalid request") @@ -77,152 +141,118 @@ func (u *LocalUserAuthenticator) Authenticate( if !userTag.IsLocal() { return nil, errors.Errorf("invalid request - expected local user") } - if authParams.Credentials == "" { - return u.authenticateMacaroons(ctx, entityFinder, userTag, authParams) - } - return u.AgentAuthenticator.Authenticate(ctx, entityFinder, authParams) -} - -// CreateLocalLoginMacaroon creates a macaroon that may be provided to a -// user as proof that they have logged in with a valid username and password. -// This macaroon may then be used to obtain a discharge macaroon so that -// the user can log in without presenting their password for a set amount -// of time. -func CreateLocalLoginMacaroon( - ctx context.Context, - tag names.UserTag, - minter MacaroonMinter, - clock clock.Clock, - version bakery.Version, -) (*bakery.Macaroon, error) { - // We create the macaroon with a random ID and random root key, which - // enables multiple clients to login as the same user and obtain separate - // macaroons without having them use the same root key. - return minter.NewMacaroon(ctx, version, []checkers.Caveat{ - {Condition: "is-authenticated-user " + tag.Id()}, - checkers.TimeBeforeCaveat(clock.Now().Add(LocalLoginInteractionTimeout)), - }, identchecker.LoginOp) -} - -// CheckLocalLoginCaveat parses and checks that the given caveat string is -// valid for a local login request, and returns the tag of the local user -// that the caveat asserts is logged in. checkers.ErrCaveatNotRecognized will -// be returned if the caveat is not recognised. -func CheckLocalLoginCaveat(caveat string) (names.UserTag, error) { - var tag names.UserTag - op, rest, err := checkers.ParseCaveat(caveat) - if err != nil { - return tag, errors.Annotatef(err, "cannot parse caveat %q", caveat) - } - if op != "is-authenticated-user" { - return tag, checkers.ErrCaveatNotRecognized - } - if !names.IsValidUser(rest) { - return tag, errors.NotValidf("username %q", rest) - } - tag = names.NewUserTag(rest) - if !tag.IsLocal() { - tag = names.UserTag{} - return tag, errors.NotValidf("non-local username %q", rest) + // Empty credentials, will attempt to authenticate with macaroons. + if authParams.Credentials == "" { + return u.authenticateMacaroons(ctx, userTag, authParams) } - return tag, nil -} -// CheckLocalLoginRequest checks that the given HTTP request contains at least -// one valid local login macaroon minted by the given service using -// CreateLocalLoginMacaroon. It returns an error with a -// *bakery.VerificationError cause if the macaroon verification failed. -func CheckLocalLoginRequest( - ctx context.Context, - auth MacaroonChecker, - req *http.Request, -) error { - a := auth.Auth(ctx, httpbakery.RequestMacaroons(req)...) - ai, err := a.Allow(ctx, identchecker.LoginOp) - if err != nil { - return errors.Annotatef(err, "local login request failed: %v", req.Header[httpbakery.MacaroonsHeader]) - } - logger.Tracef("authenticated conditions: %v", ai.Conditions()) - if len(ai.Conditions()) == 0 { - return &bakery.VerificationError{Reason: errors.New("no caveats available")} + // We believe we've got a password, so we'll try to authenticate with it. + // This will check the user service for the user, ensuring that the user + // isn't disabled or deleted. + user, err := u.UserService.GetUserByAuth(ctx, userTag.Name(), authParams.Credentials) + if errors.Is(err, usererrors.NotFound) || errors.Is(err, usererrors.Unauthorized) { + logger.Debugf("user %s not found", userTag.String()) + return nil, errors.Trace(apiservererrors.ErrBadCreds) + } else if err != nil { + return nil, errors.Trace(err) + } else if user.Disabled { + return nil, errors.Trace(apiservererrors.ErrBadCreds) } - return errors.Trace(err) -} -// DischargeCaveats returns the caveats to add to a login discharge macaroon. -func DischargeCaveats(tag names.UserTag, clock clock.Clock) []checkers.Caveat { - firstPartyCaveats := []checkers.Caveat{ - checkers.DeclaredCaveat(usernameKey, tag.Id()), - checkers.TimeBeforeCaveat(clock.Now().Add(localLoginExpiryTime)), - } - return firstPartyCaveats + // StateEntity requires the user to be returned as a state.Entity. + return taggedUser{ + User: user, + tag: userTag, + }, nil } -func (u *LocalUserAuthenticator) authenticateMacaroons( - ctx context.Context, entityFinder EntityFinder, tag names.UserTag, authParams AuthParams, -) (state.Entity, error) { +func (u *LocalUserAuthenticator) authenticateMacaroons(ctx context.Context, userTag names.UserTag, authParams AuthParams) (state.Entity, error) { // Check for a valid request macaroon. if logger.IsTraceEnabled() { mac, _ := json.Marshal(authParams.Macaroons) - logger.Tracef("authentication macaroons for %s: %s", tag, mac) + logger.Tracef("authentication macaroons for %s: %s", userTag, mac) } + + // Attempt to authenticate the user using the macaroons provided. a := u.Bakery.Auth(ctx, authParams.Macaroons...) - ai, err := a.Allow(ctx, identchecker.LoginOp) - if err == nil { - logger.Tracef("authenticated conditions: %v", ai.Conditions()) + macaroonAuthInfo, err := a.Allow(ctx, identchecker.LoginOp) + if err != nil { + return nil, u.handleDischargeRequiredError(ctx, userTag, authParams.BakeryVersion, err) + } else if macaroonAuthInfo != nil && len(macaroonAuthInfo.Conditions()) == 0 { + return nil, u.handleDischargeRequiredError(ctx, userTag, authParams.BakeryVersion, ErrInvalidLoginMacaroon) } - if err != nil || len(ai.Conditions()) == 0 { - logger.Debugf("local-login macaroon authentication failed: %v", err) - cause := err - if cause == nil { - cause = errors.New("invalid login macaroon") - } - // The root keys for these macaroons are stored in MongoDB. - // Expire the documents after after a set amount of time. - expiryTime := u.Clock.Now().Add(localLoginExpiryTime) - bakery, err := u.Bakery.ExpireStorageAfter(localLoginExpiryTime) - if err != nil { - return nil, errors.Trace(err) - } - m, err := bakery.NewMacaroon( - ctx, - authParams.BakeryVersion, - []checkers.Caveat{ - checkers.TimeBeforeCaveat(expiryTime), - checkers.NeedDeclaredCaveat( - checkers.Caveat{ - Location: u.LocalUserIdentityLocation, - Condition: "is-authenticated-user " + tag.Id(), - }, - usernameKey, - ), - }, identchecker.LoginOp) - - if err != nil { - return nil, errors.Annotate(err, "cannot create macaroon") - } - return nil, &apiservererrors.DischargeRequiredError{ - Cause: cause, - LegacyMacaroon: m.M(), - Macaroon: m, - } + logger.Tracef("authenticated conditions: %v", macaroonAuthInfo.Conditions()) + + // Locate the user name from the macaroon. + index := macaroonAuthInfo.OpIndexes[identchecker.LoginOp] + if index < 0 || index > len(macaroonAuthInfo.Macaroons) { + return nil, errors.Trace(apiservererrors.ErrBadCreds) } - loginMac := ai.Macaroons[ai.OpIndexes[identchecker.LoginOp]] + loginMac := macaroonAuthInfo.Macaroons[index] declared := checkers.InferDeclared(coremacaroon.MacaroonNamespace, loginMac) username := declared[usernameKey] - if tag.Id() != username { + + // If the userTag id is not the same as the username, then the user is not + // authenticated. + if userTag.Id() != username { return nil, apiservererrors.ErrPerm } - entity, err := entityFinder.FindEntity(tag) - if errors.Is(err, errors.NotFound) { - logger.Debugf("entity %s not found", tag.String()) + + // We've got a valid macaroon, so we can return the user. + user, err := u.UserService.GetUserByName(ctx, userTag.Name()) + if errors.Is(err, usererrors.NotFound) || errors.Is(err, usererrors.Unauthorized) { + logger.Debugf("user %s not found", userTag.String()) return nil, errors.Trace(apiservererrors.ErrBadCreds) } else if err != nil { return nil, errors.Trace(err) + } else if user.Disabled { + return nil, errors.Trace(apiservererrors.ErrBadCreds) + } + + return taggedUser{ + User: user, + tag: userTag, + }, nil +} + +func (u *LocalUserAuthenticator) handleDischargeRequiredError(ctx context.Context, userTag names.UserTag, bakeryVersion bakery.Version, cause error) error { + logger.Debugf("local-login macaroon authentication failed: %v", cause) + + // The root keys for these macaroons are stored in MongoDB. + // Expire the documents after after a set amount of time. + expiryTime := u.Clock.Now().Add(localLoginExpiryTime) + bakery, err := u.Bakery.ExpireStorageAfter(localLoginExpiryTime) + if err != nil { + return errors.Trace(err) + } + + // Make a new macaroon with a caveat for login operation. + macaroon, err := bakery.NewMacaroon( + ctx, + bakeryVersion, + []checkers.Caveat{ + checkers.TimeBeforeCaveat(expiryTime), + checkers.NeedDeclaredCaveat( + checkers.Caveat{ + Location: u.LocalUserIdentityLocation, + Condition: "is-authenticated-user " + userTag.Id(), + }, + usernameKey, + ), + }, + identchecker.LoginOp, + ) + if err != nil { + return errors.Annotate(err, "cannot create macaroon") + } + + return &apiservererrors.DischargeRequiredError{ + Cause: cause, + LegacyMacaroon: macaroon.M(), + Macaroon: macaroon, } - return entity, nil } // ExternalMacaroonAuthenticator performs authentication for external users using @@ -285,15 +315,7 @@ func (m *ExternalMacaroonAuthenticator) Authenticate(ctx context.Context, _ Enti return nil, errors.Errorf("external identity provider has provided ostensibly local name %q", username) } } - return externalUser{tag}, nil -} - -type externalUser struct { - tag names.Tag -} - -func (e externalUser) Tag() names.Tag { - return e.tag + return externalUser{tag: tag}, nil } // IdentityFromContext implements IdentityClient.IdentityFromContext. @@ -319,29 +341,77 @@ func (ExternalMacaroonAuthenticator) DeclaredIdentity(ctx context.Context, decla return nil, errors.New("no identity declared") } -// Bakery defines the subset of bakery.Bakery that we require for authentication. -type Bakery interface { - MacaroonMinter - MacaroonChecker +// CreateLocalLoginMacaroon creates a macaroon that may be provided to a +// user as proof that they have logged in with a valid username and password. +// This macaroon may then be used to obtain a discharge macaroon so that +// the user can log in without presenting their password for a set amount +// of time. +func CreateLocalLoginMacaroon( + ctx context.Context, + tag names.UserTag, + minter MacaroonMinter, + clock clock.Clock, + version bakery.Version, +) (*bakery.Macaroon, error) { + // We create the macaroon with a random ID and random root key, which + // enables multiple clients to login as the same user and obtain separate + // macaroons without having them use the same root key. + return minter.NewMacaroon(ctx, version, []checkers.Caveat{ + {Condition: "is-authenticated-user " + tag.Id()}, + checkers.TimeBeforeCaveat(clock.Now().Add(LocalLoginInteractionTimeout)), + }, identchecker.LoginOp) } -// MacaroonChecker exposes the methods needed from bakery.Checker. -type MacaroonChecker interface { - Auth(ctx context.Context, mss ...macaroon.Slice) *bakery.AuthChecker +// CheckLocalLoginCaveat parses and checks that the given caveat string is +// valid for a local login request, and returns the tag of the local user +// that the caveat asserts is logged in. checkers.ErrCaveatNotRecognized will +// be returned if the caveat is not recognised. +func CheckLocalLoginCaveat(caveat string) (names.UserTag, error) { + var tag names.UserTag + op, rest, err := checkers.ParseCaveat(caveat) + if err != nil { + return tag, errors.Annotatef(err, "cannot parse caveat %q", caveat) + } + if op != "is-authenticated-user" { + return tag, checkers.ErrCaveatNotRecognized + } + if !names.IsValidUser(rest) { + return tag, errors.NotValidf("username %q", rest) + } + tag = names.NewUserTag(rest) + if !tag.IsLocal() { + tag = names.UserTag{} + return tag, errors.NotValidf("non-local username %q", rest) + } + return tag, nil } -// MacaroonMinter exposes the methods needed from bakery.Oven. -type MacaroonMinter interface { - NewMacaroon(ctx context.Context, version bakery.Version, caveats []checkers.Caveat, ops ...bakery.Op) (*bakery.Macaroon, error) +// CheckLocalLoginRequest checks that the given HTTP request contains at least +// one valid local login macaroon minted by the given service using +// CreateLocalLoginMacaroon. It returns an error with a +// *bakery.VerificationError cause if the macaroon verification failed. +func CheckLocalLoginRequest( + ctx context.Context, + auth MacaroonChecker, + req *http.Request, +) error { + a := auth.Auth(ctx, httpbakery.RequestMacaroons(req)...) + ai, err := a.Allow(ctx, identchecker.LoginOp) + if err != nil { + return errors.Annotatef(err, "local login request failed: %v", req.Header[httpbakery.MacaroonsHeader]) + } + logger.Tracef("authenticated conditions: %v", ai.Conditions()) + if len(ai.Conditions()) == 0 { + return &bakery.VerificationError{Reason: errors.New("no caveats available")} + } + return errors.Trace(err) } -// ExpirableStorageBakery extends Bakery -// with the ExpireStorageAfter method so that root keys are -// removed from storage at that time. -type ExpirableStorageBakery interface { - Bakery - - // ExpireStorageAfter returns a new ExpirableStorageBakery with - // a store that will expire items added to it at the specified time. - ExpireStorageAfter(time.Duration) (ExpirableStorageBakery, error) +// DischargeCaveats returns the caveats to add to a login discharge macaroon. +func DischargeCaveats(tag names.UserTag, clock clock.Clock) []checkers.Caveat { + firstPartyCaveats := []checkers.Caveat{ + checkers.DeclaredCaveat(usernameKey, tag.Id()), + checkers.TimeBeforeCaveat(clock.Now().Add(localLoginExpiryTime)), + } + return firstPartyCaveats } diff --git a/apiserver/authentication/user_test.go b/apiserver/authentication/user_test.go index 59bf5311a95..8c7bc90c455 100644 --- a/apiserver/authentication/user_test.go +++ b/apiserver/authentication/user_test.go @@ -22,6 +22,7 @@ import ( "github.com/juju/juju/apiserver/authentication" apiservererrors "github.com/juju/juju/apiserver/errors" + "github.com/juju/juju/internal/auth" "github.com/juju/juju/internal/password" jujutesting "github.com/juju/juju/juju/testing" "github.com/juju/juju/state" @@ -85,42 +86,192 @@ func (s *userAuthenticatorSuite) TestUnitLoginFails(c *gc.C) { } func (s *userAuthenticatorSuite) TestValidUserLogin(c *gc.C) { - f, release := s.NewFactory(c, s.ControllerModelUUID()) - defer release() + userService := s.ControllerServiceFactory(c).User() + _, err := userService.AddUserWithPassword(context.Background(), "bobbrown", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + c.Assert(err, jc.ErrorIsNil) - user := f.MakeUser(c, &factory.UserParams{ - Name: "bobbrown", - DisplayName: "Bob Brown", - Password: "password", + // User login + authenticator := &authentication.LocalUserAuthenticator{ + UserService: s.ControllerServiceFactory(c).User(), + } + entity, err := authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + AuthTag: names.NewUserTag("bobbrown"), + Credentials: "password", }) + c.Assert(err, jc.ErrorIsNil) + c.Check(entity.Tag(), gc.Equals, names.NewUserTag("bobbrown")) +} + +func (s *userAuthenticatorSuite) TestDisabledUserLogin(c *gc.C) { + c.Skip("This test should be failing, but we don't get disabled out from the service!") + + userService := s.ControllerServiceFactory(c).User() + uuid, err := userService.AddUserWithPassword(context.Background(), "bobbrown", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + c.Assert(err, jc.ErrorIsNil) + err = userService.DisableUserAuthentication(context.Background(), uuid) + c.Assert(err, jc.ErrorIsNil) // User login - authenticator := &authentication.LocalUserAuthenticator{} - _, err := authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ - AuthTag: user.Tag(), + authenticator := &authentication.LocalUserAuthenticator{ + UserService: s.ControllerServiceFactory(c).User(), + } + _, err = authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + AuthTag: names.NewUserTag("bobbrown"), Credentials: "password", }) + c.Assert(err, jc.ErrorIs, apiservererrors.ErrBadCreds) +} + +func (s *userAuthenticatorSuite) TestRemovedUserLogin(c *gc.C) { + userService := s.ControllerServiceFactory(c).User() + uuid, err := userService.AddUserWithPassword(context.Background(), "bobbrown", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + c.Assert(err, jc.ErrorIsNil) + err = userService.RemoveUser(context.Background(), uuid) c.Assert(err, jc.ErrorIsNil) + + // User login + authenticator := &authentication.LocalUserAuthenticator{ + UserService: s.ControllerServiceFactory(c).User(), + } + _, err = authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + AuthTag: names.NewUserTag("bobbrown"), + Credentials: "password", + }) + c.Assert(err, jc.ErrorIs, apiservererrors.ErrBadCreds) } func (s *userAuthenticatorSuite) TestUserLoginWrongPassword(c *gc.C) { - f, release := s.NewFactory(c, s.ControllerModelUUID()) - defer release() + userService := s.ControllerServiceFactory(c).User() + _, err := userService.AddUserWithPassword(context.Background(), "bobbrown", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + c.Assert(err, jc.ErrorIsNil) - user := f.MakeUser(c, &factory.UserParams{ - Name: "bobbrown", - DisplayName: "Bob Brown", - Password: "password", + // User login + authenticator := &authentication.LocalUserAuthenticator{ + UserService: s.ControllerServiceFactory(c).User(), + } + _, err = authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + AuthTag: names.NewUserTag("bobbrown"), + Credentials: "wrongpassword", }) + c.Assert(err, jc.ErrorIs, apiservererrors.ErrBadCreds) +} + +func (s *userAuthenticatorSuite) TestValidMacaroonUserLogin(c *gc.C) { + userService := s.ControllerServiceFactory(c).User() + // TODO (stickupkid): This should be AddUser, but the user service isn't + // correct. This is a temporary fix until we can fix the user service. + _, err := userService.AddUserWithPassword(context.Background(), "bob", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + c.Assert(err, jc.ErrorIsNil) + + mac, err := macaroon.New(nil, nil, "", macaroon.LatestVersion) + c.Assert(err, jc.ErrorIsNil) + err = mac.AddFirstPartyCaveat([]byte("declared username bob")) + c.Assert(err, jc.ErrorIsNil) + macaroons := []macaroon.Slice{{mac}} + bakeryService := mockBakeryService{} // User login - authenticator := &authentication.LocalUserAuthenticator{} - _, err := authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ - AuthTag: user.Tag(), - Credentials: "wrongpassword", + authenticator := &authentication.LocalUserAuthenticator{ + UserService: s.ControllerServiceFactory(c).User(), + Bakery: &bakeryService, + Clock: testclock.NewClock(time.Time{}), + } + entity, err := authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + AuthTag: names.NewUserTag("bob"), + Macaroons: macaroons, + }) + c.Assert(err, jc.ErrorIsNil) + + bakeryService.CheckCallNames(c, "Auth") + call := bakeryService.Calls()[0] + c.Assert(call.Args, gc.HasLen, 1) + c.Assert(call.Args[0], jc.DeepEquals, macaroons) + c.Check(entity.Tag(), gc.Equals, names.NewUserTag("bob")) +} + +func (s *userAuthenticatorSuite) TestInvalidMacaroonUserLogin(c *gc.C) { + userService := s.ControllerServiceFactory(c).User() + // TODO (stickupkid): This should be AddUser, but the user service isn't + // correct. This is a temporary fix until we can fix the user service. + _, err := userService.AddUserWithPassword(context.Background(), "bob", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + c.Assert(err, jc.ErrorIsNil) + + mac, err := macaroon.New(nil, nil, "", macaroon.LatestVersion) + c.Assert(err, jc.ErrorIsNil) + err = mac.AddFirstPartyCaveat([]byte("declared username fred")) + c.Assert(err, jc.ErrorIsNil) + macaroons := []macaroon.Slice{{mac}} + bakeryService := mockBakeryService{} + + // User login + authenticator := &authentication.LocalUserAuthenticator{ + UserService: s.ControllerServiceFactory(c).User(), + Bakery: &bakeryService, + Clock: testclock.NewClock(time.Time{}), + } + _, err = authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + AuthTag: names.NewUserTag("bob"), + Macaroons: macaroons, }) - c.Assert(err, gc.ErrorMatches, "invalid entity name or password") + c.Assert(err, jc.ErrorIs, authentication.ErrInvalidLoginMacaroon) +} + +func (s *userAuthenticatorSuite) TestDisabledMacaroonUserLogin(c *gc.C) { + userService := s.ControllerServiceFactory(c).User() + // TODO (stickupkid): This should be AddUser, but the user service isn't + // correct. This is a temporary fix until we can fix the user service. + uuid, err := userService.AddUserWithPassword(context.Background(), "bob", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + c.Assert(err, jc.ErrorIsNil) + err = userService.DisableUserAuthentication(context.Background(), uuid) + c.Assert(err, jc.ErrorIsNil) + + mac, err := macaroon.New(nil, nil, "", macaroon.LatestVersion) + c.Assert(err, jc.ErrorIsNil) + err = mac.AddFirstPartyCaveat([]byte("declared username bob")) + c.Assert(err, jc.ErrorIsNil) + macaroons := []macaroon.Slice{{mac}} + bakeryService := mockBakeryService{} + + // User login + authenticator := &authentication.LocalUserAuthenticator{ + UserService: s.ControllerServiceFactory(c).User(), + Bakery: &bakeryService, + Clock: testclock.NewClock(time.Time{}), + } + _, err = authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + AuthTag: names.NewUserTag("bob"), + Macaroons: macaroons, + }) + c.Assert(err, jc.ErrorIs, apiservererrors.ErrBadCreds) +} + +func (s *userAuthenticatorSuite) TestRemovedMacaroonUserLogin(c *gc.C) { + userService := s.ControllerServiceFactory(c).User() + // TODO (stickupkid): This should be AddUser, but the user service isn't + // correct. This is a temporary fix until we can fix the user service. + uuid, err := userService.AddUserWithPassword(context.Background(), "bob", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + c.Assert(err, jc.ErrorIsNil) + err = userService.RemoveUser(context.Background(), uuid) + c.Assert(err, jc.ErrorIsNil) + + mac, err := macaroon.New(nil, nil, "", macaroon.LatestVersion) + c.Assert(err, jc.ErrorIsNil) + err = mac.AddFirstPartyCaveat([]byte("declared username bob")) + c.Assert(err, jc.ErrorIsNil) + macaroons := []macaroon.Slice{{mac}} + bakeryService := mockBakeryService{} + // User login + authenticator := &authentication.LocalUserAuthenticator{ + UserService: s.ControllerServiceFactory(c).User(), + Bakery: &bakeryService, + Clock: testclock.NewClock(time.Time{}), + } + _, err = authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + AuthTag: names.NewUserTag("bob"), + Macaroons: macaroons, + }) + c.Assert(err, jc.ErrorIs, apiservererrors.ErrBadCreds) } func (s *userAuthenticatorSuite) TestInvalidRelationLogin(c *gc.C) { @@ -149,34 +300,6 @@ func (s *userAuthenticatorSuite) TestInvalidRelationLogin(c *gc.C) { c.Assert(err, gc.ErrorMatches, "invalid request") } -func (s *userAuthenticatorSuite) TestValidMacaroonUserLogin(c *gc.C) { - f, release := s.NewFactory(c, s.ControllerModelUUID()) - defer release() - - user := f.MakeUser(c, &factory.UserParams{ - Name: "bob", - }) - mac, err := macaroon.New(nil, nil, "", macaroon.LatestVersion) - c.Assert(err, jc.ErrorIsNil) - err = mac.AddFirstPartyCaveat([]byte("declared username bob")) - c.Assert(err, jc.ErrorIsNil) - macaroons := []macaroon.Slice{{mac}} - service := mockBakeryService{} - - // User login - authenticator := &authentication.LocalUserAuthenticator{Bakery: &service, Clock: testclock.NewClock(time.Time{})} - _, err = authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ - AuthTag: user.Tag(), - Macaroons: macaroons, - }) - c.Assert(err, jc.ErrorIsNil) - - service.CheckCallNames(c, "Auth") - call := service.Calls()[0] - c.Assert(call.Args, gc.HasLen, 1) - c.Assert(call.Args[0], jc.DeepEquals, macaroons) -} - func (s *userAuthenticatorSuite) TestCreateLocalLoginMacaroon(c *gc.C) { service := mockBakeryService{} clock := testclock.NewClock(time.Time{}) diff --git a/apiserver/export_test.go b/apiserver/export_test.go index f4ae1a901e2..5f6ea595d75 100644 --- a/apiserver/export_test.go +++ b/apiserver/export_test.go @@ -111,8 +111,8 @@ func TestingAPIRoot(facades *facade.Registry) rpc.Root { // TestingAPIHandler gives you an APIHandler that isn't connected to // anything real. It's enough to let test some basic functionality though. -func TestingAPIHandler(c *gc.C, pool *state.StatePool, st *state.State, configGetter stateauthenticator.ControllerConfigGetter) (*apiHandler, *common.Resources) { - authenticator, err := stateauthenticator.NewAuthenticator(pool, configGetter, clock.WallClock) +func TestingAPIHandler(c *gc.C, pool *state.StatePool, st *state.State, controllerConfigService stateauthenticator.ControllerConfigService, userService stateauthenticator.UserService) (*apiHandler, *common.Resources) { + authenticator, err := stateauthenticator.NewAuthenticator(pool, controllerConfigService, userService, clock.WallClock) c.Assert(err, jc.ErrorIsNil) offerAuthCtxt, err := newOfferAuthcontext(pool) c.Assert(err, jc.ErrorIsNil) @@ -152,10 +152,11 @@ func TestingAPIHandlerWithEntity( c *gc.C, pool *state.StatePool, st *state.State, - configGetter stateauthenticator.ControllerConfigGetter, + controllerConfigService stateauthenticator.ControllerConfigService, + userService stateauthenticator.UserService, entity state.Entity, ) (*apiHandler, *common.Resources) { - h, hr := TestingAPIHandler(c, pool, st, configGetter) + h, hr := TestingAPIHandler(c, pool, st, controllerConfigService, userService) h.authInfo.Entity = entity h.authInfo.Delegator = &stateauthenticator.PermissionDelegator{State: st} return h, hr @@ -168,11 +169,12 @@ func TestingAPIHandlerWithToken( c *gc.C, pool *state.StatePool, st *state.State, - configGetter stateauthenticator.ControllerConfigGetter, + controllerConfigService stateauthenticator.ControllerConfigService, + userService stateauthenticator.UserService, jwt jwt.Token, delegator authentication.PermissionDelegator, ) (*apiHandler, *common.Resources) { - h, hr := TestingAPIHandler(c, pool, st, configGetter) + h, hr := TestingAPIHandler(c, pool, st, controllerConfigService, userService) user, err := names.ParseUserTag(jwt.Subject()) c.Assert(err, jc.ErrorIsNil) h.authInfo.Entity = authjwt.TokenEntity{User: user} diff --git a/apiserver/server_test.go b/apiserver/server_test.go index 510d7c111cb..512687c787f 100644 --- a/apiserver/server_test.go +++ b/apiserver/server_test.go @@ -272,7 +272,7 @@ func (s *serverSuite) TestAPIHandlerHasPermissionLogin(c *gc.C) { serviceFactory := s.ControllerServiceFactory(c) st := s.ControllerModel(c).State() - handler, _ := apiserver.TestingAPIHandlerWithEntity(c, s.StatePool(), st, serviceFactory.ControllerConfig(), u) + handler, _ := apiserver.TestingAPIHandlerWithEntity(c, s.StatePool(), st, serviceFactory.ControllerConfig(), serviceFactory.User(), u) defer handler.Kill() apiserver.AssertHasPermission(c, handler, permission.LoginAccess, ctag, true) @@ -286,7 +286,7 @@ func (s *serverSuite) TestAPIHandlerHasPermissionSuperUser(c *gc.C) { serviceFactory := s.ControllerServiceFactory(c) st := s.ControllerModel(c).State() - handler, _ := apiserver.TestingAPIHandlerWithEntity(c, s.StatePool(), st, serviceFactory.ControllerConfig(), u) + handler, _ := apiserver.TestingAPIHandlerWithEntity(c, s.StatePool(), st, serviceFactory.ControllerConfig(), serviceFactory.User(), u) defer handler.Kill() ua, err := st.SetUserAccess(user, ctag, permission.SuperuserAccess) @@ -313,7 +313,7 @@ func (s *serverSuite) TestAPIHandlerHasPermissionLoginToken(c *gc.C) { delegator := &jwt.PermissionDelegator{Token: token} st := s.ControllerModel(c).State() - handler, _ := apiserver.TestingAPIHandlerWithToken(c, s.StatePool(), st, serviceFactory.ControllerConfig(), token, delegator) + handler, _ := apiserver.TestingAPIHandlerWithToken(c, s.StatePool(), st, serviceFactory.ControllerConfig(), serviceFactory.User(), token, delegator) defer handler.Kill() apiserver.AssertHasPermission(c, handler, permission.LoginAccess, coretesting.ControllerTag, true) @@ -337,7 +337,7 @@ func (s *serverSuite) TestAPIHandlerMissingPermissionLoginToken(c *gc.C) { delegator := &jwt.PermissionDelegator{token} st := s.ControllerModel(c).State() - handler, _ := apiserver.TestingAPIHandlerWithToken(c, s.StatePool(), st, serviceFactory.ControllerConfig(), token, delegator) + handler, _ := apiserver.TestingAPIHandlerWithToken(c, s.StatePool(), st, serviceFactory.ControllerConfig(), serviceFactory.User(), token, delegator) defer handler.Kill() err = handler.HasPermission(permission.AdminAccess, coretesting.ModelTag) var reqError *errors.AccessRequiredError @@ -369,7 +369,7 @@ func (s *serverSuite) TestAPIHandlerConnectedModel(c *gc.C) { serviceFactory := s.ControllerServiceFactory(c) - handler, _ := apiserver.TestingAPIHandler(c, s.StatePool(), otherState, serviceFactory.ControllerConfig()) + handler, _ := apiserver.TestingAPIHandler(c, s.StatePool(), otherState, serviceFactory.ControllerConfig(), serviceFactory.User()) defer handler.Kill() c.Check(handler.ConnectedModel(), gc.Equals, otherState.ModelUUID()) } @@ -428,7 +428,7 @@ func assertStateBecomesClosed(c *gc.C, st *state.State) { func (s *serverSuite) checkAPIHandlerTeardown(c *gc.C, st *state.State) { serviceFactory := s.ControllerServiceFactory(c) - handler, resources := apiserver.TestingAPIHandler(c, s.StatePool(), st, serviceFactory.ControllerConfig()) + handler, resources := apiserver.TestingAPIHandler(c, s.StatePool(), st, serviceFactory.ControllerConfig(), serviceFactory.User()) resource := new(fakeResource) resources.Register(resource) diff --git a/apiserver/stateauthenticator/auth.go b/apiserver/stateauthenticator/auth.go index 4e11211eac8..61811c16b85 100644 --- a/apiserver/stateauthenticator/auth.go +++ b/apiserver/stateauthenticator/auth.go @@ -44,9 +44,9 @@ var AgentTags = []string{ // This Authenticator only works with requests that have been handled // by one of the httpcontext.*ModelHandler handlers. type Authenticator struct { - statePool *state.StatePool - controllerConfigGetter ControllerConfigGetter - authContext *authContext + statePool *state.StatePool + controllerConfigService ControllerConfigService + authContext *authContext } // PermissionDelegator implements authentication.PermissionDelegator @@ -54,30 +54,31 @@ type PermissionDelegator struct { State *state.State } -// ControllerConfigGetter is an interface that can be implemented by +// ControllerConfigService is an interface that can be implemented by // types that can return a controller config. -type ControllerConfigGetter interface { +type ControllerConfigService interface { ControllerConfig(context.Context) (controller.Config, error) } // NewAuthenticator returns a new Authenticator using the given StatePool. func NewAuthenticator( statePool *state.StatePool, - controllerConfigGetter ControllerConfigGetter, + controllerConfigService ControllerConfigService, + userService UserService, clock clock.Clock, ) (*Authenticator, error) { systemState, err := statePool.SystemState() if err != nil { return nil, errors.Trace(err) } - authContext, err := newAuthContext(systemState, controllerConfigGetter, clock) + authContext, err := newAuthContext(systemState, controllerConfigService, userService, clock) if err != nil { return nil, errors.Trace(err) } return &Authenticator{ - statePool: statePool, - controllerConfigGetter: controllerConfigGetter, - authContext: authContext, + statePool: statePool, + controllerConfigService: controllerConfigService, + authContext: authContext, }, nil } diff --git a/apiserver/stateauthenticator/authenticator_test.go b/apiserver/stateauthenticator/authenticator_test.go index c352a5220a0..c00be9e7fa4 100644 --- a/apiserver/stateauthenticator/authenticator_test.go +++ b/apiserver/stateauthenticator/authenticator_test.go @@ -15,17 +15,17 @@ import ( "github.com/juju/juju/apiserver/authentication" "github.com/juju/juju/apiserver/stateauthenticator" - "github.com/juju/juju/state" + coreuser "github.com/juju/juju/core/user" statetesting "github.com/juju/juju/state/testing" - "github.com/juju/juju/testing/factory" ) // TODO update these tests (moved from apiserver) to test // via the public interface, and then get rid of export_test.go. type agentAuthenticatorSuite struct { statetesting.StateSuite - authenticator *stateauthenticator.Authenticator - controllerConfigGetter *MockControllerConfigGetter + authenticator *stateauthenticator.Authenticator + controllerConfigService *MockControllerConfigService + userService *MockUserService } var _ = gc.Suite(&agentAuthenticatorSuite{}) @@ -40,20 +40,24 @@ func (s *agentAuthenticatorSuite) TestAuthenticateLoginRequestHandleNotSupported func (s *agentAuthenticatorSuite) TestAuthenticatorForTag(c *gc.C) { defer s.setupMocks(c).Finish() - user := s.Factory.MakeUser(c, &factory.UserParams{Password: "password"}) + user := coreuser.User{ + Name: "user", + } + tag := names.NewUserTag("user") - authenticator, err := stateauthenticator.EntityAuthenticator(context.Background(), s.authenticator, user.Tag()) + authenticator, err := stateauthenticator.EntityAuthenticator(context.Background(), s.authenticator, tag) c.Assert(err, jc.ErrorIsNil) c.Assert(authenticator, gc.NotNil) - userFinder := userFinder{user} - entity, err := authenticator.Authenticate(context.Background(), userFinder, authentication.AuthParams{ - AuthTag: user.Tag(), + s.userService.EXPECT().GetUserByAuth(context.Background(), "user", "password").Return(user, nil).AnyTimes() + + entity, err := authenticator.Authenticate(context.Background(), nil, authentication.AuthParams{ + AuthTag: tag, Credentials: "password", Nonce: "nonce", }) c.Assert(err, jc.ErrorIsNil) - c.Assert(entity, gc.DeepEquals, user) + c.Assert(entity.Tag(), gc.DeepEquals, tag) } func (s *agentAuthenticatorSuite) TestMachineGetsAgentAuthenticator(c *gc.C) { @@ -94,20 +98,14 @@ func (s *agentAuthenticatorSuite) TestNotSupportedTag(c *gc.C) { func (s *agentAuthenticatorSuite) setupMocks(c *gc.C) *gomock.Controller { ctrl := gomock.NewController(c) - s.controllerConfigGetter = NewMockControllerConfigGetter(ctrl) - s.controllerConfigGetter.EXPECT().ControllerConfig(gomock.Any()).Return(s.ControllerConfig, nil).AnyTimes() + s.controllerConfigService = NewMockControllerConfigService(ctrl) + s.controllerConfigService.EXPECT().ControllerConfig(gomock.Any()).Return(s.ControllerConfig, nil).AnyTimes() + + s.userService = NewMockUserService(ctrl) - authenticator, err := stateauthenticator.NewAuthenticator(s.StatePool, s.controllerConfigGetter, clock.WallClock) + authenticator, err := stateauthenticator.NewAuthenticator(s.StatePool, s.controllerConfigService, s.userService, clock.WallClock) c.Assert(err, jc.ErrorIsNil) s.authenticator = authenticator return ctrl } - -type userFinder struct { - user state.Entity -} - -func (u userFinder) FindEntity(tag names.Tag) (state.Entity, error) { - return u.user, nil -} diff --git a/apiserver/stateauthenticator/context.go b/apiserver/stateauthenticator/context.go index db2c995db69..4c2ef57b50a 100644 --- a/apiserver/stateauthenticator/context.go +++ b/apiserver/stateauthenticator/context.go @@ -23,6 +23,7 @@ import ( "github.com/juju/juju/apiserver/bakeryutil" apiservererrors "github.com/juju/juju/apiserver/errors" coremacaroon "github.com/juju/juju/core/macaroon" + coreuser "github.com/juju/juju/core/user" "github.com/juju/juju/state" ) @@ -37,11 +38,21 @@ const ( localUserIdentityLocationPath = "/auth" ) +// UserService is the interface that wraps the methods required to +// authenticate a user. +type UserService interface { + // GetUserByAuth returns the user with the given name and password. + GetUserByAuth(ctx context.Context, name, password string) (coreuser.User, error) + // GetUserByName returns the user with the given name. + GetUserByName(ctx context.Context, name string) (coreuser.User, error) +} + // authContext holds authentication context shared // between all API endpoints. type authContext struct { - st *state.State - controllerConfigGetter ControllerConfigGetter + st *state.State + controllerConfigService ControllerConfigService + userService UserService clock clock.Clock agentAuth authentication.AgentAuthenticator @@ -84,14 +95,16 @@ func (OpenLoginAuthorizer) AuthorizeOps(ctx context.Context, authorizedOp bakery // newAuthContext creates a new authentication context for st. func newAuthContext( st *state.State, - controllerConfigGetter ControllerConfigGetter, + controllerConfigService ControllerConfigService, + userService UserService, clock clock.Clock, ) (*authContext, error) { ctxt := &authContext{ - st: st, - clock: clock, - controllerConfigGetter: controllerConfigGetter, - localUserInteractions: authentication.NewInteractions(), + st: st, + clock: clock, + controllerConfigService: controllerConfigService, + userService: userService, + localUserInteractions: authentication.NewInteractions(), } // Create a bakery for discharging third-party caveats for @@ -249,6 +262,7 @@ func (a authenticator) localUserAuth() *authentication.LocalUserAuthenticator { Path: localUserIdentityLocationPath, } return &authentication.LocalUserAuthenticator{ + UserService: a.ctxt.userService, Bakery: a.ctxt.localUserBakery, Clock: a.ctxt.clock, LocalUserIdentityLocation: localUserIdentityLocation.String(), @@ -259,7 +273,7 @@ func (a authenticator) localUserAuth() *authentication.LocalUserAuthenticator { // logins for external users. If it fails once, it will always fail. func (ctxt *authContext) externalMacaroonAuth(ctx context.Context, identClient identchecker.IdentityClient) (authentication.EntityAuthenticator, error) { ctxt.macaroonAuthOnce.Do(func() { - ctxt._macaroonAuth, ctxt._macaroonAuthError = newExternalMacaroonAuth(ctx, ctxt.st, ctxt.controllerConfigGetter, ctxt.clock, externalLoginExpiryTime, identClient) + ctxt._macaroonAuth, ctxt._macaroonAuthError = newExternalMacaroonAuth(ctx, ctxt.st, ctxt.controllerConfigService, ctxt.clock, externalLoginExpiryTime, identClient) }) if ctxt._macaroonAuth == nil { return nil, errors.Trace(ctxt._macaroonAuthError) @@ -270,8 +284,8 @@ func (ctxt *authContext) externalMacaroonAuth(ctx context.Context, identClient i // newExternalMacaroonAuth returns an authenticator that can authenticate // macaroon-based logins for external users. This is just a helper function // for authCtxt.externalMacaroonAuth. -func newExternalMacaroonAuth(ctx context.Context, st *state.State, controllerConfigGetter ControllerConfigGetter, clock clock.Clock, expiryTime time.Duration, identClient identchecker.IdentityClient) (*authentication.ExternalMacaroonAuthenticator, error) { - controllerCfg, err := controllerConfigGetter.ControllerConfig(ctx) +func newExternalMacaroonAuth(ctx context.Context, st *state.State, controllerConfigService ControllerConfigService, clock clock.Clock, expiryTime time.Duration, identClient identchecker.IdentityClient) (*authentication.ExternalMacaroonAuthenticator, error) { + controllerCfg, err := controllerConfigService.ControllerConfig(ctx) if err != nil { return nil, errors.Annotate(err, "cannot get controller config") } diff --git a/apiserver/stateauthenticator/context_test.go b/apiserver/stateauthenticator/context_test.go index f3791cf702f..0fbfe2fe4e0 100644 --- a/apiserver/stateauthenticator/context_test.go +++ b/apiserver/stateauthenticator/context_test.go @@ -31,10 +31,11 @@ import ( type macaroonCommonSuite struct { statetesting.StateSuite - discharger *bakerytest.Discharger - authenticator *stateauthenticator.Authenticator - clock *testclock.Clock - controllerConfigGetter *MockControllerConfigGetter + discharger *bakerytest.Discharger + authenticator *stateauthenticator.Authenticator + clock *testclock.Clock + controllerConfigService *MockControllerConfigService + userService *MockUserService } func (s *macaroonCommonSuite) SetUpTest(c *gc.C) { @@ -52,10 +53,10 @@ func (s *macaroonCommonSuite) TearDownTest(c *gc.C) { func (s *macaroonCommonSuite) setupMocks(c *gc.C) *gomock.Controller { ctrl := gomock.NewController(c) - s.controllerConfigGetter = NewMockControllerConfigGetter(ctrl) - s.controllerConfigGetter.EXPECT().ControllerConfig(gomock.Any()).Return(s.ControllerConfig, nil).AnyTimes() + s.controllerConfigService = NewMockControllerConfigService(ctrl) + s.controllerConfigService.EXPECT().ControllerConfig(gomock.Any()).Return(s.ControllerConfig, nil).AnyTimes() - authenticator, err := stateauthenticator.NewAuthenticator(s.StatePool, s.controllerConfigGetter, s.clock) + authenticator, err := stateauthenticator.NewAuthenticator(s.StatePool, s.controllerConfigService, s.userService, s.clock) c.Assert(err, jc.ErrorIsNil) s.authenticator = authenticator diff --git a/apiserver/stateauthenticator/controller_config_mock_test.go b/apiserver/stateauthenticator/controller_config_mock_test.go deleted file mode 100644 index 32fee1e99ce..00000000000 --- a/apiserver/stateauthenticator/controller_config_mock_test.go +++ /dev/null @@ -1,56 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: github.com/juju/juju/apiserver/stateauthenticator (interfaces: ControllerConfigGetter) -// -// Generated by this command: -// -// mockgen -package stateauthenticator_test -destination controller_config_mock_test.go github.com/juju/juju/apiserver/stateauthenticator ControllerConfigGetter -// - -// Package stateauthenticator_test is a generated GoMock package. -package stateauthenticator_test - -import ( - context "context" - reflect "reflect" - - controller "github.com/juju/juju/controller" - gomock "go.uber.org/mock/gomock" -) - -// MockControllerConfigGetter is a mock of ControllerConfigGetter interface. -type MockControllerConfigGetter struct { - ctrl *gomock.Controller - recorder *MockControllerConfigGetterMockRecorder -} - -// MockControllerConfigGetterMockRecorder is the mock recorder for MockControllerConfigGetter. -type MockControllerConfigGetterMockRecorder struct { - mock *MockControllerConfigGetter -} - -// NewMockControllerConfigGetter creates a new mock instance. -func NewMockControllerConfigGetter(ctrl *gomock.Controller) *MockControllerConfigGetter { - mock := &MockControllerConfigGetter{ctrl: ctrl} - mock.recorder = &MockControllerConfigGetterMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockControllerConfigGetter) EXPECT() *MockControllerConfigGetterMockRecorder { - return m.recorder -} - -// ControllerConfig mocks base method. -func (m *MockControllerConfigGetter) ControllerConfig(arg0 context.Context) (controller.Config, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ControllerConfig", arg0) - ret0, _ := ret[0].(controller.Config) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// ControllerConfig indicates an expected call of ControllerConfig. -func (mr *MockControllerConfigGetterMockRecorder) ControllerConfig(arg0 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ControllerConfig", reflect.TypeOf((*MockControllerConfigGetter)(nil).ControllerConfig), arg0) -} diff --git a/apiserver/stateauthenticator/export_test.go b/apiserver/stateauthenticator/export_test.go index c0853fa7607..fc83f933104 100644 --- a/apiserver/stateauthenticator/export_test.go +++ b/apiserver/stateauthenticator/export_test.go @@ -27,7 +27,10 @@ func ServerBakery(ctx context.Context, a *Authenticator, identClient identchecke } func ServerBakeryExpiresImmediately(ctx context.Context, a *Authenticator, identClient identchecker.IdentityClient) (*identchecker.Bakery, error) { - auth, err := newExternalMacaroonAuth(ctx, a.authContext.st, a.authContext.controllerConfigGetter, a.authContext.clock, 0, identClient) + st := a.authContext.st + controllerConfigService := a.authContext.controllerConfigService + + auth, err := newExternalMacaroonAuth(ctx, st, controllerConfigService, a.authContext.clock, 0, identClient) if err != nil { return nil, err } diff --git a/apiserver/stateauthenticator/package_test.go b/apiserver/stateauthenticator/package_test.go index c5cb62dfa54..998542c7564 100644 --- a/apiserver/stateauthenticator/package_test.go +++ b/apiserver/stateauthenticator/package_test.go @@ -9,7 +9,7 @@ import ( coretesting "github.com/juju/juju/testing" ) -//go:generate go run go.uber.org/mock/mockgen -package stateauthenticator_test -destination controller_config_mock_test.go github.com/juju/juju/apiserver/stateauthenticator ControllerConfigGetter +//go:generate go run go.uber.org/mock/mockgen -package stateauthenticator_test -destination services_mock_test.go github.com/juju/juju/apiserver/stateauthenticator ControllerConfigService,UserService func TestPackage(t *testing.T) { coretesting.MgoTestPackage(t) diff --git a/apiserver/stateauthenticator/services_mock_test.go b/apiserver/stateauthenticator/services_mock_test.go new file mode 100644 index 00000000000..cde3c385e0f --- /dev/null +++ b/apiserver/stateauthenticator/services_mock_test.go @@ -0,0 +1,110 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/juju/juju/apiserver/stateauthenticator (interfaces: ControllerConfigService,UserService) +// +// Generated by this command: +// +// mockgen -package stateauthenticator_test -destination services_mock_test.go github.com/juju/juju/apiserver/stateauthenticator ControllerConfigService,UserService +// + +// Package stateauthenticator_test is a generated GoMock package. +package stateauthenticator_test + +import ( + context "context" + reflect "reflect" + + controller "github.com/juju/juju/controller" + user "github.com/juju/juju/core/user" + gomock "go.uber.org/mock/gomock" +) + +// MockControllerConfigService is a mock of ControllerConfigService interface. +type MockControllerConfigService struct { + ctrl *gomock.Controller + recorder *MockControllerConfigServiceMockRecorder +} + +// MockControllerConfigServiceMockRecorder is the mock recorder for MockControllerConfigService. +type MockControllerConfigServiceMockRecorder struct { + mock *MockControllerConfigService +} + +// NewMockControllerConfigService creates a new mock instance. +func NewMockControllerConfigService(ctrl *gomock.Controller) *MockControllerConfigService { + mock := &MockControllerConfigService{ctrl: ctrl} + mock.recorder = &MockControllerConfigServiceMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockControllerConfigService) EXPECT() *MockControllerConfigServiceMockRecorder { + return m.recorder +} + +// ControllerConfig mocks base method. +func (m *MockControllerConfigService) ControllerConfig(arg0 context.Context) (controller.Config, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ControllerConfig", arg0) + ret0, _ := ret[0].(controller.Config) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ControllerConfig indicates an expected call of ControllerConfig. +func (mr *MockControllerConfigServiceMockRecorder) ControllerConfig(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ControllerConfig", reflect.TypeOf((*MockControllerConfigService)(nil).ControllerConfig), arg0) +} + +// MockUserService is a mock of UserService interface. +type MockUserService struct { + ctrl *gomock.Controller + recorder *MockUserServiceMockRecorder +} + +// MockUserServiceMockRecorder is the mock recorder for MockUserService. +type MockUserServiceMockRecorder struct { + mock *MockUserService +} + +// NewMockUserService creates a new mock instance. +func NewMockUserService(ctrl *gomock.Controller) *MockUserService { + mock := &MockUserService{ctrl: ctrl} + mock.recorder = &MockUserServiceMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockUserService) EXPECT() *MockUserServiceMockRecorder { + return m.recorder +} + +// GetUserByAuth mocks base method. +func (m *MockUserService) GetUserByAuth(arg0 context.Context, arg1, arg2 string) (user.User, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetUserByAuth", arg0, arg1, arg2) + ret0, _ := ret[0].(user.User) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetUserByAuth indicates an expected call of GetUserByAuth. +func (mr *MockUserServiceMockRecorder) GetUserByAuth(arg0, arg1, arg2 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserByAuth", reflect.TypeOf((*MockUserService)(nil).GetUserByAuth), arg0, arg1, arg2) +} + +// GetUserByName mocks base method. +func (m *MockUserService) GetUserByName(arg0 context.Context, arg1 string) (user.User, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetUserByName", arg0, arg1) + ret0, _ := ret[0].(user.User) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetUserByName indicates an expected call of GetUserByName. +func (mr *MockUserServiceMockRecorder) GetUserByName(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserByName", reflect.TypeOf((*MockUserService)(nil).GetUserByName), arg0, arg1) +} diff --git a/internal/worker/httpserverargs/authenticator.go b/internal/worker/httpserverargs/authenticator.go index 66ccb7b4157..54c47f1ba9d 100644 --- a/internal/worker/httpserverargs/authenticator.go +++ b/internal/worker/httpserverargs/authenticator.go @@ -13,20 +13,31 @@ import ( "github.com/juju/juju/apiserver/authentication/macaroon" "github.com/juju/juju/apiserver/stateauthenticator" "github.com/juju/juju/controller" + coreuser "github.com/juju/juju/core/user" "github.com/juju/juju/state" ) -// ControllerConfigGetter is an interface that can be implemented by +// ControllerConfigService is an interface that can be implemented by // types that can return a controller config. -type ControllerConfigGetter interface { +type ControllerConfigService interface { ControllerConfig(context.Context) (controller.Config, error) } +// UserService is the interface that wraps the methods required to +// authenticate a user. +type UserService interface { + // GetUserByAuth returns the user with the given name and password. + GetUserByAuth(ctx context.Context, name, password string) (coreuser.User, error) + // GetUserByName returns the user with the given name. + GetUserByName(ctx context.Context, name string) (coreuser.User, error) +} + // NewStateAuthenticatorFunc is a function type satisfied by // NewStateAuthenticator. type NewStateAuthenticatorFunc func( statePool *state.StatePool, - controllerConfigGetter ControllerConfigGetter, + controllerConfigService ControllerConfigService, + userService UserService, mux *apiserverhttp.Mux, clock clock.Clock, abort <-chan struct{}, @@ -38,12 +49,13 @@ type NewStateAuthenticatorFunc func( // local macaroon logins. func NewStateAuthenticator( statePool *state.StatePool, - controllerConfigGetter ControllerConfigGetter, + controllerConfigService ControllerConfigService, + userService UserService, mux *apiserverhttp.Mux, clock clock.Clock, abort <-chan struct{}, ) (macaroon.LocalMacaroonAuthenticator, error) { - stateAuthenticator, err := stateauthenticator.NewAuthenticator(statePool, controllerConfigGetter, clock) + stateAuthenticator, err := stateauthenticator.NewAuthenticator(statePool, controllerConfigService, userService, clock) if err != nil { return nil, errors.Trace(err) } diff --git a/internal/worker/httpserverargs/controller_config_mock_test.go b/internal/worker/httpserverargs/controller_config_mock_test.go deleted file mode 100644 index 6e1cf719c8f..00000000000 --- a/internal/worker/httpserverargs/controller_config_mock_test.go +++ /dev/null @@ -1,56 +0,0 @@ -// Code generated by MockGen. DO NOT EDIT. -// Source: github.com/juju/juju/internal/worker/httpserverargs (interfaces: ControllerConfigGetter) -// -// Generated by this command: -// -// mockgen -package httpserverargs -destination controller_config_mock_test.go github.com/juju/juju/internal/worker/httpserverargs ControllerConfigGetter -// - -// Package httpserverargs is a generated GoMock package. -package httpserverargs - -import ( - context "context" - reflect "reflect" - - controller "github.com/juju/juju/controller" - gomock "go.uber.org/mock/gomock" -) - -// MockControllerConfigGetter is a mock of ControllerConfigGetter interface. -type MockControllerConfigGetter struct { - ctrl *gomock.Controller - recorder *MockControllerConfigGetterMockRecorder -} - -// MockControllerConfigGetterMockRecorder is the mock recorder for MockControllerConfigGetter. -type MockControllerConfigGetterMockRecorder struct { - mock *MockControllerConfigGetter -} - -// NewMockControllerConfigGetter creates a new mock instance. -func NewMockControllerConfigGetter(ctrl *gomock.Controller) *MockControllerConfigGetter { - mock := &MockControllerConfigGetter{ctrl: ctrl} - mock.recorder = &MockControllerConfigGetterMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockControllerConfigGetter) EXPECT() *MockControllerConfigGetterMockRecorder { - return m.recorder -} - -// ControllerConfig mocks base method. -func (m *MockControllerConfigGetter) ControllerConfig(arg0 context.Context) (controller.Config, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ControllerConfig", arg0) - ret0, _ := ret[0].(controller.Config) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// ControllerConfig indicates an expected call of ControllerConfig. -func (mr *MockControllerConfigGetterMockRecorder) ControllerConfig(arg0 any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ControllerConfig", reflect.TypeOf((*MockControllerConfigGetter)(nil).ControllerConfig), arg0) -} diff --git a/internal/worker/httpserverargs/manifold.go b/internal/worker/httpserverargs/manifold.go index d9eeac70484..d0956bd1a80 100644 --- a/internal/worker/httpserverargs/manifold.go +++ b/internal/worker/httpserverargs/manifold.go @@ -72,7 +72,8 @@ func (config ManifoldConfig) start(context context.Context, getter dependency.Ge w, err := newWorker(workerConfig{ statePool: statePool, - controllerConfigGetter: controllerServiceFactory.ControllerConfig(), + controllerConfigService: controllerServiceFactory.ControllerConfig(), + userService: controllerServiceFactory.User(), mux: apiserverhttp.NewMux(), clock: clock, newStateAuthenticatorFn: config.NewStateAuthenticator, diff --git a/internal/worker/httpserverargs/manifold_test.go b/internal/worker/httpserverargs/manifold_test.go index b0e81f67b8a..35a6d9f3281 100644 --- a/internal/worker/httpserverargs/manifold_test.go +++ b/internal/worker/httpserverargs/manifold_test.go @@ -22,6 +22,7 @@ import ( "github.com/juju/juju/apiserver/authentication" "github.com/juju/juju/apiserver/authentication/macaroon" controllerconfigservice "github.com/juju/juju/domain/controllerconfig/service" + userservice "github.com/juju/juju/domain/user/service" "github.com/juju/juju/internal/servicefactory" "github.com/juju/juju/internal/worker/httpserverargs" "github.com/juju/juju/state" @@ -75,12 +76,13 @@ func (s *ManifoldSuite) newGetter(overlay map[string]any) dependency.Getter { func (s *ManifoldSuite) newStateAuthenticator( statePool *state.StatePool, - controllerConfigGetter httpserverargs.ControllerConfigGetter, + controllerConfig httpserverargs.ControllerConfigService, + userService httpserverargs.UserService, mux *apiserverhttp.Mux, clock clock.Clock, abort <-chan struct{}, ) (macaroon.LocalMacaroonAuthenticator, error) { - s.stub.MethodCall(s, "NewStateAuthenticator", statePool, controllerConfigGetter, mux, clock, abort) + s.stub.MethodCall(s, "NewStateAuthenticator", statePool, controllerConfig, userService, mux, clock, abort) if err := s.stub.NextErr(); err != nil { return nil, err } @@ -148,8 +150,8 @@ func (s *ManifoldSuite) TestStoppingWorkerClosesAuthenticator(c *gc.C) { w := s.startWorkerClean(c) s.stub.CheckCallNames(c, "NewStateAuthenticator") authArgs := s.stub.Calls()[0].Args - c.Assert(authArgs, gc.HasLen, 5) - abort := authArgs[4].(<-chan struct{}) + c.Assert(authArgs, gc.HasLen, 6) + abort := authArgs[5].(<-chan struct{}) // abort should still be open at this point. select { @@ -184,10 +186,13 @@ func (s *ManifoldSuite) TestValidate(c *gc.C) { f: func(cfg *httpserverargs.ManifoldConfig) { cfg.NewStateAuthenticator = nil }, expect: "nil NewStateAuthenticator not valid", }} + for i, test := range tests { c.Logf("test #%d (%s)", i, test.expect) + config := s.config test.f(&config) + manifold := httpserverargs.Manifold(config) w, err := manifold.Start(context.Background(), s.getter) workertest.CheckNilOrKill(c, w) @@ -229,3 +234,8 @@ func (s *stubServiceFactory) ControllerConfig() *controllerconfigservice.Watchab s.MethodCall(s, "ControllerConfig") return nil } + +func (s *stubServiceFactory) User() *userservice.Service { + s.MethodCall(s, "User") + return nil +} diff --git a/internal/worker/httpserverargs/package_test.go b/internal/worker/httpserverargs/package_test.go index 3ae84a1d8c4..1cdfdcde7a5 100644 --- a/internal/worker/httpserverargs/package_test.go +++ b/internal/worker/httpserverargs/package_test.go @@ -9,7 +9,7 @@ import ( gc "gopkg.in/check.v1" ) -//go:generate go run go.uber.org/mock/mockgen -package httpserverargs -destination controller_config_mock_test.go github.com/juju/juju/internal/worker/httpserverargs ControllerConfigGetter +//go:generate go run go.uber.org/mock/mockgen -package httpserverargs -destination services_mock_test.go github.com/juju/juju/internal/worker/httpserverargs ControllerConfigService,UserService func TestPackage(t *testing.T) { gc.TestingT(t) diff --git a/internal/worker/httpserverargs/services_mock_test.go b/internal/worker/httpserverargs/services_mock_test.go new file mode 100644 index 00000000000..7a94c9fcf3a --- /dev/null +++ b/internal/worker/httpserverargs/services_mock_test.go @@ -0,0 +1,110 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/juju/juju/internal/worker/httpserverargs (interfaces: ControllerConfigService,UserService) +// +// Generated by this command: +// +// mockgen -package httpserverargs -destination services_mock_test.go github.com/juju/juju/internal/worker/httpserverargs ControllerConfigService,UserService +// + +// Package httpserverargs is a generated GoMock package. +package httpserverargs + +import ( + context "context" + reflect "reflect" + + controller "github.com/juju/juju/controller" + user "github.com/juju/juju/core/user" + gomock "go.uber.org/mock/gomock" +) + +// MockControllerConfigService is a mock of ControllerConfigService interface. +type MockControllerConfigService struct { + ctrl *gomock.Controller + recorder *MockControllerConfigServiceMockRecorder +} + +// MockControllerConfigServiceMockRecorder is the mock recorder for MockControllerConfigService. +type MockControllerConfigServiceMockRecorder struct { + mock *MockControllerConfigService +} + +// NewMockControllerConfigService creates a new mock instance. +func NewMockControllerConfigService(ctrl *gomock.Controller) *MockControllerConfigService { + mock := &MockControllerConfigService{ctrl: ctrl} + mock.recorder = &MockControllerConfigServiceMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockControllerConfigService) EXPECT() *MockControllerConfigServiceMockRecorder { + return m.recorder +} + +// ControllerConfig mocks base method. +func (m *MockControllerConfigService) ControllerConfig(arg0 context.Context) (controller.Config, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ControllerConfig", arg0) + ret0, _ := ret[0].(controller.Config) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ControllerConfig indicates an expected call of ControllerConfig. +func (mr *MockControllerConfigServiceMockRecorder) ControllerConfig(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ControllerConfig", reflect.TypeOf((*MockControllerConfigService)(nil).ControllerConfig), arg0) +} + +// MockUserService is a mock of UserService interface. +type MockUserService struct { + ctrl *gomock.Controller + recorder *MockUserServiceMockRecorder +} + +// MockUserServiceMockRecorder is the mock recorder for MockUserService. +type MockUserServiceMockRecorder struct { + mock *MockUserService +} + +// NewMockUserService creates a new mock instance. +func NewMockUserService(ctrl *gomock.Controller) *MockUserService { + mock := &MockUserService{ctrl: ctrl} + mock.recorder = &MockUserServiceMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockUserService) EXPECT() *MockUserServiceMockRecorder { + return m.recorder +} + +// GetUserByAuth mocks base method. +func (m *MockUserService) GetUserByAuth(arg0 context.Context, arg1, arg2 string) (user.User, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetUserByAuth", arg0, arg1, arg2) + ret0, _ := ret[0].(user.User) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetUserByAuth indicates an expected call of GetUserByAuth. +func (mr *MockUserServiceMockRecorder) GetUserByAuth(arg0, arg1, arg2 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserByAuth", reflect.TypeOf((*MockUserService)(nil).GetUserByAuth), arg0, arg1, arg2) +} + +// GetUserByName mocks base method. +func (m *MockUserService) GetUserByName(arg0 context.Context, arg1 string) (user.User, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetUserByName", arg0, arg1) + ret0, _ := ret[0].(user.User) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetUserByName indicates an expected call of GetUserByName. +func (mr *MockUserServiceMockRecorder) GetUserByName(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserByName", reflect.TypeOf((*MockUserService)(nil).GetUserByName), arg0, arg1) +} diff --git a/internal/worker/httpserverargs/worker.go b/internal/worker/httpserverargs/worker.go index 3af63f7631f..ae19cac8573 100644 --- a/internal/worker/httpserverargs/worker.go +++ b/internal/worker/httpserverargs/worker.go @@ -15,12 +15,14 @@ import ( "github.com/juju/juju/apiserver/apiserverhttp" "github.com/juju/juju/apiserver/authentication/macaroon" "github.com/juju/juju/controller" + coreuser "github.com/juju/juju/core/user" "github.com/juju/juju/state" ) type workerConfig struct { statePool *state.StatePool - controllerConfigGetter ControllerConfigGetter + controllerConfigService ControllerConfigService + userService UserService mux *apiserverhttp.Mux clock clock.Clock newStateAuthenticatorFn NewStateAuthenticatorFunc @@ -30,8 +32,11 @@ func (w workerConfig) Validate() error { if w.statePool == nil { return errors.NotValidf("empty statePool") } - if w.controllerConfigGetter == nil { - return errors.NotValidf("empty controllerConfigGetter") + if w.controllerConfigService == nil { + return errors.NotValidf("empty controllerConfigService") + } + if w.userService == nil { + return errors.NotValidf("empty userService") } if w.mux == nil { return errors.NotValidf("empty mux") @@ -46,10 +51,10 @@ func (w workerConfig) Validate() error { } type argsWorker struct { - catacomb catacomb.Catacomb - cfg workerConfig - authenticator macaroon.LocalMacaroonAuthenticator - managedCtrlConfigGetter *managedCtrlConfigGetter + catacomb catacomb.Catacomb + cfg workerConfig + authenticator macaroon.LocalMacaroonAuthenticator + managedServices *managedServices } func newWorker(cfg workerConfig) (worker.Worker, error) { @@ -57,15 +62,18 @@ func newWorker(cfg workerConfig) (worker.Worker, error) { return nil, errors.Trace(err) } w := argsWorker{ - cfg: cfg, - managedCtrlConfigGetter: newManagedCtrlConfigGetter(cfg.controllerConfigGetter), + cfg: cfg, + managedServices: newManagedServices( + cfg.controllerConfigService, + cfg.userService, + ), } if err := catacomb.Invoke(catacomb.Plan{ Site: &w.catacomb, Work: w.loop, Init: []worker.Worker{ - w.managedCtrlConfigGetter, + w.managedServices, }, }); err != nil { return nil, errors.Trace(err) @@ -73,7 +81,8 @@ func newWorker(cfg workerConfig) (worker.Worker, error) { authenticator, err := w.cfg.newStateAuthenticatorFn( w.cfg.statePool, - w.managedCtrlConfigGetter, + w.managedServices, + w.managedServices, w.cfg.mux, w.cfg.clock, w.catacomb.Dying(), @@ -101,20 +110,25 @@ func (w *argsWorker) loop() error { return w.catacomb.ErrDying() } -// managedCtrlConfigGetter is a ControllerConfigGetter that wraps another -// ControllerConfigGetter and cancels the context when the tomb is dying. +// managedServices is a ControllerConfigService that wraps another +// ControllerConfigService and cancels the context when the tomb is dying. // This is because the location of the controller config request is not // cancellable, so we need the ability to cancel the controller config // request when the tomb is dying. This should prevent any lockup when the // controller is shutting down. -type managedCtrlConfigGetter struct { - tomb tomb.Tomb - configGetter ControllerConfigGetter +type managedServices struct { + tomb tomb.Tomb + controllerConfigService ControllerConfigService + userService UserService } -func newManagedCtrlConfigGetter(configGetter ControllerConfigGetter) *managedCtrlConfigGetter { - w := &managedCtrlConfigGetter{ - configGetter: configGetter, +func newManagedServices( + controllerConfigService ControllerConfigService, + userService UserService, +) *managedServices { + w := &managedServices{ + controllerConfigService: controllerConfigService, + userService: userService, } w.tomb.Go(func() error { <-w.tomb.Dying() @@ -123,17 +137,27 @@ func newManagedCtrlConfigGetter(configGetter ControllerConfigGetter) *managedCtr return w } -// ControllerConfig is part of the ControllerConfigGetter interface. -func (b *managedCtrlConfigGetter) ControllerConfig(ctx context.Context) (controller.Config, error) { - return b.configGetter.ControllerConfig(b.tomb.Context(ctx)) +// ControllerConfig is part of the ControllerConfigService interface. +func (b *managedServices) ControllerConfig(ctx context.Context) (controller.Config, error) { + return b.controllerConfigService.ControllerConfig(b.tomb.Context(ctx)) +} + +// GetUserByName is part of the UserService interface. +func (b *managedServices) GetUserByAuth(ctx context.Context, name, password string) (coreuser.User, error) { + return b.userService.GetUserByAuth(b.tomb.Context(ctx), name, password) +} + +// GetUserByName is part of the UserService interface. +func (b *managedServices) GetUserByName(ctx context.Context, name string) (coreuser.User, error) { + return b.userService.GetUserByName(b.tomb.Context(ctx), name) } // Kill is part of the worker.Worker interface. -func (b *managedCtrlConfigGetter) Kill() { +func (b *managedServices) Kill() { b.tomb.Kill(nil) } // Wait is part of the worker.Worker interface. -func (b *managedCtrlConfigGetter) Wait() error { +func (b *managedServices) Wait() error { return b.tomb.Wait() } diff --git a/internal/worker/httpserverargs/worker_test.go b/internal/worker/httpserverargs/worker_test.go index fbefe4e6dee..87c0c6af98c 100644 --- a/internal/worker/httpserverargs/worker_test.go +++ b/internal/worker/httpserverargs/worker_test.go @@ -35,7 +35,8 @@ func (s *workerConfigSuite) SetUpTest(c *gc.C) { s.IsolationSuite.SetUpTest(c) s.config = workerConfig{ statePool: &state.StatePool{}, - controllerConfigGetter: &managedCtrlConfigGetter{}, + controllerConfigService: &managedServices{}, + userService: &managedServices{}, mux: &apiserverhttp.Mux{}, clock: clock.WallClock, newStateAuthenticatorFn: NewStateAuthenticator, @@ -67,16 +68,17 @@ func (s *workerConfigSuite) TestMissing(c *gc.C) { type workerSuite struct { testing.IsolationSuite - controllerConfigGetter *MockControllerConfigGetter + controllerConfigService *MockControllerConfigService + userService *MockUserService - stateAuthFunc func(*state.StatePool, ControllerConfigGetter, *apiserverhttp.Mux, clock.Clock, <-chan struct{}) (macaroon.LocalMacaroonAuthenticator, error) + stateAuthFunc func(*state.StatePool, ControllerConfigService, UserService, *apiserverhttp.Mux, clock.Clock, <-chan struct{}) (macaroon.LocalMacaroonAuthenticator, error) } var _ = gc.Suite(&workerSuite{}) func (s *workerSuite) TestWorkerStarted(c *gc.C) { started := make(chan struct{}) - s.stateAuthFunc = func(statePool *state.StatePool, controllerConfigGetter ControllerConfigGetter, mux *apiserverhttp.Mux, clock clock.Clock, abort <-chan struct{}) (macaroon.LocalMacaroonAuthenticator, error) { + s.stateAuthFunc = func(*state.StatePool, ControllerConfigService, UserService, *apiserverhttp.Mux, clock.Clock, <-chan struct{}) (macaroon.LocalMacaroonAuthenticator, error) { defer close(started) return nil, nil } @@ -96,10 +98,10 @@ func (s *workerSuite) TestWorkerStarted(c *gc.C) { func (s *workerSuite) TestWorkerControllerConfigContext(c *gc.C) { defer s.setupMocks(c).Finish() - s.controllerConfigGetter.EXPECT().ControllerConfig(gomock.Any()).Return(controller.Config{}, nil) + s.controllerConfigService.EXPECT().ControllerConfig(gomock.Any()).Return(controller.Config{}, nil) started := make(chan struct{}) - s.stateAuthFunc = func(statePool *state.StatePool, controllerConfigGetter ControllerConfigGetter, mux *apiserverhttp.Mux, clock clock.Clock, abort <-chan struct{}) (macaroon.LocalMacaroonAuthenticator, error) { + s.stateAuthFunc = func(*state.StatePool, ControllerConfigService, UserService, *apiserverhttp.Mux, clock.Clock, <-chan struct{}) (macaroon.LocalMacaroonAuthenticator, error) { defer close(started) return nil, nil } @@ -113,7 +115,7 @@ func (s *workerSuite) TestWorkerControllerConfigContext(c *gc.C) { c.Fatalf("timed out waiting for worker to start") } - config, err := w.(*argsWorker).managedCtrlConfigGetter.ControllerConfig(context.Background()) + config, err := w.(*argsWorker).managedServices.ControllerConfig(context.Background()) c.Assert(err, jc.ErrorIsNil) c.Assert(config, gc.NotNil) @@ -123,12 +125,12 @@ func (s *workerSuite) TestWorkerControllerConfigContext(c *gc.C) { func (s *workerSuite) TestWorkerControllerConfigContextDeadline(c *gc.C) { defer s.setupMocks(c).Finish() - s.controllerConfigGetter.EXPECT().ControllerConfig(gomock.Any()).DoAndReturn(func(ctx context.Context) (controller.Config, error) { + s.controllerConfigService.EXPECT().ControllerConfig(gomock.Any()).DoAndReturn(func(ctx context.Context) (controller.Config, error) { return nil, ctx.Err() }) started := make(chan struct{}) - s.stateAuthFunc = func(statePool *state.StatePool, controllerConfigGetter ControllerConfigGetter, mux *apiserverhttp.Mux, clock clock.Clock, abort <-chan struct{}) (macaroon.LocalMacaroonAuthenticator, error) { + s.stateAuthFunc = func(*state.StatePool, ControllerConfigService, UserService, *apiserverhttp.Mux, clock.Clock, <-chan struct{}) (macaroon.LocalMacaroonAuthenticator, error) { defer close(started) return nil, nil } @@ -144,7 +146,7 @@ func (s *workerSuite) TestWorkerControllerConfigContextDeadline(c *gc.C) { workertest.CleanKill(c, w) - _, err := w.(*argsWorker).managedCtrlConfigGetter.ControllerConfig(context.Background()) + _, err := w.(*argsWorker).managedServices.ControllerConfig(context.Background()) c.Assert(err, gc.Equals, context.Canceled) } @@ -155,11 +157,14 @@ func (s *workerSuite) newWorker(c *gc.C) worker.Worker { } func (s *workerSuite) newWorkerConfig(c *gc.C) workerConfig { + services := &managedServices{ + controllerConfigService: s.controllerConfigService, + userService: s.userService, + } return workerConfig{ - statePool: &state.StatePool{}, - controllerConfigGetter: &managedCtrlConfigGetter{ - configGetter: s.controllerConfigGetter, - }, + statePool: &state.StatePool{}, + controllerConfigService: services, + userService: services, mux: &apiserverhttp.Mux{}, clock: clock.WallClock, newStateAuthenticatorFn: s.stateAuthFunc, @@ -169,7 +174,8 @@ func (s *workerSuite) newWorkerConfig(c *gc.C) workerConfig { func (s *workerSuite) setupMocks(c *gc.C) *gomock.Controller { ctrl := gomock.NewController(c) - s.controllerConfigGetter = NewMockControllerConfigGetter(ctrl) + s.controllerConfigService = NewMockControllerConfigService(ctrl) + s.userService = NewMockUserService(ctrl) return ctrl } diff --git a/juju/testing/apiserver.go b/juju/testing/apiserver.go index cb87208f0b9..73635acb37b 100644 --- a/juju/testing/apiserver.go +++ b/juju/testing/apiserver.go @@ -151,6 +151,9 @@ type ApiServerSuite struct { WithUpgrading bool WithAuditLogConfig *auditlog.Config WithIntrospection func(func(string, http.Handler)) + + // AdminUserUUID is the root user for the controller. + AdminUserUUID coreuser.UUID } type noopRegisterer struct { @@ -307,7 +310,7 @@ func (s *ApiServerSuite) setupControllerModel(c *gc.C, controllerCfg controller. c.Assert(err, jc.ErrorIsNil) // Seed the test database with the controller cloud and credential etc. - SeedDatabase(c, s.TxnRunner(), controllerCfg) + s.AdminUserUUID = SeedDatabase(c, s.TxnRunner(), controllerCfg) } func (s *ApiServerSuite) setupApiServer(c *gc.C, controllerCfg controller.Config) { @@ -351,7 +354,8 @@ func (s *ApiServerSuite) setupApiServer(c *gc.C, controllerCfg controller.Config s.ObjectStoreGetter = cfg.ObjectStoreGetter // Set up auth handler. - authenticator, err := stateauthenticator.NewAuthenticator(cfg.StatePool, s.ControllerServiceFactory(c).ControllerConfig(), cfg.Clock) + factory := s.ControllerServiceFactory(c) + authenticator, err := stateauthenticator.NewAuthenticator(cfg.StatePool, factory.ControllerConfig(), factory.User(), cfg.Clock) c.Assert(err, jc.ErrorIsNil) cfg.LocalMacaroonAuthenticator = authenticator err = authenticator.AddHandlers(s.mux) @@ -600,19 +604,23 @@ func (s *ApiServerSuite) SeedCAASCloud(c *gc.C) { // SeedDatabase the database with a supplied controller config, and dummy // cloud and dummy credentials. -func SeedDatabase(c *gc.C, runner database.TxnRunner, controllerConfig controller.Config) { - SeedAdminUser(c, runner) +func SeedDatabase(c *gc.C, runner database.TxnRunner, controllerConfig controller.Config) coreuser.UUID { + adminUserUUID := SeedAdminUser(c, runner) err := controllerconfigbootstrap.InsertInitialControllerConfig(controllerConfig)(context.Background(), runner) c.Assert(err, jc.ErrorIsNil) SeedCloudCredentials(c, runner) + + return adminUserUUID } -func SeedAdminUser(c *gc.C, runner database.TxnRunner) { - _, userAdd := userbootstrap.AddUser(coreuser.AdminUserName) +func SeedAdminUser(c *gc.C, runner database.TxnRunner) coreuser.UUID { + adminUserUUID, userAdd := userbootstrap.AddUser(coreuser.AdminUserName) err := userAdd(context.Background(), runner) c.Assert(err, jc.ErrorIsNil) + + return adminUserUUID } func SeedCloudCredentials(c *gc.C, runner database.TxnRunner) { From 8d3fce8161b6466344217e3259098a4938f36e25 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Mon, 19 Feb 2024 15:43:34 +0000 Subject: [PATCH 02/11] Ensure we always expose disabled when getting user We need to ensure that we expose the disabled flag on the user when we authenticate. --- domain/user/state/state.go | 5 ++++- domain/user/state/state_test.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/domain/user/state/state.go b/domain/user/state/state.go index fb618122ea4..3ae475dfc5b 100644 --- a/domain/user/state/state.go +++ b/domain/user/state/state.go @@ -293,12 +293,15 @@ func (st *State) GetUserByAuth(ctx context.Context, name string, password auth.P err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { getUserWithAuthQuery := ` SELECT ( - user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at, + user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at, + user_authentication.disabled, user_password.password_hash, user_password.password_salt ) AS (&User.*) FROM user LEFT JOIN user_password ON user.uuid = user_password.user_uuid + LEFT JOIN user_authentication + ON user.uuid = user_authentication.user_uuid WHERE user.name = $M.name AND removed = false ` diff --git a/domain/user/state/state_test.go b/domain/user/state/state_test.go index 31ec2fbf36b..4c312dcd517 100644 --- a/domain/user/state/state_test.go +++ b/domain/user/state/state_test.go @@ -450,6 +450,39 @@ func (s *stateSuite) TestGetUserByAuth(c *gc.C) { c.Check(u.DisplayName, gc.Equals, "admin") c.Check(u.CreatorUUID, gc.Equals, adminUUID) c.Check(u.CreatedAt, gc.NotNil) + c.Check(u.Disabled, jc.IsFalse) +} + +// TestGetUserByAuthDisabled asserts that we can get a user by auth from the +// database and has the correct disabled flag. +func (s *stateSuite) TestGetUserByAuthDisabled(c *gc.C) { + st := NewState(s.TxnRunnerFactory()) + + // Add admin user with password hash. + adminUUID, err := user.NewUUID() + c.Assert(err, jc.ErrorIsNil) + + salt, err := auth.NewSalt() + c.Assert(err, jc.ErrorIsNil) + + passwordHash, err := auth.HashPassword(auth.NewPassword("password"), salt) + c.Assert(err, jc.ErrorIsNil) + + err = st.AddUserWithPasswordHash(context.Background(), adminUUID, "admin", "admin", adminUUID, passwordHash, salt) + c.Assert(err, jc.ErrorIsNil) + + err = st.DisableUserAuthentication(context.Background(), "admin") + c.Assert(err, jc.ErrorIsNil) + + // Get the user. + u, err := st.GetUserByAuth(context.Background(), "admin", auth.NewPassword("password")) + c.Assert(err, jc.ErrorIsNil) + + c.Check(u.Name, gc.Equals, "admin") + c.Check(u.DisplayName, gc.Equals, "admin") + c.Check(u.CreatorUUID, gc.Equals, adminUUID) + c.Check(u.CreatedAt, gc.NotNil) + c.Check(u.Disabled, jc.IsTrue) } // TestGetUserByAuthUnauthorized asserts that we get an error when we try to From 08f917a33218a14208cb6953fcb10335433114bd Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Mon, 19 Feb 2024 15:45:07 +0000 Subject: [PATCH 03/11] Add the fixes to the auth tests We can now remove the TODOs that where added because the user service wasn't correct. --- apiserver/authentication/user_test.go | 75 +++++++++++++++++++-------- internal/auth/password.go | 2 +- 2 files changed, 54 insertions(+), 23 deletions(-) diff --git a/apiserver/authentication/user_test.go b/apiserver/authentication/user_test.go index 8c7bc90c455..87a6d609aad 100644 --- a/apiserver/authentication/user_test.go +++ b/apiserver/authentication/user_test.go @@ -22,6 +22,7 @@ import ( "github.com/juju/juju/apiserver/authentication" apiservererrors "github.com/juju/juju/apiserver/errors" + "github.com/juju/juju/domain/user/service" "github.com/juju/juju/internal/auth" "github.com/juju/juju/internal/password" jujutesting "github.com/juju/juju/juju/testing" @@ -87,7 +88,12 @@ func (s *userAuthenticatorSuite) TestUnitLoginFails(c *gc.C) { func (s *userAuthenticatorSuite) TestValidUserLogin(c *gc.C) { userService := s.ControllerServiceFactory(c).User() - _, err := userService.AddUserWithPassword(context.Background(), "bobbrown", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: "bobbrown", + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword("password")), + }) c.Assert(err, jc.ErrorIsNil) // User login @@ -103,12 +109,15 @@ func (s *userAuthenticatorSuite) TestValidUserLogin(c *gc.C) { } func (s *userAuthenticatorSuite) TestDisabledUserLogin(c *gc.C) { - c.Skip("This test should be failing, but we don't get disabled out from the service!") - userService := s.ControllerServiceFactory(c).User() - uuid, err := userService.AddUserWithPassword(context.Background(), "bobbrown", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: "bobbrown", + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword("password")), + }) c.Assert(err, jc.ErrorIsNil) - err = userService.DisableUserAuthentication(context.Background(), uuid) + err = userService.DisableUserAuthentication(context.Background(), "bobbrown") c.Assert(err, jc.ErrorIsNil) // User login @@ -124,9 +133,14 @@ func (s *userAuthenticatorSuite) TestDisabledUserLogin(c *gc.C) { func (s *userAuthenticatorSuite) TestRemovedUserLogin(c *gc.C) { userService := s.ControllerServiceFactory(c).User() - uuid, err := userService.AddUserWithPassword(context.Background(), "bobbrown", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: "bobbrown", + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword("password")), + }) c.Assert(err, jc.ErrorIsNil) - err = userService.RemoveUser(context.Background(), uuid) + err = userService.RemoveUser(context.Background(), "bobbrown") c.Assert(err, jc.ErrorIsNil) // User login @@ -142,7 +156,12 @@ func (s *userAuthenticatorSuite) TestRemovedUserLogin(c *gc.C) { func (s *userAuthenticatorSuite) TestUserLoginWrongPassword(c *gc.C) { userService := s.ControllerServiceFactory(c).User() - _, err := userService.AddUserWithPassword(context.Background(), "bobbrown", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: "bobbrown", + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword("password")), + }) c.Assert(err, jc.ErrorIsNil) // User login @@ -158,9 +177,11 @@ func (s *userAuthenticatorSuite) TestUserLoginWrongPassword(c *gc.C) { func (s *userAuthenticatorSuite) TestValidMacaroonUserLogin(c *gc.C) { userService := s.ControllerServiceFactory(c).User() - // TODO (stickupkid): This should be AddUser, but the user service isn't - // correct. This is a temporary fix until we can fix the user service. - _, err := userService.AddUserWithPassword(context.Background(), "bob", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: "bob", + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + }) c.Assert(err, jc.ErrorIsNil) mac, err := macaroon.New(nil, nil, "", macaroon.LatestVersion) @@ -191,9 +212,11 @@ func (s *userAuthenticatorSuite) TestValidMacaroonUserLogin(c *gc.C) { func (s *userAuthenticatorSuite) TestInvalidMacaroonUserLogin(c *gc.C) { userService := s.ControllerServiceFactory(c).User() - // TODO (stickupkid): This should be AddUser, but the user service isn't - // correct. This is a temporary fix until we can fix the user service. - _, err := userService.AddUserWithPassword(context.Background(), "bob", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: "bobbrown", + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + }) c.Assert(err, jc.ErrorIsNil) mac, err := macaroon.New(nil, nil, "", macaroon.LatestVersion) @@ -218,11 +241,13 @@ func (s *userAuthenticatorSuite) TestInvalidMacaroonUserLogin(c *gc.C) { func (s *userAuthenticatorSuite) TestDisabledMacaroonUserLogin(c *gc.C) { userService := s.ControllerServiceFactory(c).User() - // TODO (stickupkid): This should be AddUser, but the user service isn't - // correct. This is a temporary fix until we can fix the user service. - uuid, err := userService.AddUserWithPassword(context.Background(), "bob", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: "bobbrown", + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + }) c.Assert(err, jc.ErrorIsNil) - err = userService.DisableUserAuthentication(context.Background(), uuid) + err = userService.DisableUserAuthentication(context.Background(), "bobbrown") c.Assert(err, jc.ErrorIsNil) mac, err := macaroon.New(nil, nil, "", macaroon.LatestVersion) @@ -247,11 +272,13 @@ func (s *userAuthenticatorSuite) TestDisabledMacaroonUserLogin(c *gc.C) { func (s *userAuthenticatorSuite) TestRemovedMacaroonUserLogin(c *gc.C) { userService := s.ControllerServiceFactory(c).User() - // TODO (stickupkid): This should be AddUser, but the user service isn't - // correct. This is a temporary fix until we can fix the user service. - uuid, err := userService.AddUserWithPassword(context.Background(), "bob", "Bob Brown", s.AdminUserUUID, auth.NewPassword("password")) + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: "bobbrown", + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + }) c.Assert(err, jc.ErrorIsNil) - err = userService.RemoveUser(context.Background(), uuid) + err = userService.RemoveUser(context.Background(), "bobbrown") c.Assert(err, jc.ErrorIsNil) mac, err := macaroon.New(nil, nil, "", macaroon.LatestVersion) @@ -512,3 +539,7 @@ type simpleEntity struct { func (e *simpleEntity) Tag() names.Tag { return e.tag } + +func ptr[T any](t T) *T { + return &t +} diff --git a/internal/auth/password.go b/internal/auth/password.go index 4329ce4027d..4361339f0d9 100644 --- a/internal/auth/password.go +++ b/internal/auth/password.go @@ -91,7 +91,7 @@ func NewSalt() ([]byte, error) { // NewPassword returns a Password struct wrapping the plain text password. func NewPassword(p string) Password { - return Password{[]byte(p)} + return Password{password: []byte(p)} } // String implements the stringer interface always returning an empty string and From f6ee62ae25249951640844e8dfbc8fb19c2e3638 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Mon, 19 Feb 2024 16:09:56 +0000 Subject: [PATCH 04/11] Ensure we set the user last login when checking creds We should work out if this is the right place to update the last login, as we might not be authorized to view the content in question. For now I'll leave it but bring it up for discussion. I also refactored the check creds to only test for exact tags. This should make it explicit about what we're checking credentials for. --- apiserver/admin_test.go | 63 +++--- apiserver/authentication/agent.go | 34 +++- apiserver/authentication/agent_test.go | 37 +++- apiserver/authentication/entity.go | 25 +++ apiserver/authentication/interfaces.go | 7 +- apiserver/authentication/user.go | 34 +--- apiserver/authentication/user_test.go | 62 ++---- apiserver/httpcontext.go | 2 + apiserver/stateauthenticator/auth.go | 124 +++++++++--- .../stateauthenticator/authenticator_test.go | 2 +- apiserver/stateauthenticator/context.go | 11 +- apiserver/stateauthenticator/locallogin.go | 4 +- apiserver/stateauthenticator/modeluser.go | 190 ------------------ .../stateauthenticator/services_mock_test.go | 14 ++ .../worker/httpserverargs/authenticator.go | 3 + .../httpserverargs/services_mock_test.go | 14 ++ internal/worker/httpserverargs/worker.go | 6 + state/user.go | 20 -- state/user_test.go | 17 -- testing/factory/factory_test.go | 12 -- 20 files changed, 294 insertions(+), 387 deletions(-) delete mode 100644 apiserver/stateauthenticator/modeluser.go diff --git a/apiserver/admin_test.go b/apiserver/admin_test.go index fb925b03fbb..59971578fb0 100644 --- a/apiserver/admin_test.go +++ b/apiserver/admin_test.go @@ -33,6 +33,8 @@ import ( "github.com/juju/juju/core/migration" "github.com/juju/juju/core/network" "github.com/juju/juju/core/permission" + "github.com/juju/juju/domain/user/service" + "github.com/juju/juju/internal/auth" "github.com/juju/juju/internal/password" "github.com/juju/juju/internal/uuid" jujutesting "github.com/juju/juju/juju/testing" @@ -846,32 +848,42 @@ func (s *loginSuite) assertRemoteModel(c *gc.C, conn api.Connection, expected na } func (s *loginSuite) TestLoginUpdatesLastLoginAndConnection(c *gc.C) { - now := s.Clock.Now().UTC() + userService := s.ControllerServiceFactory(c).User() + uuid, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: "bobbrown", + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword("password")), + }) + c.Assert(err, jc.ErrorIsNil) f, release := s.NewFactory(c, s.ControllerModelUUID()) defer release() - password := "shhh..." - user := f.MakeUser(c, &factory.UserParams{ - Password: password, + + f.MakeUser(c, &factory.UserParams{ + Name: "bobbrown", + DisplayName: "Bob Brown", + Password: "password", }) + now := s.Clock.Now().UTC() + info := s.ControllerModelApiInfo() - info.Tag = user.Tag() - info.Password = password + info.Tag = names.NewUserTag("bobbrown") + info.Password = "password" + apiState, err := api.Open(info, api.DialOpts{}) c.Assert(err, jc.ErrorIsNil) defer apiState.Close() // The user now has last login updated. - err = user.Refresh() - c.Assert(err, jc.ErrorIsNil) - lastLogin, err := user.LastLogin() + user, err := userService.GetUser(context.Background(), uuid) c.Assert(err, jc.ErrorIsNil) - c.Assert(lastLogin, jc.Almost, now) + c.Check(user.LastLogin, jc.Almost, now) // The model user is also updated. model := s.ControllerModel(c) - modelUser, err := model.State().UserAccess(user.UserTag(), model.ModelTag()) + modelUser, err := model.State().UserAccess(names.NewUserTag("bobbrown"), model.ModelTag()) c.Assert(err, jc.ErrorIsNil) when, err := model.LastModelConnection(modelUser.UserTag) c.Assert(err, jc.ErrorIsNil) @@ -970,21 +982,22 @@ func (s *loginV3Suite) TestClientLoginToController(c *gc.C) { } func (s *loginV3Suite) TestClientLoginToControllerNoAccessToControllerModel(c *gc.C) { - f, release := s.NewFactory(c, s.ControllerModelUUID()) - defer release() - password := "shhh..." - user := f.MakeUser(c, &factory.UserParams{ - NoModelUser: true, - Password: password, + userService := s.ControllerServiceFactory(c).User() + uuid, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: "bobbrown", + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword("password")), }) - - s.OpenControllerAPIAs(c, user.Tag(), password) - // The user now has last login updated. - err := user.Refresh() c.Assert(err, jc.ErrorIsNil) - lastLogin, err := user.LastLogin() + + now := s.Clock.Now().UTC() + + s.OpenControllerAPIAs(c, names.NewUserTag("bobbrown"), "password") + + user, err := userService.GetUser(context.Background(), uuid) c.Assert(err, jc.ErrorIsNil) - c.Assert(lastLogin, gc.NotNil) + c.Check(user.LastLogin, jc.Almost, now) } func (s *loginV3Suite) TestClientLoginToRootOldClient(c *gc.C) { @@ -1029,3 +1042,7 @@ func (t errorTransport) RoundTrip(req *http.Request) (*http.Response, error) { } return t.fallback.RoundTrip(req) } + +func ptr[T any](v T) *T { + return &v +} diff --git a/apiserver/authentication/agent.go b/apiserver/authentication/agent.go index fc050548416..29d249556b8 100644 --- a/apiserver/authentication/agent.go +++ b/apiserver/authentication/agent.go @@ -7,13 +7,32 @@ import ( "context" "github.com/juju/errors" + "github.com/juju/names/v5" apiservererrors "github.com/juju/juju/apiserver/errors" "github.com/juju/juju/state" ) +// Logger is the minimal logging interface required by the authenticator. +type Logger interface { + Debugf(string, ...interface{}) +} + // EntityAuthenticator performs authentication for juju entities. -type AgentAuthenticator struct{} +type AgentAuthenticator struct { + userService UserService + legacyState *state.State + logger Logger +} + +// NewAgentAuthenticator returns a new authenticator. +func NewAgentAuthenticator(userService UserService, legacyState *state.State, logger Logger) *AgentAuthenticator { + return &AgentAuthenticator{ + userService: userService, + legacyState: legacyState, + logger: logger, + } +} var _ EntityAuthenticator = (*AgentAuthenticator)(nil) @@ -24,8 +43,17 @@ type taggedAuthenticator interface { // Authenticate authenticates the provided entity. // It takes an entityfinder and the tag used to find the entity that requires authentication. -func (*AgentAuthenticator) Authenticate(ctx context.Context, entityFinder EntityFinder, authParams AuthParams) (state.Entity, error) { - entity, err := entityFinder.FindEntity(authParams.AuthTag) +func (a *AgentAuthenticator) Authenticate(ctx context.Context, authParams AuthParams) (state.Entity, error) { + switch authParams.AuthTag.Kind() { + case names.UserTagKind: + return nil, errors.Trace(apiservererrors.ErrBadRequest) + default: + return a.fallbackAuth(ctx, authParams) + } +} + +func (a *AgentAuthenticator) fallbackAuth(ctx context.Context, authParams AuthParams) (state.Entity, error) { + entity, err := a.legacyState.FindEntity(authParams.AuthTag) if errors.Is(err, errors.NotFound) { logger.Debugf("cannot authenticate unknown entity: %v", authParams.AuthTag) return nil, errors.Trace(apiservererrors.ErrBadCreds) diff --git a/apiserver/authentication/agent_test.go b/apiserver/authentication/agent_test.go index b6d71225096..5899c730a89 100644 --- a/apiserver/authentication/agent_test.go +++ b/apiserver/authentication/agent_test.go @@ -6,13 +6,17 @@ package authentication_test import ( "context" + "github.com/juju/names/v5" jc "github.com/juju/testing/checkers" gc "gopkg.in/check.v1" "github.com/juju/juju/apiserver/authentication" + "github.com/juju/juju/domain/user/service" + "github.com/juju/juju/internal/auth" internalpassword "github.com/juju/juju/internal/password" "github.com/juju/juju/juju/testing" "github.com/juju/juju/state" + jujutesting "github.com/juju/juju/testing" "github.com/juju/juju/testing/factory" ) @@ -22,7 +26,7 @@ type agentAuthenticatorSuite struct { machineNonce string unitPassword string machine *state.Machine - user *state.User + user state.Entity unit *state.Unit relation *state.Relation } @@ -32,13 +36,20 @@ var _ = gc.Suite(&agentAuthenticatorSuite{}) func (s *agentAuthenticatorSuite) SetUpTest(c *gc.C) { s.ApiServerSuite.SetUpTest(c) - f, release := s.NewFactory(c, s.ControllerModelUUID()) - defer release() - s.user = f.MakeUser(c, &factory.UserParams{ + userService := s.ControllerServiceFactory(c).User() + userUUID, _, err := userService.AddUser(context.Background(), service.AddUserArg{ Name: "bobbrown", DisplayName: "Bob Brown", - Password: "password", + Password: ptr(auth.NewPassword("password")), + CreatorUUID: s.AdminUserUUID, }) + 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() @@ -108,8 +119,12 @@ func (s *agentAuthenticatorSuite) TestValidLogins(c *gc.C) { st := s.ControllerModel(c).State() for i, t := range testCases { c.Logf("test %d: %s", i, t.about) - var authenticator authentication.AgentAuthenticator - entity, err := authenticator.Authenticate(context.Background(), st, authentication.AuthParams{ + authenticator := authentication.NewAgentAuthenticator( + s.ControllerServiceFactory(c).User(), + st, + jujutesting.NewCheckLogger(c), + ) + entity, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{ AuthTag: t.entity.Tag(), Credentials: t.credentials, Nonce: t.nonce, @@ -146,8 +161,12 @@ func (s *agentAuthenticatorSuite) TestInvalidLogins(c *gc.C) { st := s.ControllerModel(c).State() for i, t := range testCases { c.Logf("test %d: %s", i, t.about) - var authenticator authentication.AgentAuthenticator - entity, err := authenticator.Authenticate(context.Background(), st, authentication.AuthParams{ + authenticator := authentication.NewAgentAuthenticator( + s.ControllerServiceFactory(c).User(), + st, + jujutesting.NewCheckLogger(c), + ) + entity, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{ AuthTag: t.entity.Tag(), Credentials: t.credentials, Nonce: t.nonce, diff --git a/apiserver/authentication/entity.go b/apiserver/authentication/entity.go index 39e780042e7..eecfde9cb97 100644 --- a/apiserver/authentication/entity.go +++ b/apiserver/authentication/entity.go @@ -4,6 +4,7 @@ package authentication import ( + coreuser "github.com/juju/juju/core/user" "github.com/juju/names/v5" ) @@ -22,3 +23,27 @@ func (t *tagWrapper) Tag() names.Tag { func TagToEntity(t names.Tag) Entity { return &tagWrapper{t} } + +// TaggedUser is a user that has been tagged with a names.Tag. +type taggedUser struct { + coreuser.User + tag names.Tag +} + +// TaggedUser returns a user that has been tagged with a names.Tag. +func TaggedUser(u coreuser.User, t names.Tag) Entity { + return taggedUser{u, t} +} + +// Tag returns the tag of the user. +func (u taggedUser) Tag() names.Tag { + return u.tag +} + +type externalUser struct { + tag names.Tag +} + +func (e externalUser) Tag() names.Tag { + return e.tag +} diff --git a/apiserver/authentication/interfaces.go b/apiserver/authentication/interfaces.go index bc75e933385..6432c13eeda 100644 --- a/apiserver/authentication/interfaces.go +++ b/apiserver/authentication/interfaces.go @@ -73,7 +73,7 @@ type PermissionDelegator interface { // implement to authenticate juju entities. type EntityAuthenticator interface { // Authenticate authenticates the given entity. - Authenticate(ctx context.Context, entityFinder EntityFinder, authParams AuthParams) (state.Entity, error) + Authenticate(ctx context.Context, authParams AuthParams) (state.Entity, error) } // Authorizer is a function type for authorizing a request. @@ -89,11 +89,6 @@ type Entity interface { Tag() names.Tag } -// EntityFinder finds the entity described by the tag. -type EntityFinder interface { - FindEntity(tag names.Tag) (state.Entity, error) -} - // HTTPAuthenticator provides an interface for authenticating a raw http request // from a client. type HTTPAuthenticator interface { diff --git a/apiserver/authentication/user.go b/apiserver/authentication/user.go index 4d48cabab1b..ce8a27661db 100644 --- a/apiserver/authentication/user.go +++ b/apiserver/authentication/user.go @@ -70,25 +70,6 @@ type ExpirableStorageBakery interface { ExpireStorageAfter(time.Duration) (ExpirableStorageBakery, error) } -// TaggedUser is a user that has been tagged with a names.Tag. -type taggedUser struct { - coreuser.User - tag names.Tag -} - -// Tag returns the tag of the user. -func (u taggedUser) Tag() names.Tag { - return u.tag -} - -type externalUser struct { - tag names.Tag -} - -func (e externalUser) Tag() names.Tag { - return e.tag -} - // LocalUserAuthenticator performs authentication for local users. If a password type LocalUserAuthenticator struct { UserService UserService @@ -130,7 +111,7 @@ var _ EntityAuthenticator = (*LocalUserAuthenticator)(nil) // If and only if no password is supplied, then Authenticate will check for any // valid macaroons. Otherwise, password authentication will be performed. func (u *LocalUserAuthenticator) Authenticate( - ctx context.Context, entityFinder EntityFinder, authParams AuthParams, + ctx context.Context, authParams AuthParams, ) (state.Entity, error) { // We know this is a user tag and can be nothing but. With those assumptions // made, we don't need a full AgentAuthenticator. @@ -161,10 +142,7 @@ func (u *LocalUserAuthenticator) Authenticate( } // StateEntity requires the user to be returned as a state.Entity. - return taggedUser{ - User: user, - tag: userTag, - }, nil + return TaggedUser(user, userTag), nil } func (u *LocalUserAuthenticator) authenticateMacaroons(ctx context.Context, userTag names.UserTag, authParams AuthParams) (state.Entity, error) { @@ -211,10 +189,8 @@ func (u *LocalUserAuthenticator) authenticateMacaroons(ctx context.Context, user return nil, errors.Trace(apiservererrors.ErrBadCreds) } - return taggedUser{ - User: user, - tag: userTag, - }, nil + // StateEntity requires the user to be returned as a state.Entity. + return TaggedUser(user, userTag), nil } func (u *LocalUserAuthenticator) handleDischargeRequiredError(ctx context.Context, userTag names.UserTag, bakeryVersion bakery.Version, cause error) error { @@ -277,7 +253,7 @@ var _ EntityAuthenticator = (*ExternalMacaroonAuthenticator)(nil) // Authenticate authenticates the provided entity. If there is no macaroon provided, it will // return a *DischargeRequiredError containing a macaroon that can be used to grant access. -func (m *ExternalMacaroonAuthenticator) Authenticate(ctx context.Context, _ EntityFinder, authParams AuthParams) (state.Entity, error) { +func (m *ExternalMacaroonAuthenticator) Authenticate(ctx context.Context, authParams AuthParams) (state.Entity, error) { authChecker := m.Bakery.Checker.Auth(authParams.Macaroons...) ai, identErr := authChecker.Allow(ctx, identchecker.LoginOp) if de, ok := errors.Cause(identErr).(*bakery.DischargeRequiredError); ok { diff --git a/apiserver/authentication/user_test.go b/apiserver/authentication/user_test.go index 87a6d609aad..a4e8658727b 100644 --- a/apiserver/authentication/user_test.go +++ b/apiserver/authentication/user_test.go @@ -52,7 +52,7 @@ func (s *userAuthenticatorSuite) TestMachineLoginFails(c *gc.C) { // attempt machine login authenticator := &authentication.LocalUserAuthenticator{} - _, err = authenticator.Authenticate(context.Background(), nil, authentication.AuthParams{ + _, err = authenticator.Authenticate(context.Background(), authentication.AuthParams{ AuthTag: machine.Tag(), Credentials: machinePassword, Nonce: nonce, @@ -79,7 +79,7 @@ func (s *userAuthenticatorSuite) TestUnitLoginFails(c *gc.C) { // Attempt unit login authenticator := &authentication.LocalUserAuthenticator{} - _, err = authenticator.Authenticate(context.Background(), nil, authentication.AuthParams{ + _, err = authenticator.Authenticate(context.Background(), authentication.AuthParams{ AuthTag: unit.UnitTag(), Credentials: unitPassword, }) @@ -100,7 +100,7 @@ func (s *userAuthenticatorSuite) TestValidUserLogin(c *gc.C) { authenticator := &authentication.LocalUserAuthenticator{ UserService: s.ControllerServiceFactory(c).User(), } - entity, err := authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + entity, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{ AuthTag: names.NewUserTag("bobbrown"), Credentials: "password", }) @@ -124,7 +124,7 @@ func (s *userAuthenticatorSuite) TestDisabledUserLogin(c *gc.C) { authenticator := &authentication.LocalUserAuthenticator{ UserService: s.ControllerServiceFactory(c).User(), } - _, err = authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + _, err = authenticator.Authenticate(context.Background(), authentication.AuthParams{ AuthTag: names.NewUserTag("bobbrown"), Credentials: "password", }) @@ -147,7 +147,7 @@ func (s *userAuthenticatorSuite) TestRemovedUserLogin(c *gc.C) { authenticator := &authentication.LocalUserAuthenticator{ UserService: s.ControllerServiceFactory(c).User(), } - _, err = authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + _, err = authenticator.Authenticate(context.Background(), authentication.AuthParams{ AuthTag: names.NewUserTag("bobbrown"), Credentials: "password", }) @@ -168,7 +168,7 @@ func (s *userAuthenticatorSuite) TestUserLoginWrongPassword(c *gc.C) { authenticator := &authentication.LocalUserAuthenticator{ UserService: s.ControllerServiceFactory(c).User(), } - _, err = authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + _, err = authenticator.Authenticate(context.Background(), authentication.AuthParams{ AuthTag: names.NewUserTag("bobbrown"), Credentials: "wrongpassword", }) @@ -197,7 +197,7 @@ func (s *userAuthenticatorSuite) TestValidMacaroonUserLogin(c *gc.C) { Bakery: &bakeryService, Clock: testclock.NewClock(time.Time{}), } - entity, err := authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + entity, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{ AuthTag: names.NewUserTag("bob"), Macaroons: macaroons, }) @@ -232,7 +232,7 @@ func (s *userAuthenticatorSuite) TestInvalidMacaroonUserLogin(c *gc.C) { Bakery: &bakeryService, Clock: testclock.NewClock(time.Time{}), } - _, err = authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + _, err = authenticator.Authenticate(context.Background(), authentication.AuthParams{ AuthTag: names.NewUserTag("bob"), Macaroons: macaroons, }) @@ -263,7 +263,7 @@ func (s *userAuthenticatorSuite) TestDisabledMacaroonUserLogin(c *gc.C) { Bakery: &bakeryService, Clock: testclock.NewClock(time.Time{}), } - _, err = authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + _, err = authenticator.Authenticate(context.Background(), authentication.AuthParams{ AuthTag: names.NewUserTag("bob"), Macaroons: macaroons, }) @@ -294,7 +294,7 @@ func (s *userAuthenticatorSuite) TestRemovedMacaroonUserLogin(c *gc.C) { Bakery: &bakeryService, Clock: testclock.NewClock(time.Time{}), } - _, err = authenticator.Authenticate(context.Background(), s.ControllerModel(c).State(), authentication.AuthParams{ + _, err = authenticator.Authenticate(context.Background(), authentication.AuthParams{ AuthTag: names.NewUserTag("bob"), Macaroons: macaroons, }) @@ -320,7 +320,7 @@ func (s *userAuthenticatorSuite) TestInvalidRelationLogin(c *gc.C) { // Attempt relation login authenticator := &authentication.LocalUserAuthenticator{} - _, err = authenticator.Authenticate(context.Background(), nil, authentication.AuthParams{ + _, err = authenticator.Authenticate(context.Background(), authentication.AuthParams{ AuthTag: relation.Tag(), Credentials: "dummy-secret", }) @@ -354,7 +354,6 @@ func (s *userAuthenticatorSuite) TestAuthenticateLocalLoginMacaroon(c *gc.C) { service.SetErrors(nil, &bakery.VerificationError{}) _, err := authenticator.Authenticate( context.Background(), - authentication.EntityFinder(nil), authentication.AuthParams{ AuthTag: names.NewUserTag("bobbrown"), }, @@ -434,33 +433,24 @@ var _ = gc.Suite(&macaroonAuthenticatorSuite{}) var authenticateSuccessTests = []struct { about string dischargedUsername string - finder authentication.EntityFinder expectTag string expectError string }{{ about: "user that can be found", dischargedUsername: "bobbrown@somewhere", expectTag: "user-bobbrown@somewhere", - finder: simpleEntityFinder{}, }, { about: "user with no @ domain", dischargedUsername: "bobbrown", - finder: simpleEntityFinder{ - "user-bobbrown@external": true, - }, - expectTag: "user-bobbrown@external", + expectTag: "user-bobbrown@external", }, { about: "invalid user name", dischargedUsername: "--", - finder: simpleEntityFinder{}, expectError: `"--" is an invalid user name`, }, { about: "ostensibly local name", dischargedUsername: "cheat@local", - finder: simpleEntityFinder{ - "cheat@local": true, - }, - expectError: `external identity provider has provided ostensibly local name "cheat@local"`, + expectError: `external identity provider has provided ostensibly local name "cheat@local"`, }} type alwaysIdent struct { @@ -496,7 +486,7 @@ func (s *macaroonAuthenticatorSuite) TestMacaroonAuthentication(c *gc.C) { } // Authenticate once to obtain the macaroon to be discharged. - _, err := authenticator.Authenticate(context.Background(), test.finder, authentication.AuthParams{}) + _, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{}) // Discharge the macaroon. dischargeErr := errors.Cause(err).(*apiservererrors.DischargeRequiredError) @@ -505,7 +495,7 @@ func (s *macaroonAuthenticatorSuite) TestMacaroonAuthentication(c *gc.C) { c.Assert(err, jc.ErrorIsNil) // Authenticate again with the discharged macaroon. - entity, err := authenticator.Authenticate(context.Background(), test.finder, authentication.AuthParams{ + entity, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{ Macaroons: []macaroon.Slice{ms}, }) if test.expectError != "" { @@ -518,28 +508,6 @@ func (s *macaroonAuthenticatorSuite) TestMacaroonAuthentication(c *gc.C) { } } -type simpleEntityFinder map[string]bool - -func (f simpleEntityFinder) FindEntity(tag names.Tag) (state.Entity, error) { - if utag, ok := tag.(names.UserTag); ok { - // It's a user tag which we need to be in canonical form - // so we can look it up unambiguously. - tag = names.NewUserTag(utag.Id()) - } - if f[tag.String()] { - return &simpleEntity{tag}, nil - } - return nil, errors.NotFoundf("entity %q", tag) -} - -type simpleEntity struct { - tag names.Tag -} - -func (e *simpleEntity) Tag() names.Tag { - return e.tag -} - func ptr[T any](t T) *T { return &t } diff --git a/apiserver/httpcontext.go b/apiserver/httpcontext.go index a3b6e787653..ba98523b55f 100644 --- a/apiserver/httpcontext.go +++ b/apiserver/httpcontext.go @@ -78,6 +78,7 @@ func (ctxt *httpContext) stateForRequestAuthenticated(r *http.Request) ( if err != nil { return nil, nil, errors.Trace(err) } + fmt.Println("LOLZ", st, err) return st, authInfo.Entity, nil } @@ -168,6 +169,7 @@ func (ctxt *httpContext) stateForRequestAuthenticatedTag(r *http.Request, kinds if err != nil { return nil, nil, errors.Trace(err) } + fmt.Println("st, entity, err", st, entity, err) if ok, err := checkPermissions(entity.Tag(), common.AuthAny(funcs...)); !ok { st.Release() return nil, nil, err diff --git a/apiserver/stateauthenticator/auth.go b/apiserver/stateauthenticator/auth.go index 61811c16b85..89b5f3e9951 100644 --- a/apiserver/stateauthenticator/auth.go +++ b/apiserver/stateauthenticator/auth.go @@ -108,13 +108,8 @@ func (a *Authenticator) CreateLocalLoginMacaroon(ctx context.Context, tag names. // AddHandlers adds the handlers to the given mux for handling local // macaroon logins. func (a *Authenticator) AddHandlers(mux *apiserverhttp.Mux) error { - systemState, err := a.statePool.SystemState() - if err != nil { - return errors.Trace(err) - } h := &localLoginHandlers{ authCtxt: a.authContext, - finder: systemState, userTokens: map[string]string{}, } h.AddHandlers(mux) @@ -229,14 +224,7 @@ func (a *Authenticator) checkCreds( userLogin bool, authenticator authentication.EntityAuthenticator, ) (authentication.AuthInfo, error) { - var entityFinder authentication.EntityFinder = st - if userLogin { - // When looking up model users, use a custom - // entity finder that looks up both the local user (if the user - // tag is in the local domain) and the model user. - entityFinder = modelUserEntityFinder{st} - } - entity, err := authenticator.Authenticate(ctx, entityFinder, authParams) + entity, err := authenticator.Authenticate(ctx, authParams) if err != nil { return authentication.AuthInfo{}, errors.Trace(err) } @@ -245,22 +233,112 @@ func (a *Authenticator) checkCreds( Delegator: &PermissionDelegator{State: st}, Entity: entity, } - type withIsManager interface { - IsManager() bool + + switch entity.Tag().Kind() { + case names.UserTagKind: + // TODO (stickupkid): This is incorrect. We should only be updating the + // last login time if they've been authorized (not just authenticated). + // For now we'll leave it as is, but we should fix this. + userTag := entity.Tag().(names.UserTag) + + st := a.authContext.st + model, err := st.Model() + if err != nil { + return authentication.AuthInfo{}, errors.Trace(err) + } + modelAccess, err := st.UserAccess(userTag, model.ModelTag()) + if err != nil && !errors.Is(err, errors.NotFound) { + return authentication.AuthInfo{}, errors.Trace(err) + } + + // This is permission checking at the wrong level, but we can keep it + // here for now. + if err := a.checkPerms(ctx, modelAccess, userTag); err != nil { + return authentication.AuthInfo{}, errors.Trace(err) + } + + if err := a.updateUserLastLogin(ctx, modelAccess, userTag, model); err != nil { + return authentication.AuthInfo{}, errors.Trace(err) + } + + case names.MachineTagKind, names.ControllerAgentTagKind: + // Currently only machines and controller agents are managers in the + // context of a controller. + authInfo.Controller = a.isManager(entity) } - if entity, ok := entity.(withIsManager); ok { - authInfo.Controller = entity.IsManager() + + return authInfo, nil +} + +func (a *Authenticator) checkPerms(ctx context.Context, modelAccess permission.UserAccess, userTag names.UserTag) error { + // No model user found, so see if the user has been granted + // access to the controller. + if permission.IsEmptyUserAccess(modelAccess) { + st := a.authContext.st + controllerAccess, err := state.ControllerAccess(st, userTag) + if err != nil && !errors.Is(err, errors.NotFound) { + return errors.Trace(err) + } + // TODO(perrito666) remove the following section about everyone group + // when groups are implemented, this accounts only for the lack of a local + // ControllerUser when logging in from an external user that has not been granted + // permissions on the controller but there are permissions for the special + // everyone group. + if permission.IsEmptyUserAccess(controllerAccess) && !userTag.IsLocal() { + everyoneTag := names.NewUserTag(common.EveryoneTagName) + controllerAccess, err = st.UserAccess(everyoneTag, st.ControllerTag()) + if err != nil && !errors.Is(err, errors.NotFound) { + return errors.Annotatef(err, "obtaining ControllerUser for everyone group") + } + } + if permission.IsEmptyUserAccess(controllerAccess) { + return errors.NotFoundf("model or controller user") + } } + return nil +} - type withLogin interface { - UpdateLastLogin() error +func (a *Authenticator) updateUserLastLogin(ctx context.Context, modelAccess permission.UserAccess, userTag names.UserTag, model *state.Model) error { + updateLastLogin := func() error { + if err := a.authContext.userService.UpdateLastLogin(ctx, userTag.Name()); err != nil { + return errors.Trace(err) + } + return nil } - if entity, ok := entity.(withLogin); ok { - if err := entity.UpdateLastLogin(); err != nil { - logger.Warningf("updating last login time for %v", authParams.AuthTag) + + if !permission.IsEmptyUserAccess(modelAccess) { + if modelAccess.Object.Kind() != names.ModelTagKind { + return errors.NotValidf("%s as model user", modelAccess.Object.Kind()) + } + + if err := model.UpdateLastModelConnection(modelAccess.UserTag); err != nil { + // Attempt to update the users last login data, if the update + // fails, then just report it as a log message and return the + // original error message. + if err := updateLastLogin(); err != nil { + logger.Warningf("updating last login time for %v, %v", userTag, err) + } + return errors.Trace(err) } } - return authInfo, nil + + if err := updateLastLogin(); err != nil { + logger.Warningf("updating last login time for %v, %v", userTag, err) + } + + return nil +} + +func (a *Authenticator) isManager(entity state.Entity) bool { + type withIsManager interface { + IsManager() bool + } + + m, ok := entity.(withIsManager) + if !ok { + return false + } + return m.IsManager() } // LoginRequest extracts basic auth login details from an http.Request. diff --git a/apiserver/stateauthenticator/authenticator_test.go b/apiserver/stateauthenticator/authenticator_test.go index c00be9e7fa4..42429c481b0 100644 --- a/apiserver/stateauthenticator/authenticator_test.go +++ b/apiserver/stateauthenticator/authenticator_test.go @@ -51,7 +51,7 @@ func (s *agentAuthenticatorSuite) TestAuthenticatorForTag(c *gc.C) { s.userService.EXPECT().GetUserByAuth(context.Background(), "user", "password").Return(user, nil).AnyTimes() - entity, err := authenticator.Authenticate(context.Background(), nil, authentication.AuthParams{ + entity, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{ AuthTag: tag, Credentials: "password", Nonce: "nonce", diff --git a/apiserver/stateauthenticator/context.go b/apiserver/stateauthenticator/context.go index 4c2ef57b50a..12d9e64f24e 100644 --- a/apiserver/stateauthenticator/context.go +++ b/apiserver/stateauthenticator/context.go @@ -45,6 +45,9 @@ type UserService interface { GetUserByAuth(ctx context.Context, name, password string) (coreuser.User, error) // GetUserByName returns the user with the given name. GetUserByName(ctx context.Context, name string) (coreuser.User, error) + // UpdateLastLogin updates the last login time for the user with the + // given name. + UpdateLastLogin(ctx context.Context, name string) error } // authContext holds authentication context shared @@ -55,7 +58,7 @@ type authContext struct { userService UserService clock clock.Clock - agentAuth authentication.AgentAuthenticator + agentAuth *authentication.AgentAuthenticator // localUserBakery is the bakery.Bakery used by the controller // for authenticating local users. In time, we may want to use this for @@ -105,6 +108,7 @@ func newAuthContext( controllerConfigService: controllerConfigService, userService: userService, localUserInteractions: authentication.NewInteractions(), + agentAuth: authentication.NewAgentAuthenticator(userService, st, logger), } // Create a bakery for discharging third-party caveats for @@ -216,14 +220,13 @@ type authenticator struct { // tag. func (a authenticator) Authenticate( ctx context.Context, - entityFinder authentication.EntityFinder, authParams authentication.AuthParams, ) (state.Entity, error) { auth, err := a.authenticatorForTag(ctx, authParams.AuthTag) if err != nil { return nil, errors.Trace(err) } - return auth.Authenticate(ctx, entityFinder, authParams) + return auth.Authenticate(ctx, authParams) } // authenticatorForTag returns the authenticator appropriate @@ -247,7 +250,7 @@ func (a authenticator) authenticatorForTag(ctx context.Context, tag names.Tag) ( } for _, agentKind := range AgentTags { if tag.Kind() == agentKind { - return &a.ctxt.agentAuth, nil + return a.ctxt.agentAuth, nil } } return nil, errors.Annotatef(apiservererrors.ErrBadRequest, "unexpected login entity tag") diff --git a/apiserver/stateauthenticator/locallogin.go b/apiserver/stateauthenticator/locallogin.go index 3738d581568..f0200f750b1 100644 --- a/apiserver/stateauthenticator/locallogin.go +++ b/apiserver/stateauthenticator/locallogin.go @@ -20,12 +20,10 @@ import ( "github.com/juju/juju/apiserver/apiserverhttp" "github.com/juju/juju/apiserver/authentication" - "github.com/juju/juju/state" ) type localLoginHandlers struct { authCtxt *authContext - finder state.EntityFinder userTokens map[string]string } @@ -88,7 +86,7 @@ func (h *localLoginHandlers) formHandler(w http.ResponseWriter, req *http.Reques } authenticator := h.authCtxt.authenticator(req.Host) - if _, err := authenticator.Authenticate(ctx, h.finder, authentication.AuthParams{ + if _, err := authenticator.Authenticate(ctx, authentication.AuthParams{ AuthTag: userTag, Credentials: password, }); err != nil { diff --git a/apiserver/stateauthenticator/modeluser.go b/apiserver/stateauthenticator/modeluser.go deleted file mode 100644 index f4dc87e5949..00000000000 --- a/apiserver/stateauthenticator/modeluser.go +++ /dev/null @@ -1,190 +0,0 @@ -// Copyright 2018 Canonical Ltd. -// Licensed under the AGPLv3, see LICENCE file for details. - -package stateauthenticator - -import ( - "time" - - "github.com/juju/errors" - "github.com/juju/names/v5" - - "github.com/juju/juju/apiserver/common" - "github.com/juju/juju/core/permission" - "github.com/juju/juju/state" - stateerrors "github.com/juju/juju/state/errors" -) - -// modelUserEntityFinder implements state.EntityFinder by returning -// an Entity value for model users, ensuring that the user exists in -// the state's current model, while also supporting external users. -type modelUserEntityFinder struct { - st *state.State -} - -// FindEntity implements state.EntityFinder. -func (f modelUserEntityFinder) FindEntity(tag names.Tag) (state.Entity, error) { - utag, ok := tag.(names.UserTag) - if !ok { - return f.st.FindEntity(tag) - } - - model, err := f.st.Model() - if err != nil { - return nil, errors.Trace(err) - } - modelAccess, err := f.st.UserAccess(utag, model.ModelTag()) - if err != nil && !errors.Is(err, errors.NotFound) { - return nil, errors.Trace(err) - } - - // No model user found, so see if the user has been granted - // access to the controller. - if permission.IsEmptyUserAccess(modelAccess) { - controllerAccess, err := state.ControllerAccess(f.st, utag) - if err != nil && !errors.Is(err, errors.NotFound) { - return nil, errors.Trace(err) - } - // TODO(perrito666) remove the following section about everyone group - // when groups are implemented, this accounts only for the lack of a local - // ControllerUser when logging in from an external user that has not been granted - // permissions on the controller but there are permissions for the special - // everyone group. - if permission.IsEmptyUserAccess(controllerAccess) && !utag.IsLocal() { - everyoneTag := names.NewUserTag(common.EveryoneTagName) - controllerAccess, err = f.st.UserAccess(everyoneTag, f.st.ControllerTag()) - if err != nil && !errors.Is(err, errors.NotFound) { - return nil, errors.Annotatef(err, "obtaining ControllerUser for everyone group") - } - } - if permission.IsEmptyUserAccess(controllerAccess) { - return nil, errors.NotFoundf("model or controller user") - } - } - - u := &modelUserEntity{ - st: f.st, - modelAccess: modelAccess, - tag: utag, - } - if utag.IsLocal() { - user, err := f.st.User(utag) - if err != nil { - return nil, errors.Trace(err) - } - u.user = user - } - return u, nil -} - -// modelUserEntity encapsulates a model user -// and, if the user is local, the local state user -// as well. This enables us to implement FindEntity -// in such a way that the authentication mechanisms -// can work without knowing these details. -type modelUserEntity struct { - st *state.State - - modelAccess permission.UserAccess - tag names.Tag - - // user is nil for external users. - user *state.User -} - -// Refresh implements state.Authenticator.Refresh. -func (u *modelUserEntity) Refresh() error { - if u.user == nil { - return nil - } - return u.user.Refresh() -} - -// SetPassword implements state.Authenticator.SetPassword -// by setting the password on the local user. -func (u *modelUserEntity) SetPassword(pass string) error { - if u.user == nil { - return errors.New("cannot set password on external user") - } - return u.user.SetPassword(pass) -} - -// PasswordValid implements state.Authenticator.PasswordValid. -func (u *modelUserEntity) PasswordValid(pass string) bool { - if u.user == nil { - return false - } - return u.user.PasswordValid(pass) -} - -// Tag implements state.Entity.Tag. -func (u *modelUserEntity) Tag() names.Tag { - return u.tag -} - -// LastLogin implements loginEntity.LastLogin. -func (u *modelUserEntity) LastLogin() (time.Time, error) { - // The last connection for the model takes precedence over - // the local user last login time. - var err error - var t time.Time - - model, err := u.st.Model() - if err != nil { - return t, errors.Trace(err) - } - - if !permission.IsEmptyUserAccess(u.modelAccess) { - t, err = model.LastModelConnection(u.modelAccess.UserTag) - } else { - err = stateerrors.NewNeverConnectedError("controller user") - } - if stateerrors.IsNeverConnectedError(err) || permission.IsEmptyUserAccess(u.modelAccess) { - if u.user != nil { - // There's a global user, so use that login time instead. - return u.user.LastLogin() - } - // Since we're implementing LastLogin, we need - // to implement LastLogin error semantics too. - err = stateerrors.NewNeverLoggedInError(err.Error()) - } - return t, errors.Trace(err) -} - -// UpdateLastLogin implements loginEntity.UpdateLastLogin. -func (u *modelUserEntity) UpdateLastLogin() error { - updateLastLogin := func() error { - // If user is nil, don't attempt to perform the update and exit early. - if u.user == nil { - return nil - } - - if err := u.user.UpdateLastLogin(); err != nil { - return errors.Trace(err) - } - return nil - } - - if !permission.IsEmptyUserAccess(u.modelAccess) { - if u.modelAccess.Object.Kind() != names.ModelTagKind { - return errors.NotValidf("%s as model user", u.modelAccess.Object.Kind()) - } - - model, err := u.st.Model() - if err != nil { - return errors.Trace(err) - } - - if err := model.UpdateLastModelConnection(u.modelAccess.UserTag); err != nil { - // Attempt to update the users last login data, if the update - // fails, then just report it as a log message and return the - // original error message. - if err := updateLastLogin(); err != nil { - logger.Warningf("Unable to update last login with %s", err.Error()) - } - return errors.Trace(err) - } - } - - return updateLastLogin() -} diff --git a/apiserver/stateauthenticator/services_mock_test.go b/apiserver/stateauthenticator/services_mock_test.go index cde3c385e0f..ce11a1f218a 100644 --- a/apiserver/stateauthenticator/services_mock_test.go +++ b/apiserver/stateauthenticator/services_mock_test.go @@ -108,3 +108,17 @@ func (mr *MockUserServiceMockRecorder) GetUserByName(arg0, arg1 any) *gomock.Cal mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserByName", reflect.TypeOf((*MockUserService)(nil).GetUserByName), arg0, arg1) } + +// UpdateLastLogin mocks base method. +func (m *MockUserService) UpdateLastLogin(arg0 context.Context, arg1 string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpdateLastLogin", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// UpdateLastLogin indicates an expected call of UpdateLastLogin. +func (mr *MockUserServiceMockRecorder) UpdateLastLogin(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateLastLogin", reflect.TypeOf((*MockUserService)(nil).UpdateLastLogin), arg0, arg1) +} diff --git a/internal/worker/httpserverargs/authenticator.go b/internal/worker/httpserverargs/authenticator.go index 54c47f1ba9d..c530a6ca745 100644 --- a/internal/worker/httpserverargs/authenticator.go +++ b/internal/worker/httpserverargs/authenticator.go @@ -30,6 +30,9 @@ type UserService interface { GetUserByAuth(ctx context.Context, name, password string) (coreuser.User, error) // GetUserByName returns the user with the given name. GetUserByName(ctx context.Context, name string) (coreuser.User, error) + // UpdateLastLogin updates the last login time for the user with the + // given name. + UpdateLastLogin(ctx context.Context, name string) error } // NewStateAuthenticatorFunc is a function type satisfied by diff --git a/internal/worker/httpserverargs/services_mock_test.go b/internal/worker/httpserverargs/services_mock_test.go index 7a94c9fcf3a..a5b6e0e7708 100644 --- a/internal/worker/httpserverargs/services_mock_test.go +++ b/internal/worker/httpserverargs/services_mock_test.go @@ -108,3 +108,17 @@ func (mr *MockUserServiceMockRecorder) GetUserByName(arg0, arg1 any) *gomock.Cal mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserByName", reflect.TypeOf((*MockUserService)(nil).GetUserByName), arg0, arg1) } + +// UpdateLastLogin mocks base method. +func (m *MockUserService) UpdateLastLogin(arg0 context.Context, arg1 string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpdateLastLogin", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// UpdateLastLogin indicates an expected call of UpdateLastLogin. +func (mr *MockUserServiceMockRecorder) UpdateLastLogin(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateLastLogin", reflect.TypeOf((*MockUserService)(nil).UpdateLastLogin), arg0, arg1) +} diff --git a/internal/worker/httpserverargs/worker.go b/internal/worker/httpserverargs/worker.go index ae19cac8573..b7cb1b19700 100644 --- a/internal/worker/httpserverargs/worker.go +++ b/internal/worker/httpserverargs/worker.go @@ -152,6 +152,12 @@ func (b *managedServices) GetUserByName(ctx context.Context, name string) (coreu return b.userService.GetUserByName(b.tomb.Context(ctx), name) } +// UpdateLastLogin updates the last login time for the user with the +// given name. +func (b *managedServices) UpdateLastLogin(ctx context.Context, name string) error { + return b.userService.UpdateLastLogin(b.tomb.Context(ctx), name) +} + // Kill is part of the worker.Worker interface. func (b *managedServices) Kill() { b.tomb.Kill(nil) diff --git a/state/user.go b/state/user.go index da7a32fc1e5..b45a460f90f 100644 --- a/state/user.go +++ b/state/user.go @@ -489,26 +489,6 @@ func (u *User) UserTag() names.UserTag { return names.NewLocalUserTag(name) } -// LastLogin returns when this User last connected through the API in UTC. -// The resulting time will be nil if the user has never logged in. In the -// normal case, the LastLogin is the last time that the user connected through -// the API server. -func (u *User) LastLogin() (time.Time, error) { - lastLogins, closer := u.st.db().GetRawCollection(userLastLoginC) - defer closer() - - var lastLogin userLastLoginDoc - err := lastLogins.FindId(u.doc.DocID).Select(bson.D{{"last-login", 1}}).One(&lastLogin) - if err != nil { - if err == mgo.ErrNotFound { - err = errors.Wrap(err, newNeverLoggedInError(u.UserTag().Name())) - } - return time.Time{}, errors.Trace(err) - } - - return lastLogin.LastLogin.UTC(), nil -} - // UpdateLastLogin sets the LastLogin time of the user to be now (to the // nearest second). func (u *User) UpdateLastLogin() (err error) { diff --git a/state/user_test.go b/state/user_test.go index 19848ad2565..bbea9bd0f82 100644 --- a/state/user_test.go +++ b/state/user_test.go @@ -61,9 +61,6 @@ func (s *UserSuite) TestAddUser(c *gc.C) { c.Assert(user.CreatedBy(), gc.Equals, creator) c.Assert(user.DateCreated().After(now) || user.DateCreated().Equal(now), jc.IsTrue) - lastLogin, err := user.LastLogin() - c.Assert(err, jc.Satisfies, state.IsNeverLoggedInError) - c.Assert(lastLogin, gc.DeepEquals, time.Time{}) user, err = s.State.User(user.UserTag()) c.Assert(err, jc.ErrorIsNil) @@ -74,9 +71,6 @@ func (s *UserSuite) TestAddUser(c *gc.C) { c.Assert(user.CreatedBy(), gc.Equals, creator) c.Assert(user.DateCreated().After(now) || user.DateCreated().Equal(now), jc.IsTrue) - lastLogin, err = user.LastLogin() - c.Assert(err, jc.Satisfies, state.IsNeverLoggedInError) - c.Assert(lastLogin, gc.DeepEquals, time.Time{}) } func (s *UserSuite) TestCheckUserExists(c *gc.C) { @@ -97,17 +91,6 @@ func (s *UserSuite) TestString(c *gc.C) { c.Assert(user.String(), gc.Equals, "foo") } -func (s *UserSuite) TestUpdateLastLogin(c *gc.C) { - now := testing.NonZeroTime().Round(time.Second).UTC() - user := s.Factory.MakeUser(c, nil) - err := user.UpdateLastLogin() - c.Assert(err, jc.ErrorIsNil) - lastLogin, err := user.LastLogin() - c.Assert(err, jc.ErrorIsNil) - c.Assert(lastLogin.After(now) || - lastLogin.Equal(now), jc.IsTrue) -} - func (s *UserSuite) TestSetPassword(c *gc.C) { user := s.Factory.MakeUser(c, nil) testSetPassword(c, s.State, func() (state.Authenticator, error) { diff --git a/testing/factory/factory_test.go b/testing/factory/factory_test.go index a89f2f36159..513d739f269 100644 --- a/testing/factory/factory_test.go +++ b/testing/factory/factory_test.go @@ -53,12 +53,6 @@ func (s *factorySuite) TestMakeUserNil(c *gc.C) { c.Assert(saved.CreatedBy(), gc.Equals, user.CreatedBy()) c.Assert(saved.DateCreated(), gc.Equals, user.DateCreated()) c.Assert(saved.IsDisabled(), gc.Equals, user.IsDisabled()) - - savedLastLogin, err := saved.LastLogin() - c.Assert(err, jc.Satisfies, state.IsNeverLoggedInError) - lastLogin, err := user.LastLogin() - c.Assert(err, jc.Satisfies, state.IsNeverLoggedInError) - c.Assert(savedLastLogin, gc.Equals, lastLogin) } func (s *factorySuite) TestMakeUserParams(c *gc.C) { @@ -87,12 +81,6 @@ func (s *factorySuite) TestMakeUserParams(c *gc.C) { c.Assert(saved.DateCreated(), gc.Equals, user.DateCreated()) c.Assert(saved.IsDisabled(), gc.Equals, user.IsDisabled()) - savedLastLogin, err := saved.LastLogin() - c.Assert(err, jc.Satisfies, state.IsNeverLoggedInError) - lastLogin, err := user.LastLogin() - c.Assert(err, jc.Satisfies, state.IsNeverLoggedInError) - c.Assert(savedLastLogin, gc.Equals, lastLogin) - _, err = s.State.UserAccess(user.UserTag(), s.Model.ModelTag()) c.Assert(err, jc.ErrorIsNil) } From 7ad73b7a7647bfb8a9646671d962e9be2a7559bc Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Wed, 21 Feb 2024 17:10:11 +0000 Subject: [PATCH 05/11] Ensure we encode the error correctly The user state domain was encoding the error incorrectly when getting a user from state. By masking the error as a normal error we hid the fact that it should be not authorized. --- apiserver/authentication/agent.go | 4 +-- apiserver/authentication/user.go | 10 ++++---- apiserver/authentication/user_test.go | 10 ++++---- apiserver/errors/errors.go | 4 +-- apiserver/errors/errors_test.go | 2 +- apiserver/stateauthenticator/auth.go | 13 +++++++--- domain/credential/errors/errors.go | 3 +++ domain/user/state/state.go | 30 +++++++++++++---------- domain/user/state/state_test.go | 27 +++++++++++++++++--- internal/worker/apicaller/connect.go | 2 +- internal/worker/apicaller/connect_test.go | 2 +- 11 files changed, 70 insertions(+), 37 deletions(-) diff --git a/apiserver/authentication/agent.go b/apiserver/authentication/agent.go index 29d249556b8..cbd96e59b99 100644 --- a/apiserver/authentication/agent.go +++ b/apiserver/authentication/agent.go @@ -56,7 +56,7 @@ func (a *AgentAuthenticator) fallbackAuth(ctx context.Context, authParams AuthPa entity, err := a.legacyState.FindEntity(authParams.AuthTag) if errors.Is(err, errors.NotFound) { logger.Debugf("cannot authenticate unknown entity: %v", authParams.AuthTag) - return nil, errors.Trace(apiservererrors.ErrBadCreds) + return nil, errors.Trace(apiservererrors.ErrUnauthorized) } if err != nil { return nil, errors.Trace(err) @@ -66,7 +66,7 @@ func (a *AgentAuthenticator) fallbackAuth(ctx context.Context, authParams AuthPa return nil, errors.Trace(apiservererrors.ErrBadRequest) } if !authenticator.PasswordValid(authParams.Credentials) { - return nil, errors.Trace(apiservererrors.ErrBadCreds) + return nil, errors.Trace(apiservererrors.ErrUnauthorized) } // If this is a machine agent connecting, we need to check the diff --git a/apiserver/authentication/user.go b/apiserver/authentication/user.go index ce8a27661db..8b6f9584690 100644 --- a/apiserver/authentication/user.go +++ b/apiserver/authentication/user.go @@ -134,11 +134,11 @@ func (u *LocalUserAuthenticator) Authenticate( user, err := u.UserService.GetUserByAuth(ctx, userTag.Name(), authParams.Credentials) if errors.Is(err, usererrors.NotFound) || errors.Is(err, usererrors.Unauthorized) { logger.Debugf("user %s not found", userTag.String()) - return nil, errors.Trace(apiservererrors.ErrBadCreds) + return nil, errors.Trace(apiservererrors.ErrUnauthorized) } else if err != nil { return nil, errors.Trace(err) } else if user.Disabled { - return nil, errors.Trace(apiservererrors.ErrBadCreds) + return nil, errors.Trace(apiservererrors.ErrUnauthorized) } // StateEntity requires the user to be returned as a state.Entity. @@ -166,7 +166,7 @@ func (u *LocalUserAuthenticator) authenticateMacaroons(ctx context.Context, user // Locate the user name from the macaroon. index := macaroonAuthInfo.OpIndexes[identchecker.LoginOp] if index < 0 || index > len(macaroonAuthInfo.Macaroons) { - return nil, errors.Trace(apiservererrors.ErrBadCreds) + return nil, errors.Trace(apiservererrors.ErrUnauthorized) } loginMac := macaroonAuthInfo.Macaroons[index] declared := checkers.InferDeclared(coremacaroon.MacaroonNamespace, loginMac) @@ -182,11 +182,11 @@ func (u *LocalUserAuthenticator) authenticateMacaroons(ctx context.Context, user user, err := u.UserService.GetUserByName(ctx, userTag.Name()) if errors.Is(err, usererrors.NotFound) || errors.Is(err, usererrors.Unauthorized) { logger.Debugf("user %s not found", userTag.String()) - return nil, errors.Trace(apiservererrors.ErrBadCreds) + return nil, errors.Trace(apiservererrors.ErrUnauthorized) } else if err != nil { return nil, errors.Trace(err) } else if user.Disabled { - return nil, errors.Trace(apiservererrors.ErrBadCreds) + return nil, errors.Trace(apiservererrors.ErrUnauthorized) } // StateEntity requires the user to be returned as a state.Entity. diff --git a/apiserver/authentication/user_test.go b/apiserver/authentication/user_test.go index a4e8658727b..a1ff5bd99a0 100644 --- a/apiserver/authentication/user_test.go +++ b/apiserver/authentication/user_test.go @@ -128,7 +128,7 @@ func (s *userAuthenticatorSuite) TestDisabledUserLogin(c *gc.C) { AuthTag: names.NewUserTag("bobbrown"), Credentials: "password", }) - c.Assert(err, jc.ErrorIs, apiservererrors.ErrBadCreds) + c.Assert(err, jc.ErrorIs, apiservererrors.ErrUnauthorized) } func (s *userAuthenticatorSuite) TestRemovedUserLogin(c *gc.C) { @@ -151,7 +151,7 @@ func (s *userAuthenticatorSuite) TestRemovedUserLogin(c *gc.C) { AuthTag: names.NewUserTag("bobbrown"), Credentials: "password", }) - c.Assert(err, jc.ErrorIs, apiservererrors.ErrBadCreds) + c.Assert(err, jc.ErrorIs, apiservererrors.ErrUnauthorized) } func (s *userAuthenticatorSuite) TestUserLoginWrongPassword(c *gc.C) { @@ -172,7 +172,7 @@ func (s *userAuthenticatorSuite) TestUserLoginWrongPassword(c *gc.C) { AuthTag: names.NewUserTag("bobbrown"), Credentials: "wrongpassword", }) - c.Assert(err, jc.ErrorIs, apiservererrors.ErrBadCreds) + c.Assert(err, jc.ErrorIs, apiservererrors.ErrUnauthorized) } func (s *userAuthenticatorSuite) TestValidMacaroonUserLogin(c *gc.C) { @@ -267,7 +267,7 @@ func (s *userAuthenticatorSuite) TestDisabledMacaroonUserLogin(c *gc.C) { AuthTag: names.NewUserTag("bob"), Macaroons: macaroons, }) - c.Assert(err, jc.ErrorIs, apiservererrors.ErrBadCreds) + c.Assert(err, jc.ErrorIs, apiservererrors.ErrUnauthorized) } func (s *userAuthenticatorSuite) TestRemovedMacaroonUserLogin(c *gc.C) { @@ -298,7 +298,7 @@ func (s *userAuthenticatorSuite) TestRemovedMacaroonUserLogin(c *gc.C) { AuthTag: names.NewUserTag("bob"), Macaroons: macaroons, }) - c.Assert(err, jc.ErrorIs, apiservererrors.ErrBadCreds) + c.Assert(err, jc.ErrorIs, apiservererrors.ErrUnauthorized) } func (s *userAuthenticatorSuite) TestInvalidRelationLogin(c *gc.C) { diff --git a/apiserver/errors/errors.go b/apiserver/errors/errors.go index d7da946ad00..294c626443b 100644 --- a/apiserver/errors/errors.go +++ b/apiserver/errors/errors.go @@ -24,7 +24,7 @@ var logger = loggo.GetLogger("juju.apiserver.common.errors") const ( // TODO(juju3): move to params ErrBadId = errors.ConstError("id not found") - ErrBadCreds = errors.ConstError("invalid entity name or password") + ErrUnauthorized = errors.ConstError("invalid entity name or password") ErrNoCreds = errors.ConstError("no credentials provided") ErrLoginExpired = errors.ConstError("login expired") ErrPerm = errors.ConstError("permission denied") @@ -59,7 +59,7 @@ var singletonErrorCodes = map[errors.ConstError]string{ leadership.ErrClaimDenied: params.CodeLeadershipClaimDenied, lease.ErrClaimDenied: params.CodeLeaseClaimDenied, ErrBadId: params.CodeNotFound, - ErrBadCreds: params.CodeUnauthorized, + ErrUnauthorized: params.CodeUnauthorized, ErrNoCreds: params.CodeNoCreds, ErrLoginExpired: params.CodeLoginExpired, ErrPerm: params.CodeUnauthorized, diff --git a/apiserver/errors/errors_test.go b/apiserver/errors/errors_test.go index c4b00ba4f5e..6f1b60aac93 100644 --- a/apiserver/errors/errors_test.go +++ b/apiserver/errors/errors_test.go @@ -91,7 +91,7 @@ var errorTransformTests = []struct { return errors.Is(e, apiservererrors.NoAddressSetError) }, }, { - err: apiservererrors.ErrBadCreds, + err: apiservererrors.ErrUnauthorized, code: params.CodeUnauthorized, status: http.StatusUnauthorized, helperFunc: params.IsCodeUnauthorized, diff --git a/apiserver/stateauthenticator/auth.go b/apiserver/stateauthenticator/auth.go index 89b5f3e9951..e45608e1b02 100644 --- a/apiserver/stateauthenticator/auth.go +++ b/apiserver/stateauthenticator/auth.go @@ -161,7 +161,7 @@ func (a *Authenticator) AuthenticateLoginRequest( defer st.Release() authenticator := a.authContext.authenticator(serverHost) - authInfo, err := a.checkCreds(ctx, st.State, authParams, true, authenticator) + authInfo, err := a.checkCreds(ctx, st.State, authParams, authenticator) if err == nil { return authInfo, err } @@ -184,7 +184,7 @@ func (a *Authenticator) AuthenticateLoginRequest( authInfo, err2 = a.checkCreds( ctx, systemState, - authParams, false, authenticator, + authParams, authenticator, ) if err2 == nil && authInfo.Controller { err = nil @@ -193,7 +193,7 @@ func (a *Authenticator) AuthenticateLoginRequest( if err != nil { return authentication.AuthInfo{}, errors.NewUnauthorized(err, "") } - authInfo.Delegator = &PermissionDelegator{systemState} + authInfo.Delegator = &PermissionDelegator{State: systemState} return authInfo, nil } @@ -221,7 +221,6 @@ func (a *Authenticator) checkCreds( ctx context.Context, st *state.State, authParams authentication.AuthParams, - userLogin bool, authenticator authentication.EntityAuthenticator, ) (authentication.AuthInfo, error) { entity, err := authenticator.Authenticate(ctx, authParams) @@ -300,6 +299,12 @@ func (a *Authenticator) checkPerms(ctx context.Context, modelAccess permission.U func (a *Authenticator) updateUserLastLogin(ctx context.Context, modelAccess permission.UserAccess, userTag names.UserTag, model *state.Model) error { updateLastLogin := func() error { + // If the user is not local, we don't update the last login time. + if !userTag.IsLocal() { + return nil + } + + // Update the last login time for the user. if err := a.authContext.userService.UpdateLastLogin(ctx, userTag.Name()); err != nil { return errors.Trace(err) } diff --git a/domain/credential/errors/errors.go b/domain/credential/errors/errors.go index 9df4f80b6d4..fc9bc61f81b 100644 --- a/domain/credential/errors/errors.go +++ b/domain/credential/errors/errors.go @@ -15,4 +15,7 @@ const ( // UnknownCloud describes an error that occurs when a credential for cloud // not known to the controller is updated. UnknownCloud = errors.ConstError("unknown cloud") + + // UserNotFound describes an error that occurs when a user is not found. + UserNotFound = errors.ConstError("user not found") ) diff --git a/domain/user/state/state.go b/domain/user/state/state.go index 3ae475dfc5b..e4840a62d83 100644 --- a/domain/user/state/state.go +++ b/domain/user/state/state.go @@ -238,9 +238,9 @@ AND user.removed = false return user.UUID(result["userUUID"].(string)), nil } -// GetUserByName will retrieve the user with authentication information (last login, disabled) -// specified by name from the database. If the user does not exist an error that satisfies -// usererrors.NotFound will be returned. +// GetUserByName will retrieve the user with authentication information +// (last login, disabled) specified by name from the database. If the user does +// not exist an error that satisfies usererrors.NotFound will be returned. func (st *State) GetUserByName(ctx context.Context, name string) (user.User, error) { db, err := st.DB() if err != nil { @@ -262,7 +262,7 @@ AND removed = false` var result User err = tx.Query(ctx, selectGetUserByNameStmt, sqlair.M{"name": name}).Get(&result) - if err != nil && errors.Is(err, sql.ErrNoRows) { + if errors.Is(err, sql.ErrNoRows) { return errors.Annotatef(usererrors.NotFound, "%q", name) } else if err != nil { return errors.Annotatef(err, "getting user with name %q", name) @@ -281,8 +281,8 @@ AND removed = false` // GetUserByAuth will retrieve the user with checking authentication // information specified by UUID and password from the database. -// If the user does not exist or the user does not authenticate an -// error that satisfies usererrors.Unauthorized will be returned. +// If the user does not exist an error that satisfies usererrors.NotFound will +// be returned, otherwise unauthorized will be returned. func (st *State) GetUserByAuth(ctx context.Context, name string, password auth.Password) (user.User, error) { db, err := st.DB() if err != nil { @@ -294,14 +294,12 @@ func (st *State) GetUserByAuth(ctx context.Context, name string, password auth.P getUserWithAuthQuery := ` SELECT ( user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at, - user_authentication.disabled, + user.disabled, user_password.password_hash, user_password.password_salt ) AS (&User.*) -FROM user +FROM v_user_auth AS user LEFT JOIN user_password ON user.uuid = user_password.user_uuid - LEFT JOIN user_authentication - ON user.uuid = user_authentication.user_uuid WHERE user.name = $M.name AND removed = false ` @@ -312,8 +310,10 @@ AND removed = false } err = tx.Query(ctx, selectGetUserByAuthStmt, sqlair.M{"name": name}).Get(&result) - if err != nil { - return errors.Annotatef(usererrors.Unauthorized, "%q", name) + if errors.Is(err, sql.ErrNoRows) { + return errors.Annotatef(usererrors.NotFound, "%q", name) + } else if err != nil { + return errors.Annotatef(err, "getting user with name %q", name) } return nil @@ -323,7 +323,11 @@ AND removed = false } passwordHash, err := auth.HashPassword(password, result.PasswordSalt) - if err != nil { + if errors.Is(err, errors.NotValid) { + // If the user has no salt, then they don't have a password correctly + // set. In this case, we should return an unauthorized error. + return user.User{}, errors.Annotatef(usererrors.Unauthorized, "%q", name) + } else if err != nil { return user.User{}, errors.Annotatef(err, "hashing password for user with name %q", name) } else if passwordHash != result.PasswordHash { return user.User{}, errors.Annotatef(usererrors.Unauthorized, "%q", name) diff --git a/domain/user/state/state_test.go b/domain/user/state/state_test.go index 4c312dcd517..df9b55e0d72 100644 --- a/domain/user/state/state_test.go +++ b/domain/user/state/state_test.go @@ -453,6 +453,27 @@ func (s *stateSuite) TestGetUserByAuth(c *gc.C) { c.Check(u.Disabled, jc.IsFalse) } +// TestGetUserByAuthWithInvalidSalt asserts that we correctly send an +// unauthorized error if the user doesn't have a valid salt. +func (s *stateSuite) TestGetUserByAuthWithInvalidSalt(c *gc.C) { + st := NewState(s.TxnRunnerFactory()) + + // Add admin user. + adminUUID, err := user.NewUUID() + c.Assert(err, jc.ErrorIsNil) + + err = st.AddUserWithPasswordHash( + context.Background(), adminUUID, + "admin", "admin", + adminUUID, "passwordHash", []byte{}, + ) + c.Assert(err, jc.ErrorIsNil) + + // Get the user. + _, err = st.GetUserByAuth(context.Background(), "admin", auth.NewPassword("passwordHash")) + c.Assert(err, jc.ErrorIs, usererrors.Unauthorized) +} + // TestGetUserByAuthDisabled asserts that we can get a user by auth from the // database and has the correct disabled flag. func (s *stateSuite) TestGetUserByAuthDisabled(c *gc.C) { @@ -512,14 +533,14 @@ func (s *stateSuite) TestGetUserByAuthUnauthorized(c *gc.C) { c.Assert(err, jc.ErrorIs, usererrors.Unauthorized) } -// TestGetUserByAutUnexcitingUser asserts that we get an error when we try to +// TestGetUserByAuthDoesNotExist asserts that we get an error when we try to // get a user by auth that does not exist. -func (s *stateSuite) TestGetUserByAuthNotExtantUnauthorized(c *gc.C) { +func (s *stateSuite) TestGetUserByAuthDoesNotExist(c *gc.C) { st := NewState(s.TxnRunnerFactory()) // Get the user. _, err := st.GetUserByAuth(context.Background(), "admin", auth.NewPassword("password")) - c.Assert(err, jc.ErrorIs, usererrors.Unauthorized) + c.Assert(err, jc.ErrorIs, usererrors.NotFound) } // TestRemoveUser asserts that we can remove a user from the database. diff --git a/internal/worker/apicaller/connect.go b/internal/worker/apicaller/connect.go index 68d323b00fe..77ea05b5433 100644 --- a/internal/worker/apicaller/connect.go +++ b/internal/worker/apicaller/connect.go @@ -116,7 +116,7 @@ func connectFallback( if !didFallback { logger.Debugf("connecting with current password") tryConnect() - if params.IsCodeUnauthorized(err) || errors.Cause(err) == apiservererrors.ErrBadCreds { + if params.IsCodeUnauthorized(err) || errors.Cause(err) == apiservererrors.ErrUnauthorized { didFallback = true } diff --git a/internal/worker/apicaller/connect_test.go b/internal/worker/apicaller/connect_test.go index 09f42789e2d..e9b4d4d4091 100644 --- a/internal/worker/apicaller/connect_test.go +++ b/internal/worker/apicaller/connect_test.go @@ -204,7 +204,7 @@ func (s *ScaryConnectSuite) TestChangePasswordSuccessAfterUnauthorisedError(c *g func (s *ScaryConnectSuite) TestChangePasswordSuccessAfterBadCurrentPasswordError(c *gc.C) { // This will try to login with old password if current one fails. - stub := createPasswordCheckStub(apiservererrors.ErrBadCreds) + stub := createPasswordCheckStub(apiservererrors.ErrUnauthorized) s.assertChangePasswordSuccess(c, stub) } From 21c141c1482e802871183a7f2a9d9fa755e71fc7 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Wed, 21 Feb 2024 20:35:07 +0000 Subject: [PATCH 06/11] Agent authenticator factory The following allows to create an authenticator that closes over a given state. This is so we can swap between the system state and the model state. I've also created an interface between stateautthenticator and authentication. This splits up the concerns and dependencies between the two. We're not passing all the tests yet, but we're a step closer. --- apiserver/admin_test.go | 87 +++++++++++++++---- apiserver/authentication/agent.go | 44 ++++++++-- apiserver/authentication/agent_test.go | 21 ++--- apiserver/export_test.go | 4 +- apiserver/stateauthenticator/auth.go | 20 +++-- .../authentication_mock_test.go | 57 ++++++++++++ .../stateauthenticator/authenticator_test.go | 83 ++++++++++++------ apiserver/stateauthenticator/context.go | 48 ++++++++-- apiserver/stateauthenticator/context_test.go | 17 ++-- apiserver/stateauthenticator/export_test.go | 7 -- apiserver/stateauthenticator/package_test.go | 3 +- .../stateauthenticator/services_mock_test.go | 61 ++++++++++++- domain/schema/testing/schema.go | 2 +- .../worker/httpserverargs/authenticator.go | 8 +- juju/testing/apiserver.go | 11 ++- 15 files changed, 368 insertions(+), 105 deletions(-) create mode 100644 apiserver/stateauthenticator/authentication_mock_test.go diff --git a/apiserver/admin_test.go b/apiserver/admin_test.go index 59971578fb0..3c6ca54bc7f 100644 --- a/apiserver/admin_test.go +++ b/apiserver/admin_test.go @@ -177,8 +177,10 @@ func (s *loginSuite) TestLoginAsDeactivatedUser(c *gc.C) { // Since these are user login tests, the nonce is empty. err = st.Login(context.Background(), u.Tag(), password, "", nil) + + // The error message should not leak that the user is disabled. c.Assert(errors.Cause(err), gc.DeepEquals, &rpc.RequestError{ - Message: fmt.Sprintf("user %q is disabled", u.Tag().Id()), + Message: "invalid entity name or password", Code: "unauthorized access", }) @@ -207,7 +209,7 @@ func (s *loginSuite) TestLoginAsDeletedUser(c *gc.C) { // Since these are user login tests, the nonce is empty. err = st.Login(context.Background(), u.Tag(), password, "", nil) c.Assert(errors.Cause(err), gc.DeepEquals, &rpc.RequestError{ - Message: fmt.Sprintf("user %q is permanently deleted", u.Tag().Id()), + Message: "invalid entity name or password", Code: "unauthorized access", }) @@ -398,14 +400,30 @@ func (s *loginSuite) infoForNewUser(c *gc.C, info *api.Info) *api.Info { // Make a copy newInfo := *info + userTag := names.NewUserTag("charlie") + password := "shhh..." + + userService := s.ControllerServiceFactory(c).User() + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + DisplayName: "Charlie Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword(password)), + }) + c.Assert(err, jc.ErrorIsNil) + + // TODO (stickupkid): Remove the make user call when permissions are + // written to state. f, release := s.NewFactory(c, info.ModelTag.Id()) defer release() - password := "shhh..." - user := f.MakeUser(c, &factory.UserParams{ - Password: password, + + f.MakeUser(c, &factory.UserParams{ + Name: userTag.Name(), + DisplayName: "Charlie Brown", + Password: password, }) - newInfo.Tag = user.Tag() + newInfo.Tag = userTag newInfo.Password = password return &newInfo } @@ -632,19 +650,34 @@ func (s *loginSuite) TestInvalidModel(c *gc.C) { } func (s *loginSuite) TestOtherModel(c *gc.C) { + userTag := names.NewUserTag("charlie") + password := "shhh..." + + userService := s.ControllerServiceFactory(c).User() + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + DisplayName: "Charlie Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword(password)), + }) + c.Assert(err, jc.ErrorIsNil) + f, release := s.NewFactory(c, s.ControllerModelUUID()) defer release() - modelOwner := f.MakeUser(c, nil) + f.MakeUser(c, &factory.UserParams{ + Name: userTag.Name(), + }) modelState := f.MakeModel(c, &factory.ModelParams{ - Owner: modelOwner.UserTag(), + Owner: userTag, }) defer modelState.Close() + model, err := modelState.Model() c.Assert(err, jc.ErrorIsNil) st := s.openModelAPIWithoutLogin(c, model.UUID()) - err = st.Login(context.Background(), modelOwner.UserTag(), "password", "", nil) + err = st.Login(context.Background(), userTag, password, "", nil) c.Assert(err, jc.ErrorIsNil) s.assertRemoteModel(c, st, model.ModelTag()) } @@ -782,32 +815,48 @@ func (s *loginSuite) TestOtherModelWhenNotController(c *gc.C) { assertInvalidEntityPassword(c, err) } -func (s *loginSuite) loginLocalUser(c *gc.C, info *api.Info) (*state.User, params.LoginResult) { +func (s *loginSuite) loginLocalUser(c *gc.C, info *api.Info) (names.UserTag, params.LoginResult) { + userTag := names.NewUserTag("charlie") + password := "shhh..." + + userService := s.ControllerServiceFactory(c).User() + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + DisplayName: "Charlie Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword(password)), + }) + c.Assert(err, jc.ErrorIsNil) + + // TODO (stickupkid): Remove the make user call when permissions are + // written to state. f, release := s.NewFactory(c, info.ModelTag.Id()) defer release() - password := "shhh..." - user := f.MakeUser(c, &factory.UserParams{ + + f.MakeUser(c, &factory.UserParams{ + Name: userTag.Name(), Password: password, }) + conn := s.openAPIWithoutLogin(c) var result params.LoginResult request := ¶ms.LoginRequest{ - AuthTag: user.Tag().String(), + AuthTag: userTag.String(), Credentials: password, ClientVersion: jujuversion.Current.String(), } - err := conn.APICall(context.Background(), "Admin", 3, "", "Login", request, &result) + err = conn.APICall(context.Background(), "Admin", 3, "", "Login", request, &result) c.Assert(err, jc.ErrorIsNil) c.Assert(result.UserInfo, gc.NotNil) - return user, result + return userTag, result } func (s *loginSuite) TestLoginResultLocalUser(c *gc.C) { info := s.ControllerModelApiInfo() - user, result := s.loginLocalUser(c, info) - c.Check(result.UserInfo.Identity, gc.Equals, user.Tag().String()) + userTag, result := s.loginLocalUser(c, info) + c.Check(result.UserInfo.Identity, gc.Equals, userTag.String()) c.Check(result.UserInfo.ControllerAccess, gc.Equals, "login") c.Check(result.UserInfo.ModelAccess, gc.Equals, "admin") } @@ -817,8 +866,8 @@ func (s *loginSuite) TestLoginResultLocalUserEveryoneCreateOnlyNonLocal(c *gc.C) setEveryoneAccess(c, s.ControllerModel(c).State(), jujutesting.AdminUser, permission.SuperuserAccess) - user, result := s.loginLocalUser(c, info) - c.Check(result.UserInfo.Identity, gc.Equals, user.Tag().String()) + userTag, result := s.loginLocalUser(c, info) + c.Check(result.UserInfo.Identity, gc.Equals, userTag.String()) c.Check(result.UserInfo.ControllerAccess, gc.Equals, "login") c.Check(result.UserInfo.ModelAccess, gc.Equals, "admin") } diff --git a/apiserver/authentication/agent.go b/apiserver/authentication/agent.go index cbd96e59b99..ecc7aec7522 100644 --- a/apiserver/authentication/agent.go +++ b/apiserver/authentication/agent.go @@ -18,23 +18,49 @@ type Logger interface { Debugf(string, ...interface{}) } -// EntityAuthenticator performs authentication for juju entities. -type AgentAuthenticator struct { +// AgentAuthenticatorFactory is a factory for creating authenticators, which +// can create authenticators for a given state. +type AgentAuthenticatorFactory struct { userService UserService legacyState *state.State logger Logger } -// NewAgentAuthenticator returns a new authenticator. -func NewAgentAuthenticator(userService UserService, legacyState *state.State, logger Logger) *AgentAuthenticator { - return &AgentAuthenticator{ +// NewAgentAuthenticatorFactory returns a new agent authenticator factory, for +// a known state. +func NewAgentAuthenticatorFactory(userService UserService, legacyState *state.State, logger Logger) AgentAuthenticatorFactory { + return AgentAuthenticatorFactory{ userService: userService, legacyState: legacyState, logger: logger, } } -var _ EntityAuthenticator = (*AgentAuthenticator)(nil) +// Authenticator returns an authenticator using the factory's state. +func (f AgentAuthenticatorFactory) Authenticator() EntityAuthenticator { + return agentAuthenticator{ + userService: f.userService, + state: f.legacyState, + logger: f.logger, + } +} + +// AuthenticatorForState returns an authenticator for the given state. +func (f AgentAuthenticatorFactory) AuthenticatorForState(st *state.State) EntityAuthenticator { + return agentAuthenticator{ + userService: f.userService, + state: st, + logger: f.logger, + } +} + +type agentAuthenticator struct { + userService UserService + state *state.State + logger Logger +} + +var _ EntityAuthenticator = (*agentAuthenticator)(nil) type taggedAuthenticator interface { state.Entity @@ -43,7 +69,7 @@ type taggedAuthenticator interface { // Authenticate authenticates the provided entity. // It takes an entityfinder and the tag used to find the entity that requires authentication. -func (a *AgentAuthenticator) Authenticate(ctx context.Context, authParams AuthParams) (state.Entity, error) { +func (a agentAuthenticator) Authenticate(ctx context.Context, authParams AuthParams) (state.Entity, error) { switch authParams.AuthTag.Kind() { case names.UserTagKind: return nil, errors.Trace(apiservererrors.ErrBadRequest) @@ -52,8 +78,8 @@ func (a *AgentAuthenticator) Authenticate(ctx context.Context, authParams AuthPa } } -func (a *AgentAuthenticator) fallbackAuth(ctx context.Context, authParams AuthParams) (state.Entity, error) { - entity, err := a.legacyState.FindEntity(authParams.AuthTag) +func (a *agentAuthenticator) fallbackAuth(ctx context.Context, authParams AuthParams) (state.Entity, error) { + entity, err := a.state.FindEntity(authParams.AuthTag) if errors.Is(err, errors.NotFound) { logger.Debugf("cannot authenticate unknown entity: %v", authParams.AuthTag) return nil, errors.Trace(apiservererrors.ErrUnauthorized) diff --git a/apiserver/authentication/agent_test.go b/apiserver/authentication/agent_test.go index 5899c730a89..2040b2bbc79 100644 --- a/apiserver/authentication/agent_test.go +++ b/apiserver/authentication/agent_test.go @@ -102,10 +102,6 @@ type testCase struct { func (s *agentAuthenticatorSuite) TestValidLogins(c *gc.C) { testCases := []testCase{{ - entity: s.user, - credentials: "password", - about: "user login", - }, { entity: s.machine, credentials: s.machinePassword, nonce: s.machineNonce, @@ -119,12 +115,12 @@ func (s *agentAuthenticatorSuite) TestValidLogins(c *gc.C) { st := s.ControllerModel(c).State() for i, t := range testCases { c.Logf("test %d: %s", i, t.about) - authenticator := authentication.NewAgentAuthenticator( + factory := authentication.NewAgentAuthenticatorFactory( s.ControllerServiceFactory(c).User(), st, jujutesting.NewCheckLogger(c), ) - entity, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{ + entity, err := factory.Authenticator().Authenticate(context.Background(), authentication.AuthParams{ AuthTag: t.entity.Tag(), Credentials: t.credentials, Nonce: t.nonce, @@ -136,6 +132,11 @@ func (s *agentAuthenticatorSuite) TestValidLogins(c *gc.C) { 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", @@ -144,7 +145,7 @@ func (s *agentAuthenticatorSuite) TestInvalidLogins(c *gc.C) { entity: s.user, credentials: "wrongpassword", about: "user login for nonexistant user", - errorMessage: "invalid entity name or password", + errorMessage: "invalid request", }, { entity: s.machine, credentials: s.machinePassword, @@ -155,18 +156,18 @@ func (s *agentAuthenticatorSuite) TestInvalidLogins(c *gc.C) { entity: s.user, credentials: "wrong-secret", about: "user login for nonexistant user", - errorMessage: "invalid entity name or password", + errorMessage: "invalid request", }} st := s.ControllerModel(c).State() for i, t := range testCases { c.Logf("test %d: %s", i, t.about) - authenticator := authentication.NewAgentAuthenticator( + factory := authentication.NewAgentAuthenticatorFactory( s.ControllerServiceFactory(c).User(), st, jujutesting.NewCheckLogger(c), ) - entity, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{ + entity, err := factory.Authenticator().Authenticate(context.Background(), authentication.AuthParams{ AuthTag: t.entity.Tag(), Credentials: t.credentials, Nonce: t.nonce, diff --git a/apiserver/export_test.go b/apiserver/export_test.go index 5f6ea595d75..25e814877bc 100644 --- a/apiserver/export_test.go +++ b/apiserver/export_test.go @@ -27,6 +27,7 @@ import ( "github.com/juju/juju/internal/worker/trace" "github.com/juju/juju/rpc" "github.com/juju/juju/state" + jujutesting "github.com/juju/juju/testing" ) var ( @@ -112,7 +113,8 @@ func TestingAPIRoot(facades *facade.Registry) rpc.Root { // TestingAPIHandler gives you an APIHandler that isn't connected to // anything real. It's enough to let test some basic functionality though. func TestingAPIHandler(c *gc.C, pool *state.StatePool, st *state.State, controllerConfigService stateauthenticator.ControllerConfigService, userService stateauthenticator.UserService) (*apiHandler, *common.Resources) { - authenticator, err := stateauthenticator.NewAuthenticator(pool, controllerConfigService, userService, clock.WallClock) + agentAuthFactory := authentication.NewAgentAuthenticatorFactory(userService, st, jujutesting.NewCheckLogger(c)) + authenticator, err := stateauthenticator.NewAuthenticator(pool, st, controllerConfigService, userService, agentAuthFactory, clock.WallClock) c.Assert(err, jc.ErrorIsNil) offerAuthCtxt, err := newOfferAuthcontext(pool) c.Assert(err, jc.ErrorIsNil) diff --git a/apiserver/stateauthenticator/auth.go b/apiserver/stateauthenticator/auth.go index e45608e1b02..29ebc8bc3e9 100644 --- a/apiserver/stateauthenticator/auth.go +++ b/apiserver/stateauthenticator/auth.go @@ -63,15 +63,13 @@ type ControllerConfigService interface { // NewAuthenticator returns a new Authenticator using the given StatePool. func NewAuthenticator( statePool *state.StatePool, + systemState *state.State, controllerConfigService ControllerConfigService, userService UserService, + agentAuthFactory AgentAuthenticatorFactory, clock clock.Clock, ) (*Authenticator, error) { - systemState, err := statePool.SystemState() - if err != nil { - return nil, errors.Trace(err) - } - authContext, err := newAuthContext(systemState, controllerConfigService, userService, clock) + authContext, err := newAuthContext(systemState, controllerConfigService, userService, agentAuthFactory, clock) if err != nil { return nil, errors.Trace(err) } @@ -160,10 +158,11 @@ func (a *Authenticator) AuthenticateLoginRequest( } defer st.Release() - authenticator := a.authContext.authenticator(serverHost) + authenticator := a.authContext.authenticatorForState(serverHost, st.State) authInfo, err := a.checkCreds(ctx, st.State, authParams, authenticator) if err == nil { - return authInfo, err + + return authInfo, nil } var dischargeRequired *apiservererrors.DischargeRequiredError @@ -172,14 +171,16 @@ func (a *Authenticator) AuthenticateLoginRequest( return authentication.AuthInfo{}, errors.Trace(err) } - _, isMachineTag := authParams.AuthTag.(names.MachineTag) - _, isControllerAgentTag := authParams.AuthTag.(names.ControllerAgentTag) systemState, errS := a.statePool.SystemState() if errS != nil { return authentication.AuthInfo{}, errors.Trace(err) } + + _, isMachineTag := authParams.AuthTag.(names.MachineTag) + _, isControllerAgentTag := authParams.AuthTag.(names.ControllerAgentTag) if (isMachineTag || isControllerAgentTag) && !st.IsController() { // Controller agents are allowed to log into any model. + authenticator := a.authContext.authenticator(serverHost) var err2 error authInfo, err2 = a.checkCreds( ctx, @@ -193,6 +194,7 @@ func (a *Authenticator) AuthenticateLoginRequest( if err != nil { return authentication.AuthInfo{}, errors.NewUnauthorized(err, "") } + authInfo.Delegator = &PermissionDelegator{State: systemState} return authInfo, nil } diff --git a/apiserver/stateauthenticator/authentication_mock_test.go b/apiserver/stateauthenticator/authentication_mock_test.go new file mode 100644 index 00000000000..531bfbc7f2c --- /dev/null +++ b/apiserver/stateauthenticator/authentication_mock_test.go @@ -0,0 +1,57 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/juju/juju/apiserver/authentication (interfaces: EntityAuthenticator) +// +// Generated by this command: +// +// mockgen -package stateauthenticator -destination authentication_mock_test.go github.com/juju/juju/apiserver/authentication EntityAuthenticator +// + +// Package stateauthenticator is a generated GoMock package. +package stateauthenticator + +import ( + context "context" + reflect "reflect" + + authentication "github.com/juju/juju/apiserver/authentication" + state "github.com/juju/juju/state" + gomock "go.uber.org/mock/gomock" +) + +// MockEntityAuthenticator is a mock of EntityAuthenticator interface. +type MockEntityAuthenticator struct { + ctrl *gomock.Controller + recorder *MockEntityAuthenticatorMockRecorder +} + +// MockEntityAuthenticatorMockRecorder is the mock recorder for MockEntityAuthenticator. +type MockEntityAuthenticatorMockRecorder struct { + mock *MockEntityAuthenticator +} + +// NewMockEntityAuthenticator creates a new mock instance. +func NewMockEntityAuthenticator(ctrl *gomock.Controller) *MockEntityAuthenticator { + mock := &MockEntityAuthenticator{ctrl: ctrl} + mock.recorder = &MockEntityAuthenticatorMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockEntityAuthenticator) EXPECT() *MockEntityAuthenticatorMockRecorder { + return m.recorder +} + +// Authenticate mocks base method. +func (m *MockEntityAuthenticator) Authenticate(arg0 context.Context, arg1 authentication.AuthParams) (state.Entity, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Authenticate", arg0, arg1) + ret0, _ := ret[0].(state.Entity) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Authenticate indicates an expected call of Authenticate. +func (mr *MockEntityAuthenticatorMockRecorder) Authenticate(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Authenticate", reflect.TypeOf((*MockEntityAuthenticator)(nil).Authenticate), arg0, arg1) +} diff --git a/apiserver/stateauthenticator/authenticator_test.go b/apiserver/stateauthenticator/authenticator_test.go index 42429c481b0..db6ac390157 100644 --- a/apiserver/stateauthenticator/authenticator_test.go +++ b/apiserver/stateauthenticator/authenticator_test.go @@ -1,7 +1,7 @@ // Copyright 2014-2018 Canonical Ltd. All rights reserved. // Licensed under the AGPLv3, see LICENCE file for details. -package stateauthenticator_test +package stateauthenticator import ( "context" @@ -14,7 +14,6 @@ import ( gc "gopkg.in/check.v1" "github.com/juju/juju/apiserver/authentication" - "github.com/juju/juju/apiserver/stateauthenticator" coreuser "github.com/juju/juju/core/user" statetesting "github.com/juju/juju/state/testing" ) @@ -23,9 +22,12 @@ import ( // via the public interface, and then get rid of export_test.go. type agentAuthenticatorSuite struct { statetesting.StateSuite - authenticator *stateauthenticator.Authenticator - controllerConfigService *MockControllerConfigService - userService *MockUserService + + authenticator *Authenticator + entityAuthenticator *MockEntityAuthenticator + agentAuthenticatorFactory *MockAgentAuthenticatorFactory + controllerConfigService *MockControllerConfigService + userService *MockUserService } var _ = gc.Suite(&agentAuthenticatorSuite{}) @@ -33,6 +35,8 @@ var _ = gc.Suite(&agentAuthenticatorSuite{}) func (s *agentAuthenticatorSuite) TestAuthenticateLoginRequestHandleNotSupportedRequests(c *gc.C) { defer s.setupMocks(c).Finish() + s.agentAuthenticatorFactory.EXPECT().AuthenticatorForState(gomock.Any()).Return(s.entityAuthenticator) + _, err := s.authenticator.AuthenticateLoginRequest(context.Background(), "", "", authentication.AuthParams{Token: "token"}) c.Assert(err, jc.ErrorIs, errors.NotSupported) } @@ -45,9 +49,11 @@ func (s *agentAuthenticatorSuite) TestAuthenticatorForTag(c *gc.C) { } tag := names.NewUserTag("user") - authenticator, err := stateauthenticator.EntityAuthenticator(context.Background(), s.authenticator, tag) + s.agentAuthenticatorFactory.EXPECT().Authenticator().Return(s.entityAuthenticator) + + authenticator, err := s.authenticatorForTag(context.Background(), s.authenticator, tag) c.Assert(err, jc.ErrorIsNil) - c.Assert(authenticator, gc.NotNil) + c.Check(authenticator, gc.NotNil) s.userService.EXPECT().GetUserByAuth(context.Background(), "user", "password").Return(user, nil).AnyTimes() @@ -57,55 +63,82 @@ func (s *agentAuthenticatorSuite) TestAuthenticatorForTag(c *gc.C) { Nonce: "nonce", }) c.Assert(err, jc.ErrorIsNil) - c.Assert(entity.Tag(), gc.DeepEquals, tag) + c.Check(entity.Tag(), gc.DeepEquals, tag) +} + +func (s *agentAuthenticatorSuite) TestNotSupportedTag(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.agentAuthenticatorFactory.EXPECT().Authenticator().Return(s.entityAuthenticator) + + authenticator, err := s.authenticatorForTag(context.Background(), s.authenticator, names.NewCloudTag("not-support")) + c.Assert(err, gc.ErrorMatches, "unexpected login entity tag: invalid request") + c.Check(authenticator, gc.IsNil) } func (s *agentAuthenticatorSuite) TestMachineGetsAgentAuthenticator(c *gc.C) { defer s.setupMocks(c).Finish() - authenticator, err := stateauthenticator.EntityAuthenticator(context.Background(), s.authenticator, names.NewMachineTag("0")) + tag := names.NewMachineTag("0") + + s.agentAuthenticatorFactory.EXPECT().Authenticator().Return(s.entityAuthenticator) + s.entityAuthenticator.EXPECT().Authenticate(gomock.Any(), authentication.AuthParams{}).Return(authentication.TagToEntity(tag), nil) + + authenticator, err := s.authenticatorForTag(context.Background(), s.authenticator, tag) + c.Assert(err, jc.ErrorIsNil) + entity, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{}) c.Assert(err, jc.ErrorIsNil) - _, ok := authenticator.(*authentication.AgentAuthenticator) - c.Assert(ok, jc.IsTrue) + c.Check(entity.Tag(), gc.Equals, tag) } func (s *agentAuthenticatorSuite) TestModelGetsAgentAuthenticator(c *gc.C) { defer s.setupMocks(c).Finish() - authenticator, err := stateauthenticator.EntityAuthenticator(context.Background(), s.authenticator, names.NewModelTag("deadbeef-0bad-400d-8000-4b1d0d06f00d")) + tag := names.NewModelTag("deadbeef-0bad-400d-8000-4b1d0d06f00d") + + s.agentAuthenticatorFactory.EXPECT().Authenticator().Return(s.entityAuthenticator) + s.entityAuthenticator.EXPECT().Authenticate(gomock.Any(), authentication.AuthParams{}).Return(authentication.TagToEntity(tag), nil) + + authenticator, err := s.authenticatorForTag(context.Background(), s.authenticator, tag) c.Assert(err, jc.ErrorIsNil) - _, ok := authenticator.(*authentication.AgentAuthenticator) - c.Assert(ok, jc.IsTrue) + entity, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{}) + c.Assert(err, jc.ErrorIsNil) + c.Check(entity.Tag(), gc.Equals, tag) } func (s *agentAuthenticatorSuite) TestUnitGetsAgentAuthenticator(c *gc.C) { defer s.setupMocks(c).Finish() - authenticator, err := stateauthenticator.EntityAuthenticator(context.Background(), s.authenticator, names.NewUnitTag("wordpress/0")) - c.Assert(err, jc.ErrorIsNil) - _, ok := authenticator.(*authentication.AgentAuthenticator) - c.Assert(ok, jc.IsTrue) -} + tag := names.NewUnitTag("wordpress/0") -func (s *agentAuthenticatorSuite) TestNotSupportedTag(c *gc.C) { - defer s.setupMocks(c).Finish() + s.agentAuthenticatorFactory.EXPECT().Authenticator().Return(s.entityAuthenticator) + s.entityAuthenticator.EXPECT().Authenticate(gomock.Any(), authentication.AuthParams{}).Return(authentication.TagToEntity(tag), nil) - authenticator, err := stateauthenticator.EntityAuthenticator(context.Background(), s.authenticator, names.NewCloudTag("not-support")) - c.Assert(err, gc.ErrorMatches, "unexpected login entity tag: invalid request") - c.Assert(authenticator, gc.IsNil) + authenticator, err := s.authenticatorForTag(context.Background(), s.authenticator, tag) + c.Assert(err, jc.ErrorIsNil) + entity, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{}) + c.Assert(err, jc.ErrorIsNil) + c.Check(entity.Tag(), gc.Equals, tag) } func (s *agentAuthenticatorSuite) setupMocks(c *gc.C) *gomock.Controller { ctrl := gomock.NewController(c) + s.agentAuthenticatorFactory = NewMockAgentAuthenticatorFactory(ctrl) + s.entityAuthenticator = NewMockEntityAuthenticator(ctrl) + s.controllerConfigService = NewMockControllerConfigService(ctrl) s.controllerConfigService.EXPECT().ControllerConfig(gomock.Any()).Return(s.ControllerConfig, nil).AnyTimes() s.userService = NewMockUserService(ctrl) - authenticator, err := stateauthenticator.NewAuthenticator(s.StatePool, s.controllerConfigService, s.userService, clock.WallClock) + authenticator, err := NewAuthenticator(s.StatePool, s.State, s.controllerConfigService, s.userService, s.agentAuthenticatorFactory, clock.WallClock) c.Assert(err, jc.ErrorIsNil) s.authenticator = authenticator return ctrl } + +func (s *agentAuthenticatorSuite) authenticatorForTag(ctx context.Context, authenticator *Authenticator, tag names.Tag) (authentication.EntityAuthenticator, error) { + return authenticator.authContext.authenticator("testing.invalid:1234").authenticatorForTag(ctx, tag) +} diff --git a/apiserver/stateauthenticator/context.go b/apiserver/stateauthenticator/context.go index 12d9e64f24e..429c933b9fe 100644 --- a/apiserver/stateauthenticator/context.go +++ b/apiserver/stateauthenticator/context.go @@ -50,6 +50,16 @@ type UserService interface { UpdateLastLogin(ctx context.Context, name string) error } +// AgentAuthenticatorFactory is a factory for creating authenticators, which +// can create authenticators for a given state. +type AgentAuthenticatorFactory interface { + // Authenticator returns an authenticator using the factory's state. + Authenticator() authentication.EntityAuthenticator + + // AuthenticatorForState returns an authenticator for the given state. + AuthenticatorForState(st *state.State) authentication.EntityAuthenticator +} + // authContext holds authentication context shared // between all API endpoints. type authContext struct { @@ -57,8 +67,8 @@ type authContext struct { controllerConfigService ControllerConfigService userService UserService - clock clock.Clock - agentAuth *authentication.AgentAuthenticator + clock clock.Clock + agentAuthFactory AgentAuthenticatorFactory // localUserBakery is the bakery.Bakery used by the controller // for authenticating local users. In time, we may want to use this for @@ -100,6 +110,7 @@ func newAuthContext( st *state.State, controllerConfigService ControllerConfigService, userService UserService, + agentAuthFactory AgentAuthenticatorFactory, clock clock.Clock, ) (*authContext, error) { ctxt := &authContext{ @@ -108,7 +119,7 @@ func newAuthContext( controllerConfigService: controllerConfigService, userService: userService, localUserInteractions: authentication.NewInteractions(), - agentAuth: authentication.NewAgentAuthenticator(userService, st, logger), + agentAuthFactory: agentAuthFactory, } // Create a bakery for discharging third-party caveats for @@ -202,17 +213,32 @@ func (ctxt *authContext) DischargeCaveats(tag names.UserTag) []checkers.Caveat { return authentication.DischargeCaveats(tag, ctxt.clock) } +// authenticatorForState returns an authenticator.Authenticator for the API +// connection associated with the specified API server host and state. +func (ctxt *authContext) authenticatorForState(serverHost string, st *state.State) authenticator { + return authenticator{ + ctxt: ctxt, + serverHost: serverHost, + agentAuthenticator: ctxt.agentAuthFactory.AuthenticatorForState(st), + } +} + // authenticator returns an authenticator.Authenticator for the API // connection associated with the specified API server host. func (ctxt *authContext) authenticator(serverHost string) authenticator { - return authenticator{ctxt: ctxt, serverHost: serverHost} + return authenticator{ + ctxt: ctxt, + serverHost: serverHost, + agentAuthenticator: ctxt.agentAuthFactory.Authenticator(), + } } // authenticator implements authenticator.Authenticator, delegating // to the appropriate authenticator based on the tag kind. type authenticator struct { - ctxt *authContext - serverHost string + ctxt *authContext + serverHost string + agentAuthenticator authentication.EntityAuthenticator } // Authenticate implements authentication.Authenticator @@ -248,9 +274,13 @@ func (a authenticator) authenticatorForTag(ctx context.Context, tag names.Tag) ( } return auth, nil } - for _, agentKind := range AgentTags { - if tag.Kind() == agentKind { - return a.ctxt.agentAuth, nil + // If the tag is not a user tag, it must be an agent tag, attempt to locate + // it. + // TODO (stickupkid): This should just be a switch. We don't need to loop + // through all the agent tags, it's pointless. + for _, kind := range AgentTags { + if tag.Kind() == kind { + return a.agentAuthenticator, nil } } return nil, errors.Annotatef(apiservererrors.ErrBadRequest, "unexpected login entity tag") diff --git a/apiserver/stateauthenticator/context_test.go b/apiserver/stateauthenticator/context_test.go index 0fbfe2fe4e0..f9d9723adbc 100644 --- a/apiserver/stateauthenticator/context_test.go +++ b/apiserver/stateauthenticator/context_test.go @@ -1,7 +1,7 @@ // Copyright 2018 Canonical Ltd. All rights reserved. // Licensed under the AGPLv3, see LICENCE file for details. -package stateauthenticator_test +package stateauthenticator import ( "context" @@ -19,9 +19,10 @@ import ( gc "gopkg.in/check.v1" "gopkg.in/macaroon.v2" - "github.com/juju/juju/apiserver/stateauthenticator" + "github.com/juju/juju/apiserver/authentication" "github.com/juju/juju/controller" statetesting "github.com/juju/juju/state/testing" + "github.com/juju/juju/testing" ) // TODO(babbageclunk): These have been extracted pretty mechanically @@ -32,7 +33,7 @@ import ( type macaroonCommonSuite struct { statetesting.StateSuite discharger *bakerytest.Discharger - authenticator *stateauthenticator.Authenticator + authenticator *Authenticator clock *testclock.Clock controllerConfigService *MockControllerConfigService userService *MockUserService @@ -56,7 +57,9 @@ func (s *macaroonCommonSuite) setupMocks(c *gc.C) *gomock.Controller { s.controllerConfigService = NewMockControllerConfigService(ctrl) s.controllerConfigService.EXPECT().ControllerConfig(gomock.Any()).Return(s.ControllerConfig, nil).AnyTimes() - authenticator, err := stateauthenticator.NewAuthenticator(s.StatePool, s.controllerConfigService, s.userService, s.clock) + agentAuthFactory := authentication.NewAgentAuthenticatorFactory(s.userService, s.State, testing.NewCheckLogger(c)) + + authenticator, err := NewAuthenticator(s.StatePool, s.State, s.controllerConfigService, s.userService, agentAuthFactory, s.clock) c.Assert(err, jc.ErrorIsNil) s.authenticator = authenticator @@ -105,7 +108,7 @@ func (s *macaroonAuthSuite) TestServerBakery(c *gc.C) { return nil, errors.New("unexpected caveat") }) - bsvc, err := stateauthenticator.ServerBakery(context.Background(), s.authenticator, &alwaysIdent{IdentityLocation: discharger.Location()}) + bsvc, err := ServerBakery(context.Background(), s.authenticator, &alwaysIdent{IdentityLocation: discharger.Location()}) c.Assert(err, gc.IsNil) cav := []checkers.Caveat{ @@ -136,7 +139,7 @@ func (s *macaroonAuthSuite) TestServerBakery(c *gc.C) { func (s *macaroonAuthSuite) TestExpiredKey(c *gc.C) { defer s.setupMocks(c).Finish() - bsvc, err := stateauthenticator.ServerBakeryExpiresImmediately(context.Background(), s.authenticator, &alwaysIdent{}) + bsvc, err := ServerBakeryExpiresImmediately(context.Background(), s.authenticator, &alwaysIdent{}) c.Assert(err, gc.IsNil) cav := []checkers.Caveat{ @@ -214,6 +217,6 @@ func (s *macaroonNoURLSuite) TestNoBakeryWhenNoIdentityURL(c *gc.C) { defer s.setupMocks(c).Finish() // By default, when there is no identity location, no bakery is created. - _, err := stateauthenticator.ServerBakery(context.Background(), s.authenticator, nil) + _, err := ServerBakery(context.Background(), s.authenticator, nil) c.Assert(err, gc.ErrorMatches, "macaroon authentication is not configured") } diff --git a/apiserver/stateauthenticator/export_test.go b/apiserver/stateauthenticator/export_test.go index fc83f933104..968ebe4a196 100644 --- a/apiserver/stateauthenticator/export_test.go +++ b/apiserver/stateauthenticator/export_test.go @@ -7,17 +7,10 @@ import ( "context" "github.com/go-macaroon-bakery/macaroon-bakery/v3/bakery/identchecker" - "github.com/juju/names/v5" "github.com/juju/juju/apiserver/authentication" ) -// TODO update the tests moved from apiserver to test via the public -// interface, and then get rid of these. -func EntityAuthenticator(ctx context.Context, authenticator *Authenticator, tag names.Tag) (authentication.EntityAuthenticator, error) { - return authenticator.authContext.authenticator("testing.invalid:1234").authenticatorForTag(ctx, tag) -} - func ServerBakery(ctx context.Context, a *Authenticator, identClient identchecker.IdentityClient) (*identchecker.Bakery, error) { auth, err := a.authContext.externalMacaroonAuth(ctx, identClient) if err != nil { diff --git a/apiserver/stateauthenticator/package_test.go b/apiserver/stateauthenticator/package_test.go index 998542c7564..827a17a3d7a 100644 --- a/apiserver/stateauthenticator/package_test.go +++ b/apiserver/stateauthenticator/package_test.go @@ -9,7 +9,8 @@ import ( coretesting "github.com/juju/juju/testing" ) -//go:generate go run go.uber.org/mock/mockgen -package stateauthenticator_test -destination services_mock_test.go github.com/juju/juju/apiserver/stateauthenticator ControllerConfigService,UserService +//go:generate go run go.uber.org/mock/mockgen -package stateauthenticator -destination services_mock_test.go github.com/juju/juju/apiserver/stateauthenticator ControllerConfigService,UserService,AgentAuthenticatorFactory +//go:generate go run go.uber.org/mock/mockgen -package stateauthenticator -destination authentication_mock_test.go github.com/juju/juju/apiserver/authentication EntityAuthenticator func TestPackage(t *testing.T) { coretesting.MgoTestPackage(t) diff --git a/apiserver/stateauthenticator/services_mock_test.go b/apiserver/stateauthenticator/services_mock_test.go index ce11a1f218a..abaea005fdf 100644 --- a/apiserver/stateauthenticator/services_mock_test.go +++ b/apiserver/stateauthenticator/services_mock_test.go @@ -1,20 +1,22 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: github.com/juju/juju/apiserver/stateauthenticator (interfaces: ControllerConfigService,UserService) +// Source: github.com/juju/juju/apiserver/stateauthenticator (interfaces: ControllerConfigService,UserService,AgentAuthenticatorFactory) // // Generated by this command: // -// mockgen -package stateauthenticator_test -destination services_mock_test.go github.com/juju/juju/apiserver/stateauthenticator ControllerConfigService,UserService +// mockgen -package stateauthenticator -destination services_mock_test.go github.com/juju/juju/apiserver/stateauthenticator ControllerConfigService,UserService,AgentAuthenticatorFactory // -// Package stateauthenticator_test is a generated GoMock package. -package stateauthenticator_test +// Package stateauthenticator is a generated GoMock package. +package stateauthenticator import ( context "context" reflect "reflect" + authentication "github.com/juju/juju/apiserver/authentication" controller "github.com/juju/juju/controller" user "github.com/juju/juju/core/user" + state "github.com/juju/juju/state" gomock "go.uber.org/mock/gomock" ) @@ -122,3 +124,54 @@ func (mr *MockUserServiceMockRecorder) UpdateLastLogin(arg0, arg1 any) *gomock.C mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateLastLogin", reflect.TypeOf((*MockUserService)(nil).UpdateLastLogin), arg0, arg1) } + +// MockAgentAuthenticatorFactory is a mock of AgentAuthenticatorFactory interface. +type MockAgentAuthenticatorFactory struct { + ctrl *gomock.Controller + recorder *MockAgentAuthenticatorFactoryMockRecorder +} + +// MockAgentAuthenticatorFactoryMockRecorder is the mock recorder for MockAgentAuthenticatorFactory. +type MockAgentAuthenticatorFactoryMockRecorder struct { + mock *MockAgentAuthenticatorFactory +} + +// NewMockAgentAuthenticatorFactory creates a new mock instance. +func NewMockAgentAuthenticatorFactory(ctrl *gomock.Controller) *MockAgentAuthenticatorFactory { + mock := &MockAgentAuthenticatorFactory{ctrl: ctrl} + mock.recorder = &MockAgentAuthenticatorFactoryMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockAgentAuthenticatorFactory) EXPECT() *MockAgentAuthenticatorFactoryMockRecorder { + return m.recorder +} + +// Authenticator mocks base method. +func (m *MockAgentAuthenticatorFactory) Authenticator() authentication.EntityAuthenticator { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Authenticator") + ret0, _ := ret[0].(authentication.EntityAuthenticator) + return ret0 +} + +// Authenticator indicates an expected call of Authenticator. +func (mr *MockAgentAuthenticatorFactoryMockRecorder) Authenticator() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Authenticator", reflect.TypeOf((*MockAgentAuthenticatorFactory)(nil).Authenticator)) +} + +// AuthenticatorForState mocks base method. +func (m *MockAgentAuthenticatorFactory) AuthenticatorForState(arg0 *state.State) authentication.EntityAuthenticator { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AuthenticatorForState", arg0) + ret0, _ := ret[0].(authentication.EntityAuthenticator) + return ret0 +} + +// AuthenticatorForState indicates an expected call of AuthenticatorForState. +func (mr *MockAgentAuthenticatorFactoryMockRecorder) AuthenticatorForState(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AuthenticatorForState", reflect.TypeOf((*MockAgentAuthenticatorFactory)(nil).AuthenticatorForState), arg0) +} diff --git a/domain/schema/testing/schema.go b/domain/schema/testing/schema.go index c834437010d..99f8cbc0290 100644 --- a/domain/schema/testing/schema.go +++ b/domain/schema/testing/schema.go @@ -18,7 +18,7 @@ type SchemaApplier struct { func (s *SchemaApplier) Apply(c *gc.C, ctx context.Context, runner database.TxnRunner) { s.Schema.Hook(func(i int) error { - c.Log("Applying schema change", i) + //c.Log("Applying schema change", i) return nil }) changeSet, err := s.Schema.Ensure(ctx, runner) diff --git a/internal/worker/httpserverargs/authenticator.go b/internal/worker/httpserverargs/authenticator.go index c530a6ca745..70cdeb487cd 100644 --- a/internal/worker/httpserverargs/authenticator.go +++ b/internal/worker/httpserverargs/authenticator.go @@ -10,6 +10,7 @@ import ( "github.com/juju/errors" "github.com/juju/juju/apiserver/apiserverhttp" + "github.com/juju/juju/apiserver/authentication" "github.com/juju/juju/apiserver/authentication/macaroon" "github.com/juju/juju/apiserver/stateauthenticator" "github.com/juju/juju/controller" @@ -58,7 +59,12 @@ func NewStateAuthenticator( clock clock.Clock, abort <-chan struct{}, ) (macaroon.LocalMacaroonAuthenticator, error) { - stateAuthenticator, err := stateauthenticator.NewAuthenticator(statePool, controllerConfigService, userService, clock) + systemState, err := statePool.SystemState() + if err != nil { + return nil, errors.Trace(err) + } + agentAuthFactory := authentication.NewAgentAuthenticatorFactory(userService, systemState, nil) + stateAuthenticator, err := stateauthenticator.NewAuthenticator(statePool, systemState, controllerConfigService, userService, agentAuthFactory, clock) if err != nil { return nil, errors.Trace(err) } diff --git a/juju/testing/apiserver.go b/juju/testing/apiserver.go index 73635acb37b..bf8145db7c9 100644 --- a/juju/testing/apiserver.go +++ b/juju/testing/apiserver.go @@ -29,6 +29,7 @@ import ( "github.com/juju/juju/api" "github.com/juju/juju/apiserver" "github.com/juju/juju/apiserver/apiserverhttp" + "github.com/juju/juju/apiserver/authentication" "github.com/juju/juju/apiserver/authentication/macaroon" "github.com/juju/juju/apiserver/observer" "github.com/juju/juju/apiserver/observer/fakeobserver" @@ -55,6 +56,7 @@ import ( credentialstate "github.com/juju/juju/domain/credential/state" servicefactorytesting "github.com/juju/juju/domain/servicefactory/testing" userbootstrap "github.com/juju/juju/domain/user/bootstrap" + "github.com/juju/juju/internal/auth" databasetesting "github.com/juju/juju/internal/database/testing" internallease "github.com/juju/juju/internal/lease" "github.com/juju/juju/internal/mongo" @@ -355,7 +357,12 @@ func (s *ApiServerSuite) setupApiServer(c *gc.C, controllerCfg controller.Config // Set up auth handler. factory := s.ControllerServiceFactory(c) - authenticator, err := stateauthenticator.NewAuthenticator(cfg.StatePool, factory.ControllerConfig(), factory.User(), cfg.Clock) + + systemState, err := cfg.StatePool.SystemState() + c.Assert(err, jc.ErrorIsNil) + agentAuthFactory := authentication.NewAgentAuthenticatorFactory(factory.User(), systemState, nil) + + authenticator, err := stateauthenticator.NewAuthenticator(cfg.StatePool, systemState, factory.ControllerConfig(), factory.User(), agentAuthFactory, cfg.Clock) c.Assert(err, jc.ErrorIsNil) cfg.LocalMacaroonAuthenticator = authenticator err = authenticator.AddHandlers(s.mux) @@ -616,7 +623,7 @@ func SeedDatabase(c *gc.C, runner database.TxnRunner, controllerConfig controlle } func SeedAdminUser(c *gc.C, runner database.TxnRunner) coreuser.UUID { - adminUserUUID, userAdd := userbootstrap.AddUser(coreuser.AdminUserName) + adminUserUUID, userAdd := userbootstrap.AddUserWithPassword(coreuser.AdminUserName, auth.NewPassword(AdminSecret)) err := userAdd(context.Background(), runner) c.Assert(err, jc.ErrorIsNil) From 2aa7bac8a7a53a55a9337262a9fb360efda83f90 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Fri, 23 Feb 2024 15:40:54 +0000 Subject: [PATCH 07/11] Agent Authenticator doesn't require user login According to all the code flows, there isn't anywhere that allows a user login via the agent authenticator. --- apiserver/authentication/agent.go | 19 +++++++------------ apiserver/authentication/agent_test.go | 2 -- apiserver/authentication/entity.go | 3 ++- apiserver/export_test.go | 2 +- apiserver/stateauthenticator/context_test.go | 2 +- .../worker/httpserverargs/authenticator.go | 2 +- juju/testing/apiserver.go | 2 +- state/errors.go | 1 - 8 files changed, 13 insertions(+), 20 deletions(-) diff --git a/apiserver/authentication/agent.go b/apiserver/authentication/agent.go index ecc7aec7522..9259346d4d8 100644 --- a/apiserver/authentication/agent.go +++ b/apiserver/authentication/agent.go @@ -21,16 +21,14 @@ type Logger interface { // AgentAuthenticatorFactory is a factory for creating authenticators, which // can create authenticators for a given state. type AgentAuthenticatorFactory struct { - userService UserService legacyState *state.State logger Logger } // NewAgentAuthenticatorFactory returns a new agent authenticator factory, for // a known state. -func NewAgentAuthenticatorFactory(userService UserService, legacyState *state.State, logger Logger) AgentAuthenticatorFactory { +func NewAgentAuthenticatorFactory(legacyState *state.State, logger Logger) AgentAuthenticatorFactory { return AgentAuthenticatorFactory{ - userService: userService, legacyState: legacyState, logger: logger, } @@ -39,25 +37,22 @@ func NewAgentAuthenticatorFactory(userService UserService, legacyState *state.St // Authenticator returns an authenticator using the factory's state. func (f AgentAuthenticatorFactory) Authenticator() EntityAuthenticator { return agentAuthenticator{ - userService: f.userService, - state: f.legacyState, - logger: f.logger, + state: f.legacyState, + logger: f.logger, } } // AuthenticatorForState returns an authenticator for the given state. func (f AgentAuthenticatorFactory) AuthenticatorForState(st *state.State) EntityAuthenticator { return agentAuthenticator{ - userService: f.userService, - state: st, - logger: f.logger, + state: st, + logger: f.logger, } } type agentAuthenticator struct { - userService UserService - state *state.State - logger Logger + state *state.State + logger Logger } var _ EntityAuthenticator = (*agentAuthenticator)(nil) diff --git a/apiserver/authentication/agent_test.go b/apiserver/authentication/agent_test.go index 2040b2bbc79..f75881406d7 100644 --- a/apiserver/authentication/agent_test.go +++ b/apiserver/authentication/agent_test.go @@ -116,7 +116,6 @@ func (s *agentAuthenticatorSuite) TestValidLogins(c *gc.C) { for i, t := range testCases { c.Logf("test %d: %s", i, t.about) factory := authentication.NewAgentAuthenticatorFactory( - s.ControllerServiceFactory(c).User(), st, jujutesting.NewCheckLogger(c), ) @@ -163,7 +162,6 @@ func (s *agentAuthenticatorSuite) TestInvalidLogins(c *gc.C) { for i, t := range testCases { c.Logf("test %d: %s", i, t.about) factory := authentication.NewAgentAuthenticatorFactory( - s.ControllerServiceFactory(c).User(), st, jujutesting.NewCheckLogger(c), ) diff --git a/apiserver/authentication/entity.go b/apiserver/authentication/entity.go index eecfde9cb97..cd4684256ef 100644 --- a/apiserver/authentication/entity.go +++ b/apiserver/authentication/entity.go @@ -4,8 +4,9 @@ package authentication import ( - coreuser "github.com/juju/juju/core/user" "github.com/juju/names/v5" + + coreuser "github.com/juju/juju/core/user" ) // TagWrapper is a utility struct to take a names tag and wrap it as to conform diff --git a/apiserver/export_test.go b/apiserver/export_test.go index 25e814877bc..b3cd94e3a4b 100644 --- a/apiserver/export_test.go +++ b/apiserver/export_test.go @@ -113,7 +113,7 @@ func TestingAPIRoot(facades *facade.Registry) rpc.Root { // TestingAPIHandler gives you an APIHandler that isn't connected to // anything real. It's enough to let test some basic functionality though. func TestingAPIHandler(c *gc.C, pool *state.StatePool, st *state.State, controllerConfigService stateauthenticator.ControllerConfigService, userService stateauthenticator.UserService) (*apiHandler, *common.Resources) { - agentAuthFactory := authentication.NewAgentAuthenticatorFactory(userService, st, jujutesting.NewCheckLogger(c)) + agentAuthFactory := authentication.NewAgentAuthenticatorFactory(st, jujutesting.NewCheckLogger(c)) authenticator, err := stateauthenticator.NewAuthenticator(pool, st, controllerConfigService, userService, agentAuthFactory, clock.WallClock) c.Assert(err, jc.ErrorIsNil) offerAuthCtxt, err := newOfferAuthcontext(pool) diff --git a/apiserver/stateauthenticator/context_test.go b/apiserver/stateauthenticator/context_test.go index f9d9723adbc..bbf6b3e2da5 100644 --- a/apiserver/stateauthenticator/context_test.go +++ b/apiserver/stateauthenticator/context_test.go @@ -57,7 +57,7 @@ func (s *macaroonCommonSuite) setupMocks(c *gc.C) *gomock.Controller { s.controllerConfigService = NewMockControllerConfigService(ctrl) s.controllerConfigService.EXPECT().ControllerConfig(gomock.Any()).Return(s.ControllerConfig, nil).AnyTimes() - agentAuthFactory := authentication.NewAgentAuthenticatorFactory(s.userService, s.State, testing.NewCheckLogger(c)) + agentAuthFactory := authentication.NewAgentAuthenticatorFactory(s.State, testing.NewCheckLogger(c)) authenticator, err := NewAuthenticator(s.StatePool, s.State, s.controllerConfigService, s.userService, agentAuthFactory, s.clock) c.Assert(err, jc.ErrorIsNil) diff --git a/internal/worker/httpserverargs/authenticator.go b/internal/worker/httpserverargs/authenticator.go index 70cdeb487cd..989a7ad5d65 100644 --- a/internal/worker/httpserverargs/authenticator.go +++ b/internal/worker/httpserverargs/authenticator.go @@ -63,7 +63,7 @@ func NewStateAuthenticator( if err != nil { return nil, errors.Trace(err) } - agentAuthFactory := authentication.NewAgentAuthenticatorFactory(userService, systemState, nil) + agentAuthFactory := authentication.NewAgentAuthenticatorFactory(systemState, nil) stateAuthenticator, err := stateauthenticator.NewAuthenticator(statePool, systemState, controllerConfigService, userService, agentAuthFactory, clock) if err != nil { return nil, errors.Trace(err) diff --git a/juju/testing/apiserver.go b/juju/testing/apiserver.go index bf8145db7c9..b19bacef6fb 100644 --- a/juju/testing/apiserver.go +++ b/juju/testing/apiserver.go @@ -360,7 +360,7 @@ func (s *ApiServerSuite) setupApiServer(c *gc.C, controllerCfg controller.Config systemState, err := cfg.StatePool.SystemState() c.Assert(err, jc.ErrorIsNil) - agentAuthFactory := authentication.NewAgentAuthenticatorFactory(factory.User(), systemState, nil) + agentAuthFactory := authentication.NewAgentAuthenticatorFactory(systemState, nil) authenticator, err := stateauthenticator.NewAuthenticator(cfg.StatePool, systemState, factory.ControllerConfig(), factory.User(), agentAuthFactory, cfg.Clock) c.Assert(err, jc.ErrorIsNil) diff --git a/state/errors.go b/state/errors.go index 9d5e0bd3e35..e81010dcb94 100644 --- a/state/errors.go +++ b/state/errors.go @@ -15,7 +15,6 @@ var ( newParentDeviceHasChildrenError = stateerrors.NewParentDeviceHasChildrenError newErrCharmAlreadyUploaded = stateerrors.NewErrCharmAlreadyUploaded newDeletedUserError = stateerrors.NewDeletedUserError - newNeverLoggedInError = stateerrors.NewNeverLoggedInError newNeverConnectedError = stateerrors.NewNeverConnectedError newVersionInconsistentError = stateerrors.NewVersionInconsistentError From 0fbb0bbd9a853721f10acc64ae514510ac16cacc Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Mon, 26 Feb 2024 15:43:39 +0000 Subject: [PATCH 08/11] Fix remaining tests The tests require a user in the user service with a hack for permissions. --- apiserver/admin_test.go | 10 +++- apiserver/auditconfig_test.go | 71 +++++++++++++++++++++------- apiserver/charms_test.go | 22 ++++++++- apiserver/httpcontext.go | 2 - apiserver/introspection_test.go | 70 +++++++++++++++++---------- apiserver/macaroon_test.go | 4 +- apiserver/stateauthenticator/auth.go | 7 ++- 7 files changed, 138 insertions(+), 48 deletions(-) diff --git a/apiserver/admin_test.go b/apiserver/admin_test.go index 3c6ca54bc7f..033fb970ce6 100644 --- a/apiserver/admin_test.go +++ b/apiserver/admin_test.go @@ -1040,13 +1040,21 @@ func (s *loginV3Suite) TestClientLoginToControllerNoAccessToControllerModel(c *g }) c.Assert(err, jc.ErrorIsNil) + // TODO (stickupkid): Permissions: This is only required to insert admin + // permissions into the state, remove when permissions are written to state. + f, release := s.NewFactory(c, s.ControllerModelUUID()) + defer release() + f.MakeUser(c, &factory.UserParams{ + Name: "bobbrown", + }) + now := s.Clock.Now().UTC() s.OpenControllerAPIAs(c, names.NewUserTag("bobbrown"), "password") user, err := userService.GetUser(context.Background(), uuid) c.Assert(err, jc.ErrorIsNil) - c.Check(user.LastLogin, jc.Almost, now) + c.Check(user.LastLogin, jc.After, now) } func (s *loginV3Suite) TestClientLoginToRootOldClient(c *gc.C) { diff --git a/apiserver/auditconfig_test.go b/apiserver/auditconfig_test.go index 20691b9aa48..bbd95dc563e 100644 --- a/apiserver/auditconfig_test.go +++ b/apiserver/auditconfig_test.go @@ -10,6 +10,7 @@ import ( "github.com/juju/collections/set" "github.com/juju/errors" + "github.com/juju/names/v5" jc "github.com/juju/testing/checkers" gc "gopkg.in/check.v1" @@ -18,6 +19,8 @@ import ( servertesting "github.com/juju/juju/apiserver/testing" "github.com/juju/juju/core/auditlog" "github.com/juju/juju/core/model" + "github.com/juju/juju/domain/user/service" + "github.com/juju/juju/internal/auth" "github.com/juju/juju/juju/testing" "github.com/juju/juju/rpc/params" "github.com/juju/juju/testing/factory" @@ -48,23 +51,35 @@ func (s *auditConfigSuite) TestLoginAddsAuditConversationEventually(c *gc.C) { Target: log, } + userTag := names.NewUserTag("bobbrown") + password := "shhh..." + userService := s.ControllerServiceFactory(c).User() + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword(password)), + }) + c.Assert(err, jc.ErrorIsNil) + + // TODO (stickupkid): Permissions: This is only required to insert admin + // permissions into the state, remove when permissions are written to state. f, release := s.NewFactory(c, s.ControllerModelUUID()) defer release() - password := "shhh..." - user := f.MakeUser(c, &factory.UserParams{ - Password: password, + f.MakeUser(c, &factory.UserParams{ + Name: userTag.Name(), }) conn := s.openAPIWithoutLogin(c) var result params.LoginResult request := ¶ms.LoginRequest{ - AuthTag: user.Tag().String(), + AuthTag: userTag.String(), Credentials: password, CLIArgs: "hey you guys", ClientVersion: jujuversion.Current.String(), } loginTime := s.Clock.Now() - err := conn.APICall(context.Background(), "Admin", 3, "", "Login", request, &result) + err = conn.APICall(context.Background(), "Admin", 3, "", "Login", request, &result) c.Assert(err, jc.ErrorIsNil) c.Assert(result.UserInfo, gc.NotNil) // Nothing's logged at this point because there haven't been any @@ -95,7 +110,7 @@ func (s *auditConfigSuite) TestLoginAddsAuditConversationEventually(c *gc.C) { return math.Abs(t.Sub(loginTime).Seconds()) < 1.0 }) c.Assert(convo, mc, auditlog.Conversation{ - Who: user.Tag().Id(), + Who: userTag.Id(), What: "hey you guys", ModelName: "controller", ModelUUID: s.ControllerModelUUID(), @@ -128,22 +143,34 @@ func (s *auditConfigSuite) TestAuditLoggingFailureOnInterestingRequest(c *gc.C) Target: log, } + userTag := names.NewUserTag("bobbrown") + password := "shhh..." + userService := s.ControllerServiceFactory(c).User() + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword(password)), + }) + c.Assert(err, jc.ErrorIsNil) + + // TODO (stickupkid): Permissions: This is only required to insert admin + // permissions into the state, remove when permissions are written to state. f, release := s.NewFactory(c, s.ControllerModelUUID()) defer release() - password := "shhh..." - user := f.MakeUser(c, &factory.UserParams{ - Password: password, + f.MakeUser(c, &factory.UserParams{ + Name: userTag.Name(), }) conn := s.openAPIWithoutLogin(c) var result params.LoginResult request := ¶ms.LoginRequest{ - AuthTag: user.Tag().String(), + AuthTag: userTag.String(), Credentials: password, CLIArgs: "hey you guys", ClientVersion: jujuversion.Current.String(), } - err := conn.APICall(context.Background(), "Admin", 3, "", "Login", request, &result) + err = conn.APICall(context.Background(), "Admin", 3, "", "Login", request, &result) // No error yet since logging the conversation is deferred until // something happens. c.Assert(err, jc.ErrorIsNil) @@ -166,22 +193,34 @@ func (s *auditConfigSuite) TestAuditLoggingUsesExcludeMethods(c *gc.C) { Target: log, } + userTag := names.NewUserTag("bobbrown") + password := "shhh..." + userService := s.ControllerServiceFactory(c).User() + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword(password)), + }) + c.Assert(err, jc.ErrorIsNil) + + // TODO (stickupkid): Permissions: This is only required to insert admin + // permissions into the state, remove when permissions are written to state. f, release := s.NewFactory(c, s.ControllerModelUUID()) defer release() - password := "shhh..." - user := f.MakeUser(c, &factory.UserParams{ - Password: password, + f.MakeUser(c, &factory.UserParams{ + Name: userTag.Name(), }) conn := s.openAPIWithoutLogin(c) var result params.LoginResult request := ¶ms.LoginRequest{ - AuthTag: user.Tag().String(), + AuthTag: userTag.String(), Credentials: password, CLIArgs: "hey you guys", ClientVersion: jujuversion.Current.String(), } - err := conn.APICall(context.Background(), "Admin", 3, "", "Login", request, &result) + err = conn.APICall(context.Background(), "Admin", 3, "", "Login", request, &result) c.Assert(err, jc.ErrorIsNil) c.Assert(result.UserInfo, gc.NotNil) // Nothing's logged at this point because there haven't been any diff --git a/apiserver/charms_test.go b/apiserver/charms_test.go index cd2e09dccee..9704e019aba 100644 --- a/apiserver/charms_test.go +++ b/apiserver/charms_test.go @@ -18,6 +18,7 @@ import ( "path/filepath" "github.com/juju/charm/v13" + "github.com/juju/names/v5" jc "github.com/juju/testing/checkers" "github.com/juju/utils/v4" gc "gopkg.in/check.v1" @@ -25,6 +26,8 @@ import ( "github.com/juju/juju/apiserver" "github.com/juju/juju/apiserver/common" apitesting "github.com/juju/juju/apiserver/testing" + "github.com/juju/juju/domain/user/service" + "github.com/juju/juju/internal/auth" jujutesting "github.com/juju/juju/juju/testing" "github.com/juju/juju/rpc/params" "github.com/juju/juju/state" @@ -518,15 +521,30 @@ func (s *charmsSuite) TestMigrateCharmNotMigrating(c *gc.C) { } func (s *charmsSuite) TestMigrateCharmUnauthorized(c *gc.C) { + userService := s.ControllerServiceFactory(c).User() + userTag := names.NewUserTag("bobbrown") + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword("hunter2")), + }) + c.Assert(err, jc.ErrorIsNil) + + // TODO (stickupkid): Permissions: This is only required to insert admin + // permissions into the state, remove when permissions are written to state. f, release := s.NewFactory(c, s.ControllerModelUUID()) defer release() - user := f.MakeUser(c, &factory.UserParams{Password: "hunter2"}) + f.MakeUser(c, &factory.UserParams{ + Name: userTag.Name(), + }) + url := s.charmsURL("series=quantal") url.Path = "/migrate/charms" resp := apitesting.SendHTTPRequest(c, apitesting.HTTPRequestParams{ Method: "POST", URL: url.String(), - Tag: user.Tag().String(), + Tag: userTag.String(), Password: "hunter2", }) body := apitesting.AssertResponse(c, resp, http.StatusForbidden, "text/plain; charset=utf-8") diff --git a/apiserver/httpcontext.go b/apiserver/httpcontext.go index ba98523b55f..a3b6e787653 100644 --- a/apiserver/httpcontext.go +++ b/apiserver/httpcontext.go @@ -78,7 +78,6 @@ func (ctxt *httpContext) stateForRequestAuthenticated(r *http.Request) ( if err != nil { return nil, nil, errors.Trace(err) } - fmt.Println("LOLZ", st, err) return st, authInfo.Entity, nil } @@ -169,7 +168,6 @@ func (ctxt *httpContext) stateForRequestAuthenticatedTag(r *http.Request, kinds if err != nil { return nil, nil, errors.Trace(err) } - fmt.Println("st, entity, err", st, entity, err) if ok, err := checkPermissions(entity.Tag(), common.AuthAny(funcs...)); !ok { st.Release() return nil, nil, err diff --git a/apiserver/introspection_test.go b/apiserver/introspection_test.go index cbecf64513a..00639269628 100644 --- a/apiserver/introspection_test.go +++ b/apiserver/introspection_test.go @@ -4,22 +4,25 @@ package apiserver_test import ( + "context" "io" "net/http" "net/url" + "github.com/juju/names/v5" jc "github.com/juju/testing/checkers" gc "gopkg.in/check.v1" apitesting "github.com/juju/juju/apiserver/testing" "github.com/juju/juju/core/permission" + "github.com/juju/juju/domain/user/service" + "github.com/juju/juju/internal/auth" "github.com/juju/juju/juju/testing" - "github.com/juju/juju/state" + "github.com/juju/juju/testing/factory" ) type introspectionSuite struct { testing.ApiServerSuite - bob *state.User url string } @@ -32,23 +35,53 @@ func (s *introspectionSuite) SetUpTest(c *gc.C) { })) } s.ApiServerSuite.SetUpTest(c) - bob, err := s.ControllerModel(c).State().AddUser("bob", "", "hunter2", "admin") - c.Assert(err, jc.ErrorIsNil) - s.bob = bob s.url = s.URL("/introspection/navel", url.Values{}).String() } func (s *introspectionSuite) TestAccess(c *gc.C) { + userService := s.ControllerServiceFactory(c).User() + userTag := names.NewUserTag("bobbrown") + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword("hunter2")), + }) + c.Assert(err, jc.ErrorIsNil) + s.testAccess(c, testing.AdminUser.String(), testing.AdminSecret) - _, err := s.ControllerModel(c).AddUser( - state.UserAccessSpec{ - User: s.bob.UserTag(), - CreatedBy: testing.AdminUser, - Access: permission.ReadAccess, - }, - ) + + // TODO (stickupkid): Permissions: This is only required to insert admin + // permissions into the state, remove when permissions are written to state. + f, release := s.NewFactory(c, s.ControllerModelUUID()) + defer release() + f.MakeUser(c, &factory.UserParams{ + Name: userTag.Name(), + Access: permission.ReadAccess, + }) + + s.testAccess(c, "user-bobbrown", "hunter2") +} + +func (s *introspectionSuite) TestAccessDenied(c *gc.C) { + userService := s.ControllerServiceFactory(c).User() + userTag := names.NewUserTag("bobbrown") + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword("hunter2")), + }) c.Assert(err, jc.ErrorIsNil) - s.testAccess(c, "user-bob", "hunter2") + + resp := apitesting.SendHTTPRequest(c, apitesting.HTTPRequestParams{ + Method: "GET", + URL: s.url, + Tag: "user-bobbrown", + Password: "hunter2", + }) + defer resp.Body.Close() + c.Assert(resp.StatusCode, gc.Equals, http.StatusUnauthorized) } func (s *introspectionSuite) testAccess(c *gc.C, tag, password string) { @@ -64,14 +97,3 @@ func (s *introspectionSuite) testAccess(c *gc.C, tag, password string) { c.Assert(err, jc.ErrorIsNil) c.Assert(string(content), gc.Equals, "gazing") } - -func (s *introspectionSuite) TestAccessDenied(c *gc.C) { - resp := apitesting.SendHTTPRequest(c, apitesting.HTTPRequestParams{ - Method: "GET", - URL: s.url, - Tag: "user-bob", - Password: "hunter2", - }) - defer resp.Body.Close() - c.Assert(resp.StatusCode, gc.Equals, http.StatusForbidden) -} diff --git a/apiserver/macaroon_test.go b/apiserver/macaroon_test.go index ac158c28a69..04ce9d58468 100644 --- a/apiserver/macaroon_test.go +++ b/apiserver/macaroon_test.go @@ -112,7 +112,7 @@ func (s *macaroonLoginSuite) login(c *gc.C, info *api.Info) (params.LoginResult, return result, err } -func (s *macaroonLoginSuite) TestremoteUserLoginToControllerNoAccess(c *gc.C) { +func (s *macaroonLoginSuite) TestRemoteUserLoginToControllerNoAccess(c *gc.C) { s.DischargerLogin = func() string { return remoteUser } @@ -124,7 +124,7 @@ func (s *macaroonLoginSuite) TestremoteUserLoginToControllerNoAccess(c *gc.C) { assertPermissionDenied(c, err) } -func (s *macaroonLoginSuite) TestremoteUserLoginToControllerLoginAccess(c *gc.C) { +func (s *macaroonLoginSuite) TestRemoteUserLoginToControllerLoginAccess(c *gc.C) { setEveryoneAccess(c, s.ControllerModel(c).State(), jujutesting.AdminUser, permission.LoginAccess) var remoteUserTag = names.NewUserTag(remoteUser) diff --git a/apiserver/stateauthenticator/auth.go b/apiserver/stateauthenticator/auth.go index 29ebc8bc3e9..1d4ee0b1ca5 100644 --- a/apiserver/stateauthenticator/auth.go +++ b/apiserver/stateauthenticator/auth.go @@ -161,7 +161,6 @@ func (a *Authenticator) AuthenticateLoginRequest( authenticator := a.authContext.authenticatorForState(serverHost, st.State) authInfo, err := a.checkCreds(ctx, st.State, authParams, authenticator) if err == nil { - return authInfo, nil } @@ -272,6 +271,12 @@ func (a *Authenticator) checkCreds( } func (a *Authenticator) checkPerms(ctx context.Context, modelAccess permission.UserAccess, userTag names.UserTag) error { + // If the user tag is not local, we don't need to check for the model user + // permissions. This is generally the case for macaroon-based logins. + if !userTag.IsLocal() { + return nil + } + // No model user found, so see if the user has been granted // access to the controller. if permission.IsEmptyUserAccess(modelAccess) { From 7d99afcd5eb648033fb6c649a41d82ae9d7fd07e Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Mon, 26 Feb 2024 17:22:19 +0000 Subject: [PATCH 09/11] Ensure you can set the password more than once Setting the password on the user can happen multiple times. Using on conflict do nothing means it's not possible to set the password again. The fix is simple, on conflict set the password. --- apiserver/facades/client/client/api_test.go | 51 ++++++++++--- apiserver/objects_test.go | 21 +++++- apiserver/pubsub_test.go | 24 ++++++- apiserver/ratelimit_test.go | 80 ++++++++++++--------- apiserver/tools_integration_test.go | 27 +++++-- apiserver/tools_test.go | 24 ++++++- domain/user/state/state.go | 38 +++++----- domain/user/state/state_test.go | 45 ++++++++++++ internal/auth/password.go | 4 ++ 9 files changed, 240 insertions(+), 74 deletions(-) diff --git a/apiserver/facades/client/client/api_test.go b/apiserver/facades/client/client/api_test.go index 01b768c0d15..d22491fc204 100644 --- a/apiserver/facades/client/client/api_test.go +++ b/apiserver/facades/client/client/api_test.go @@ -4,6 +4,7 @@ package client_test import ( + "context" "fmt" "time" @@ -20,7 +21,9 @@ import ( "github.com/juju/juju/core/model" "github.com/juju/juju/core/network" "github.com/juju/juju/core/status" + "github.com/juju/juju/domain/user/service" "github.com/juju/juju/environs/config" + "github.com/juju/juju/internal/auth" "github.com/juju/juju/juju/testing" "github.com/juju/juju/rpc/params" "github.com/juju/juju/state" @@ -390,23 +393,41 @@ var scenarioStatus = ¶ms.FullStatus{ // controller (bootstrap machine), so is // hopefully easier to remember as such. func (s *baseSuite) setUpScenario(c *gc.C) (entities []names.Tag) { + st := s.ControllerModel(c).State() + f, release := s.NewFactory(c, s.ControllerModelUUID()) + defer release() + + userService := s.ControllerServiceFactory(c).User() + add := func(e state.Entity) { entities = append(entities, e.Tag()) } - st := s.ControllerModel(c).State() - u, err := st.User(testing.AdminUser) + + // Add the admin user. + adminPassword := defaultPassword(testing.AdminUser) + err := userService.SetPassword(context.Background(), testing.AdminUser.Name(), auth.NewPassword(adminPassword)) c.Assert(err, jc.ErrorIsNil) - setDefaultPassword(c, u) - add(u) + add(taggedUser{tag: testing.AdminUser}) + err = s.ControllerModel(c).UpdateModelConfig(map[string]interface{}{ config.AgentVersionKey: "2.0.0"}, nil) c.Assert(err, jc.ErrorIsNil) - f, release := s.NewFactory(c, s.ControllerModelUUID()) - defer release() - u = f.MakeUser(c, &factory.UserParams{Name: "other"}) - setDefaultPassword(c, u) - add(u) + // Add another user. + userTag := names.NewUserTag("other") + userPassword := defaultPassword(userTag) + _, _, err = userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword(userPassword)), + }) + c.Assert(err, jc.ErrorIsNil) + + // TODO (stickupkid): Permissions: This is only required to insert admin + // permissions into the state, remove when permissions are written to state. + f.MakeUser(c, &factory.UserParams{Name: userTag.Name()}) + add(taggedUser{tag: userTag}) m, err := st.AddMachine(state.UbuntuBase("12.10"), state.JobManageModel) c.Assert(err, jc.ErrorIsNil) @@ -546,3 +567,15 @@ func (s *baseSuite) setUpScenario(c *gc.C) (entities []names.Tag) { } return } + +type taggedUser struct { + tag names.Tag +} + +func (u taggedUser) Tag() names.Tag { + return u.tag +} + +func ptr[T any](t T) *T { + return &t +} diff --git a/apiserver/objects_test.go b/apiserver/objects_test.go index 9605d3c3995..f2bfb053f05 100644 --- a/apiserver/objects_test.go +++ b/apiserver/objects_test.go @@ -15,11 +15,14 @@ import ( "strings" "github.com/juju/charm/v13" + "github.com/juju/names/v5" jc "github.com/juju/testing/checkers" "github.com/juju/utils/v4" gc "gopkg.in/check.v1" apitesting "github.com/juju/juju/apiserver/testing" + "github.com/juju/juju/domain/user/service" + "github.com/juju/juju/internal/auth" jujutesting "github.com/juju/juju/juju/testing" "github.com/juju/juju/rpc/params" "github.com/juju/juju/state" @@ -553,9 +556,23 @@ func (s *putCharmObjectSuite) TestMigrateCharmNotMigrating(c *gc.C) { func (s *putCharmObjectSuite) TestMigrateCharmUnauthorized(c *gc.C) { s.setModelImporting(c) + userService := s.ControllerServiceFactory(c).User() + userTag := names.NewUserTag("bobbrown") + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword("hunter2")), + }) + c.Assert(err, jc.ErrorIsNil) + + // TODO (stickupkid): Permissions: This is only required to insert admin + // permissions into the state, remove when permissions are written to state. fact, release := s.NewFactory(c, s.ControllerModelUUID()) defer release() - user := fact.MakeUser(c, &factory.UserParams{Password: "hunter2"}) + fact.MakeUser(c, &factory.UserParams{ + Name: userTag.Name(), + }) ch := testcharms.Repo.CharmArchive(c.MkDir(), "dummy") f, err := os.Open(ch.Path) @@ -567,7 +584,7 @@ func (s *putCharmObjectSuite) TestMigrateCharmUnauthorized(c *gc.C) { resp := apitesting.SendHTTPRequest(c, apitesting.HTTPRequestParams{ Method: "PUT", URL: url, - Tag: user.Tag().String(), + Tag: userTag.String(), Password: "hunter2", Body: f, ExtraHeaders: map[string]string{ diff --git a/apiserver/pubsub_test.go b/apiserver/pubsub_test.go index 2b125b6a61a..fda80175dff 100644 --- a/apiserver/pubsub_test.go +++ b/apiserver/pubsub_test.go @@ -4,6 +4,7 @@ package apiserver_test import ( + "context" "fmt" "io" "net/http" @@ -20,6 +21,8 @@ import ( "github.com/juju/juju/apiserver" "github.com/juju/juju/apiserver/websocket/websockettest" + "github.com/juju/juju/domain/user/service" + "github.com/juju/juju/internal/auth" jujutesting "github.com/juju/juju/juju/testing" "github.com/juju/juju/rpc/params" "github.com/juju/juju/state" @@ -61,11 +64,26 @@ func (s *pubsubSuite) TestNoAuth(c *gc.C) { } func (s *pubsubSuite) TestRejectsUserLogins(c *gc.C) { + userService := s.ControllerServiceFactory(c).User() + userTag := names.NewUserTag("bobbrown") + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword("hunter2")), + }) + c.Assert(err, jc.ErrorIsNil) + + // TODO (stickupkid): Permissions: This is only required to insert admin + // permissions into the state, remove when permissions are written to state. f, release := s.NewFactory(c, s.ControllerModelUUID()) defer release() - user := f.MakeUser(c, &factory.UserParams{Password: "sekrit"}) - header := jujuhttp.BasicAuthHeader(user.Tag().String(), "sekrit") - s.checkAuthFails(c, header, http.StatusForbidden, "authorization failed: user username-.* is not a controller") + f.MakeUser(c, &factory.UserParams{ + Name: userTag.Name(), + }) + + header := jujuhttp.BasicAuthHeader(userTag.String(), "hunter2") + s.checkAuthFails(c, header, http.StatusForbidden, "authorization failed: user .* is not a controller") } func (s *pubsubSuite) TestRejectsNonServerMachineLogins(c *gc.C) { diff --git a/apiserver/ratelimit_test.go b/apiserver/ratelimit_test.go index 8e7a7b38d94..73ec40d78f8 100644 --- a/apiserver/ratelimit_test.go +++ b/apiserver/ratelimit_test.go @@ -4,14 +4,18 @@ package apiserver_test import ( + "context" "time" "github.com/juju/clock/testclock" + "github.com/juju/names/v5" jc "github.com/juju/testing/checkers" gc "gopkg.in/check.v1" "github.com/juju/juju/api" corecontroller "github.com/juju/juju/controller" + "github.com/juju/juju/domain/user/service" + "github.com/juju/juju/internal/auth" jujutesting "github.com/juju/juju/juju/testing" "github.com/juju/juju/testing/factory" ) @@ -31,37 +35,6 @@ func (s *rateLimitSuite) SetUpTest(c *gc.C) { s.ApiServerSuite.SetUpTest(c) } -func (s *rateLimitSuite) infoForNewMachine(c *gc.C, info *api.Info) *api.Info { - // Make a copy - newInfo := *info - - f, release := s.NewFactory(c, info.ModelTag.Id()) - defer release() - machine, password := f.MakeMachineReturningPassword( - c, &factory.MachineParams{Nonce: "fake_nonce"}) - - newInfo.Tag = machine.Tag() - newInfo.Password = password - newInfo.Nonce = "fake_nonce" - return &newInfo -} - -func (s *rateLimitSuite) infoForNewUser(c *gc.C, info *api.Info) *api.Info { - // Make a copy - newInfo := *info - - f, release := s.NewFactory(c, info.ModelTag.Id()) - defer release() - password := "shhh..." - user := f.MakeUser(c, &factory.UserParams{ - Password: password, - }) - - newInfo.Tag = user.Tag() - newInfo.Password = password - return &newInfo -} - func (s *rateLimitSuite) TestRateLimitAgents(c *gc.C) { c.Assert(s.Server.Report(), jc.DeepEquals, map[string]interface{}{ "agent-ratelimit-max": 1, @@ -102,13 +75,54 @@ func (s *rateLimitSuite) TestRateLimitNotApplicableToUsers(c *gc.C) { defer conn1.Close() // User connections are fine. - user := s.infoForNewUser(c, info) + user := s.infoForNewUser(c, info, "fredrikthordendal") conn2, err := api.Open(user, fastDialOpts) c.Assert(err, jc.ErrorIsNil) defer conn2.Close() - user2 := s.infoForNewUser(c, info) + user2 := s.infoForNewUser(c, info, "jenskidman") conn3, err := api.Open(user2, fastDialOpts) c.Assert(err, jc.ErrorIsNil) defer conn3.Close() } + +func (s *rateLimitSuite) infoForNewMachine(c *gc.C, info *api.Info) *api.Info { + // Make a copy + newInfo := *info + + f, release := s.NewFactory(c, info.ModelTag.Id()) + defer release() + machine, password := f.MakeMachineReturningPassword( + c, &factory.MachineParams{Nonce: "fake_nonce"}) + + newInfo.Tag = machine.Tag() + newInfo.Password = password + newInfo.Nonce = "fake_nonce" + return &newInfo +} + +func (s *rateLimitSuite) infoForNewUser(c *gc.C, info *api.Info, name string) *api.Info { + // Make a copy + newInfo := *info + + userService := s.ControllerServiceFactory(c).User() + userTag := names.NewUserTag(name) + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword("hunter2")), + }) + c.Assert(err, jc.ErrorIsNil) + + // TODO (stickupkid): Permissions: This is only required to insert admin + // permissions into the state, remove when permissions are written to state. + f, release := s.NewFactory(c, s.ControllerModelUUID()) + defer release() + f.MakeUser(c, &factory.UserParams{ + Name: userTag.Name(), + }) + + newInfo.Tag = userTag + newInfo.Password = "hunter2" + return &newInfo +} diff --git a/apiserver/tools_integration_test.go b/apiserver/tools_integration_test.go index c42a958ee9b..afac7e25f0f 100644 --- a/apiserver/tools_integration_test.go +++ b/apiserver/tools_integration_test.go @@ -4,6 +4,7 @@ package apiserver_test import ( + "context" "encoding/json" "fmt" "io" @@ -20,6 +21,8 @@ import ( apiauthentication "github.com/juju/juju/api/authentication" apitesting "github.com/juju/juju/apiserver/testing" + "github.com/juju/juju/domain/user/service" + "github.com/juju/juju/internal/auth" jujutesting "github.com/juju/juju/juju/testing" "github.com/juju/juju/rpc/params" "github.com/juju/juju/testing/factory" @@ -122,10 +125,24 @@ func (s *toolsWithMacaroonsIntegrationSuite) TestCanPostWithDischargedMacaroon(c func (s *toolsWithMacaroonsIntegrationSuite) TestCanPostWithLocalLogin(c *gc.C) { // Create a new local user that we can log in as // using macaroon authentication. - const password = "hunter2" + password := "hunter2" + userService := s.ControllerServiceFactory(c).User() + userTag := names.NewUserTag("bobbrown") + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword(password)), + }) + c.Assert(err, jc.ErrorIsNil) + + // TODO (stickupkid): Permissions: This is only required to insert admin + // permissions into the state, remove when permissions are written to state. f, release := s.NewFactory(c, s.ControllerModelUUID()) defer release() - user := f.MakeUser(c, &factory.UserParams{Password: password}) + f.MakeUser(c, &factory.UserParams{ + Name: userTag.Name(), + }) // Install a "web-page" visitor that deals with the interaction // method that Juju controllers support for authenticating local @@ -141,9 +158,9 @@ func (s *toolsWithMacaroonsIntegrationSuite) TestCanPostWithLocalLogin(c *gc.C) ) bakeryClient.Client = client.Client() bakeryClient.AddInteractor(apiauthentication.NewInteractor( - user.UserTag().Id(), + userTag.Id(), func(username string) (string, error) { - c.Assert(username, gc.Equals, user.UserTag().Id()) + c.Assert(username, gc.Equals, userTag.Id()) prompted = true return password, nil }, @@ -156,7 +173,7 @@ func (s *toolsWithMacaroonsIntegrationSuite) TestCanPostWithLocalLogin(c *gc.C) resp := apitesting.SendHTTPRequest(c, apitesting.HTTPRequestParams{ Method: "POST", URL: s.toolsURI(""), - Tag: user.UserTag().String(), + Tag: userTag.String(), Password: "", // no password forces macaroon usage Do: bakeryDo, }) diff --git a/apiserver/tools_test.go b/apiserver/tools_test.go index fad10dfe5e6..c63fa9d7b4f 100644 --- a/apiserver/tools_test.go +++ b/apiserver/tools_test.go @@ -19,18 +19,21 @@ import ( "time" "github.com/juju/errors" + "github.com/juju/names/v5" jc "github.com/juju/testing/checkers" "github.com/juju/version/v2" gc "gopkg.in/check.v1" apitesting "github.com/juju/juju/apiserver/testing" "github.com/juju/juju/core/objectstore" + "github.com/juju/juju/domain/user/service" "github.com/juju/juju/environs" "github.com/juju/juju/environs/simplestreams" "github.com/juju/juju/environs/storage" envtesting "github.com/juju/juju/environs/testing" envtools "github.com/juju/juju/environs/tools" toolstesting "github.com/juju/juju/environs/tools/testing" + "github.com/juju/juju/internal/auth" "github.com/juju/juju/internal/password" coretools "github.com/juju/juju/internal/tools" jujutesting "github.com/juju/juju/juju/testing" @@ -334,16 +337,31 @@ func (s *toolsSuite) TestMigrateToolsNotMigrating(c *gc.C) { ) } -func (s *toolsSuite) TestMigrateToolsUnauth(c *gc.C) { +func (s *toolsSuite) TestMigrateToolsForUser(c *gc.C) { // Try uploading as a non controller admin. + userService := s.ControllerServiceFactory(c).User() + userTag := names.NewUserTag("bobbrown") + _, _, err := userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + DisplayName: "Bob Brown", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword("hunter2")), + }) + c.Assert(err, jc.ErrorIsNil) + + // TODO (stickupkid): Permissions: This is only required to insert admin + // permissions into the state, remove when permissions are written to state. f, release := s.NewFactory(c, s.ControllerModelUUID()) defer release() + f.MakeUser(c, &factory.UserParams{ + Name: userTag.Name(), + }) + url := s.URL("/migrate/tools", nil).String() - user := f.MakeUser(c, &factory.UserParams{Password: "hunter2"}) resp := apitesting.SendHTTPRequest(c, apitesting.HTTPRequestParams{ Method: "POST", URL: url, - Tag: user.Tag().String(), + Tag: userTag.String(), Password: "hunter2", }) s.assertPlainErrorResponse( diff --git a/domain/user/state/state.go b/domain/user/state/state.go index e4840a62d83..aad91c39e6a 100644 --- a/domain/user/state/state.go +++ b/domain/user/state/state.go @@ -289,26 +289,26 @@ func (st *State) GetUserByAuth(ctx context.Context, name string, password auth.P return user.User{}, errors.Annotate(err, "getting DB access") } - var result User - err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { - getUserWithAuthQuery := ` -SELECT ( - user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at, - user.disabled, - user_password.password_hash, user_password.password_salt - ) AS (&User.*) -FROM v_user_auth AS user - LEFT JOIN user_password - ON user.uuid = user_password.user_uuid -WHERE user.name = $M.name -AND removed = false -` + getUserWithAuthQuery := ` + SELECT ( + user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at, + user.disabled, + user_password.password_hash, user_password.password_salt + ) AS (&User.*) + FROM v_user_auth AS user + LEFT JOIN user_password + ON user.uuid = user_password.user_uuid + WHERE user.name = $M.name + AND removed = false + ` - selectGetUserByAuthStmt, err := sqlair.Prepare(getUserWithAuthQuery, User{}, sqlair.M{}) - if err != nil { - return errors.Annotate(err, "preparing select getUserWithAuth query") - } + selectGetUserByAuthStmt, err := sqlair.Prepare(getUserWithAuthQuery, User{}, sqlair.M{}) + if err != nil { + return user.User{}, errors.Annotate(err, "preparing select getUserWithAuth query") + } + var result User + err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { err = tx.Query(ctx, selectGetUserByAuthStmt, sqlair.M{"name": name}).Get(&result) if errors.Is(err, sql.ErrNoRows) { return errors.Annotatef(usererrors.NotFound, "%q", name) @@ -709,7 +709,7 @@ INSERT INTO user_password (user_uuid, password_hash, password_salt) FROM user WHERE name = $M.name AND removed = false -ON CONFLICT(user_uuid) DO NOTHING` +ON CONFLICT(user_uuid) DO UPDATE SET password_hash = excluded.password_hash, password_salt = excluded.password_salt` insertSetPasswordHashStmt, err := sqlair.Prepare(setPasswordHashQuery, sqlair.M{}) if err != nil { diff --git a/domain/user/state/state_test.go b/domain/user/state/state_test.go index df9b55e0d72..85eb78e6769 100644 --- a/domain/user/state/state_test.go +++ b/domain/user/state/state_test.go @@ -745,6 +745,51 @@ WHERE user_uuid = ? c.Assert(err, jc.ErrorIs, sql.ErrNoRows) } +// TestSetPasswordHash asserts that we can set a password hash for a user twice. +func (s *stateSuite) TestSetPasswordHashTwice(c *gc.C) { + st := NewState(s.TxnRunnerFactory()) + + // Add admin user with activation key. + adminUUID, err := user.NewUUID() + c.Assert(err, jc.ErrorIsNil) + + newActivationKey, err := generateActivationKey() + c.Assert(err, jc.ErrorIsNil) + err = st.AddUserWithActivationKey( + context.Background(), adminUUID, + "admin", "admin", + adminUUID, newActivationKey, + ) + c.Assert(err, jc.ErrorIsNil) + + salt, err := auth.NewSalt() + c.Assert(err, jc.ErrorIsNil) + + // Set password hash. + err = st.SetPasswordHash(context.Background(), "admin", "passwordHash", salt) + c.Assert(err, jc.ErrorIsNil) + + // Set password hash again + err = st.SetPasswordHash(context.Background(), "admin", "passwordHashAgain", salt) + c.Assert(err, jc.ErrorIsNil) + + // Check that the password hash was set correctly. + db := s.DB() + + row := db.QueryRow(` +SELECT password_hash +FROM user_password +WHERE user_uuid = ? + `, adminUUID) + c.Assert(row.Err(), jc.ErrorIsNil) + + var passwordHash string + err = row.Scan(&passwordHash) + c.Assert(err, jc.ErrorIsNil) + + c.Check(passwordHash, gc.Equals, "passwordHashAgain") +} + // TestAddUserWithPasswordHash asserts that we can add a user with a password // hash. func (s *stateSuite) TestAddUserWithPasswordHash(c *gc.C) { diff --git a/internal/auth/password.go b/internal/auth/password.go index 4361339f0d9..52b7dedede3 100644 --- a/internal/auth/password.go +++ b/internal/auth/password.go @@ -148,3 +148,7 @@ func (p Password) Validate() error { } return nil } + +func (p Password) RAW() string { + return string(p.password) +} From 501b24257dba95f5d4f666b6f454c6502464f64e Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Tue, 27 Feb 2024 09:18:41 +0000 Subject: [PATCH 10/11] Test client watch all The test didn't actually test for read permission, but the user was actually an admin permission set. Fixing it to read only, ensured that the test worked correctly. --- apiserver/authentication/agent.go | 2 -- apiserver/facades/client/client/client_test.go | 16 +++++++++++++++- internal/worker/httpserverargs/worker.go | 11 +++++------ 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/apiserver/authentication/agent.go b/apiserver/authentication/agent.go index 9259346d4d8..0f25c847f04 100644 --- a/apiserver/authentication/agent.go +++ b/apiserver/authentication/agent.go @@ -55,8 +55,6 @@ type agentAuthenticator struct { logger Logger } -var _ EntityAuthenticator = (*agentAuthenticator)(nil) - type taggedAuthenticator interface { state.Entity state.Authenticator diff --git a/apiserver/facades/client/client/client_test.go b/apiserver/facades/client/client/client_test.go index 7555e67add0..7af657bdc71 100644 --- a/apiserver/facades/client/client/client_test.go +++ b/apiserver/facades/client/client/client_test.go @@ -10,6 +10,7 @@ import ( "github.com/juju/charm/v13" "github.com/juju/errors" "github.com/juju/loggo/v2" + "github.com/juju/names/v5" jtesting "github.com/juju/testing" jc "github.com/juju/testing/checkers" "github.com/juju/version/v2" @@ -30,8 +31,10 @@ import ( coreos "github.com/juju/juju/core/os" "github.com/juju/juju/core/permission" "github.com/juju/juju/core/status" + "github.com/juju/juju/domain/user/service" "github.com/juju/juju/environs/config" envtools "github.com/juju/juju/environs/tools" + "github.com/juju/juju/internal/auth" "github.com/juju/juju/internal/docker" "github.com/juju/juju/internal/docker/registry" "github.com/juju/juju/internal/docker/registry/image" @@ -180,10 +183,21 @@ func (s *clientWatchSuite) TestClientWatchAllReadPermission(c *gc.C) { err = m.SetProvisioned("i-0", "", agent.BootstrapNonce, nil) c.Assert(err, jc.ErrorIsNil) + userService := s.ControllerServiceFactory(c).User() + userTag := names.NewUserTag("fred") + _, _, err = userService.AddUser(context.Background(), service.AddUserArg{ + Name: userTag.Name(), + DisplayName: "Fred Flintstone", + CreatorUUID: s.AdminUserUUID, + Password: ptr(auth.NewPassword("ro-password")), + }) + c.Assert(err, jc.ErrorIsNil) + f, release := s.NewFactory(c, s.ControllerModelUUID()) defer release() user := f.MakeUser(c, &factory.UserParams{ - Password: "ro-password", + Name: userTag.Name(), + Access: permission.ReadAccess, }) c.Assert(err, jc.ErrorIsNil) conn := s.OpenModelAPIAs(c, s.ControllerModelUUID(), user.UserTag(), "ro-password", "") diff --git a/internal/worker/httpserverargs/worker.go b/internal/worker/httpserverargs/worker.go index b7cb1b19700..932756ad5e3 100644 --- a/internal/worker/httpserverargs/worker.go +++ b/internal/worker/httpserverargs/worker.go @@ -110,12 +110,11 @@ func (w *argsWorker) loop() error { return w.catacomb.ErrDying() } -// managedServices is a ControllerConfigService that wraps another -// ControllerConfigService and cancels the context when the tomb is dying. -// This is because the location of the controller config request is not -// cancellable, so we need the ability to cancel the controller config -// request when the tomb is dying. This should prevent any lockup when the -// controller is shutting down. +// managedServices is a ControllerConfigService and a UserService that wraps +// the underlying services and cancels the context when the tomb is dying. +// This is because the location of the request is not cancellable, so we need +// the ability to cancel the request when the tomb is dying. This should +// prevent any lockup when the controller is shutting down. type managedServices struct { tomb tomb.Tomb controllerConfigService ControllerConfigService From b12c310ec11bc15c4797a38278bfcf1413b9c9ba Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Tue, 27 Feb 2024 18:06:26 +0000 Subject: [PATCH 11/11] Wire up activation key to register To ensure we're only allowing one activation key for a user, we need to remove the old state code which sets the key. In the old code that was called a secret. We want to remove that terminology, as it's confusing with the new ongoing secret work. The code pushes the nacl secret box into the service layers, so we can live closer to state. The API only then unpacks the transport layers and hands it off. --- apiserver/admin_test.go | 2 +- apiserver/apiserver.go | 19 ++++ apiserver/embeddedcli.go | 7 -- apiserver/registration.go | 82 ++++++----------- apiserver/registration_test.go | 76 +++++++++------- domain/user/errors/errors.go | 8 ++ domain/user/service/service.go | 119 ++++++++++++++++++++++--- domain/user/service/service_test.go | 83 +++++++++++++++++ domain/user/service/state_mock_test.go | 15 ++++ domain/user/state/state.go | 67 +++++++++++--- domain/user/state/state_test.go | 31 ++++--- domain/user/state/types.go | 9 +- internal/auth/password.go | 4 - 13 files changed, 389 insertions(+), 133 deletions(-) diff --git a/apiserver/admin_test.go b/apiserver/admin_test.go index 033fb970ce6..371f231d3bb 100644 --- a/apiserver/admin_test.go +++ b/apiserver/admin_test.go @@ -1054,7 +1054,7 @@ func (s *loginV3Suite) TestClientLoginToControllerNoAccessToControllerModel(c *g user, err := userService.GetUser(context.Background(), uuid) c.Assert(err, jc.ErrorIsNil) - c.Check(user.LastLogin, jc.After, now) + c.Check(user.LastLogin, gc.Not(jc.Before), now) } func (s *loginV3Suite) TestClientLoginToRootOldClient(c *gc.C) { diff --git a/apiserver/apiserver.go b/apiserver/apiserver.go index 40f657c74e2..77dde3faa98 100644 --- a/apiserver/apiserver.go +++ b/apiserver/apiserver.go @@ -52,6 +52,8 @@ import ( "github.com/juju/juju/core/presence" "github.com/juju/juju/core/resources" coretrace "github.com/juju/juju/core/trace" + coreuser "github.com/juju/juju/core/user" + userservice "github.com/juju/juju/domain/user/service" controllermsg "github.com/juju/juju/internal/pubsub/controller" "github.com/juju/juju/internal/resource" "github.com/juju/juju/internal/servicefactory" @@ -65,6 +67,23 @@ var logger = loggo.GetLogger("juju.apiserver") var defaultHTTPMethods = []string{"GET", "POST", "HEAD", "PUT", "DELETE", "OPTIONS"} +// ControllerConfigService defines the methods required to get the controller +// configuration. +type ControllerConfigService interface { + ControllerConfig(context.Context) (controller.Config, error) +} + +// UserService defines the methods required to get user details. +type UserService interface { + // GetUserByName returns the user with the given name. + GetUserByName(context.Context, string) (coreuser.User, error) + // SetPasswordWithActivationKey will use the activation key from the user. To + // then apply the payload password. If the user does not exist an error that + // satisfies usererrors.NotFound will be return. If the nonce is not the + // correct length an error that satisfies errors.NotValid will be returned. + SetPasswordWithActivationKey(ctx context.Context, name string, nonce, box []byte) (userservice.Sealer, error) +} + // Server holds the server side of the API. type Server struct { tomb tomb.Tomb diff --git a/apiserver/embeddedcli.go b/apiserver/embeddedcli.go index 786c4dd6288..4ae7121535a 100644 --- a/apiserver/embeddedcli.go +++ b/apiserver/embeddedcli.go @@ -23,7 +23,6 @@ import ( apiservererrors "github.com/juju/juju/apiserver/errors" "github.com/juju/juju/apiserver/httpcontext" "github.com/juju/juju/apiserver/websocket" - "github.com/juju/juju/controller" "github.com/juju/juju/core/database" "github.com/juju/juju/core/model" "github.com/juju/juju/internal/feature" @@ -234,12 +233,6 @@ func newLineReader(r io.Reader) *linereader.Reader { return result } -// ControllerConfigService defines the methods required to get the controller -// configuration. -type ControllerConfigService interface { - ControllerConfig(context.Context) (controller.Config, error) -} - // ExecEmbeddedCommandFunc defines a function which runs a named Juju command // with the whitelisted sub commands. type ExecEmbeddedCommandFunc func(ctx *cmd.Context, store jujuclient.ClientStore, whitelist []string, cmdPlusArgs string) int diff --git a/apiserver/registration.go b/apiserver/registration.go index 9882ede26ca..224bf33a747 100644 --- a/apiserver/registration.go +++ b/apiserver/registration.go @@ -13,22 +13,17 @@ import ( "github.com/go-macaroon-bakery/macaroon-bakery/v3/httpbakery" "github.com/juju/errors" "github.com/juju/names/v5" - "golang.org/x/crypto/nacl/secretbox" "gopkg.in/macaroon.v2" "github.com/juju/juju/apiserver/common" coremacaroon "github.com/juju/juju/core/macaroon" + usererrors "github.com/juju/juju/domain/user/errors" "github.com/juju/juju/environs" "github.com/juju/juju/rpc/params" "github.com/juju/juju/state" "github.com/juju/juju/state/stateenvirons" ) -const ( - secretboxNonceLength = 24 - secretboxKeyLength = 32 -) - // registerUserHandler is an http.Handler for the "/register" endpoint. This is // used to complete a secure user registration process, and provide controller // login credentials. @@ -61,6 +56,7 @@ func (h *registerUserHandler) ServeHTTP(w http.ResponseWriter, req *http.Request req, st.State, serviceFactory.ControllerConfig(), + serviceFactory.User(), serviceFactory.Cloud(), serviceFactory.Credential(), ) @@ -116,80 +112,60 @@ func (h *registerUserHandler) processPost( req *http.Request, st *state.State, controllerConfigService ControllerConfigService, + userService UserService, cloudService common.CloudService, credentialService common.CredentialService, ) ( names.UserTag, *params.SecretKeyLoginResponse, error, ) { - ctx := req.Context() - - failure := func(err error) (names.UserTag, *params.SecretKeyLoginResponse, error) { - return names.UserTag{}, nil, err - } - data, err := io.ReadAll(req.Body) if err != nil { - return failure(err) + return names.UserTag{}, nil, errors.Trace(err) } var loginRequest params.SecretKeyLoginRequest if err := json.Unmarshal(data, &loginRequest); err != nil { - return failure(err) + return names.UserTag{}, nil, errors.Trace(err) } // Basic validation: ensure that the request contains a valid user tag, // nonce, and ciphertext of the expected length. userTag, err := names.ParseUserTag(loginRequest.User) if err != nil { - return failure(err) - } - if len(loginRequest.Nonce) != secretboxNonceLength { - return failure(errors.NotValidf("nonce")) + return names.UserTag{}, nil, errors.Trace(err) } - // Decrypt the ciphertext with the user's secret key (if it has one). - user, err := st.User(userTag) + // Decrypt the ciphertext with the user's activation key (if it has one). + sealer, err := userService.SetPasswordWithActivationKey(req.Context(), userTag.Name(), loginRequest.Nonce, loginRequest.PayloadCiphertext) if err != nil { - return failure(err) - } - if len(user.SecretKey()) != secretboxKeyLength { - return failure(errors.NotFoundf("secret key for user %q", user.Name())) - } - var key [secretboxKeyLength]byte - var nonce [secretboxNonceLength]byte - copy(key[:], user.SecretKey()) - copy(nonce[:], loginRequest.Nonce) - payloadBytes, ok := secretbox.Open(nil, loginRequest.PayloadCiphertext, &nonce, &key) - if !ok { - // Cannot decrypt the ciphertext, which implies that the secret - // key specified by the client is invalid. - return failure(errors.NotValidf("secret key")) - } - - // Unmarshal the request payload, which contains the new password to - // set for the user. - var requestPayload params.SecretKeyLoginRequestPayload - if err := json.Unmarshal(payloadBytes, &requestPayload); err != nil { - return failure(errors.Annotate(err, "cannot unmarshal payload")) - } - if err := user.SetPassword(requestPayload.Password); err != nil { - return failure(errors.Annotate(err, "setting new password")) + if errors.Is(err, usererrors.ActivationKeyNotValid) { + return names.UserTag{}, nil, errors.NotValidf("activation key") + } else if errors.Is(err, usererrors.ActivationKeyNotFound) { + return names.UserTag{}, nil, errors.NotFoundf("activation key") + } + return names.UserTag{}, nil, errors.Trace(err) } // Respond with the CA-cert and password, encrypted again with the - // secret key. - responsePayload, err := h.getSecretKeyLoginResponsePayload(ctx, st, controllerConfigService, cloudService, credentialService) + // activation key. + responsePayload, err := h.getSecretKeyLoginResponsePayload(req.Context(), st, controllerConfigService, cloudService, credentialService) if err != nil { - return failure(errors.Trace(err)) + return names.UserTag{}, nil, errors.Trace(err) } - payloadBytes, err = json.Marshal(responsePayload) + payloadBytes, err := json.Marshal(responsePayload) if err != nil { - return failure(errors.Trace(err)) + return names.UserTag{}, nil, errors.Trace(err) } - if _, err := rand.Read(nonce[:]); err != nil { - return failure(errors.Trace(err)) + if _, err := rand.Read(loginRequest.Nonce); err != nil { + return names.UserTag{}, nil, errors.Trace(err) + } + + // Seal the response payload with the user's activation key. + sealed, err := sealer.Seal(loginRequest.Nonce, payloadBytes) + if err != nil { + return names.UserTag{}, nil, errors.Trace(err) } response := ¶ms.SecretKeyLoginResponse{ - Nonce: nonce[:], - PayloadCiphertext: secretbox.Seal(nil, payloadBytes, &nonce, &key), + Nonce: loginRequest.Nonce, + PayloadCiphertext: sealed, } return userTag, response, nil } diff --git a/apiserver/registration_test.go b/apiserver/registration_test.go index 9d26dbdcb0d..254b6159092 100644 --- a/apiserver/registration_test.go +++ b/apiserver/registration_test.go @@ -23,17 +23,23 @@ import ( "github.com/juju/juju/apiserver" "github.com/juju/juju/apiserver/common" + "github.com/juju/juju/core/user" + usererrors "github.com/juju/juju/domain/user/errors" + "github.com/juju/juju/domain/user/service" "github.com/juju/juju/environs" + "github.com/juju/juju/internal/auth" jujutesting "github.com/juju/juju/juju/testing" "github.com/juju/juju/rpc/params" - "github.com/juju/juju/state" "github.com/juju/juju/state/stateenvirons" coretesting "github.com/juju/juju/testing" + "github.com/juju/juju/testing/factory" ) type registrationSuite struct { jujutesting.ApiServerSuite - bob *state.User + userService *service.Service + userUUID user.UUID + activationKey []byte registrationURL string } @@ -41,9 +47,26 @@ var _ = gc.Suite(®istrationSuite{}) func (s *registrationSuite) SetUpTest(c *gc.C) { s.ApiServerSuite.SetUpTest(c) - bob, err := s.ControllerModel(c).State().AddUserWithSecretKey("bob", "", "admin") + + s.userService = s.ControllerServiceFactory(c).User() + var err error + s.userUUID, _, err = s.userService.AddUser(context.Background(), service.AddUserArg{ + Name: "bob", + CreatorUUID: s.AdminUserUUID, + }) c.Assert(err, jc.ErrorIsNil) - s.bob = bob + + s.activationKey, err = s.userService.ResetPassword(context.Background(), "bob") + c.Assert(err, jc.ErrorIsNil) + + // TODO (stickupkid): Permissions: This is only required to insert admin + // permissions into the state, remove when permissions are written to state. + f, release := s.NewFactory(c, s.ControllerModelUUID()) + defer release() + f.MakeUser(c, &factory.UserParams{ + Name: "bob", + }) + s.registrationURL = s.URL("/register", url.Values{}).String() } @@ -73,14 +96,15 @@ func (s *registrationSuite) assertRegisterNoProxy(c *gc.C, hasProxy bool) { proxier.EXPECT().Type().Return("kubernetes-port-forward") } - // Ensure we cannot log in with the password yet. - const password = "hunter2" - c.Assert(s.bob.PasswordValid(password), jc.IsFalse) + password := "hunter2" + // It should be not possible to log in as bob with the password "hunter2" + // now. + _, err := s.userService.GetUserByAuth(context.Background(), "bob", password) + c.Assert(err, jc.ErrorIs, usererrors.Unauthorized) validNonce := []byte(strings.Repeat("X", 24)) - secretKey := s.bob.SecretKey() ciphertext := s.sealBox( - c, validNonce, secretKey, fmt.Sprintf(`{"password": "%s"}`, password), + c, validNonce, s.activationKey, fmt.Sprintf(`{"password": "%s"}`, password), ) client := jujuhttp.NewClient(jujuhttp.WithSkipHostnameVerification(true)) resp := httptesting.Do(c, httptesting.DoRequestParams{ @@ -99,10 +123,9 @@ func (s *registrationSuite) assertRegisterNoProxy(c *gc.C, hasProxy bool) { // It should be possible to log in as bob with the // password "hunter2" now, and there should be no // secret key any longer. - err := s.bob.Refresh() + user, err := s.userService.GetUserByAuth(context.Background(), "bob", password) c.Assert(err, jc.ErrorIsNil) - c.Assert(s.bob.PasswordValid(password), jc.IsTrue) - c.Assert(s.bob.SecretKey(), gc.IsNil) + c.Check(user.UUID, gc.Equals, s.userUUID) var response params.SecretKeyLoginResponse bodyData, err := io.ReadAll(resp.Body) @@ -110,7 +133,9 @@ func (s *registrationSuite) assertRegisterNoProxy(c *gc.C, hasProxy bool) { err = json.Unmarshal(bodyData, &response) c.Assert(err, jc.ErrorIsNil) c.Assert(response.Nonce, gc.HasLen, len(validNonce)) - plaintext := s.openBox(c, response.PayloadCiphertext, response.Nonce, secretKey) + + // Open the box to ensure that the response is as expected. + plaintext := s.openBox(c, response.PayloadCiphertext, response.Nonce, s.activationKey) var responsePayload params.SecretKeyLoginResponsePayload err = json.Unmarshal(plaintext, &responsePayload) @@ -177,38 +202,25 @@ func (s *registrationSuite) TestRegisterInvalidCiphertext(c *gc.C) { fmt.Sprintf( `{"user": "user-bob", "nonce": "%s"}`, base64.StdEncoding.EncodeToString(validNonce), - ), `secret key not valid`, params.CodeNotValid, + ), `activation key not valid`, params.CodeNotValid, http.StatusInternalServerError, ) } func (s *registrationSuite) TestRegisterNoSecretKey(c *gc.C) { - err := s.bob.SetPassword("anything") + err := s.userService.SetPassword(context.Background(), "bob", auth.NewPassword("anything")) c.Assert(err, jc.ErrorIsNil) + validNonce := []byte(strings.Repeat("X", 24)) s.testInvalidRequest(c, fmt.Sprintf( `{"user": "user-bob", "nonce": "%s"}`, base64.StdEncoding.EncodeToString(validNonce), - ), `secret key for user "bob" not found`, params.CodeNotFound, + ), `activation key not found`, params.CodeNotFound, http.StatusNotFound, ) } -func (s *registrationSuite) TestRegisterInvalidRequestPayload(c *gc.C) { - validNonce := []byte(strings.Repeat("X", 24)) - ciphertext := s.sealBox(c, validNonce, s.bob.SecretKey(), "[]") - s.testInvalidRequest(c, - fmt.Sprintf( - `{"user": "user-bob", "nonce": "%s", "cipher-text": "%s"}`, - base64.StdEncoding.EncodeToString(validNonce), - base64.StdEncoding.EncodeToString(ciphertext), - ), - `cannot unmarshal payload: json: cannot unmarshal array into Go value of type params.SecretKeyLoginRequestPayload`, "", - http.StatusInternalServerError, - ) -} - func (s *registrationSuite) testInvalidRequest(c *gc.C, requestBody, errorMessage, errorCode string, statusCode int) { client := jujuhttp.NewClient(jujuhttp.WithSkipHostnameVerification(true)) httptesting.AssertJSONCall(c, httptesting.JSONCallParams{ @@ -234,8 +246,8 @@ func (s *registrationSuite) sealBox(c *gc.C, nonce, key []byte, message string) func (s *registrationSuite) openBox(c *gc.C, ciphertext, nonce, key []byte) []byte { var nonceArray [24]byte var keyArray [32]byte - c.Assert(copy(nonceArray[:], nonce), gc.Equals, len(nonceArray)) - c.Assert(copy(keyArray[:], key), gc.Equals, len(keyArray)) + c.Assert(copy(nonceArray[:], nonce), gc.Equals, len(nonceArray), gc.Commentf("nonce: %v", nonce)) + c.Assert(copy(keyArray[:], key), gc.Equals, len(keyArray), gc.Commentf("key: %v", key)) message, ok := secretbox.Open(nil, ciphertext, &nonceArray, &keyArray) c.Assert(ok, jc.IsTrue) return message diff --git a/domain/user/errors/errors.go b/domain/user/errors/errors.go index 850a666cebf..2b47087c97c 100644 --- a/domain/user/errors/errors.go +++ b/domain/user/errors/errors.go @@ -37,4 +37,12 @@ const ( // Unauthorized describes an error that occurs when the user does not have // the required permissions to perform an action. Unauthorized = errors.ConstError("user unauthorized") + + // ActivationKeyNotFound describes an error that occurs when the activation + // key is not found. + ActivationKeyNotFound = errors.ConstError("activation key not found") + + // ActivationKeyNotValid describes an error that occurs when the activation + // key is not valid. + ActivationKeyNotValid = errors.ConstError("activation key not valid") ) diff --git a/domain/user/service/service.go b/domain/user/service/service.go index 4572e12b440..c4c80ae8ed6 100644 --- a/domain/user/service/service.go +++ b/domain/user/service/service.go @@ -6,8 +6,10 @@ package service import ( "context" "crypto/rand" + "encoding/json" "github.com/juju/errors" + "golang.org/x/crypto/nacl/secretbox" "github.com/juju/juju/core/user" domainuser "github.com/juju/juju/domain/user" @@ -85,6 +87,11 @@ type State interface { // is returned that satisfies usererrors.NotFound. SetActivationKey(context.Context, string, []byte) error + // GetActivationKey will retrieve the activation key for the user. + // If no user is found for the supplied user name an error is returned that + // satisfies usererrors.NotFound. + GetActivationKey(context.Context, string) ([]byte, error) + // SetPasswordHash removes any active activation keys and sets the user // password hash and salt. If no user is found for the supplied user name an error // is returned that satisfies usererrors.NotFound. @@ -304,20 +311,8 @@ func (s *Service) SetPassword(ctx context.Context, name string, pass auth.Passwo return errors.Trace(err) } - salt, err := auth.NewSalt() - if err != nil { - return errors.Annotatef(err, "generating password salt for user %q", name) - } - - pwHash, err := auth.HashPassword(pass, salt) - if err != nil { - return errors.Annotatef(err, "hashing password for user %q", name) - } - - if err = s.st.SetPasswordHash(ctx, name, pwHash, salt); err != nil { - return errors.Annotatef(err, "setting password for user %q", name) - } - return nil + err := s.setPassword(ctx, name, pass) + return errors.Trace(err) } // ResetPassword will remove any active passwords for a user and generate a new @@ -398,3 +393,99 @@ func generateActivationKey() ([]byte, error) { } return activationKey[:], nil } + +// activationBoxNonceLength is the number of bytes in the nonce for the +// activation box. +const activationBoxNonceLength = 24 + +// Sealer is an interface that can be used to seal a byte slice. +// This will use the nonce and box for a given user to seal the payload. +type Sealer interface { + // Seal will seal the payload using the nonce and box for the user. + Seal(nonce, payload []byte) ([]byte, error) +} + +// SetPasswordWithActivationKey will use the activation key from the user. To +// then apply the payload password. If the user does not exist an error that +// satisfies usererrors.NotFound will be return. If the nonce is not the +// correct length an error that satisfies errors.NotValid will be returned. +// +// This will use the NaCl secretbox to open the box and then unmarshal the +// payload to set the new password for the user. If the payload cannot be +// unmarshalled an error will be returned. +// To prevent the leaking of the key and nonce (which can unbox the secret), +// a Sealer will be returned that can be used to seal the response payload. +func (s *Service) SetPasswordWithActivationKey(ctx context.Context, name string, nonce, box []byte) (Sealer, error) { + if len(nonce) != activationBoxNonceLength { + return nil, errors.NotValidf("nonce") + } + + // Get the activation key for the user. + key, err := s.st.GetActivationKey(ctx, name) + if err != nil { + return nil, errors.Trace(err) + } + + // Copy the nonce and the key to arrays which can be used for the secretbox. + var sbKey [activationKeyLength]byte + var sbNonce [activationBoxNonceLength]byte + copy(sbKey[:], key) + copy(sbNonce[:], nonce) + + // The box is the payload that has been sealed with the nonce and key, so + // let's open it. + boxPayloadBytes, ok := secretbox.Open(nil, box, &sbNonce, &sbKey) + if !ok { + return nil, usererrors.ActivationKeyNotValid + } + + // We expect the payload to be a JSON object with a password field. + var payload struct { + // Password is the new password to set for the user. + Password string `json:"password"` + } + if err := json.Unmarshal(boxPayloadBytes, &payload); err != nil { + return nil, errors.Annotate(err, "cannot unmarshal payload") + } + + if err := s.setPassword(ctx, name, auth.NewPassword(payload.Password)); err != nil { + return nil, errors.Annotate(err, "setting new password") + } + + return boxSealer{ + key: sbKey, + }, nil +} + +func (s *Service) setPassword(ctx context.Context, name string, pass auth.Password) error { + salt, err := auth.NewSalt() + if err != nil { + return errors.Annotatef(err, "generating password salt for user %q", name) + } + + pwHash, err := auth.HashPassword(pass, salt) + if err != nil { + return errors.Annotatef(err, "hashing password for user %q", name) + } + + if err = s.st.SetPasswordHash(ctx, name, pwHash, salt); err != nil { + return errors.Annotatef(err, "setting password for user %q", name) + } + + return nil +} + +// boxSealer is a Sealer that uses the NaCl secretbox to seal a payload. +type boxSealer struct { + key [activationKeyLength]byte +} + +func (s boxSealer) Seal(nonce, payload []byte) ([]byte, error) { + if len(nonce) != activationBoxNonceLength { + return nil, errors.NotValidf("nonce") + } + + var sbNonce [activationBoxNonceLength]byte + copy(sbNonce[:], nonce) + return secretbox.Seal(nil, payload, &sbNonce, &s.key), nil +} diff --git a/domain/user/service/service_test.go b/domain/user/service/service_test.go index fc9a9bcccd4..24ae9879d3a 100644 --- a/domain/user/service/service_test.go +++ b/domain/user/service/service_test.go @@ -7,12 +7,14 @@ import ( "context" "crypto/rand" "encoding/base64" + "encoding/json" "errors" "fmt" "testing" jc "github.com/juju/testing/checkers" "go.uber.org/mock/gomock" + "golang.org/x/crypto/nacl/secretbox" gc "gopkg.in/check.v1" "github.com/juju/juju/core/user" @@ -337,7 +339,88 @@ func (s *serviceSuite) TestDisableUserAuthentication(c *gc.C) { err := s.service().DisableUserAuthentication(context.Background(), "name") c.Assert(err, jc.ErrorIsNil) +} + +// TestSetPasswordWithActivationKey tests setting a password with an activation +// key. +func (s *serviceSuite) TestSetPasswordWithActivationKey(c *gc.C) { + defer s.setupMocks(c).Finish() + + password := "password123" + + // Create a key for the activation box. + key := make([]byte, activationKeyLength) + _, err := rand.Read(key) + c.Assert(err, jc.ErrorIsNil) + + s.state.EXPECT().GetActivationKey(gomock.Any(), "name").Return(key, nil) + s.state.EXPECT().SetPasswordHash(gomock.Any(), "name", gomock.Any(), gomock.Any()).Return(nil) + + // Create a nonce for the activation box. + nonce := make([]byte, activationBoxNonceLength) + _, err = rand.Read(nonce) + c.Assert(err, jc.ErrorIsNil) + + type payload struct { + Password string `json:"password"` + } + p := payload{ + Password: password, + } + payloadBytes, err := json.Marshal(p) + c.Assert(err, jc.ErrorIsNil) + + box := s.sealBox(c, key, nonce, payloadBytes) + + _, err = s.service().SetPasswordWithActivationKey(context.Background(), "name", nonce, box) + c.Assert(err, jc.ErrorIsNil) +} + +// TestSetPasswordWithActivationKeyWithInvalidKey tests setting a password +// with an invalid activation key. +func (s *serviceSuite) TestSetPasswordWithActivationKeyWithInvalidKey(c *gc.C) { + defer s.setupMocks(c).Finish() + + password := "password123" + + // Create a key for the activation box. + key := make([]byte, activationKeyLength) + _, err := rand.Read(key) + c.Assert(err, jc.ErrorIsNil) + + s.state.EXPECT().GetActivationKey(gomock.Any(), "name").Return(key, nil) + + // Create a nonce for the activation box. + nonce := make([]byte, activationBoxNonceLength) + _, err = rand.Read(nonce) + c.Assert(err, jc.ErrorIsNil) + + type payload struct { + Password string `json:"password"` + } + p := payload{ + Password: password, + } + payloadBytes, err := json.Marshal(p) + c.Assert(err, jc.ErrorIsNil) + + box := s.sealBox(c, key, nonce, payloadBytes) + + // Replace the nonce with a different nonce. + _, err = rand.Read(nonce) + c.Assert(err, jc.ErrorIsNil) + + _, err = s.service().SetPasswordWithActivationKey(context.Background(), "name", nonce, box) + c.Assert(err, jc.ErrorIs, usererrors.ActivationKeyNotValid) +} + +func (s *serviceSuite) sealBox(c *gc.C, key, nonce, payload []byte) []byte { + var sbKey [activationKeyLength]byte + var sbNonce [activationBoxNonceLength]byte + copy(sbKey[:], key) + copy(sbNonce[:], nonce) + return secretbox.Seal(nil, payload, &sbNonce, &sbKey) } // FuzzGetUser is a fuzz test for GetUser() that stresses the username input of diff --git a/domain/user/service/state_mock_test.go b/domain/user/service/state_mock_test.go index 157df4c3395..1b42f6b52f8 100644 --- a/domain/user/service/state_mock_test.go +++ b/domain/user/service/state_mock_test.go @@ -111,6 +111,21 @@ func (mr *MockStateMockRecorder) EnableUserAuthentication(arg0, arg1 any) *gomoc return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EnableUserAuthentication", reflect.TypeOf((*MockState)(nil).EnableUserAuthentication), arg0, arg1) } +// GetActivationKey mocks base method. +func (m *MockState) GetActivationKey(arg0 context.Context, arg1 string) ([]byte, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetActivationKey", arg0, arg1) + ret0, _ := ret[0].([]byte) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetActivationKey indicates an expected call of GetActivationKey. +func (mr *MockStateMockRecorder) GetActivationKey(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetActivationKey", reflect.TypeOf((*MockState)(nil).GetActivationKey), arg0, arg1) +} + // GetAllUsers mocks base method. func (m *MockState) GetAllUsers(arg0 context.Context) ([]user.User, error) { m.ctrl.T.Helper() diff --git a/domain/user/state/state.go b/domain/user/state/state.go index aad91c39e6a..595151e8efc 100644 --- a/domain/user/state/state.go +++ b/domain/user/state/state.go @@ -212,7 +212,7 @@ AND user.removed = false return user.UUID(""), errors.Trace(err) } - var result = sqlair.M{} + result := sqlair.M{} err = tx.Query(ctx, selectUserUUIDStmt, sqlair.M{"name": name}).Get(&result) if errors.Is(err, sql.ErrNoRows) { return user.UUID(""), fmt.Errorf( @@ -290,16 +290,16 @@ func (st *State) GetUserByAuth(ctx context.Context, name string, password auth.P } getUserWithAuthQuery := ` - SELECT ( - user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at, - user.disabled, - user_password.password_hash, user_password.password_salt - ) AS (&User.*) - FROM v_user_auth AS user - LEFT JOIN user_password - ON user.uuid = user_password.user_uuid - WHERE user.name = $M.name - AND removed = false +SELECT ( + user.uuid, user.name, user.display_name, user.created_by_uuid, user.created_at, + user.disabled, + user_password.password_hash, user_password.password_salt + ) AS (&User.*) +FROM v_user_auth AS user + LEFT JOIN user_password + ON user.uuid = user_password.user_uuid +WHERE user.name = $M.name +AND removed = false ` selectGetUserByAuthStmt, err := sqlair.Prepare(getUserWithAuthQuery, User{}, sqlair.M{}) @@ -429,6 +429,51 @@ func (st *State) SetActivationKey(ctx context.Context, name string, activationKe }) } +// GetActivationKey retrieves the activation key for the user with the supplied +// user name. If the user does not exist an error that satisfies +// usererrors.NotFound will be returned. +func (st *State) GetActivationKey(ctx context.Context, name string) ([]byte, error) { + db, err := st.DB() + if err != nil { + return nil, errors.Annotate(err, "getting DB access") + } + + uuidStmt, err := st.getActiveUUIDStmt() + if err != nil { + return nil, errors.Trace(err) + } + + m := make(sqlair.M, 1) + + selectKeyStmt, err := sqlair.Prepare("SELECT (*) AS (&ActivationKey.*) FROM user_activation_key WHERE user_uuid = $M.uuid", m, ActivationKey{}) + if err != nil { + return nil, errors.Annotate(err, "preparing activation get query") + } + + var key ActivationKey + err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error { + uuid, err := st.uuidForName(ctx, tx, uuidStmt, name) + if err != nil { + return errors.Trace(err) + } + + if err := tx.Query(ctx, selectKeyStmt, sqlair.M{"uuid": uuid}).Get(&key); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return errors.Annotatef(usererrors.ActivationKeyNotFound, "activation key for %q", name) + } + return errors.Annotatef(err, "selecting activation key for %q", name) + } + return errors.Trace(err) + }) + if err != nil { + return nil, errors.Annotatef(err, "getting activation key for %q", name) + } + if len(key.ActivationKey) == 0 { + return nil, errors.Annotatef(usererrors.ActivationKeyNotValid, "activation key for %q", name) + } + return []byte(key.ActivationKey), nil +} + // SetPasswordHash removes any active activation keys and sets the user // password hash and salt. If no user is found for the supplied user name an error // is returned that satisfies usererrors.NotFound. diff --git a/domain/user/state/state_test.go b/domain/user/state/state_test.go index 85eb78e6769..740e572db48 100644 --- a/domain/user/state/state_test.go +++ b/domain/user/state/state_test.go @@ -866,20 +866,31 @@ func (s *stateSuite) TestAddUserWithActivationKey(c *gc.C) { c.Assert(err, jc.ErrorIsNil) // Check that the activation key was set correctly. - db := s.DB() + activationKey, err := st.GetActivationKey(context.Background(), "admin") + c.Assert(err, jc.ErrorIsNil) - row := db.QueryRow(` -SELECT activation_key -FROM user_activation_key -WHERE user_uuid = ? - `, adminUUID) - c.Assert(row.Err(), jc.ErrorIsNil) + c.Check(activationKey, gc.DeepEquals, adminActivationKey) +} - var activationKey string - err = row.Scan(&activationKey) +// TestGetActivationKeyNotFound asserts that if we try to get an activation key +// for a user that does not exist, we get an error. +func (s *stateSuite) TestGetActivationKeyNotFound(c *gc.C) { + st := NewState(s.TxnRunnerFactory()) + + // Add admin user. + adminUUID, err := user.NewUUID() c.Assert(err, jc.ErrorIsNil) - c.Check(activationKey, gc.Equals, string(adminActivationKey)) + err = st.AddUser( + context.Background(), adminUUID, + "admin", "admin", + adminUUID, + ) + c.Assert(err, jc.ErrorIsNil) + + // Check that the activation key was set correctly. + _, err = st.GetActivationKey(context.Background(), "admin") + c.Assert(err, jc.ErrorIs, usererrors.ActivationKeyNotFound) } // TestAddUserWithActivationKeyWhichCreatorDoesNotExist asserts that we get an diff --git a/domain/user/state/types.go b/domain/user/state/types.go index 26f5f042482..6638fa6425c 100644 --- a/domain/user/state/types.go +++ b/domain/user/state/types.go @@ -9,7 +9,8 @@ import ( "github.com/juju/juju/core/user" ) -// User represents a user in the state layer with the associated fields in the database. +// User represents a user in the state layer with the associated fields in the +// database. type User struct { // UUID is the unique identifier for the user. UUID user.UUID `db:"uuid"` @@ -58,3 +59,9 @@ func (u User) toCoreUser() user.User { Disabled: u.Disabled, } } + +// ActivationKey represents an activation key in the state layer with the +// associated fields in the database. +type ActivationKey struct { + ActivationKey string `db:"activation_key"` +} diff --git a/internal/auth/password.go b/internal/auth/password.go index 52b7dedede3..4361339f0d9 100644 --- a/internal/auth/password.go +++ b/internal/auth/password.go @@ -148,7 +148,3 @@ func (p Password) Validate() error { } return nil } - -func (p Password) RAW() string { - return string(p.password) -}