From 647aa5496d9efa4e557ba037f4967a3bb038c7b2 Mon Sep 17 00:00:00 2001 From: ale8k Date: Wed, 11 Dec 2024 13:39:47 +0000 Subject: [PATCH 01/11] test(modelmanager): k8s tests fix The api was trying to call /version, but our mock didn't implement it. --- internal/jujuapi/modelmanager_test.go | 11 +------- internal/testutils/kubetest/kubetest.go | 35 ++++++++++++++++--------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/internal/jujuapi/modelmanager_test.go b/internal/jujuapi/modelmanager_test.go index c65d09a6a..a0b4ba367 100644 --- a/internal/jujuapi/modelmanager_test.go +++ b/internal/jujuapi/modelmanager_test.go @@ -1374,15 +1374,12 @@ func (s *caasModelManagerSuite) SetUpTest(c *gc.C) { } func (s *caasModelManagerSuite) TestCreateModelKubernetes(c *gc.C) { - // TODO (ashipika): remove skip when the issue is resolved - // Error message: enumerating features supported by environment: querying kubernetes API version: the server could not find the requested resource - c.Skip("k8s_issue") conn := s.open(c, nil, "bob") defer conn.Close() client := modelmanager.NewClient(conn) mi, err := client.CreateModel("k8s-model-1", "bob@canonical.com", "bob-cloud", "", s.cred, nil) - c.Assert(err, gc.Equals, nil) + c.Assert(err, gc.Equals, nil) // ERROR IS HERE c.Assert(mi.Name, gc.Equals, "k8s-model-1") c.Assert(mi.Type, gc.Equals, model.CAAS) @@ -1393,9 +1390,6 @@ func (s *caasModelManagerSuite) TestCreateModelKubernetes(c *gc.C) { } func (s *caasModelManagerSuite) TestListCAASModelSummaries(c *gc.C) { - // TODO (ashipika): remove skip when the issue is resolved - // Error message: enumerating features supported by environment: querying kubernetes API version: the server could not find the requested resource - c.Skip("k8s_issue") conn := s.open(c, nil, "bob") defer conn.Close() @@ -1489,9 +1483,6 @@ func (s *caasModelManagerSuite) TestListCAASModelSummaries(c *gc.C) { } func (s *caasModelManagerSuite) TestListCAASModels(c *gc.C) { - // TODO (ashipika): remove skip when the issue is resolved - // Error message: enumerating features supported by environment: querying kubernetes API version: the server could not find the requested resource - c.Skip("k8s_issue") conn := s.open(c, nil, "bob") defer conn.Close() diff --git a/internal/testutils/kubetest/kubetest.go b/internal/testutils/kubetest/kubetest.go index a57489f39..9e637d0a7 100644 --- a/internal/testutils/kubetest/kubetest.go +++ b/internal/testutils/kubetest/kubetest.go @@ -16,25 +16,34 @@ const ( Password = "test-kubernetes-password" ) -// NewFakeKubernetes creates a minimal kubernetes API server which -// response to just the API calls required by the tests. func NewFakeKubernetes(c *gc.C) *httptest.Server { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - if req.URL.Path != "/api/v1/namespaces" { - w.WriteHeader(http.StatusNotFound) - return - } - if req.Method != "POST" { - w.WriteHeader(http.StatusMethodNotAllowed) - return - } if username, password, ok := req.BasicAuth(); !ok || username != Username || password != Password { w.WriteHeader(http.StatusUnauthorized) return } - w.Header().Set("Content-Type", req.Header.Get("Content-Type")) - _, err := io.Copy(w, req.Body) - c.Assert(err, gc.IsNil) + + switch req.URL.Path { + case "/version": + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"major":"1","minor":"21","gitVersion":"v1.21.0"}`)) + case "/api/v1/namespaces": + if req.Method != "POST" { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + w.Header().Set("Content-Type", req.Header.Get("Content-Type")) + _, err := io.Copy(w, req.Body) + c.Assert(err, gc.IsNil) + case "/api": + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"versions":["v1"]}`)) + case "/apis": + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"groups":[{"name":"apps","versions":[{"groupVersion":"apps/v1","version":"v1"}]}]}`)) + default: + w.WriteHeader(http.StatusNotFound) + } })) return srv } From 3d8aa1bc13e8f89d99c3f77a7efcd5b46b640559 Mon Sep 17 00:00:00 2001 From: ale8k Date: Thu, 12 Dec 2024 13:54:44 +0000 Subject: [PATCH 02/11] feat(listmodels): enable listmodels within the jujuclient pkg --- internal/jujuclient/modelmanager.go | 13 ++++++++ internal/jujuclient/modelmanager_test.go | 39 ++++++++++++++++++++++++ internal/testutils/jimmtest/api.go | 8 +++++ 3 files changed, 60 insertions(+) diff --git a/internal/jujuclient/modelmanager.go b/internal/jujuclient/modelmanager.go index b19808df6..b97e9c733 100644 --- a/internal/jujuclient/modelmanager.go +++ b/internal/jujuclient/modelmanager.go @@ -7,6 +7,8 @@ import ( "time" jujuerrors "github.com/juju/errors" + "github.com/juju/juju/api/base" + "github.com/juju/juju/api/client/modelmanager" jujuparams "github.com/juju/juju/rpc/params" "github.com/juju/names/v5" @@ -325,3 +327,14 @@ func (c Connection) ChangeModelCredential(ctx context.Context, model names.Model } return out.OneError() } + +// ListModels returns the models that the specified user +// has access to in the current server. Only that controller owner +// can list models for any user (at this stage). Other users +// can only ask about their own models. +// +// In our wrapper, we ask for the owner. So expect ALL models from +// the controller. +func (c Connection) ListModels(ctx context.Context) ([]base.UserModel, error) { + return modelmanager.NewClient(&c).ListModels("admin") +} diff --git a/internal/jujuclient/modelmanager_test.go b/internal/jujuclient/modelmanager_test.go index 3d7c7d964..b3a379b94 100644 --- a/internal/jujuclient/modelmanager_test.go +++ b/internal/jujuclient/modelmanager_test.go @@ -8,6 +8,7 @@ import ( "time" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/juju/juju/api/base" "github.com/juju/juju/core/life" jujuparams "github.com/juju/juju/rpc/params" "github.com/juju/juju/state" @@ -288,3 +289,41 @@ func (s *modelmanagerSuite) TestChangeModelCredential(c *gc.C) { err = s.API.ChangeModelCredential(ctx, names.NewModelTag(info.UUID), ct) c.Assert(err, gc.Equals, nil) } + +func (s *modelmanagerSuite) TestListModels(c *gc.C) { + ctx := context.Background() + + var info jujuparams.ModelInfo + err := s.API.CreateModel(ctx, &jujuparams.ModelCreateArgs{ + Name: "test-model", + OwnerTag: names.NewUserTag("test-user@canonical.com").String(), + }, &info) + c.Assert(err, gc.Equals, nil) + + models, err := s.API.ListModels(ctx) + c.Assert(err, gc.IsNil) + c.Assert( + models, + jimmtest.CmpEquals( + cmpopts.IgnoreTypes( + &time.Time{}, + ), + ), + []base.UserModel{ + { + Name: "controller", + UUID: "deadbeef-0bad-400d-8000-4b1d0d06f00d", + Type: "iaas", + Owner: "admin", + LastConnection: nil, + }, + { + Name: "test-model", + UUID: info.UUID, + Type: "iaas", + Owner: "test-user@canonical.com", + LastConnection: nil, + }, + }, + ) +} diff --git a/internal/testutils/jimmtest/api.go b/internal/testutils/jimmtest/api.go index d2851027c..a88ef3e50 100644 --- a/internal/testutils/jimmtest/api.go +++ b/internal/testutils/jimmtest/api.go @@ -165,6 +165,7 @@ type API struct { ListFilesystems_ func(ctx context.Context, machines []string) ([]jujuparams.FilesystemDetailsListResult, error) ListVolumes_ func(ctx context.Context, machines []string) ([]jujuparams.VolumeDetailsListResult, error) ListStorageDetails_ func(ctx context.Context) ([]jujuparams.StorageDetails, error) + ListModels_ func(ctx context.Context) ([]base.UserModel, error) } func (a *API) AddCloud(ctx context.Context, tag names.CloudTag, cld jujuparams.Cloud, force bool) error { @@ -466,4 +467,11 @@ func (a *API) ListStorageDetails(ctx context.Context) ([]jujuparams.StorageDetai return a.ListStorageDetails_(ctx) } +func (a *API) ListModels(ctx context.Context) ([]base.UserModel, error) { + if a.ListModels_ == nil { + return nil, errors.E(errors.CodeNotImplemented) + } + return a.ListModels_(ctx) +} + var _ jimm.API = &API{} From abc0676373ba907b9afca0584eed9fbc8e6bf20e Mon Sep 17 00:00:00 2001 From: ale8k Date: Thu, 12 Dec 2024 13:55:39 +0000 Subject: [PATCH 03/11] fix(listmodels): enable list models via controller We now list models via contacting controllers directly. --- internal/jimm/jimm.go | 9 ++ internal/jimm/model.go | 66 ++++++++ internal/jimm/model_test.go | 194 +++++++++++++++++++++++ internal/jujuapi/controllerroot.go | 2 + internal/jujuapi/modelmanager.go | 29 +++- internal/jujuapi/modelmanager_test.go | 189 +++++++++++----------- internal/testutils/jimmtest/jimm_mock.go | 8 + internal/testutils/kubetest/kubetest.go | 9 +- 8 files changed, 404 insertions(+), 102 deletions(-) diff --git a/internal/jimm/jimm.go b/internal/jimm/jimm.go index 07a4c12e3..99fab9b4c 100644 --- a/internal/jimm/jimm.go +++ b/internal/jimm/jimm.go @@ -494,6 +494,15 @@ type API interface { // ListStorageDetails lists all storage. ListStorageDetails(ctx context.Context) ([]jujuparams.StorageDetails, error) + + // ListModels returns the models that the specified user + // has access to in the current server. Only that controller owner + // can list models for any user (at this stage). Other users + // can only ask about their own models. + // + // In our wrapper, we ask for the owner. So expect ALL models from + // the controller. + ListModels(ctx context.Context) ([]base.UserModel, error) } // forEachController runs a given function on multiple controllers diff --git a/internal/jimm/model.go b/internal/jimm/model.go index e3c56e08d..fee0cd12d 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -7,11 +7,13 @@ import ( "database/sql" "fmt" "math/rand" + "slices" "sort" "strings" "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 +781,12 @@ func (j *JIMM) ModelSummaries(ctx context.Context, user *openfga.User, maskingCo 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 { @@ -829,6 +833,68 @@ func (j *JIMM) ModelSummaries(ctx context.Context, user *openfga.User, maskingCo }, nil } +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 models uuids user has access to + uuids, err := user.ListModels(ctx, ofganames.ReaderRelation) + if err != nil { + return nil, errors.E(op, err, "failed to list models") + } + + // Get the models themselves + models, err := j.DB().GetModelsByUUID(ctx, uuids) + if err != nil { + return nil, errors.E(op, err, "failed to get models by uuid") + } + + // Find the controllers these models reside on and remove duplicates + var controllers []dbmodel.Controller + seen := make(map[uint]bool) + for _, model := range models { + 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: We skip controller models as ListModels is used for login and register. + // The models returned are stored locally and used for reference. In the case of JIMM, + // we do not want to show the controller models. + for _, m := range ums { + if slices.Contains(uuids, m.UUID) { + userModels = append(userModels, m) + } + } + return nil + }) + if err != nil { + return nil, errors.E(op, err, "failed to list models") + } + + return userModels, nil +} + // mergeModelInfo replaces fields on the juju model info object with // information from JIMM where JIMM specific information should be used. func (j *JIMM) mergeModelInfo(ctx context.Context, user *openfga.User, modelInfo *jujuparams.ModelInfo, jimmModel dbmodel.Model) (*jujuparams.ModelInfo, error) { diff --git a/internal/jimm/model_test.go b/internal/jimm/model_test.go index cd33831bd..35fcdfac2 100644 --- a/internal/jimm/model_test.go +++ b/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: alice@canonical.com + 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: alice@canonical.com + life: alive + users: + - user: alice@canonical.com + access: admin + - user: bob@canonical.com + 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: alice@canonical.com + life: alive + users: + - user: alice@canonical.com + access: admin + - user: bob@canonical.com + 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: alice@canonical.com + life: alive + users: + - user: alice@canonical.com + access: admin + - user: bob@canonical.com + 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: alice@canonical.com + life: alive + users: + - user: alice@canonical.com + access: admin + +users: +- username: alice@canonical.com + 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: "bob@canonical.com", + expectedUserModels: []base.UserModel{ + {UUID: "00000002-0000-0000-0000-000000000001"}, + {UUID: "00000002-0000-0000-0000-000000000002"}, + {UUID: "00000002-0000-0000-0000-000000000003"}, + }, + 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: "alice@canonical.com", + expectedUserModels: []base.UserModel{ + {UUID: "00000002-0000-0000-0000-000000000001"}, + {UUID: "00000002-0000-0000-0000-000000000002"}, + {UUID: "00000002-0000-0000-0000-000000000003"}, + {UUID: "00000002-0000-0000-0000-000000000004"}, + }, + 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: "alice@canonical.com", + 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 } diff --git a/internal/jujuapi/controllerroot.go b/internal/jujuapi/controllerroot.go index acaf97daf..c0fe44465 100644 --- a/internal/jujuapi/controllerroot.go +++ 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. diff --git a/internal/jujuapi/modelmanager.go b/internal/jujuapi/modelmanager.go index 65b0ea68e..91f5d9cd8 100644 --- a/internal/jujuapi/modelmanager.go +++ b/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. diff --git a/internal/jujuapi/modelmanager_test.go b/internal/jujuapi/modelmanager_test.go index 989e7db7b..dfecdcdc8 100644 --- a/internal/jujuapi/modelmanager_test.go +++ b/internal/jujuapi/modelmanager_test.go @@ -221,23 +221,35 @@ func (s *modelManagerSuite) TestListModelSummariesWithoutControllerUUIDMasking(c } func (s *modelManagerSuite) TestListModels(c *gc.C) { - conn := s.open(c, nil, "bob") + conn := s.open(c, nil, "charlie@canonical.com") defer conn.Close() client := modelmanager.NewClient(conn) - models, err := client.ListModels("bob") + models, err := client.ListModels("charlie@canonical.com") c.Assert(err, gc.Equals, nil) - c.Assert(models, jc.SameContents, []base.UserModel{{ - Name: "model-1", - UUID: s.Model.UUID.String, - Owner: "bob@canonical.com", - Type: "iaas", - }, { - Name: "model-3", - UUID: s.Model3.UUID.String, - Owner: "charlie@canonical.com", - Type: "iaas", - }}) + c.Assert( + models, + jimmtest.CmpEquals( + cmpopts.IgnoreTypes(&time.Time{}), + cmpopts.SortSlices(func(a, b base.UserModelSummary) bool { + return a.Name < b.Name + }), + ), + []base.UserModel{ + { + Name: "model-2", + UUID: s.Model2.UUID.String, + Owner: "charlie@canonical.com", + Type: "iaas", + }, { + Name: "model-3", + UUID: s.Model3.UUID.String, + Owner: "charlie@canonical.com", + Type: "iaas", + }, + }, + ) + } func (s *modelManagerSuite) TestModelInfo(c *gc.C) { @@ -1364,7 +1376,7 @@ func (s *caasModelManagerSuite) TestCreateModelKubernetes(c *gc.C) { client := modelmanager.NewClient(conn) mi, err := client.CreateModel("k8s-model-1", "bob@canonical.com", "bob-cloud", "", s.cred, nil) - c.Assert(err, gc.Equals, nil) // ERROR IS HERE + c.Assert(err, gc.Equals, nil) c.Assert(mi.Name, gc.Equals, "k8s-model-1") c.Assert(mi.Type, gc.Equals, model.CAAS) @@ -1384,87 +1396,56 @@ func (s *caasModelManagerSuite) TestListCAASModelSummaries(c *gc.C) { models, err := client.ListModelSummaries("bob", false) c.Assert(err, gc.Equals, nil) - c.Assert(models, jimmtest.CmpEquals( - cmpopts.IgnoreTypes(&time.Time{}), - cmpopts.SortSlices(func(a, b base.UserModelSummary) bool { - return a.Name < b.Name - }), - ), []base.UserModelSummary{{ + + var caasMS *base.UserModelSummary + for _, m := range models { + if m.Name == "k8s-model-1" { + caasMS = &m + } + } + if caasMS == nil { + c.Fail() + } + expectedCaas := &base.UserModelSummary{ Name: "k8s-model-1", UUID: mi.UUID, + Type: "caas", ControllerUUID: jimmtest.ControllerUUID, + IsController: false, ProviderType: "kubernetes", DefaultSeries: "jammy", Cloud: "bob-cloud", CloudRegion: "default", - CloudCredential: s.cred.Id(), - Owner: "bob@canonical.com", - Life: life.Value(state.Alive.String()), - Status: base.Status{ - Status: status.Available, - Data: map[string]interface{}{}, - }, - ModelUserAccess: "admin", - Counts: []base.EntityCount{{ - Entity: "machines", - Count: 0, - }, { - Entity: "cores", - Count: 0, - }, { - Entity: "units", - Count: 0, - }}, - AgentVersion: &jujuversion.Current, - Type: "caas", - SLA: &base.SLASummary{ - Level: "unsupported", - }, - }, { - Name: "model-1", - UUID: s.Model.UUID.String, - Type: "iaas", - ControllerUUID: jimmtest.ControllerUUID, - ProviderType: jimmtest.TestProviderType, - DefaultSeries: "jammy", - Cloud: jimmtest.TestCloudName, - CloudRegion: jimmtest.TestCloudRegionName, - CloudCredential: jimmtest.TestCloudName + "/bob@canonical.com/cred", + CloudCredential: "bob-cloud/bob@canonical.com/k8s", Owner: "bob@canonical.com", - Life: life.Value(state.Alive.String()), + Life: "alive", Status: base.Status{ - Status: status.Available, + Status: "available", + Info: "", Data: map[string]interface{}{}, + Since: nil, }, - ModelUserAccess: "admin", - Counts: []base.EntityCount{{Entity: "machines"}, {Entity: "cores"}, {Entity: "units"}}, - AgentVersion: &jujuversion.Current, + ModelUserAccess: "admin", + UserLastConnection: nil, + Counts: []base.EntityCount{}, + AgentVersion: &jujuversion.Current, + Error: nil, + Migration: nil, SLA: &base.SLASummary{ - Level: "unsupported", - }, - }, { - Name: "model-3", - UUID: s.Model3.UUID.String, - Type: "iaas", - ControllerUUID: jimmtest.ControllerUUID, - ProviderType: jimmtest.TestProviderType, - DefaultSeries: "jammy", - Cloud: jimmtest.TestCloudName, - CloudRegion: jimmtest.TestCloudRegionName, - CloudCredential: jimmtest.TestCloudName + "/charlie@canonical.com/cred", - Owner: "charlie@canonical.com", - Life: life.Value(state.Alive.String()), - Status: base.Status{ - Status: status.Available, - Data: map[string]interface{}{}, - }, - ModelUserAccess: "read", - Counts: []base.EntityCount{{Entity: "machines"}, {Entity: "cores"}, {Entity: "units"}}, - AgentVersion: &jujuversion.Current, - SLA: &base.SLASummary{ - Level: "unsupported", + Level: "", + Owner: "bob@canonical.com", }, - }}) + } + c.Assert( + caasMS, + jimmtest.CmpEquals( + cmpopts.IgnoreTypes( + &time.Time{}, + &base.MigrationSummary{}, + ), + ), + expectedCaas, + ) } func (s *caasModelManagerSuite) TestListCAASModels(c *gc.C) { @@ -1477,22 +1458,34 @@ func (s *caasModelManagerSuite) TestListCAASModels(c *gc.C) { models, err := client.ListModels("bob") c.Assert(err, gc.Equals, nil) - c.Assert(models, jc.SameContents, []base.UserModel{{ - Name: "k8s-model-1", - UUID: mi.UUID, - Owner: "bob@canonical.com", - Type: "caas", - }, { - Name: "model-1", - UUID: s.Model.UUID.String, - Owner: "bob@canonical.com", - Type: "iaas", - }, { - Name: "model-3", - UUID: s.Model3.UUID.String, - Owner: "charlie@canonical.com", - Type: "iaas", - }}) + sort.Slice(models, func(i, j int) bool { return i < j }) + + c.Assert( + models, + jimmtest.CmpEquals( + cmpopts.IgnoreTypes( + &time.Time{}, + ), + ), + []base.UserModel{ + { + Name: "k8s-model-1", + UUID: mi.UUID, + Owner: "bob@canonical.com", + Type: "caas", + }, { + Name: "model-1", + UUID: s.Model.UUID.String, + Owner: "bob@canonical.com", + Type: "iaas", + }, { + Name: "model-3", + UUID: s.Model3.UUID.String, + Owner: "charlie@canonical.com", + Type: "iaas", + }, + }, + ) } func assertModelInfo(c *gc.C, obtained, expected []jujuparams.ModelInfoResult) { diff --git a/internal/testutils/jimmtest/jimm_mock.go b/internal/testutils/jimmtest/jimm_mock.go index e59968d3d..7d264085f 100644 --- a/internal/testutils/jimmtest/jimm_mock.go +++ b/internal/testutils/jimmtest/jimm_mock.go @@ -8,6 +8,7 @@ import ( "github.com/go-macaroon-bakery/macaroon-bakery/v3/bakery" "github.com/google/uuid" + "github.com/juju/juju/api/base" jujuparams "github.com/juju/juju/rpc/params" "github.com/juju/names/v5" @@ -87,6 +88,7 @@ type JIMM struct { UpdateCloud_ func(ctx context.Context, u *openfga.User, ct names.CloudTag, cloud jujuparams.Cloud) error UpdateCloudCredential_ func(ctx context.Context, u *openfga.User, args jimm.UpdateCloudCredentialArgs) ([]jujuparams.UpdateCredentialModelResult, error) UserLogin_ func(ctx context.Context, identityName string) (*openfga.User, error) + ListModels_ func(ctx context.Context, user *openfga.User) ([]base.UserModel, error) } func (j *JIMM) AddAuditLogEntry(ale *dbmodel.AuditLogEntry) { @@ -426,3 +428,9 @@ func (j *JIMM) UserLogin(ctx context.Context, identityName string) (*openfga.Use } return j.UserLogin_(ctx, identityName) } +func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserModel, error) { + if j.ListModels_ == nil { + return nil, errors.E(errors.CodeNotImplemented) + } + return j.ListModels_(ctx, user) +} diff --git a/internal/testutils/kubetest/kubetest.go b/internal/testutils/kubetest/kubetest.go index 9e637d0a7..c6901b17e 100644 --- a/internal/testutils/kubetest/kubetest.go +++ b/internal/testutils/kubetest/kubetest.go @@ -26,7 +26,8 @@ func NewFakeKubernetes(c *gc.C) *httptest.Server { switch req.URL.Path { case "/version": w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"major":"1","minor":"21","gitVersion":"v1.21.0"}`)) + _, err := w.Write([]byte(`{"major":"1","minor":"21","gitVersion":"v1.21.0"}`)) + c.Assert(err, gc.IsNil) case "/api/v1/namespaces": if req.Method != "POST" { w.WriteHeader(http.StatusMethodNotAllowed) @@ -37,10 +38,12 @@ func NewFakeKubernetes(c *gc.C) *httptest.Server { c.Assert(err, gc.IsNil) case "/api": w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"versions":["v1"]}`)) + _, err := w.Write([]byte(`{"versions":["v1"]}`)) + c.Assert(err, gc.IsNil) case "/apis": w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"groups":[{"name":"apps","versions":[{"groupVersion":"apps/v1","version":"v1"}]}]}`)) + _, err := w.Write([]byte(`{"groups":[{"name":"apps","versions":[{"groupVersion":"apps/v1","version":"v1"}]}]}`)) + c.Assert(err, gc.IsNil) default: w.WriteHeader(http.StatusNotFound) } From ddd07f30ab136e56da9fef3ed2188b8ddd2b7a96 Mon Sep 17 00:00:00 2001 From: ale8k Date: Thu, 12 Dec 2024 13:59:55 +0000 Subject: [PATCH 04/11] docs(listmodels): update jimm pkg listmodels godoc --- internal/jimm/model.go | 126 +++++++++++++++++++++-------------------- 1 file changed, 64 insertions(+), 62 deletions(-) diff --git a/internal/jimm/model.go b/internal/jimm/model.go index f79ccb34b..ab7d6381c 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -833,68 +833,6 @@ func (j *JIMM) ListModelSummaries(ctx context.Context, user *openfga.User, maski }, nil } -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 models uuids user has access to - uuids, err := user.ListModels(ctx, ofganames.ReaderRelation) - if err != nil { - return nil, errors.E(op, err, "failed to list models") - } - - // Get the models themselves - models, err := j.DB().GetModelsByUUID(ctx, uuids) - if err != nil { - return nil, errors.E(op, err, "failed to get models by uuid") - } - - // Find the controllers these models reside on and remove duplicates - var controllers []dbmodel.Controller - seen := make(map[uint]bool) - for _, model := range models { - 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: We skip controller models as ListModels is used for login and register. - // The models returned are stored locally and used for reference. In the case of JIMM, - // we do not want to show the controller models. - for _, m := range ums { - if slices.Contains(uuids, m.UUID) { - userModels = append(userModels, m) - } - } - return nil - }) - if err != nil { - return nil, errors.E(op, err, "failed to list models") - } - - return userModels, nil -} - // mergeModelInfo replaces fields on the juju model info object with // information from JIMM where JIMM specific information should be used. func (j *JIMM) mergeModelInfo(ctx context.Context, user *openfga.User, modelInfo *jujuparams.ModelInfo, jimmModel dbmodel.Model) (*jujuparams.ModelInfo, error) { @@ -1413,3 +1351,67 @@ 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 models uuids user has access to + uuids, err := user.ListModels(ctx, ofganames.ReaderRelation) + if err != nil { + return nil, errors.E(op, err, "failed to list models") + } + + // Get the models themselves + models, err := j.DB().GetModelsByUUID(ctx, uuids) + if err != nil { + return nil, errors.E(op, err, "failed to get models by uuid") + } + + // Find the controllers these models reside on and remove duplicates + var controllers []dbmodel.Controller + seen := make(map[uint]bool) + for _, model := range models { + 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: We skip controller models as ListModels is used for login and register. + // The models returned are stored locally and used for reference. In the case of JIMM, + // we do not want to show the controller models. + for _, m := range ums { + if slices.Contains(uuids, m.UUID) { + userModels = append(userModels, m) + } + } + return nil + }) + if err != nil { + return nil, errors.E(op, err, "failed to list models") + } + + return userModels, nil +} From ade1505d4bee0585d7053b443b4b194073421611 Mon Sep 17 00:00:00 2001 From: ale8k Date: Fri, 13 Dec 2024 08:53:53 +0000 Subject: [PATCH 05/11] refactor(pr): pr comments --- internal/jimm/jimm.go | 2 +- internal/jimm/model.go | 6 +++--- internal/jujuclient/modelmanager.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/jimm/jimm.go b/internal/jimm/jimm.go index 99fab9b4c..7690d78c8 100644 --- a/internal/jimm/jimm.go +++ b/internal/jimm/jimm.go @@ -500,7 +500,7 @@ type API interface { // can list models for any user (at this stage). Other users // can only ask about their own models. // - // In our wrapper, we ask for the owner. So expect ALL models from + // In our wrapper, we ask as the controller admin. So expect ALL models from // the controller. ListModels(ctx context.Context) ([]base.UserModel, error) } diff --git a/internal/jimm/model.go b/internal/jimm/model.go index ab7d6381c..1b43da5a7 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1358,13 +1358,13 @@ func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserM const op = errors.Op("jimm.ListModels") zapctx.Info(ctx, string(op)) - // Get models uuids user has access to + // 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 models") + return nil, errors.E(op, err, "failed to list user models") } - // Get the models themselves + // 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") diff --git a/internal/jujuclient/modelmanager.go b/internal/jujuclient/modelmanager.go index b97e9c733..3b4cbee75 100644 --- a/internal/jujuclient/modelmanager.go +++ b/internal/jujuclient/modelmanager.go @@ -333,7 +333,7 @@ func (c Connection) ChangeModelCredential(ctx context.Context, model names.Model // can list models for any user (at this stage). Other users // can only ask about their own models. // -// In our wrapper, we ask for the owner. So expect ALL models from +// In our wrapper, we ask as the controller admin. So expect ALL models from // the controller. func (c Connection) ListModels(ctx context.Context) ([]base.UserModel, error) { return modelmanager.NewClient(&c).ListModels("admin") From 0141ef3fed599954f454317a0701c871123630bb Mon Sep 17 00:00:00 2001 From: ale8k Date: Fri, 13 Dec 2024 10:21:12 +0000 Subject: [PATCH 06/11] chore: pr comments --- internal/db/model.go | 3 +++ internal/db/model_test.go | 1 + internal/jimm/jimm.go | 6 ++---- internal/jimm/model.go | 15 ++++++++++++--- internal/jimm/model_test.go | 14 +++++++------- internal/jujuclient/modelmanager.go | 6 ++---- 6 files changed, 27 insertions(+), 18 deletions(-) diff --git a/internal/db/model.go b/internal/db/model.go index f998ccb48..b78fda687 100644 --- a/internal/db/model.go +++ b/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") diff --git a/internal/db/model_test.go b/internal/db/model_test.go index f80816bb4..4b3a1f8b9 100644 --- a/internal/db/model_test.go +++ b/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") diff --git a/internal/jimm/jimm.go b/internal/jimm/jimm.go index 7690d78c8..75cfa2abf 100644 --- a/internal/jimm/jimm.go +++ b/internal/jimm/jimm.go @@ -495,10 +495,8 @@ type API interface { // ListStorageDetails lists all storage. ListStorageDetails(ctx context.Context) ([]jujuparams.StorageDetails, error) - // ListModels returns the models that the specified user - // has access to in the current server. Only that controller owner - // can list models for any user (at this stage). Other users - // can only ask about their own models. + // ListModels returns UserModel's for the user that is logged in. If the user logged + // in is "admin" they may specify another user's models. // // In our wrapper, we ask as the controller admin. So expect ALL models from // the controller. diff --git a/internal/jimm/model.go b/internal/jimm/model.go index 1b43da5a7..21cb7204c 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1402,9 +1402,18 @@ func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserM // NOTE: We skip controller models as ListModels is used for login and register. // The models returned are stored locally and used for reference. In the case of JIMM, // we do not want to show the controller models. - for _, m := range ums { - if slices.Contains(uuids, m.UUID) { - userModels = append(userModels, m) + for _, um := range ums { + // Filter models that match authorised uuids list + if slices.Contains(uuids, um.UUID) { + // Find that model in the db models + index := slices.IndexFunc(models, func(m dbmodel.Model) bool { + return m.UUID.String == um.UUID + }) + if index != -1 { + // Override owner and append to result + um.Owner = models[index].OwnerIdentityName + userModels = append(userModels, um) + } } } return nil diff --git a/internal/jimm/model_test.go b/internal/jimm/model_test.go index 33f2fe2f3..4b70e4855 100644 --- a/internal/jimm/model_test.go +++ b/internal/jimm/model_test.go @@ -3901,9 +3901,9 @@ var modelListTests = []struct { env: listModelsTestEnv, username: "bob@canonical.com", expectedUserModels: []base.UserModel{ - {UUID: "00000002-0000-0000-0000-000000000001"}, - {UUID: "00000002-0000-0000-0000-000000000002"}, - {UUID: "00000002-0000-0000-0000-000000000003"}, + {UUID: "00000002-0000-0000-0000-000000000001", Owner: "alice@canonical.com"}, + {UUID: "00000002-0000-0000-0000-000000000002", Owner: "alice@canonical.com"}, + {UUID: "00000002-0000-0000-0000-000000000003", Owner: "alice@canonical.com"}, }, listModelsMockByControllerName: map[string]func(context.Context) ([]base.UserModel, error){ "controller-1": func(ctx context.Context) ([]base.UserModel, error) { @@ -3924,10 +3924,10 @@ var modelListTests = []struct { env: listModelsTestEnv, username: "alice@canonical.com", expectedUserModels: []base.UserModel{ - {UUID: "00000002-0000-0000-0000-000000000001"}, - {UUID: "00000002-0000-0000-0000-000000000002"}, - {UUID: "00000002-0000-0000-0000-000000000003"}, - {UUID: "00000002-0000-0000-0000-000000000004"}, + {UUID: "00000002-0000-0000-0000-000000000001", Owner: "alice@canonical.com"}, + {UUID: "00000002-0000-0000-0000-000000000002", Owner: "alice@canonical.com"}, + {UUID: "00000002-0000-0000-0000-000000000003", Owner: "alice@canonical.com"}, + {UUID: "00000002-0000-0000-0000-000000000004", Owner: "alice@canonical.com"}, }, listModelsMockByControllerName: map[string]func(context.Context) ([]base.UserModel, error){ "controller-1": func(ctx context.Context) ([]base.UserModel, error) { diff --git a/internal/jujuclient/modelmanager.go b/internal/jujuclient/modelmanager.go index 3b4cbee75..26acf5cde 100644 --- a/internal/jujuclient/modelmanager.go +++ b/internal/jujuclient/modelmanager.go @@ -328,10 +328,8 @@ func (c Connection) ChangeModelCredential(ctx context.Context, model names.Model return out.OneError() } -// ListModels returns the models that the specified user -// has access to in the current server. Only that controller owner -// can list models for any user (at this stage). Other users -// can only ask about their own models. +// ListModels returns UserModel's for the user that is logged in. If the user logged +// in is "admin" they may specify another user's models. // // In our wrapper, we ask as the controller admin. So expect ALL models from // the controller. From 2ad39e0894a32230fca4c22147fdf2b8357eb78a Mon Sep 17 00:00:00 2001 From: ale8k Date: Fri, 13 Dec 2024 10:28:20 +0000 Subject: [PATCH 07/11] chore: pr comments --- internal/jimm/jimm.go | 6 +----- internal/jimm/model.go | 7 ++++--- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/internal/jimm/jimm.go b/internal/jimm/jimm.go index 75cfa2abf..fdadd7977 100644 --- a/internal/jimm/jimm.go +++ b/internal/jimm/jimm.go @@ -495,11 +495,7 @@ type API interface { // ListStorageDetails lists all storage. ListStorageDetails(ctx context.Context) ([]jujuparams.StorageDetails, error) - // ListModels returns UserModel's for the user that is logged in. If the user logged - // in is "admin" they may specify another user's models. - // - // In our wrapper, we ask as the controller admin. So expect ALL models from - // the controller. + // ListModels returns all UserModel's on the controller. ListModels(ctx context.Context) ([]base.UserModel, error) } diff --git a/internal/jimm/model.go b/internal/jimm/model.go index 21cb7204c..e706c6f99 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1399,9 +1399,10 @@ func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserM // Filter the models returned according to the uuids // returned from OpenFGA for read access. // - // NOTE: We skip controller models as ListModels is used for login and register. - // The models returned are stored locally and used for reference. In the case of JIMM, - // we do not want to show the controller models. + // NOTE: The controller models are skipped because we do not relate users + // to controller models, we skip the controller models as ListModels is used for + // login and register - the models returned are stored locally and used for reference. + // In the case of JIMM, we do not want to show the controller models. for _, um := range ums { // Filter models that match authorised uuids list if slices.Contains(uuids, um.UUID) { From 69d2604b8d8f718567c8844b9d9a622df938fdbb Mon Sep 17 00:00:00 2001 From: ale8k Date: Fri, 13 Dec 2024 10:41:58 +0000 Subject: [PATCH 08/11] chore: pr comments --- internal/jimm/model.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/internal/jimm/model.go b/internal/jimm/model.go index e706c6f99..04c9a7dba 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1370,6 +1370,12 @@ func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserM return nil, errors.E(op, err, "failed to get models by uuid") } + // Create map for lookup later + modelsMap := make(map[string]dbmodel.Model) + for _, m := range models { + modelsMap[m.UUID.String] = m + } + // Find the controllers these models reside on and remove duplicates var controllers []dbmodel.Controller seen := make(map[uint]bool) @@ -1406,15 +1412,8 @@ func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserM for _, um := range ums { // Filter models that match authorised uuids list if slices.Contains(uuids, um.UUID) { - // Find that model in the db models - index := slices.IndexFunc(models, func(m dbmodel.Model) bool { - return m.UUID.String == um.UUID - }) - if index != -1 { - // Override owner and append to result - um.Owner = models[index].OwnerIdentityName - userModels = append(userModels, um) - } + um.Owner = modelsMap[um.UUID].OwnerIdentityName + userModels = append(userModels, um) } } return nil From e829e1a1715ed43767da4dcc96bfdfe1b1f21b05 Mon Sep 17 00:00:00 2001 From: ale8k Date: Fri, 13 Dec 2024 10:47:35 +0000 Subject: [PATCH 09/11] chore: pr comment --- internal/jimm/model.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/internal/jimm/model.go b/internal/jimm/model.go index 04c9a7dba..9648ba629 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1372,14 +1372,11 @@ func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserM // Create map for lookup later modelsMap := make(map[string]dbmodel.Model) - for _, m := range models { - modelsMap[m.UUID.String] = m - } - // 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 } From 614f63ac577b1051d7a379c4749a4388613f8acc Mon Sep 17 00:00:00 2001 From: ale8k Date: Fri, 13 Dec 2024 10:57:39 +0000 Subject: [PATCH 10/11] chore: pr comments --- internal/jimm/model.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/internal/jimm/model.go b/internal/jimm/model.go index 9648ba629..41b0df16e 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1402,14 +1402,17 @@ func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserM // Filter the models returned according to the uuids // returned from OpenFGA for read access. // - // NOTE: The controller models are skipped because we do not relate users - // to controller models, we skip the controller models as ListModels is used for - // login and register - the models returned are stored locally and used for reference. - // In the case of JIMM, we do not want to show the controller models. + // 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 { // Filter models that match authorised uuids list if slices.Contains(uuids, um.UUID) { - um.Owner = modelsMap[um.UUID].OwnerIdentityName + mapModel, ok := modelsMap[um.UUID] + if !ok { + continue + } + um.Owner = mapModel.OwnerIdentityName userModels = append(userModels, um) } } From 62437ccad4329571228d04ec5c0a687a30a82bbc Mon Sep 17 00:00:00 2001 From: ale8k Date: Fri, 13 Dec 2024 10:59:33 +0000 Subject: [PATCH 11/11] chore: pr comment --- internal/jimm/model.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/internal/jimm/model.go b/internal/jimm/model.go index 41b0df16e..8fd46257b 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -7,7 +7,6 @@ import ( "database/sql" "fmt" "math/rand" - "slices" "sort" "strings" "sync" @@ -1406,15 +1405,12 @@ func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserM // controller models to users, and as such, they will not appear in the // authorised uuid map. for _, um := range ums { - // Filter models that match authorised uuids list - if slices.Contains(uuids, um.UUID) { - mapModel, ok := modelsMap[um.UUID] - if !ok { - continue - } - um.Owner = mapModel.OwnerIdentityName - userModels = append(userModels, um) + mapModel, ok := modelsMap[um.UUID] + if !ok { + continue } + um.Owner = mapModel.OwnerIdentityName + userModels = append(userModels, um) } return nil })