feat(modelmanager): listmodels (#1493)

* test(modelmanager): k8s tests fix

The api was trying to call /version, but our mock didn't implement it.

* feat(listmodels): enable listmodels within the jujuclient pkg

* fix(listmodels): enable list models via controller

We now list models via contacting controllers directly.

* docs(listmodels): update jimm pkg listmodels godoc

* refactor(pr): pr comments

* chore: pr comments

* chore: pr comments

* chore: pr comments

* chore: pr comment

* chore: pr comments

* chore: pr comment

internal/db/model.go

@@ -176,6 +176,9 @@ func (d *Database) ForEachModel(ctx context.Context, f func(m *dbmodel.Model) er
 
 // GetModelsByUUID retrieves a list of models where the model UUIDs are in
 // the provided modelUUIDs slice.
+//
+// If the UUID cannot be resolved to a model, it is skipped from the result and
+// no error is returned.
 func (d *Database) GetModelsByUUID(ctx context.Context, modelUUIDs []string) (_ []dbmodel.Model, err error) {
 	const op = errors.Op("db.GetModelsByUUID")
 

internal/db/model_test.go

@@ -570,6 +570,7 @@ func (s *dbSuite) TestGetModelsByUUID(c *qt.C) {
 	sort.Slice(models, func(i, j int) bool {
 		return models[i].UUID.String < models[j].UUID.String
 	})
+	c.Assert(models, qt.HasLen, 3)
 	c.Check(models[0].UUID.String, qt.Equals, "00000002-0000-0000-0000-000000000001")
 	c.Check(models[0].Controller.Name, qt.Not(qt.Equals), "")
 	c.Check(models[1].UUID.String, qt.Equals, "00000002-0000-0000-0000-000000000002")

internal/jimm/jimm.go

@@ -494,6 +494,9 @@ type API interface {
 
 	// ListStorageDetails lists all storage.
 	ListStorageDetails(ctx context.Context) ([]jujuparams.StorageDetails, error)
+
+	// ListModels returns all UserModel's on the controller.
+	ListModels(ctx context.Context) ([]base.UserModel, error)
 }
 
 // forEachController runs a given function on multiple controllers

internal/jimm/model.go

@@ -12,6 +12,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/juju/juju/api/base"
 	jujupermission "github.com/juju/juju/core/permission"
 	jujuparams "github.com/juju/juju/rpc/params"
 	"github.com/juju/juju/state"
@@ -779,10 +780,12 @@ func (j *JIMM) ListModelSummaries(ctx context.Context, user *openfga.User, maski
 			model      *dbmodel.Model
 			userAccess jujuparams.UserAccessPermission
 		}{model: m, userAccess: uap})
+
 		if _, ok := uniqueControllerMap[m.Controller.UUID]; !ok {
 			uniqueControllers = append(uniqueControllers, m.Controller)
 			uniqueControllerMap[m.Controller.UUID] = struct{}{}
 		}
+
 		return nil
 	})
 	if err != nil {
@@ -1347,3 +1350,73 @@ func (j *JIMM) ChangeModelCredential(ctx context.Context, user *openfga.User, mo
 
 	return nil
 }
+
+// ListModels list the models that the user has access to. It intentionally excludes the
+// controller model as this call is used within the context of login and register commands.
+func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserModel, error) {
+	const op = errors.Op("jimm.ListModels")
+	zapctx.Info(ctx, string(op))
+
+	// Get uuids of models the user has access to
+	uuids, err := user.ListModels(ctx, ofganames.ReaderRelation)
+	if err != nil {
+		return nil, errors.E(op, err, "failed to list user models")
+	}
+
+	// Get the models from the database
+	models, err := j.DB().GetModelsByUUID(ctx, uuids)
+	if err != nil {
+		return nil, errors.E(op, err, "failed to get models by uuid")
+	}
+
+	// Create map for lookup later
+	modelsMap := make(map[string]dbmodel.Model)
+	// Find the controllers these models reside on and remove duplicates
+	var controllers []dbmodel.Controller
+	seen := make(map[uint]bool)
+	for _, model := range models {
+		modelsMap[model.UUID.String] = model // Set map for lookup
+		if seen[model.ControllerID] {
+			continue
+		}
+		seen[model.ControllerID] = true
+		controllers = append(controllers, model.Controller)
+	}
+
+	// Call controllers for their models. We always call as admin, and we're
+	// filtering ourselves. We do this rather than send the user to be 100%
+	// certain that the models do belong to user according to OpenFGA. We could
+	// in theory rely on Juju correctly returning the models (by owner), but this
+	// is more reliable.
+	var userModels []base.UserModel
+	var mutex sync.Mutex
+	err = j.forEachController(ctx, controllers, func(_ *dbmodel.Controller, api API) error {
+		ums, err := api.ListModels(ctx)
+		if err != nil {
+			return err
+		}
+		mutex.Lock()
+		defer mutex.Unlock()
+
+		// Filter the models returned according to the uuids
+		// returned from OpenFGA for read access.
+		//
+		// NOTE: Controller models are not included because we never relate
+		// controller models to users, and as such, they will not appear in the
+		// authorised uuid map.
+		for _, um := range ums {
+			mapModel, ok := modelsMap[um.UUID]
+			if !ok {
+				continue
+			}
+			um.Owner = mapModel.OwnerIdentityName
+			userModels = append(userModels, um)
+		}
+		return nil
+	})
+	if err != nil {
+		return nil, errors.E(op, err, "failed to list models")
+	}
+
+	return userModels, nil
+}

internal/jimm/model_test.go

@@ -13,6 +13,7 @@ import (
 	qt "github.com/frankban/quicktest"
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/juju/juju/api/base"
 	"github.com/juju/juju/core/life"
 	"github.com/juju/juju/rpc/params"
 	jujuparams "github.com/juju/juju/rpc/params"
@@ -3804,6 +3805,199 @@ controllers:
 	c.Assert(model.Controller.Name, qt.Equals, "controller-3")
 }
 
+const listModelsTestEnv = `clouds:
+- name: test-cloud
+  type: test-provider
+  regions:
+  - name: test-cloud-region
+cloud-credentials:
+- owner: [email protected]
+  name: cred-1
+  cloud: test-cloud
+
+controllers:
+- name: controller-1
+  uuid: 00000001-0000-0000-0000-000000000001
+  cloud: test-cloud
+  region: test-cloud-region
+
+- name: controller-2
+  uuid: 00000001-0000-0000-0000-000000000001
+  cloud: test-cloud
+  region: test-cloud-region
+
+models:
+- name: model-1
+  uuid: 00000002-0000-0000-0000-000000000001
+  controller: controller-1
+  cloud: test-cloud
+  region: test-cloud-region
+  cloud-credential: cred-1
+  owner: [email protected]
+  life: alive
+  users:
+  - user: [email protected]
+    access: admin
+  - user: [email protected]
+    access: admin
+
+- name: model-2
+  uuid: 00000002-0000-0000-0000-000000000002
+  controller: controller-1
+  cloud: test-cloud
+  region: test-cloud-region
+  cloud-credential: cred-1
+  owner: [email protected]
+  life: alive
+  users:
+  - user: [email protected]
+    access: admin
+  - user: [email protected]
+    access: write
+  sla:
+    level: unsupported
+
+- name: model-3
+  uuid: 00000002-0000-0000-0000-000000000003
+  controller: controller-2
+  cloud: test-cloud
+  region: test-cloud-region
+  cloud-credential: cred-1
+  owner: [email protected]
+  life: alive
+  users:
+  - user: [email protected]
+    access: admin
+  - user: [email protected]
+    access: read
+
+- name: model-4
+  uuid: 00000002-0000-0000-0000-000000000004
+  controller: controller-1
+  cloud: test-cloud
+  region: test-cloud-region
+  cloud-credential: cred-1
+  owner: [email protected]
+  life: alive
+  users:
+  - user: [email protected]
+    access: admin
+
+users:
+- username: [email protected]
+  controller-access: superuser
+`
+
+var modelListTests = []struct {
+	name                           string
+	env                            string
+	username                       string
+	expectedUserModels             []base.UserModel
+	expectedError                  string
+	listModelsMockByControllerName map[string]func(context.Context) ([]base.UserModel, error)
+}{
+	{
+		name:     "Bob lists models across controllers 1 and 2",
+		env:      listModelsTestEnv,
+		username: "[email protected]",
+		expectedUserModels: []base.UserModel{
+			{UUID: "00000002-0000-0000-0000-000000000001", Owner: "[email protected]"},
+			{UUID: "00000002-0000-0000-0000-000000000002", Owner: "[email protected]"},
+			{UUID: "00000002-0000-0000-0000-000000000003", Owner: "[email protected]"},
+		},
+		listModelsMockByControllerName: map[string]func(context.Context) ([]base.UserModel, error){
+			"controller-1": func(ctx context.Context) ([]base.UserModel, error) {
+				return []base.UserModel{
+					{UUID: "00000002-0000-0000-0000-000000000001"},
+					{UUID: "00000002-0000-0000-0000-000000000002"},
+				}, nil
+			},
+			"controller-2": func(ctx context.Context) ([]base.UserModel, error) {
+				return []base.UserModel{
+					{UUID: "00000002-0000-0000-0000-000000000003"},
+				}, nil
+			},
+		},
+	},
+	{
+		name:     "Alice lists models across controllers 1 and 2",
+		env:      listModelsTestEnv,
+		username: "[email protected]",
+		expectedUserModels: []base.UserModel{
+			{UUID: "00000002-0000-0000-0000-000000000001", Owner: "[email protected]"},
+			{UUID: "00000002-0000-0000-0000-000000000002", Owner: "[email protected]"},
+			{UUID: "00000002-0000-0000-0000-000000000003", Owner: "[email protected]"},
+			{UUID: "00000002-0000-0000-0000-000000000004", Owner: "[email protected]"},
+		},
+		listModelsMockByControllerName: map[string]func(context.Context) ([]base.UserModel, error){
+			"controller-1": func(ctx context.Context) ([]base.UserModel, error) {
+				return []base.UserModel{
+					{UUID: "00000002-0000-0000-0000-000000000001"},
+					{UUID: "00000002-0000-0000-0000-000000000002"},
+					{UUID: "00000002-0000-0000-0000-000000000004"},
+				}, nil
+			},
+			"controller-2": func(ctx context.Context) ([]base.UserModel, error) {
+				return []base.UserModel{
+					{UUID: "00000002-0000-0000-0000-000000000003"},
+				}, nil
+			},
+		},
+	},
+	{
+		name:               "Alice lists models across controllers 1 and 2",
+		env:                listModelsTestEnv,
+		username:           "[email protected]",
+		expectedUserModels: []base.UserModel{},
+		expectedError:      "failed to list models",
+		listModelsMockByControllerName: map[string]func(context.Context) ([]base.UserModel, error){
+			"controller-1": func(ctx context.Context) ([]base.UserModel, error) {
+				return []base.UserModel{}, errors.E("test error")
+			},
+		},
+	},
+}
+
+func TestListModels(t *testing.T) {
+	c := qt.New(t)
+
+	for _, test := range modelListTests {
+		c.Run(
+			test.name,
+			func(c *qt.C) {
+				j := jimmtest.NewJIMM(c, &jimm.Parameters{
+					Dialer: jimmtest.DialerMap{
+						"controller-1": &jimmtest.Dialer{
+							API: &jimmtest.API{
+								ListModels_: test.listModelsMockByControllerName["controller-1"],
+							},
+						},
+						"controller-2": &jimmtest.Dialer{
+							API: &jimmtest.API{
+								ListModels_: test.listModelsMockByControllerName["controller-2"],
+							},
+						},
+					},
+				})
+
+				env := jimmtest.ParseEnvironment(c, test.env)
+				env.PopulateDBAndPermissions(c, j.ResourceTag(), j.Database, j.OpenFGAClient)
+
+				dbUser, err := dbmodel.NewIdentity(test.username)
+				c.Assert(err, qt.IsNil)
+				user := openfga.NewUser(dbUser, j.OpenFGAClient)
+
+				models, err := j.ListModels(context.Background(), user)
+				if test.expectedError != "" {
+					c.Assert(err, qt.ErrorMatches, test.expectedError)
+				} else {
+					c.Assert(models, qt.ContentEquals, test.expectedUserModels)
+				}
+			},
+		)
+	}
+}
+
 func newBool(b bool) *bool {
 	return &b
 }

internal/jujuapi/controllerroot.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/go-macaroon-bakery/macaroon-bakery/v3/bakery"
+	"github.com/juju/juju/api/base"
 	jujuparams "github.com/juju/juju/rpc/params"
 	"github.com/juju/names/v5"
 	"github.com/rogpeppe/fastuuid"
@@ -84,6 +85,7 @@ type JIMM interface {
 	UpdateCloud(ctx context.Context, u *openfga.User, ct names.CloudTag, cloud jujuparams.Cloud) error
 	UpdateCloudCredential(ctx context.Context, u *openfga.User, args jimm.UpdateCloudCredentialArgs) ([]jujuparams.UpdateCredentialModelResult, error)
 	UserLogin(ctx context.Context, identityName string) (*openfga.User, error)
+	ListModels(ctx context.Context, user *openfga.User) ([]base.UserModel, error)
 }
 
 // controllerRoot is the root for endpoints served on controller connections.

internal/jujuapi/modelmanager.go

@@ -124,7 +124,34 @@ func (r *controllerRoot) ListModelSummaries(ctx context.Context, _ jujuparams.Mo
 // ListModels returns the models that the authenticated user
 // has access to. The user parameter is ignored.
 func (r *controllerRoot) ListModels(ctx context.Context, _ jujuparams.Entity) (jujuparams.UserModelList, error) {
-	return r.allModels(ctx)
+	const op = errors.Op("jujuapi.ListModels")
+	zapctx.Info(ctx, string(op))
+
+	res := jujuparams.UserModelList{}
+
+	models, err := r.jimm.ListModels(ctx, r.user)
+	if err != nil {
+		return res, err
+	}
+
+	for _, m := range models {
+		if !names.IsValidUser(m.Owner) {
+			zapctx.Error(ctx, fmt.Sprintf("%s is not a valid user", m.Owner))
+		}
+		ownerTag := names.NewUserTag(m.Owner)
+
+		res.UserModels = append(res.UserModels, jujuparams.UserModel{
+			Model: jujuparams.Model{
+				Name:     m.Name,
+				UUID:     m.UUID,
+				Type:     string(m.Type),
+				OwnerTag: ownerTag.String(),
+			},
+			LastConnection: m.LastConnection,
+		})
+	}
+
+	return res, nil
 }
 
 // ModelInfo implements the ModelManager facade's ModelInfo method.