Merge pull request juju#16940 from SimonRichardson/remove-user-find-e…

…ntity

juju#16940

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 believe we should probably yank all the auth out into one location. That might be a task for another day.

There is a lot of confusion between authentication (authn) and authorization (authz) with the find entity. There seems to be optimisation of attempting to authz early on in the authn stack, which causes problems of forcing cross-cutting concerns that's proving hard to untangle. For example, the modelUserEntityFinder is deeply confused. 

~~More work is require, probably requiring juju#16935 to land first, as the user domain state package has bugs.~~

<!-- Why this change is needed and what it does. -->

## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [ ] Code style: imports ordered, good names, simple structure, etc
- [ ] Comments saying why design decisions were made
- [ ] Go unit tests, with comments saying what you're testing
- [ ] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing
- [ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages

## QA steps

<!-- Describe steps to verify that the change works. -->

## Documentation changes

<!-- How it affects user workflow (CLI or API). -->

## Links

<!-- Link to all relevant specification, documentation, bug, issue or JIRA card. -->

**Launchpad bug:** https://bugs.launchpad.net/juju/+bug/

**Jira card:** JUJU-

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"
@@ -175,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",
 	})
 
@@ -205,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",
 	})
 
@@ -396,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
 }
@@ -630,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())
 }
@@ -780,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 := &params.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")
 }
@@ -815,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")
 }
@@ -846,32 +897,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()
+	user, err := userService.GetUser(context.Background(), uuid)
 	c.Assert(err, jc.ErrorIsNil)
-	lastLogin, err := user.LastLogin()
-	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 +1031,30 @@ func (s *loginV3Suite) TestClientLoginToController(c *gc.C) {
 }
 
 func (s *loginV3Suite) TestClientLoginToControllerNoAccessToControllerModel(c *gc.C) {
+	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)
+
+	// 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{
-		NoModelUser: true,
-		Password:    password,
+	f.MakeUser(c, &factory.UserParams{
+		Name: "bobbrown",
 	})
 
-	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, gc.Not(jc.Before), now)
 }
 
 func (s *loginV3Suite) TestClientLoginToRootOldClient(c *gc.C) {
@@ -1029,3 +1099,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
+}

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