From f1325524e42e92c970fd02d218141187ef3c02e6 Mon Sep 17 00:00:00 2001 From: Ales Stimec Date: Fri, 6 Dec 2024 12:10:14 +0100 Subject: [PATCH] refactor[internal/openfga] simplifies some model access checks --- internal/jimm/model.go | 28 ++++++------------------ internal/jujuapi/cloud.go | 41 +++++++++++------------------------ internal/openfga/user.go | 9 ++++++++ internal/openfga/user_test.go | 12 ++++++++++ 4 files changed, 41 insertions(+), 49 deletions(-) diff --git a/internal/jimm/model.go b/internal/jimm/model.go index 5891f6fd6..ea238025a 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1000,10 +1000,10 @@ func (j *JIMM) RevokeModelAccess(ctx context.Context, user *openfga.User, mt nam return errors.E(op, errors.CodeBadRequest, fmt.Sprintf("failed to recognize given access: %q", access), err) } - requiredAccess := "admin" + requiredAccess := ofganames.AdministratorRelation if user.Tag() == ut { // If the user is attempting to revoke their own access. - requiredAccess = "read" + requiredAccess = ofganames.ReaderRelation } err = j.doModel(ctx, user, mt, requiredAccess, func(_ *dbmodel.Model, _ API) error { @@ -1173,7 +1173,7 @@ func (j *JIMM) ValidateModelUpgrade(ctx context.Context, user *openfga.User, mt // returned from the dial operation. If the given function returns an error // that error will be returned with the code unmasked. func (j *JIMM) doModelAdmin(ctx context.Context, user *openfga.User, mt names.ModelTag, f func(*dbmodel.Model, API) error) error { - return j.doModel(ctx, user, mt, "admin", f) + return j.doModel(ctx, user, mt, ofganames.AdministratorRelation, f) } // GetUserModelAccess returns the access level a user has against a specific model. @@ -1182,7 +1182,7 @@ func (j *JIMM) GetUserModelAccess(ctx context.Context, user *openfga.User, model return ToModelAccessString(accessLevel), nil } -func (j *JIMM) doModel(ctx context.Context, user *openfga.User, mt names.ModelTag, access string, f func(*dbmodel.Model, API) error) error { +func (j *JIMM) doModel(ctx context.Context, user *openfga.User, mt names.ModelTag, requireRelation openfga.Relation, f func(*dbmodel.Model, API) error) error { const op = errors.Op("jimm.doModel") zapctx.Info(ctx, string(op)) @@ -1193,11 +1193,12 @@ func (j *JIMM) doModel(ctx context.Context, user *openfga.User, mt names.ModelTa return errors.E(op, err) } - accessLevel, err := j.GetUserModelAccess(ctx, user, mt) + hasAccess, err := user.HasModelRelation(ctx, mt, requireRelation) if err != nil { return errors.E(op, err) } - if !allowedModelAccess[access][accessLevel] { + + if !hasAccess { // If the user doesn't have correct access on the model return // an unauthorized error. return errors.E(op, errors.CodeUnauthorized, "unauthorized") @@ -1214,21 +1215,6 @@ func (j *JIMM) doModel(ctx context.Context, user *openfga.User, mt names.ModelTa return nil } -var allowedModelAccess = map[string]map[string]bool{ - "admin": { - "admin": true, - }, - "write": { - "admin": true, - "write": true, - }, - "read": { - "admin": true, - "write": true, - "read": true, - }, -} - // ChangeModelCredential changes the credential used with a model on both // the controller and the local database. func (j *JIMM) ChangeModelCredential(ctx context.Context, user *openfga.User, modelTag names.ModelTag, cloudCredentialTag names.CloudCredentialTag) error { diff --git a/internal/jujuapi/cloud.go b/internal/jujuapi/cloud.go index 1adb9b775..baf25e2a5 100644 --- a/internal/jujuapi/cloud.go +++ b/internal/jujuapi/cloud.go @@ -17,6 +17,7 @@ import ( "github.com/canonical/jimm/v3/internal/jimm" "github.com/canonical/jimm/v3/internal/jujuapi/rpc" "github.com/canonical/jimm/v3/internal/openfga" + ofganames "github.com/canonical/jimm/v3/internal/openfga/names" ) func init() { @@ -276,30 +277,18 @@ func collapseUpdateCredentialResults(args jujuparams.TaggedCredentials, updateRe return results } -func userModelAccess(ctx context.Context, user *openfga.User, model names.ModelTag) (string, error) { - isModelAdmin, err := openfga.IsAdministrator(ctx, user, model) - if err != nil { - return "", errors.E(err) - } - if isModelAdmin { - return "admin", nil - } - hasWriteAccess, err := user.IsModelWriter(ctx, model) - if err != nil { - return "", errors.E(err) - } - if hasWriteAccess { - return "write", nil - } - hasReadAccess, err := user.IsModelReader(ctx, model) - if err != nil { - return "", errors.E(err) - } - if hasReadAccess { - return "read", nil +func userModelAccess(ctx context.Context, user *openfga.User, model names.ModelTag) string { + userRelation := user.GetModelAccess(ctx, model) + switch userRelation { + case ofganames.AdministratorRelation: + return "admin" + case ofganames.WriterRelation: + return "write" + case ofganames.ReaderRelation: + return "read" + default: + return "" } - - return "", nil } // CredentialContents implements the CredentialContents method of the Cloud (v5) facade. @@ -326,13 +315,9 @@ func getIdentityCredentials(ctx context.Context, user *openfga.User, j JIMM, arg } mas := make([]jujuparams.ModelAccess, len(c.Models)) for i, m := range c.Models { - userModelAccess, err := userModelAccess(ctx, user, m.ResourceTag()) - if err != nil { - return nil, errors.E(err) - } mas[i] = jujuparams.ModelAccess{ Model: m.Name, - Access: userModelAccess, + Access: userModelAccess(ctx, user, m.ResourceTag()), } } return &jujuparams.ControllerCredentialInfo{ diff --git a/internal/openfga/user.go b/internal/openfga/user.go index 169746f39..529f7f80a 100644 --- a/internal/openfga/user.go +++ b/internal/openfga/user.go @@ -79,6 +79,15 @@ func (u *User) IsModelWriter(ctx context.Context, resource names.ModelTag) (bool return isWriter, nil } +// HasModelRelation returns true if user has the specified relation to the model. +func (u *User) HasModelRelation(ctx context.Context, resource names.ModelTag, relation Relation) (bool, error) { + hasRelation, err := checkRelation(ctx, u, resource, relation) + if err != nil { + return false, errors.E(err) + } + return hasRelation, nil +} + // IsServiceAccountAdmin returns true if the user has administrator relation to the service account. func (u *User) IsServiceAccountAdmin(ctx context.Context, clientID jimmnames.ServiceAccountTag) (bool, error) { isAdmin, err := checkRelation(ctx, u, clientID, ofganames.AdministratorRelation) diff --git a/internal/openfga/user_test.go b/internal/openfga/user_test.go index b381f33bd..febb43e84 100644 --- a/internal/openfga/user_test.go +++ b/internal/openfga/user_test.go @@ -133,6 +133,18 @@ func (s *userTestSuite) TestModelAccess(c *gc.C) { allowed, err = adamUser.IsModelWriter(ctx, model) c.Assert(err, gc.IsNil) c.Assert(allowed, gc.Equals, false) + + allowed, err = eveUser.HasModelRelation(ctx, model, ofganames.ReaderRelation) + c.Assert(err, gc.IsNil) + c.Assert(allowed, gc.Equals, true) + + allowed, err = eveUser.HasModelRelation(ctx, model, ofganames.AdministratorRelation) + c.Assert(err, gc.IsNil) + c.Assert(allowed, gc.Equals, true) + + allowed, err = adamUser.HasModelRelation(ctx, model, ofganames.ReaderRelation) + c.Assert(err, gc.IsNil) + c.Assert(allowed, gc.Equals, false) } func (s *userTestSuite) TestSetModelAccess(c *gc.C) {