From 6fa9787672d0729ec2e9642cfe2879f073b245d1 Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Mon, 16 Dec 2024 14:40:40 +0100 Subject: [PATCH 1/5] add test for group deletion (#1497) * add test for group hard deletion --- internal/dbmodel/group_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/internal/dbmodel/group_test.go b/internal/dbmodel/group_test.go index e9b12b849..6d3725f1e 100644 --- a/internal/dbmodel/group_test.go +++ b/internal/dbmodel/group_test.go @@ -36,3 +36,25 @@ func TestGroupEntry(t *testing.T) { c.Assert(result.Error, qt.IsNil) c.Assert(ge3, qt.DeepEquals, ge) } + +// TestHardDeleteGroupEntry tests hard delete of groups, to make sure we can create a group with the same name after deleting it. +func TestHardDeleteGroupEntry(t *testing.T) { + c := qt.New(t) + db := gormDB(t) + + ge := dbmodel.GroupEntry{ + Name: "test-group-1", + } + c.Assert(db.Create(&ge).Error, qt.IsNil) + c.Assert(ge.ID, qt.Equals, uint(1)) + + c.Assert(db.Delete(ge).Error, qt.IsNil) + + result := db.First(&ge) + c.Assert(result.Error, qt.ErrorMatches, "record not found") + + ge1 := dbmodel.GroupEntry{ + Name: "test-group-1", + } + c.Assert(db.Create(&ge1).Error, qt.IsNil) +} From 10c5461d86b5cdfdcc5f455a314ec6d7d47c789b Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Tue, 17 Dec 2024 14:48:40 +0100 Subject: [PATCH 2/5] move jimm interface to a separate file (#1499) --- internal/jujuapi/controllerroot.go | 70 ------------------------- internal/jujuapi/interface.go | 84 ++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 70 deletions(-) create mode 100644 internal/jujuapi/interface.go diff --git a/internal/jujuapi/controllerroot.go b/internal/jujuapi/controllerroot.go index c0fe44465..778e6d85d 100644 --- a/internal/jujuapi/controllerroot.go +++ b/internal/jujuapi/controllerroot.go @@ -6,88 +6,18 @@ import ( "context" "fmt" "sync" - "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" "golang.org/x/oauth2" - "github.com/canonical/jimm/v3/internal/common/pagination" - "github.com/canonical/jimm/v3/internal/db" - "github.com/canonical/jimm/v3/internal/dbmodel" "github.com/canonical/jimm/v3/internal/errors" "github.com/canonical/jimm/v3/internal/jimm" - "github.com/canonical/jimm/v3/internal/jimm/credentials" "github.com/canonical/jimm/v3/internal/jujuapi/rpc" "github.com/canonical/jimm/v3/internal/openfga" - ofganames "github.com/canonical/jimm/v3/internal/openfga/names" - "github.com/canonical/jimm/v3/internal/pubsub" jimmnames "github.com/canonical/jimm/v3/pkg/names" ) -type JIMM interface { - RelationService - ControllerService - LoginService - ModelManager - AddAuditLogEntry(ale *dbmodel.AuditLogEntry) - AddCloudToController(ctx context.Context, user *openfga.User, controllerName string, tag names.CloudTag, cloud jujuparams.Cloud, force bool) error - AddHostedCloud(ctx context.Context, user *openfga.User, tag names.CloudTag, cloud jujuparams.Cloud, force bool) error - AddServiceAccount(ctx context.Context, u *openfga.User, clientId string) error - CopyServiceAccountCredential(ctx context.Context, u *openfga.User, svcAcc *openfga.User, cloudCredentialTag names.CloudCredentialTag) (names.CloudCredentialTag, []jujuparams.UpdateCredentialModelResult, error) - CountIdentities(ctx context.Context, user *openfga.User) (int, error) - DestroyOffer(ctx context.Context, user *openfga.User, offerURL string, force bool) error - FindApplicationOffers(ctx context.Context, user *openfga.User, filters ...jujuparams.OfferFilter) ([]jujuparams.ApplicationOfferAdminDetailsV5, error) - FindAuditEvents(ctx context.Context, user *openfga.User, filter db.AuditLogFilter) ([]dbmodel.AuditLogEntry, error) - ForEachCloud(ctx context.Context, user *openfga.User, f func(*dbmodel.Cloud) error) error - ForEachUserCloud(ctx context.Context, user *openfga.User, f func(*dbmodel.Cloud) error) error - ForEachUserCloudCredential(ctx context.Context, u *dbmodel.Identity, ct names.CloudTag, f func(cred *dbmodel.CloudCredential) error) error - GetApplicationOffer(ctx context.Context, user *openfga.User, offerURL string) (*jujuparams.ApplicationOfferAdminDetailsV5, error) - GetApplicationOfferConsumeDetails(ctx context.Context, user *openfga.User, details *jujuparams.ConsumeOfferDetails, v bakery.Version) error - GetCloud(ctx context.Context, u *openfga.User, tag names.CloudTag) (dbmodel.Cloud, error) - GetCloudCredential(ctx context.Context, user *openfga.User, tag names.CloudCredentialTag) (*dbmodel.CloudCredential, error) - GetCloudCredentialAttributes(ctx context.Context, u *openfga.User, cred *dbmodel.CloudCredential, hidden bool) (attrs map[string]string, redacted []string, err error) - GetCredentialStore() credentials.CredentialStore - RoleManager() jimm.RoleManager - GroupManager() jimm.GroupManager - GetJimmControllerAccess(ctx context.Context, user *openfga.User, tag names.UserTag) (string, error) - // FetchIdentity finds the user in jimm or returns a not-found error - FetchIdentity(ctx context.Context, username string) (*openfga.User, error) - GetUserCloudAccess(ctx context.Context, user *openfga.User, cloud names.CloudTag) (string, error) - GetUserControllerAccess(ctx context.Context, user *openfga.User, controller names.ControllerTag) (string, error) - GetUserModelAccess(ctx context.Context, user *openfga.User, model names.ModelTag) (string, error) - GrantAuditLogAccess(ctx context.Context, user *openfga.User, targetUserTag names.UserTag) error - GrantCloudAccess(ctx context.Context, user *openfga.User, ct names.CloudTag, ut names.UserTag, access string) error - GrantModelAccess(ctx context.Context, user *openfga.User, mt names.ModelTag, ut names.UserTag, access jujuparams.UserAccessPermission) error - GrantOfferAccess(ctx context.Context, u *openfga.User, offerURL string, ut names.UserTag, access jujuparams.OfferAccessPermission) error - GrantServiceAccountAccess(ctx context.Context, u *openfga.User, svcAccTag jimmnames.ServiceAccountTag, tags []string) error - InitiateInternalMigration(ctx context.Context, user *openfga.User, modelTag names.ModelTag, targetController string) (jujuparams.InitiateMigrationResult, error) - InitiateMigration(ctx context.Context, user *openfga.User, spec jujuparams.MigrationSpec) (jujuparams.InitiateMigrationResult, error) - ListApplicationOffers(ctx context.Context, user *openfga.User, filters ...jujuparams.OfferFilter) ([]jujuparams.ApplicationOfferAdminDetailsV5, error) - ListIdentities(ctx context.Context, user *openfga.User, pagination pagination.LimitOffsetPagination, match string) ([]openfga.User, error) - ListResources(ctx context.Context, user *openfga.User, filter pagination.LimitOffsetPagination, namePrefixFilter, typeFilter string) ([]db.Resource, error) - Offer(ctx context.Context, user *openfga.User, offer jimm.AddApplicationOfferParams) error - PubSubHub() *pubsub.Hub - PurgeLogs(ctx context.Context, user *openfga.User, before time.Time) (int64, error) - RemoveCloud(ctx context.Context, u *openfga.User, ct names.CloudTag) error - RemoveCloudFromController(ctx context.Context, u *openfga.User, controllerName string, ct names.CloudTag) error - RemoveController(ctx context.Context, user *openfga.User, controllerName string, force bool) error - ResourceTag() names.ControllerTag - RevokeAuditLogAccess(ctx context.Context, user *openfga.User, targetUserTag names.UserTag) error - RevokeCloudAccess(ctx context.Context, user *openfga.User, ct names.CloudTag, ut names.UserTag, access string) error - RevokeCloudCredential(ctx context.Context, user *dbmodel.Identity, tag names.CloudCredentialTag, force bool) error - RevokeModelAccess(ctx context.Context, user *openfga.User, mt names.ModelTag, ut names.UserTag, access jujuparams.UserAccessPermission) error - RevokeOfferAccess(ctx context.Context, user *openfga.User, offerURL string, ut names.UserTag, access jujuparams.OfferAccessPermission) (err error) - ToJAASTag(ctx context.Context, tag *ofganames.Tag, resolveUUIDs bool) (string, error) - 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. type controllerRoot struct { rpc.Root diff --git a/internal/jujuapi/interface.go b/internal/jujuapi/interface.go new file mode 100644 index 000000000..1565abf9d --- /dev/null +++ b/internal/jujuapi/interface.go @@ -0,0 +1,84 @@ +// Copyright 2024 Canonical. + +package jujuapi + +import ( + "context" + "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/canonical/jimm/v3/internal/common/pagination" + "github.com/canonical/jimm/v3/internal/db" + "github.com/canonical/jimm/v3/internal/dbmodel" + "github.com/canonical/jimm/v3/internal/jimm" + "github.com/canonical/jimm/v3/internal/jimm/credentials" + "github.com/canonical/jimm/v3/internal/openfga" + ofganames "github.com/canonical/jimm/v3/internal/openfga/names" + "github.com/canonical/jimm/v3/internal/pubsub" + jimmnames "github.com/canonical/jimm/v3/pkg/names" +) + +// JIMM defines a comprehensive interface for all sort of operations with our application logic. +type JIMM interface { + RelationService + ControllerService + LoginService + ModelManager + AddAuditLogEntry(ale *dbmodel.AuditLogEntry) + AddCloudToController(ctx context.Context, user *openfga.User, controllerName string, tag names.CloudTag, cloud jujuparams.Cloud, force bool) error + AddHostedCloud(ctx context.Context, user *openfga.User, tag names.CloudTag, cloud jujuparams.Cloud, force bool) error + AddServiceAccount(ctx context.Context, u *openfga.User, clientId string) error + CopyServiceAccountCredential(ctx context.Context, u *openfga.User, svcAcc *openfga.User, cloudCredentialTag names.CloudCredentialTag) (names.CloudCredentialTag, []jujuparams.UpdateCredentialModelResult, error) + CountIdentities(ctx context.Context, user *openfga.User) (int, error) + DestroyOffer(ctx context.Context, user *openfga.User, offerURL string, force bool) error + FindApplicationOffers(ctx context.Context, user *openfga.User, filters ...jujuparams.OfferFilter) ([]jujuparams.ApplicationOfferAdminDetailsV5, error) + FindAuditEvents(ctx context.Context, user *openfga.User, filter db.AuditLogFilter) ([]dbmodel.AuditLogEntry, error) + ForEachCloud(ctx context.Context, user *openfga.User, f func(*dbmodel.Cloud) error) error + ForEachUserCloud(ctx context.Context, user *openfga.User, f func(*dbmodel.Cloud) error) error + ForEachUserCloudCredential(ctx context.Context, u *dbmodel.Identity, ct names.CloudTag, f func(cred *dbmodel.CloudCredential) error) error + GetApplicationOffer(ctx context.Context, user *openfga.User, offerURL string) (*jujuparams.ApplicationOfferAdminDetailsV5, error) + GetApplicationOfferConsumeDetails(ctx context.Context, user *openfga.User, details *jujuparams.ConsumeOfferDetails, v bakery.Version) error + GetCloud(ctx context.Context, u *openfga.User, tag names.CloudTag) (dbmodel.Cloud, error) + GetCloudCredential(ctx context.Context, user *openfga.User, tag names.CloudCredentialTag) (*dbmodel.CloudCredential, error) + GetCloudCredentialAttributes(ctx context.Context, u *openfga.User, cred *dbmodel.CloudCredential, hidden bool) (attrs map[string]string, redacted []string, err error) + GetCredentialStore() credentials.CredentialStore + RoleManager() jimm.RoleManager + GroupManager() jimm.GroupManager + GetJimmControllerAccess(ctx context.Context, user *openfga.User, tag names.UserTag) (string, error) + // FetchIdentity finds the user in jimm or returns a not-found error + FetchIdentity(ctx context.Context, username string) (*openfga.User, error) + GetUserCloudAccess(ctx context.Context, user *openfga.User, cloud names.CloudTag) (string, error) + GetUserControllerAccess(ctx context.Context, user *openfga.User, controller names.ControllerTag) (string, error) + GetUserModelAccess(ctx context.Context, user *openfga.User, model names.ModelTag) (string, error) + GrantAuditLogAccess(ctx context.Context, user *openfga.User, targetUserTag names.UserTag) error + GrantCloudAccess(ctx context.Context, user *openfga.User, ct names.CloudTag, ut names.UserTag, access string) error + GrantModelAccess(ctx context.Context, user *openfga.User, mt names.ModelTag, ut names.UserTag, access jujuparams.UserAccessPermission) error + GrantOfferAccess(ctx context.Context, u *openfga.User, offerURL string, ut names.UserTag, access jujuparams.OfferAccessPermission) error + GrantServiceAccountAccess(ctx context.Context, u *openfga.User, svcAccTag jimmnames.ServiceAccountTag, tags []string) error + InitiateInternalMigration(ctx context.Context, user *openfga.User, modelTag names.ModelTag, targetController string) (jujuparams.InitiateMigrationResult, error) + InitiateMigration(ctx context.Context, user *openfga.User, spec jujuparams.MigrationSpec) (jujuparams.InitiateMigrationResult, error) + ListApplicationOffers(ctx context.Context, user *openfga.User, filters ...jujuparams.OfferFilter) ([]jujuparams.ApplicationOfferAdminDetailsV5, error) + ListIdentities(ctx context.Context, user *openfga.User, pagination pagination.LimitOffsetPagination, match string) ([]openfga.User, error) + ListModels(ctx context.Context, user *openfga.User) ([]base.UserModel, error) + ListResources(ctx context.Context, user *openfga.User, filter pagination.LimitOffsetPagination, namePrefixFilter, typeFilter string) ([]db.Resource, error) + Offer(ctx context.Context, user *openfga.User, offer jimm.AddApplicationOfferParams) error + PubSubHub() *pubsub.Hub + PurgeLogs(ctx context.Context, user *openfga.User, before time.Time) (int64, error) + RemoveCloud(ctx context.Context, u *openfga.User, ct names.CloudTag) error + RemoveCloudFromController(ctx context.Context, u *openfga.User, controllerName string, ct names.CloudTag) error + RemoveController(ctx context.Context, user *openfga.User, controllerName string, force bool) error + ResourceTag() names.ControllerTag + RevokeAuditLogAccess(ctx context.Context, user *openfga.User, targetUserTag names.UserTag) error + RevokeCloudAccess(ctx context.Context, user *openfga.User, ct names.CloudTag, ut names.UserTag, access string) error + RevokeCloudCredential(ctx context.Context, user *dbmodel.Identity, tag names.CloudCredentialTag, force bool) error + RevokeModelAccess(ctx context.Context, user *openfga.User, mt names.ModelTag, ut names.UserTag, access jujuparams.UserAccessPermission) error + RevokeOfferAccess(ctx context.Context, user *openfga.User, offerURL string, ut names.UserTag, access jujuparams.OfferAccessPermission) (err error) + ToJAASTag(ctx context.Context, tag *ofganames.Tag, resolveUUIDs bool) (string, error) + 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) +} From 71cdcfce082b23e256ad2e0863fb0ac0d7e00588 Mon Sep 17 00:00:00 2001 From: Kian Parvin <46668016+kian99@users.noreply.github.com> Date: Wed, 18 Dec 2024 10:00:03 +0200 Subject: [PATCH 3/5] chore: remove unneeded getters (#1502) Remove getters that allowed accessing the database or authorization client from outside the application layer. --- cmd/jaas/cmd/updatecredentials_test.go | 2 +- internal/jimm/jimm.go | 15 --------------- internal/jimm/model.go | 2 +- internal/jimm/model_cleanup.go | 4 ++-- internal/jimm/model_cleanup_test.go | 6 +++--- internal/jimmhttp/httpproxy_handler.go | 2 +- internal/jimmhttp/httpproxy_handler_test.go | 4 ++-- internal/jujuapi/access_control_test.go | 2 +- internal/jujuapi/interface.go | 2 -- 9 files changed, 11 insertions(+), 28 deletions(-) diff --git a/cmd/jaas/cmd/updatecredentials_test.go b/cmd/jaas/cmd/updatecredentials_test.go index f7899e3fa..f41119c2b 100644 --- a/cmd/jaas/cmd/updatecredentials_test.go +++ b/cmd/jaas/cmd/updatecredentials_test.go @@ -76,7 +76,7 @@ func (s *updateCredentialsSuite) TestUpdateCredentialsWithLocalCredentials(c *gc models: [] `) - ofgaUser := openfga.NewUser(sa, s.JIMM.AuthorizationClient()) + ofgaUser := openfga.NewUser(sa, s.JIMM.OpenFGAClient) cloudCredentialTag := names.NewCloudCredentialTag("test-cloud/" + clientIDWithDomain + "/test-credentials") cloudCredential2, err := s.JIMM.GetCloudCredential(ctx, ofgaUser, cloudCredentialTag) c.Assert(err, gc.IsNil) diff --git a/internal/jimm/jimm.go b/internal/jimm/jimm.go index fdadd7977..2995f3c34 100644 --- a/internal/jimm/jimm.go +++ b/internal/jimm/jimm.go @@ -276,21 +276,11 @@ func (j *JIMM) ResourceTag() names.ControllerTag { return names.NewControllerTag(j.UUID) } -// DB returns the database used by JIMM. -func (j *JIMM) DB() *db.Database { - return j.Database -} - // PubsubHub returns the pub-sub hub used for buffering model summaries. func (j *JIMM) PubSubHub() *pubsub.Hub { return j.Pubsub } -// AuthorizationClient return the OpenFGA client used by JIMM. -func (j *JIMM) AuthorizationClient() *openfga.OFGAClient { - return j.OpenFGAClient -} - // RoleManager returns a manager that enables role management. func (j *JIMM) RoleManager() RoleManager { return j.roleManager @@ -301,11 +291,6 @@ func (j *JIMM) GroupManager() GroupManager { return j.groupManager } -// GetCredentialStore returns the credential store used by JIMM. -func (j *JIMM) GetCredentialStore() credentials.CredentialStore { - return j.CredentialStore -} - type permission struct { resource string relation string diff --git a/internal/jimm/model.go b/internal/jimm/model.go index ea6c16d7d..482086160 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1355,7 +1355,7 @@ func (j *JIMM) ListModels(ctx context.Context, user *openfga.User) ([]base.UserM } // Get the models from the database - models, err := j.DB().GetModelsByUUID(ctx, uuids) + models, err := j.Database.GetModelsByUUID(ctx, uuids) if err != nil { return nil, errors.E(op, err, "failed to get models by uuid") } diff --git a/internal/jimm/model_cleanup.go b/internal/jimm/model_cleanup.go index 8b5aafa87..2f3f04310 100644 --- a/internal/jimm/model_cleanup.go +++ b/internal/jimm/model_cleanup.go @@ -23,7 +23,7 @@ func (j *JIMM) CleanupDyingModels(ctx context.Context) (err error) { durationObserver := servermon.DurationObserver(servermon.JimmMethodsDurationHistogram, string(op)) defer durationObserver() - err = j.DB().ForEachModel(ctx, func(m *dbmodel.Model) error { + err = j.Database.ForEachModel(ctx, func(m *dbmodel.Model) error { if m.Life != state.Dying.String() { return nil } @@ -37,7 +37,7 @@ func (j *JIMM) CleanupDyingModels(ctx context.Context) (err error) { if err := api.ModelInfo(ctx, &jujuparams.ModelInfo{UUID: m.UUID.String}); err != nil { // Some versions of juju return unauthorized for models that cannot be found. if errors.ErrorCode(err) == errors.CodeNotFound || errors.ErrorCode(err) == errors.CodeUnauthorized { - if err := j.DB().DeleteModel(ctx, m); err != nil { + if err := j.Database.DeleteModel(ctx, m); err != nil { zapctx.Error(ctx, fmt.Sprintf("cannot delete model %s: %s\n", m.UUID.String, err)) } else { return nil diff --git a/internal/jimm/model_cleanup_test.go b/internal/jimm/model_cleanup_test.go index 3241a9dbd..9e8354ffd 100644 --- a/internal/jimm/model_cleanup_test.go +++ b/internal/jimm/model_cleanup_test.go @@ -135,7 +135,7 @@ func (s *modelCleanupSuite) TestPollModelsDying(c *qt.C) { Valid: true, }, } - err = s.jimm.DB().GetModel(ctx, &model) + err = s.jimm.Database.GetModel(ctx, &model) c.Assert(err, qt.ErrorMatches, "model not found") model = dbmodel.Model{ @@ -144,7 +144,7 @@ func (s *modelCleanupSuite) TestPollModelsDying(c *qt.C) { Valid: true, }, } - err = s.jimm.DB().GetModel(ctx, &model) + err = s.jimm.Database.GetModel(ctx, &model) c.Assert(err, qt.IsNil) } @@ -174,7 +174,7 @@ func (s *modelCleanupSuite) TestPollModelsDyingControllerErrors(c *qt.C) { Valid: true, }, } - err = s.jimm.DB().GetModel(ctx, &model) + err = s.jimm.Database.GetModel(ctx, &model) c.Assert(err, qt.IsNil) c.Assert(model.Life, qt.Equals, state.Dying.String()) } diff --git a/internal/jimmhttp/httpproxy_handler.go b/internal/jimmhttp/httpproxy_handler.go index 5e8070bf5..9067aa0ec 100644 --- a/internal/jimmhttp/httpproxy_handler.go +++ b/internal/jimmhttp/httpproxy_handler.go @@ -63,7 +63,7 @@ func (hph *HTTPProxyHandler) ProxyHTTP(w http.ResponseWriter, req *http.Request) writeError(ctx, w, http.StatusNotFound, err, "cannot get model") return } - u, p, err := hph.jimm.GetCredentialStore().GetControllerCredentials(ctx, model.Controller.Name) + u, p, err := hph.jimm.CredentialStore.GetControllerCredentials(ctx, model.Controller.Name) if err != nil { writeError(ctx, w, http.StatusNotFound, err, "cannot retrieve credentials") return diff --git a/internal/jimmhttp/httpproxy_handler_test.go b/internal/jimmhttp/httpproxy_handler_test.go index b84c94358..a9d07e669 100644 --- a/internal/jimmhttp/httpproxy_handler_test.go +++ b/internal/jimmhttp/httpproxy_handler_test.go @@ -66,14 +66,14 @@ func (s *httpProxySuite) SetUpTest(c *gc.C) { err := s.JIMM.Database.GetModel(ctx, model) c.Assert(err, gc.IsNil) s.model = model - err = s.JIMM.GetCredentialStore().PutControllerCredentials(ctx, model.Controller.Name, "user", "psw") + err = s.JIMM.CredentialStore.PutControllerCredentials(ctx, model.Controller.Name, "user", "psw") c.Assert(err, gc.IsNil) } func (s *httpProxySuite) TestHTTPProxyHandler(c *gc.C) { ctx := context.Background() httpProxier := jimmhttp.NewHTTPProxyHandler(s.JIMM) - expectU, expectP, err := s.JIMM.GetCredentialStore().GetControllerCredentials(ctx, s.model.Controller.Name) + expectU, expectP, err := s.JIMM.CredentialStore.GetControllerCredentials(ctx, s.model.Controller.Name) c.Assert(err, gc.IsNil) // we expect the controller to respond with TLS fakeController := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/jujuapi/access_control_test.go b/internal/jujuapi/access_control_test.go index d8e5b8491..a26006411 100644 --- a/internal/jujuapi/access_control_test.go +++ b/internal/jujuapi/access_control_test.go @@ -891,7 +891,7 @@ func (s *accessControlSuite) TestListRelationshipTuplesNoUUIDResolution(c *gc.C) c.Assert(err, jc.ErrorIsNil) groupOrange := dbmodel.GroupEntry{Name: "orange"} - err = s.JIMM.DB().GetGroup(ctx, &groupOrange) + err = s.JIMM.Database.GetGroup(ctx, &groupOrange) c.Assert(err, jc.ErrorIsNil) expected := []apiparams.RelationshipTuple{{ Object: "group-" + groupOrange.UUID + "#member", diff --git a/internal/jujuapi/interface.go b/internal/jujuapi/interface.go index 1565abf9d..e57b1341c 100644 --- a/internal/jujuapi/interface.go +++ b/internal/jujuapi/interface.go @@ -15,7 +15,6 @@ import ( "github.com/canonical/jimm/v3/internal/db" "github.com/canonical/jimm/v3/internal/dbmodel" "github.com/canonical/jimm/v3/internal/jimm" - "github.com/canonical/jimm/v3/internal/jimm/credentials" "github.com/canonical/jimm/v3/internal/openfga" ofganames "github.com/canonical/jimm/v3/internal/openfga/names" "github.com/canonical/jimm/v3/internal/pubsub" @@ -45,7 +44,6 @@ type JIMM interface { GetCloud(ctx context.Context, u *openfga.User, tag names.CloudTag) (dbmodel.Cloud, error) GetCloudCredential(ctx context.Context, user *openfga.User, tag names.CloudCredentialTag) (*dbmodel.CloudCredential, error) GetCloudCredentialAttributes(ctx context.Context, u *openfga.User, cred *dbmodel.CloudCredential, hidden bool) (attrs map[string]string, redacted []string, err error) - GetCredentialStore() credentials.CredentialStore RoleManager() jimm.RoleManager GroupManager() jimm.GroupManager GetJimmControllerAccess(ctx context.Context, user *openfga.User, tag names.UserTag) (string, error) From 5cf6fc6e2d0c38f1738e57d5e624facb4836a117 Mon Sep 17 00:00:00 2001 From: Kian Parvin <46668016+kian99@users.noreply.github.com> Date: Wed, 18 Dec 2024 14:20:21 +0200 Subject: [PATCH 4/5] chore: move jwt generator (#1501) * chore: move jwt generator Move the jwtGenerator into a separate package. * chore: update package godoc and names * chore: rename jwtgenerator to jujuauth --- internal/jimm/access.go | 159 --------- internal/jimm/access_test.go | 342 ------------------- internal/jimm/jujuauth/jwtgenerator.go | 180 ++++++++++ internal/jimm/jujuauth/jwtgenerator_test.go | 358 ++++++++++++++++++++ internal/jujuapi/websocket.go | 5 +- 5 files changed, 541 insertions(+), 503 deletions(-) create mode 100644 internal/jimm/jujuauth/jwtgenerator.go create mode 100644 internal/jimm/jujuauth/jwtgenerator_test.go diff --git a/internal/jimm/access.go b/internal/jimm/access.go index 98f809786..de85a10a8 100644 --- a/internal/jimm/access.go +++ b/internal/jimm/access.go @@ -8,7 +8,6 @@ import ( "fmt" "regexp" "strings" - "sync" "github.com/canonical/ofga" "github.com/google/uuid" @@ -20,7 +19,6 @@ import ( "github.com/canonical/jimm/v3/internal/db" "github.com/canonical/jimm/v3/internal/dbmodel" "github.com/canonical/jimm/v3/internal/errors" - "github.com/canonical/jimm/v3/internal/jimmjwx" "github.com/canonical/jimm/v3/internal/openfga" ofganames "github.com/canonical/jimm/v3/internal/openfga/names" "github.com/canonical/jimm/v3/internal/servermon" @@ -147,163 +145,6 @@ func ToOfferRelation(accessLevel string) (openfga.Relation, error) { } } -// JWTGeneratorDatabase specifies the database interface used by the -// JWT generator. -type JWTGeneratorDatabase interface { - GetController(ctx context.Context, controller *dbmodel.Controller) error -} - -// JWTGeneratorAccessChecker specifies the access checker used by the JWT -// generator to obtain user's access rights to various entities. -type JWTGeneratorAccessChecker interface { - GetUserModelAccess(context.Context, *openfga.User, names.ModelTag) (string, error) - GetUserControllerAccess(context.Context, *openfga.User, names.ControllerTag) (string, error) - GetUserCloudAccess(context.Context, *openfga.User, names.CloudTag) (string, error) - CheckPermission(context.Context, *openfga.User, map[string]string, map[string]interface{}) (map[string]string, error) -} - -// JWTService specifies the service JWT generator uses to generate JWTs. -type JWTService interface { - NewJWT(context.Context, jimmjwx.JWTParams) ([]byte, error) -} - -// JWTGenerator provides the necessary state and methods to authorize a user and generate JWT tokens. -type JWTGenerator struct { - database JWTGeneratorDatabase - accessChecker JWTGeneratorAccessChecker - jwtService JWTService - - mu sync.Mutex - accessMapCache map[string]string - mt names.ModelTag - ct names.ControllerTag - user *openfga.User - callCount int -} - -// NewJWTGenerator returns a new JwtAuthorizer struct -func NewJWTGenerator(database JWTGeneratorDatabase, accessChecker JWTGeneratorAccessChecker, jwtService JWTService) JWTGenerator { - return JWTGenerator{ - database: database, - accessChecker: accessChecker, - jwtService: jwtService, - } -} - -// SetTags implements TokenGenerator -func (auth *JWTGenerator) SetTags(mt names.ModelTag, ct names.ControllerTag) { - auth.mt = mt - auth.ct = ct -} - -// SetTags implements TokenGenerator -func (auth *JWTGenerator) GetUser() names.UserTag { - if auth.user != nil { - return auth.user.ResourceTag() - } - return names.UserTag{} -} - -// MakeLoginToken authorizes the user based on the provided login requests and returns -// a JWT containing claims about user's access to the controller, model (if applicable) -// and all clouds that the controller knows about. -func (auth *JWTGenerator) MakeLoginToken(ctx context.Context, user *openfga.User) ([]byte, error) { - const op = errors.Op("jimm.MakeLoginToken") - - auth.mu.Lock() - defer auth.mu.Unlock() - - if user == nil { - return nil, errors.E(op, "user not specified") - } - auth.user = user - - // Recreate the accessMapCache to prevent leaking permissions across multiple login requests. - auth.accessMapCache = make(map[string]string) - var authErr error - - var modelAccess string - if auth.mt.Id() == "" { - return nil, errors.E(op, "model not set") - } - modelAccess, authErr = auth.accessChecker.GetUserModelAccess(ctx, auth.user, auth.mt) - if authErr != nil { - zapctx.Error(ctx, "model access check failed", zap.Error(authErr)) - return nil, authErr - } - auth.accessMapCache[auth.mt.String()] = modelAccess - - if auth.ct.Id() == "" { - return nil, errors.E(op, "controller not set") - } - var controllerAccess string - controllerAccess, authErr = auth.accessChecker.GetUserControllerAccess(ctx, auth.user, auth.ct) - if authErr != nil { - return nil, authErr - } - auth.accessMapCache[auth.ct.String()] = controllerAccess - - var ctl dbmodel.Controller - ctl.SetTag(auth.ct) - err := auth.database.GetController(ctx, &ctl) - if err != nil { - zapctx.Error(ctx, "failed to fetch controller", zap.Error(err)) - return nil, errors.E(op, "failed to fetch controller", err) - } - clouds := make(map[names.CloudTag]bool) - for _, cloudRegion := range ctl.CloudRegions { - clouds[cloudRegion.CloudRegion.Cloud.ResourceTag()] = true - } - for cloudTag := range clouds { - accessLevel, err := auth.accessChecker.GetUserCloudAccess(ctx, auth.user, cloudTag) - if err != nil { - zapctx.Error(ctx, "cloud access check failed", zap.Error(err)) - return nil, errors.E(op, "failed to check user's cloud access", err) - } - auth.accessMapCache[cloudTag.String()] = accessLevel - } - - return auth.jwtService.NewJWT(ctx, jimmjwx.JWTParams{ - Controller: auth.ct.Id(), - User: auth.user.Tag().String(), - Access: auth.accessMapCache, - }) -} - -// MakeToken assumes MakeLoginToken has already been called and checks the permissions -// specified in the permissionMap. If the logged in user has all those permissions -// a JWT will be returned with assertions confirming all those permissions. -func (auth *JWTGenerator) MakeToken(ctx context.Context, permissionMap map[string]interface{}) ([]byte, error) { - const op = errors.Op("jimm.MakeToken") - - auth.mu.Lock() - defer auth.mu.Unlock() - - if auth.callCount >= 10 { - return nil, errors.E(op, "Permission check limit exceeded") - } - auth.callCount++ - if auth.user == nil { - return nil, errors.E(op, "User authorization missing.") - } - if permissionMap != nil { - var err error - auth.accessMapCache, err = auth.accessChecker.CheckPermission(ctx, auth.user, auth.accessMapCache, permissionMap) - if err != nil { - return nil, err - } - } - jwt, err := auth.jwtService.NewJWT(ctx, jimmjwx.JWTParams{ - Controller: auth.ct.Id(), - User: auth.user.Tag().String(), - Access: auth.accessMapCache, - }) - if err != nil { - return nil, err - } - return jwt, nil -} - // CheckPermission loops over the desired permissions in desiredPerms and adds these permissions // to cachedPerms if they exist. If the user does not have any of the desired permissions then an // error is returned. diff --git a/internal/jimm/access_test.go b/internal/jimm/access_test.go index 5efbfba74..4d9d83107 100644 --- a/internal/jimm/access_test.go +++ b/internal/jimm/access_test.go @@ -14,98 +14,13 @@ import ( "github.com/juju/names/v5" "github.com/canonical/jimm/v3/internal/dbmodel" - "github.com/canonical/jimm/v3/internal/errors" "github.com/canonical/jimm/v3/internal/jimm" - "github.com/canonical/jimm/v3/internal/jimmjwx" "github.com/canonical/jimm/v3/internal/openfga" ofganames "github.com/canonical/jimm/v3/internal/openfga/names" "github.com/canonical/jimm/v3/internal/testutils/jimmtest" jimmnames "github.com/canonical/jimm/v3/pkg/names" ) -// testDatabase is a database implementation intended for testing the token generator. -type testDatabase struct { - ctl dbmodel.Controller - err error -} - -// GetController implements the GetController method of the JWTGeneratorDatabase interface. -func (tdb *testDatabase) GetController(ctx context.Context, controller *dbmodel.Controller) error { - if tdb.err != nil { - return tdb.err - } - *controller = tdb.ctl - return nil -} - -// testAccessChecker is an access checker implementation intended for testing the -// token generator. -type testAccessChecker struct { - controllerAccess map[string]string - controllerAccessCheckErr error - modelAccess map[string]string - modelAccessCheckErr error - cloudAccess map[string]string - cloudAccessCheckErr error - permissions map[string]string - permissionCheckErr error -} - -// GetUserModelAccess implements the GetUserModelAccess method of the JWTGeneratorAccessChecker interface. -func (tac *testAccessChecker) GetUserModelAccess(ctx context.Context, user *openfga.User, mt names.ModelTag) (string, error) { - if tac.modelAccessCheckErr != nil { - return "", tac.modelAccessCheckErr - } - return tac.modelAccess[mt.String()], nil -} - -// GetUserControllerAccess implements the GetUserControllerAccess method of the JWTGeneratorAccessChecker interface. -func (tac *testAccessChecker) GetUserControllerAccess(ctx context.Context, user *openfga.User, ct names.ControllerTag) (string, error) { - if tac.controllerAccessCheckErr != nil { - return "", tac.controllerAccessCheckErr - } - return tac.controllerAccess[ct.String()], nil -} - -// GetUserCloudAccess implements the GetUserCloudAccess method of the JWTGeneratorAccessChecker interface. -func (tac *testAccessChecker) GetUserCloudAccess(ctx context.Context, user *openfga.User, ct names.CloudTag) (string, error) { - if tac.cloudAccessCheckErr != nil { - return "", tac.cloudAccessCheckErr - } - return tac.cloudAccess[ct.String()], nil -} - -// CheckPermission implements the CheckPermission methods of the JWTGeneratorAccessChecker interface. -func (tac *testAccessChecker) CheckPermission(ctx context.Context, user *openfga.User, accessMap map[string]string, permissions map[string]interface{}) (map[string]string, error) { - if tac.permissionCheckErr != nil { - return nil, tac.permissionCheckErr - } - access := make(map[string]string) - for k, v := range accessMap { - access[k] = v - } - for k, v := range tac.permissions { - access[k] = v - } - return access, nil -} - -// testJWTService is a jwt service implementation intended for testing the token generator. -type testJWTService struct { - newJWTError error - - params jimmjwx.JWTParams -} - -// NewJWT implements the NewJWT methods of the JWTService interface. -func (t *testJWTService) NewJWT(ctx context.Context, params jimmjwx.JWTParams) ([]byte, error) { - if t.newJWTError != nil { - return nil, t.newJWTError - } - t.params = params - return []byte("test jwt"), nil -} - func TestAuditLogAccess(t *testing.T) { c := qt.New(t) @@ -154,263 +69,6 @@ func TestAuditLogAccess(t *testing.T) { c.Assert(err, qt.ErrorMatches, "unauthorized") } -func TestJWTGeneratorMakeLoginToken(t *testing.T) { - c := qt.New(t) - - ct := names.NewControllerTag(uuid.New().String()) - mt := names.NewModelTag(uuid.New().String()) - - tests := []struct { - about string - username string - database *testDatabase - accessChecker *testAccessChecker - jwtService *testJWTService - expectedError string - expectedJWTParams jimmjwx.JWTParams - }{{ - about: "initial login, all is well", - username: "eve@canonical.com", - database: &testDatabase{ - ctl: dbmodel.Controller{ - CloudRegions: []dbmodel.CloudRegionControllerPriority{{ - CloudRegion: dbmodel.CloudRegion{ - Cloud: dbmodel.Cloud{ - Name: "test-cloud", - }, - }, - }}, - }, - }, - accessChecker: &testAccessChecker{ - modelAccess: map[string]string{ - mt.String(): "admin", - }, - controllerAccess: map[string]string{ - ct.String(): "superuser", - }, - cloudAccess: map[string]string{ - names.NewCloudTag("test-cloud").String(): "add-model", - }, - }, - jwtService: &testJWTService{}, - expectedJWTParams: jimmjwx.JWTParams{ - Controller: ct.Id(), - User: names.NewUserTag("eve@canonical.com").String(), - Access: map[string]string{ - ct.String(): "superuser", - mt.String(): "admin", - names.NewCloudTag("test-cloud").String(): "add-model", - }, - }, - }, { - about: "model access check fails", - username: "eve@canonical.com", - accessChecker: &testAccessChecker{ - modelAccessCheckErr: errors.E("a test error"), - }, - jwtService: &testJWTService{}, - expectedError: "a test error", - }, { - about: "controller access check fails", - username: "eve@canonical.com", - accessChecker: &testAccessChecker{ - modelAccess: map[string]string{ - mt.String(): "admin", - }, - controllerAccessCheckErr: errors.E("a test error"), - }, - expectedError: "a test error", - }, { - about: "get controller from db fails", - username: "eve@canonical.com", - database: &testDatabase{ - err: errors.E("a test error"), - }, - accessChecker: &testAccessChecker{ - modelAccess: map[string]string{ - mt.String(): "admin", - }, - controllerAccess: map[string]string{ - ct.String(): "superuser", - }, - }, - expectedError: "failed to fetch controller", - }, { - about: "cloud access check fails", - username: "eve@canonical.com", - database: &testDatabase{ - ctl: dbmodel.Controller{ - CloudRegions: []dbmodel.CloudRegionControllerPriority{{ - CloudRegion: dbmodel.CloudRegion{ - Cloud: dbmodel.Cloud{ - Name: "test-cloud", - }, - }, - }}, - }, - }, - accessChecker: &testAccessChecker{ - modelAccess: map[string]string{ - mt.String(): "admin", - }, - controllerAccess: map[string]string{ - ct.String(): "superuser", - }, - cloudAccessCheckErr: errors.E("a test error"), - }, - expectedError: "failed to check user's cloud access", - }, { - about: "jwt service errors out", - username: "eve@canonical.com", - database: &testDatabase{ - ctl: dbmodel.Controller{ - CloudRegions: []dbmodel.CloudRegionControllerPriority{{ - CloudRegion: dbmodel.CloudRegion{ - Cloud: dbmodel.Cloud{ - Name: "test-cloud", - }, - }, - }}, - }, - }, - accessChecker: &testAccessChecker{ - modelAccess: map[string]string{ - mt.String(): "admin", - }, - controllerAccess: map[string]string{ - ct.String(): "superuser", - }, - cloudAccess: map[string]string{ - names.NewCloudTag("test-cloud").String(): "add-model", - }, - }, - jwtService: &testJWTService{ - newJWTError: errors.E("a test error"), - }, - expectedError: "a test error", - }} - - for _, test := range tests { - generator := jimm.NewJWTGenerator(test.database, test.accessChecker, test.jwtService) - generator.SetTags(mt, ct) - - i, err := dbmodel.NewIdentity(test.username) - c.Assert(err, qt.IsNil) - _, err = generator.MakeLoginToken(context.Background(), &openfga.User{ - Identity: i, - }) - if test.expectedError != "" { - c.Assert(err, qt.ErrorMatches, test.expectedError) - } else { - c.Assert(err, qt.IsNil) - c.Assert(test.jwtService.params, qt.DeepEquals, test.expectedJWTParams) - } - } -} - -func TestJWTGeneratorMakeToken(t *testing.T) { - c := qt.New(t) - - ct := names.NewControllerTag(uuid.New().String()) - mt := names.NewModelTag(uuid.New().String()) - - tests := []struct { - about string - checkPermissions map[string]string - checkPermissionsError error - jwtService *testJWTService - expectedError string - permissions map[string]interface{} - expectedJWTParams jimmjwx.JWTParams - }{{ - about: "all is well", - jwtService: &testJWTService{}, - expectedJWTParams: jimmjwx.JWTParams{ - Controller: ct.Id(), - User: names.NewUserTag("eve@canonical.com").String(), - Access: map[string]string{ - ct.String(): "superuser", - mt.String(): "admin", - names.NewCloudTag("test-cloud").String(): "add-model", - }, - }, - }, { - about: "check permission fails", - jwtService: &testJWTService{}, - permissions: map[string]interface{}{ - "entity1": "access_level1", - }, - checkPermissionsError: errors.E("a test error"), - expectedError: "a test error", - }, { - about: "additional permissions need checking", - jwtService: &testJWTService{}, - permissions: map[string]interface{}{ - "entity1": "access_level1", - }, - checkPermissions: map[string]string{ - "entity1": "access_level1", - }, - expectedJWTParams: jimmjwx.JWTParams{ - Controller: ct.Id(), - User: names.NewUserTag("eve@canonical.com").String(), - Access: map[string]string{ - ct.String(): "superuser", - mt.String(): "admin", - names.NewCloudTag("test-cloud").String(): "add-model", - "entity1": "access_level1", - }, - }, - }} - - for _, test := range tests { - generator := jimm.NewJWTGenerator( - &testDatabase{ - ctl: dbmodel.Controller{ - CloudRegions: []dbmodel.CloudRegionControllerPriority{{ - CloudRegion: dbmodel.CloudRegion{ - Cloud: dbmodel.Cloud{ - Name: "test-cloud", - }, - }, - }}, - }, - }, - &testAccessChecker{ - modelAccess: map[string]string{ - mt.String(): "admin", - }, - controllerAccess: map[string]string{ - ct.String(): "superuser", - }, - cloudAccess: map[string]string{ - names.NewCloudTag("test-cloud").String(): "add-model", - }, - permissions: test.checkPermissions, - permissionCheckErr: test.checkPermissionsError, - }, - test.jwtService, - ) - generator.SetTags(mt, ct) - - i, err := dbmodel.NewIdentity("eve@canonical.com") - c.Assert(err, qt.IsNil) - _, err = generator.MakeLoginToken(context.Background(), &openfga.User{ - Identity: i, - }) - c.Assert(err, qt.IsNil) - - _, err = generator.MakeToken(context.Background(), test.permissions) - if test.expectedError != "" { - c.Assert(err, qt.ErrorMatches, test.expectedError) - } else { - c.Assert(err, qt.IsNil) - c.Assert(test.jwtService.params, qt.DeepEquals, test.expectedJWTParams) - } - } -} - func TestParseAndValidateTag(t *testing.T) { c := qt.New(t) ctx := context.Background() diff --git a/internal/jimm/jujuauth/jwtgenerator.go b/internal/jimm/jujuauth/jwtgenerator.go new file mode 100644 index 000000000..7924d2ce8 --- /dev/null +++ b/internal/jimm/jujuauth/jwtgenerator.go @@ -0,0 +1,180 @@ +// Copyright 2024 Canonical. + +// Package jujuauth generates JWT tokens to +// authenticate and authorize messages to Juju controllers. +// This package is more specialised than a generic +// JWT token generator as it crafts Juju specific +// permissions that are added as claims to the JWT +// and therefore exists in JIMM's business logic layer. +package jujuauth + +import ( + "context" + "sync" + + "github.com/juju/names/v5" + "github.com/juju/zaputil/zapctx" + "go.uber.org/zap" + + "github.com/canonical/jimm/v3/internal/dbmodel" + "github.com/canonical/jimm/v3/internal/errors" + "github.com/canonical/jimm/v3/internal/jimmjwx" + "github.com/canonical/jimm/v3/internal/openfga" +) + +// GeneratorDatabase specifies the database interface used by the +// JWT generator. +type GeneratorDatabase interface { + GetController(ctx context.Context, controller *dbmodel.Controller) error +} + +// GeneratorAccessChecker specifies the access checker used by the JWT +// generator to obtain user's access rights to various entities. +type GeneratorAccessChecker interface { + GetUserModelAccess(context.Context, *openfga.User, names.ModelTag) (string, error) + GetUserControllerAccess(context.Context, *openfga.User, names.ControllerTag) (string, error) + GetUserCloudAccess(context.Context, *openfga.User, names.CloudTag) (string, error) + CheckPermission(context.Context, *openfga.User, map[string]string, map[string]interface{}) (map[string]string, error) +} + +// JWTService specifies the service JWT generator uses to generate JWTs. +type JWTService interface { + NewJWT(context.Context, jimmjwx.JWTParams) ([]byte, error) +} + +// TokenGenerator provides the necessary state and methods to authorize a user and generate JWT tokens. +type TokenGenerator struct { + database GeneratorDatabase + accessChecker GeneratorAccessChecker + jwtService JWTService + + mu sync.Mutex + accessMapCache map[string]string + mt names.ModelTag + ct names.ControllerTag + user *openfga.User + callCount int +} + +// New returns a new JWTGenerator. +func New(database GeneratorDatabase, accessChecker GeneratorAccessChecker, jwtService JWTService) TokenGenerator { + return TokenGenerator{ + database: database, + accessChecker: accessChecker, + jwtService: jwtService, + } +} + +// SetTags implements TokenGenerator. +func (auth *TokenGenerator) SetTags(mt names.ModelTag, ct names.ControllerTag) { + auth.mt = mt + auth.ct = ct +} + +// SetTags implements TokenGenerator. +func (auth *TokenGenerator) GetUser() names.UserTag { + if auth.user != nil { + return auth.user.ResourceTag() + } + return names.UserTag{} +} + +// MakeLoginToken authorizes the user based on the provided login requests and returns +// a JWT containing claims about user's access to the controller, model (if applicable) +// and all clouds that the controller knows about. +func (auth *TokenGenerator) MakeLoginToken(ctx context.Context, user *openfga.User) ([]byte, error) { + const op = errors.Op("jimm.MakeLoginToken") + + auth.mu.Lock() + defer auth.mu.Unlock() + + if user == nil { + return nil, errors.E(op, "user not specified") + } + auth.user = user + + // Recreate the accessMapCache to prevent leaking permissions across multiple login requests. + auth.accessMapCache = make(map[string]string) + var authErr error + + var modelAccess string + if auth.mt.Id() == "" { + return nil, errors.E(op, "model not set") + } + modelAccess, authErr = auth.accessChecker.GetUserModelAccess(ctx, auth.user, auth.mt) + if authErr != nil { + zapctx.Error(ctx, "model access check failed", zap.Error(authErr)) + return nil, authErr + } + auth.accessMapCache[auth.mt.String()] = modelAccess + + if auth.ct.Id() == "" { + return nil, errors.E(op, "controller not set") + } + var controllerAccess string + controllerAccess, authErr = auth.accessChecker.GetUserControllerAccess(ctx, auth.user, auth.ct) + if authErr != nil { + return nil, authErr + } + auth.accessMapCache[auth.ct.String()] = controllerAccess + + var ctl dbmodel.Controller + ctl.SetTag(auth.ct) + err := auth.database.GetController(ctx, &ctl) + if err != nil { + zapctx.Error(ctx, "failed to fetch controller", zap.Error(err)) + return nil, errors.E(op, "failed to fetch controller", err) + } + clouds := make(map[names.CloudTag]bool) + for _, cloudRegion := range ctl.CloudRegions { + clouds[cloudRegion.CloudRegion.Cloud.ResourceTag()] = true + } + for cloudTag := range clouds { + accessLevel, err := auth.accessChecker.GetUserCloudAccess(ctx, auth.user, cloudTag) + if err != nil { + zapctx.Error(ctx, "cloud access check failed", zap.Error(err)) + return nil, errors.E(op, "failed to check user's cloud access", err) + } + auth.accessMapCache[cloudTag.String()] = accessLevel + } + + return auth.jwtService.NewJWT(ctx, jimmjwx.JWTParams{ + Controller: auth.ct.Id(), + User: auth.user.Tag().String(), + Access: auth.accessMapCache, + }) +} + +// MakeToken assumes MakeLoginToken has already been called and checks the permissions +// specified in the permissionMap. If the logged in user has all those permissions +// a JWT will be returned with assertions confirming all those permissions. +func (auth *TokenGenerator) MakeToken(ctx context.Context, permissionMap map[string]interface{}) ([]byte, error) { + const op = errors.Op("jimm.MakeToken") + + auth.mu.Lock() + defer auth.mu.Unlock() + + if auth.callCount >= 10 { + return nil, errors.E(op, "Permission check limit exceeded") + } + auth.callCount++ + if auth.user == nil { + return nil, errors.E(op, "User authorization missing.") + } + if permissionMap != nil { + var err error + auth.accessMapCache, err = auth.accessChecker.CheckPermission(ctx, auth.user, auth.accessMapCache, permissionMap) + if err != nil { + return nil, err + } + } + jwt, err := auth.jwtService.NewJWT(ctx, jimmjwx.JWTParams{ + Controller: auth.ct.Id(), + User: auth.user.Tag().String(), + Access: auth.accessMapCache, + }) + if err != nil { + return nil, err + } + return jwt, nil +} diff --git a/internal/jimm/jujuauth/jwtgenerator_test.go b/internal/jimm/jujuauth/jwtgenerator_test.go new file mode 100644 index 000000000..f04519486 --- /dev/null +++ b/internal/jimm/jujuauth/jwtgenerator_test.go @@ -0,0 +1,358 @@ +// Copyright 2024 Canonical. + +package jujuauth_test + +import ( + "context" + "testing" + + qt "github.com/frankban/quicktest" + "github.com/google/uuid" + "github.com/juju/names/v5" + + "github.com/canonical/jimm/v3/internal/dbmodel" + "github.com/canonical/jimm/v3/internal/errors" + "github.com/canonical/jimm/v3/internal/jimm/jujuauth" + "github.com/canonical/jimm/v3/internal/jimmjwx" + "github.com/canonical/jimm/v3/internal/openfga" +) + +// testDatabase is a database implementation intended for testing the token generator. +type testDatabase struct { + ctl dbmodel.Controller + err error +} + +// GetController implements the GetController method of the JWTGeneratorDatabase interface. +func (tdb *testDatabase) GetController(ctx context.Context, controller *dbmodel.Controller) error { + if tdb.err != nil { + return tdb.err + } + *controller = tdb.ctl + return nil +} + +// testAccessChecker is an access checker implementation intended for testing the +// token generator. +type testAccessChecker struct { + controllerAccess map[string]string + controllerAccessCheckErr error + modelAccess map[string]string + modelAccessCheckErr error + cloudAccess map[string]string + cloudAccessCheckErr error + permissions map[string]string + permissionCheckErr error +} + +// GetUserModelAccess implements the GetUserModelAccess method of the JWTGeneratorAccessChecker interface. +func (tac *testAccessChecker) GetUserModelAccess(ctx context.Context, user *openfga.User, mt names.ModelTag) (string, error) { + if tac.modelAccessCheckErr != nil { + return "", tac.modelAccessCheckErr + } + return tac.modelAccess[mt.String()], nil +} + +// GetUserControllerAccess implements the GetUserControllerAccess method of the JWTGeneratorAccessChecker interface. +func (tac *testAccessChecker) GetUserControllerAccess(ctx context.Context, user *openfga.User, ct names.ControllerTag) (string, error) { + if tac.controllerAccessCheckErr != nil { + return "", tac.controllerAccessCheckErr + } + return tac.controllerAccess[ct.String()], nil +} + +// GetUserCloudAccess implements the GetUserCloudAccess method of the JWTGeneratorAccessChecker interface. +func (tac *testAccessChecker) GetUserCloudAccess(ctx context.Context, user *openfga.User, ct names.CloudTag) (string, error) { + if tac.cloudAccessCheckErr != nil { + return "", tac.cloudAccessCheckErr + } + return tac.cloudAccess[ct.String()], nil +} + +// CheckPermission implements the CheckPermission methods of the JWTGeneratorAccessChecker interface. +func (tac *testAccessChecker) CheckPermission(ctx context.Context, user *openfga.User, accessMap map[string]string, permissions map[string]interface{}) (map[string]string, error) { + if tac.permissionCheckErr != nil { + return nil, tac.permissionCheckErr + } + access := make(map[string]string) + for k, v := range accessMap { + access[k] = v + } + for k, v := range tac.permissions { + access[k] = v + } + return access, nil +} + +// testJWTService is a jwt service implementation intended for testing the token generator. +type testJWTService struct { + newJWTError error + + params jimmjwx.JWTParams +} + +// NewJWT implements the NewJWT methods of the JWTService interface. +func (t *testJWTService) NewJWT(ctx context.Context, params jimmjwx.JWTParams) ([]byte, error) { + if t.newJWTError != nil { + return nil, t.newJWTError + } + t.params = params + return []byte("test jwt"), nil +} + +func TestJWTGeneratorMakeLoginToken(t *testing.T) { + c := qt.New(t) + + ct := names.NewControllerTag(uuid.New().String()) + mt := names.NewModelTag(uuid.New().String()) + + tests := []struct { + about string + username string + database *testDatabase + accessChecker *testAccessChecker + jwtService *testJWTService + expectedError string + expectedJWTParams jimmjwx.JWTParams + }{{ + about: "initial login, all is well", + username: "eve@canonical.com", + database: &testDatabase{ + ctl: dbmodel.Controller{ + CloudRegions: []dbmodel.CloudRegionControllerPriority{{ + CloudRegion: dbmodel.CloudRegion{ + Cloud: dbmodel.Cloud{ + Name: "test-cloud", + }, + }, + }}, + }, + }, + accessChecker: &testAccessChecker{ + modelAccess: map[string]string{ + mt.String(): "admin", + }, + controllerAccess: map[string]string{ + ct.String(): "superuser", + }, + cloudAccess: map[string]string{ + names.NewCloudTag("test-cloud").String(): "add-model", + }, + }, + jwtService: &testJWTService{}, + expectedJWTParams: jimmjwx.JWTParams{ + Controller: ct.Id(), + User: names.NewUserTag("eve@canonical.com").String(), + Access: map[string]string{ + ct.String(): "superuser", + mt.String(): "admin", + names.NewCloudTag("test-cloud").String(): "add-model", + }, + }, + }, { + about: "model access check fails", + username: "eve@canonical.com", + accessChecker: &testAccessChecker{ + modelAccessCheckErr: errors.E("a test error"), + }, + jwtService: &testJWTService{}, + expectedError: "a test error", + }, { + about: "controller access check fails", + username: "eve@canonical.com", + accessChecker: &testAccessChecker{ + modelAccess: map[string]string{ + mt.String(): "admin", + }, + controllerAccessCheckErr: errors.E("a test error"), + }, + expectedError: "a test error", + }, { + about: "get controller from db fails", + username: "eve@canonical.com", + database: &testDatabase{ + err: errors.E("a test error"), + }, + accessChecker: &testAccessChecker{ + modelAccess: map[string]string{ + mt.String(): "admin", + }, + controllerAccess: map[string]string{ + ct.String(): "superuser", + }, + }, + expectedError: "failed to fetch controller", + }, { + about: "cloud access check fails", + username: "eve@canonical.com", + database: &testDatabase{ + ctl: dbmodel.Controller{ + CloudRegions: []dbmodel.CloudRegionControllerPriority{{ + CloudRegion: dbmodel.CloudRegion{ + Cloud: dbmodel.Cloud{ + Name: "test-cloud", + }, + }, + }}, + }, + }, + accessChecker: &testAccessChecker{ + modelAccess: map[string]string{ + mt.String(): "admin", + }, + controllerAccess: map[string]string{ + ct.String(): "superuser", + }, + cloudAccessCheckErr: errors.E("a test error"), + }, + expectedError: "failed to check user's cloud access", + }, { + about: "jwt service errors out", + username: "eve@canonical.com", + database: &testDatabase{ + ctl: dbmodel.Controller{ + CloudRegions: []dbmodel.CloudRegionControllerPriority{{ + CloudRegion: dbmodel.CloudRegion{ + Cloud: dbmodel.Cloud{ + Name: "test-cloud", + }, + }, + }}, + }, + }, + accessChecker: &testAccessChecker{ + modelAccess: map[string]string{ + mt.String(): "admin", + }, + controllerAccess: map[string]string{ + ct.String(): "superuser", + }, + cloudAccess: map[string]string{ + names.NewCloudTag("test-cloud").String(): "add-model", + }, + }, + jwtService: &testJWTService{ + newJWTError: errors.E("a test error"), + }, + expectedError: "a test error", + }} + + for _, test := range tests { + generator := jujuauth.New(test.database, test.accessChecker, test.jwtService) + generator.SetTags(mt, ct) + + i, err := dbmodel.NewIdentity(test.username) + c.Assert(err, qt.IsNil) + _, err = generator.MakeLoginToken(context.Background(), &openfga.User{ + Identity: i, + }) + if test.expectedError != "" { + c.Assert(err, qt.ErrorMatches, test.expectedError) + } else { + c.Assert(err, qt.IsNil) + c.Assert(test.jwtService.params, qt.DeepEquals, test.expectedJWTParams) + } + } +} + +func TestJWTGeneratorMakeToken(t *testing.T) { + c := qt.New(t) + + ct := names.NewControllerTag(uuid.New().String()) + mt := names.NewModelTag(uuid.New().String()) + + tests := []struct { + about string + checkPermissions map[string]string + checkPermissionsError error + jwtService *testJWTService + expectedError string + permissions map[string]interface{} + expectedJWTParams jimmjwx.JWTParams + }{{ + about: "all is well", + jwtService: &testJWTService{}, + expectedJWTParams: jimmjwx.JWTParams{ + Controller: ct.Id(), + User: names.NewUserTag("eve@canonical.com").String(), + Access: map[string]string{ + ct.String(): "superuser", + mt.String(): "admin", + names.NewCloudTag("test-cloud").String(): "add-model", + }, + }, + }, { + about: "check permission fails", + jwtService: &testJWTService{}, + permissions: map[string]interface{}{ + "entity1": "access_level1", + }, + checkPermissionsError: errors.E("a test error"), + expectedError: "a test error", + }, { + about: "additional permissions need checking", + jwtService: &testJWTService{}, + permissions: map[string]interface{}{ + "entity1": "access_level1", + }, + checkPermissions: map[string]string{ + "entity1": "access_level1", + }, + expectedJWTParams: jimmjwx.JWTParams{ + Controller: ct.Id(), + User: names.NewUserTag("eve@canonical.com").String(), + Access: map[string]string{ + ct.String(): "superuser", + mt.String(): "admin", + names.NewCloudTag("test-cloud").String(): "add-model", + "entity1": "access_level1", + }, + }, + }} + + for _, test := range tests { + generator := jujuauth.New( + &testDatabase{ + ctl: dbmodel.Controller{ + CloudRegions: []dbmodel.CloudRegionControllerPriority{{ + CloudRegion: dbmodel.CloudRegion{ + Cloud: dbmodel.Cloud{ + Name: "test-cloud", + }, + }, + }}, + }, + }, + &testAccessChecker{ + modelAccess: map[string]string{ + mt.String(): "admin", + }, + controllerAccess: map[string]string{ + ct.String(): "superuser", + }, + cloudAccess: map[string]string{ + names.NewCloudTag("test-cloud").String(): "add-model", + }, + permissions: test.checkPermissions, + permissionCheckErr: test.checkPermissionsError, + }, + test.jwtService, + ) + generator.SetTags(mt, ct) + + i, err := dbmodel.NewIdentity("eve@canonical.com") + c.Assert(err, qt.IsNil) + _, err = generator.MakeLoginToken(context.Background(), &openfga.User{ + Identity: i, + }) + c.Assert(err, qt.IsNil) + + _, err = generator.MakeToken(context.Background(), test.permissions) + if test.expectedError != "" { + c.Assert(err, qt.ErrorMatches, test.expectedError) + } else { + c.Assert(err, qt.IsNil) + c.Assert(test.jwtService.params, qt.DeepEquals, test.expectedJWTParams) + } + } +} diff --git a/internal/jujuapi/websocket.go b/internal/jujuapi/websocket.go index cf8f278d8..3f8d70efa 100644 --- a/internal/jujuapi/websocket.go +++ b/internal/jujuapi/websocket.go @@ -21,6 +21,7 @@ import ( "github.com/canonical/jimm/v3/internal/dbmodel" "github.com/canonical/jimm/v3/internal/errors" "github.com/canonical/jimm/v3/internal/jimm" + "github.com/canonical/jimm/v3/internal/jimm/jujuauth" "github.com/canonical/jimm/v3/internal/jimmhttp" jimmRPC "github.com/canonical/jimm/v3/internal/rpc" ) @@ -172,7 +173,7 @@ func modelInfoFromPath(path string) (uuid string, finalPath string, err error) { // We act as a proxier, handling auth on requests before forwarding the // requests to the appropriate Juju controller. func (s apiProxier) ServeWS(ctx context.Context, clientConn *websocket.Conn) { - jwtGenerator := jimm.NewJWTGenerator(s.jimm.Database, s.jimm, s.jimm.JWTService) + jwtGenerator := jujuauth.New(s.jimm.Database, s.jimm, s.jimm.JWTService) connectionFunc := controllerConnectionFunc(s, &jwtGenerator) zapctx.Debug(ctx, "Starting proxier") auditLogger := s.jimm.AddAuditLogEntry @@ -191,7 +192,7 @@ func (s apiProxier) ServeWS(ctx context.Context, clientConn *websocket.Conn) { // controllerConnectionFunc returns a function that will be used to // connect to a controller when a client makes a request. -func controllerConnectionFunc(s apiProxier, jwtGenerator *jimm.JWTGenerator) func(context.Context) (jimmRPC.WebsocketConnectionWithMetadata, error) { +func controllerConnectionFunc(s apiProxier, jwtGenerator *jujuauth.TokenGenerator) func(context.Context) (jimmRPC.WebsocketConnectionWithMetadata, error) { return func(ctx context.Context) (jimmRPC.WebsocketConnectionWithMetadata, error) { const op = errors.Op("proxy.controllerConnectionFunc") path := jimmhttp.PathElementFromContext(ctx, "path") From aec55fcbb33ee3bbcbeda200404325e7c77d1f91 Mon Sep 17 00:00:00 2001 From: Kian Parvin <46668016+kian99@users.noreply.github.com> Date: Wed, 18 Dec 2024 16:03:33 +0200 Subject: [PATCH 5/5] chore: remove JWK service from jimm (#1503) The JWK service that creates and rotates a JSON web key set doesn't and arguably shouldn't exist on the jimm struct as it goes unused. The service is quite separate from jimm. --- cmd/jimmsrv/service/service.go | 7 ++++--- internal/jimm/jimm.go | 8 -------- internal/testutils/jimmtest/jimm.go | 4 ---- internal/testutils/jimmtest/suite.go | 1 - 4 files changed, 4 insertions(+), 16 deletions(-) diff --git a/cmd/jimmsrv/service/service.go b/cmd/jimmsrv/service/service.go index 29008dae5..1a53ff085 100644 --- a/cmd/jimmsrv/service/service.go +++ b/cmd/jimmsrv/service/service.go @@ -198,7 +198,8 @@ type Params struct { // A Service is the implementation of a JIMM server. type Service struct { - jimm *jimm.JIMM + jimm *jimm.JIMM + jwkService *jimmjwx.JWKSService isLeader bool auditLogCleanupPeriod int @@ -231,7 +232,7 @@ func (s *Service) WatchModelSummaries(ctx context.Context) error { // StartJWKSRotator see internal/jimmjwx/jwks.go for details. func (s *Service) StartJWKSRotator(ctx context.Context, checkRotateRequired <-chan time.Time, initialRotateRequiredTime time.Time) error { - return s.jimm.JWKService.StartJWKSRotator(ctx, checkRotateRequired, initialRotateRequiredTime) + return s.jwkService.StartJWKSRotator(ctx, checkRotateRequired, initialRotateRequiredTime) } // MonitorResources periodically updates metrics. @@ -382,7 +383,7 @@ func NewService(ctx context.Context, p Params) (*Service, error) { p.JWTExpiryDuration = 24 * time.Hour } - jimmParameters.JWKService = jimmjwx.NewJWKSService(credentialStore) + s.jwkService = jimmjwx.NewJWKSService(credentialStore) jimmParameters.JWTService = jimmjwx.NewJWTService(jimmjwx.JWTServiceParams{ Host: p.PublicDNSName, Store: credentialStore, diff --git a/internal/jimm/jimm.go b/internal/jimm/jimm.go index 2995f3c34..cca0121d0 100644 --- a/internal/jimm/jimm.go +++ b/internal/jimm/jimm.go @@ -173,10 +173,6 @@ type Parameters struct { // with the OpenFGA ReBAC system. OpenFGAClient *openfga.OFGAClient - // JWKService holds a service responsible for generating and delivering a JWKS - // for consumption within Juju controllers. - JWKService *jimmjwx.JWKSService - // JWTService is responsible for minting JWTs to access controllers. JWTService *jimmjwx.JWTService @@ -210,10 +206,6 @@ func (p *Parameters) Validate() error { return errors.E("missing openfga client") } - if p.JWKService == nil { - return errors.E("missing jwks service") - } - if p.JWTService == nil { return errors.E("missing jwt service") } diff --git a/internal/testutils/jimmtest/jimm.go b/internal/testutils/jimmtest/jimm.go index 71137eacc..ca1fd8496 100644 --- a/internal/testutils/jimmtest/jimm.go +++ b/internal/testutils/jimmtest/jimm.go @@ -31,7 +31,6 @@ func NewJIMM(t Tester, additionalParameters *jimm.Parameters, options ...Option) UUID: uuid.NewString(), Dialer: &Dialer{}, Pubsub: &pubsub.Hub{}, - JWKService: &jimmjwx.JWKSService{}, JWTService: &jimmjwx.JWTService{}, OAuthAuthenticator: &auth, } @@ -58,9 +57,6 @@ func NewJIMM(t Tester, additionalParameters *jimm.Parameters, options ...Option) if additionalParameters.OpenFGAClient != nil { p.OpenFGAClient = additionalParameters.OpenFGAClient } - if additionalParameters.JWKService != nil { - p.JWKService = additionalParameters.JWKService - } if additionalParameters.JWTService != nil { p.JWTService = additionalParameters.JWTService } diff --git a/internal/testutils/jimmtest/suite.go b/internal/testutils/jimmtest/suite.go index a2713c7bf..3c77c4460 100644 --- a/internal/testutils/jimmtest/suite.go +++ b/internal/testutils/jimmtest/suite.go @@ -152,7 +152,6 @@ func (s *JIMMSuite) SetUpTest(c *gc.C) { OpenFGAClient: s.OFGAClient, OAuthAuthenticator: &authenticator, - JWKService: jwksService, JWTService: jwtService, })