Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor[internal/openfga] simplifies some model access checks #1484

Merged
merged 3 commits into from
Dec 12, 2024
Merged

refactor[internal/openfga] simplifies some model access checks #1484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f132552
refactor[internal/openfga] simplifies some model access checks
alesstimec Dec 6, 2024
51104c1
Merge branch 'v3' into simplify-model-access-checks
alesstimec Dec 6, 2024
e894f1f
Merge branch 'v3' into simplify-model-access-checks
alesstimec Dec 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 7 additions & 21 deletions internal/jimm/model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1081,10 +1081,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 {
Expand Down Expand Up @@ -1260,7 +1260,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.
Expand All @@ -1269,7 +1269,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))

Expand All @@ -1280,11 +1280,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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically before we were handling hierarchy between roles manually? And now we rely on our authorization model, good

// If the user doesn't have correct access on the model return
// an unauthorized error.
return errors.E(op, errors.CodeUnauthorized, "unauthorized")
Expand All @@ -1301,21 +1302,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 {
Expand Down
41 changes: 13 additions & 28 deletions internal/jujuapi/cloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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 {
Copy link
Contributor

@SimoneDutto SimoneDutto Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

godoc please

userRelation := user.GetModelAccess(ctx, model)
switch userRelation {
case ofganames.AdministratorRelation:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we have ofganames now, but I really think permission package with access constants would've been better from juju.

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.
Expand All @@ -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{
Expand Down
9 changes: 9 additions & 0 deletions internal/openfga/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
12 changes: 12 additions & 0 deletions internal/openfga/user_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Loading