Skip to content

Commit

Permalink
refactor openfga helper functions to reduce duplication (#1369)
Browse files Browse the repository at this point in the history 
* refactor openfga helper functions to reduce duplication

* move method to openfga.go

* fix TestRevokeOfferAccess tests
  • Loading branch information
@kian99
kian99 authored Sep 18, 2024
1 parent 99d5fc3 commit 428b2ce
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 141 deletions.
2 changes: 1 addition & 1 deletion internal/jimm/applicationoffer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ func (j *JIMM) RevokeOfferAccess(ctx context.Context, user *openfga.User, offerU
if err != nil {
return errors.E(op, err)
}
err = tUser.UnsetApplicationOfferAccess(ctx, offer.ResourceTag(), targetRelation, false)
err = tUser.UnsetApplicationOfferAccess(ctx, offer.ResourceTag(), targetRelation)
if err != nil {
return errors.E(op, err, "failed to unset given access")
}
Expand Down
36 changes: 13 additions & 23 deletions internal/jimm/applicationoffer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,11 +195,11 @@ func TestRevokeOfferAccess(t *testing.T) {
expectedAccessLevel string
expectedAccessLevelOnError string // This expectation is meant to ensure there'll be no unpredicted behavior (like changing existing relations) after an error has occurred
}{{
about: "admin revokes a model's admin user's admin access - an error returns (relation is indirect)",
about: "admin revokes a model admin user's admin access - an error returns (relation is indirect)",
parameterFunc: func(env *environment) (dbmodel.Identity, dbmodel.Identity, string, jujuparams.OfferAccessPermission) {
return env.users[1], env.users[0], "test-offer-url", jujuparams.OfferAdminAccess
},
expectedError: "failed to unset given access",
expectedError: "unable to completely revoke given access due to other relations.*",
expectedAccessLevelOnError: "admin",
}, {
about: "model admin revokes an admin user admin access - user has no access",
Expand All @@ -219,66 +219,56 @@ func TestRevokeOfferAccess(t *testing.T) {
return env.users[6], env.users[1], "test-offer-url", jujuparams.OfferAdminAccess
},
expectedAccessLevel: "",
}, {
about: "admin revokes an admin user consume access - an error returns (no direct relation to remove)",
parameterFunc: func(env *environment) (dbmodel.Identity, dbmodel.Identity, string, jujuparams.OfferAccessPermission) {
return env.users[0], env.users[1], "test-offer-url", jujuparams.OfferConsumeAccess
},
expectedError: "failed to unset given access",
expectedAccessLevelOnError: "admin",
}, {
about: "admin revokes an admin user read access - an error returns (no direct relation to remove)",
parameterFunc: func(env *environment) (dbmodel.Identity, dbmodel.Identity, string, jujuparams.OfferAccessPermission) {
return env.users[0], env.users[1], "test-offer-url", jujuparams.OfferReadAccess
},
expectedError: "failed to unset given access",
expectedError: "unable to completely revoke given access due to other relations.*",
expectedAccessLevelOnError: "admin",
}, {
about: "admin revokes a consume user admin access - an error returns (no direct relation to remove)",
about: "admin revokes a consume user admin access - user keeps consume access",
parameterFunc: func(env *environment) (dbmodel.Identity, dbmodel.Identity, string, jujuparams.OfferAccessPermission) {
return env.users[0], env.users[2], "test-offer-url", jujuparams.OfferAdminAccess
},
expectedError: "failed to unset given access",
expectedAccessLevelOnError: "consume",
expectedAccessLevel: "consume",
}, {
about: "admin revokes a consume user consume access - user has no access",
parameterFunc: func(env *environment) (dbmodel.Identity, dbmodel.Identity, string, jujuparams.OfferAccessPermission) {
return env.users[0], env.users[2], "test-offer-url", jujuparams.OfferConsumeAccess
},
expectedAccessLevel: "",
}, {
about: "admin revokes a consume user read access - an error returns (no direct relation to remove)",
about: "admin revokes a consume user read access - user still has consume access",
parameterFunc: func(env *environment) (dbmodel.Identity, dbmodel.Identity, string, jujuparams.OfferAccessPermission) {
return env.users[0], env.users[2], "test-offer-url", jujuparams.OfferReadAccess
},
expectedError: "failed to unset given access",
expectedError: "unable to completely revoke given access due to other relations.*",
expectedAccessLevelOnError: "consume",
}, {
about: "admin revokes a read user admin access - an error returns (no direct relation to remove)",
about: "admin revokes a read user admin access - user keeps read access",
parameterFunc: func(env *environment) (dbmodel.Identity, dbmodel.Identity, string, jujuparams.OfferAccessPermission) {
return env.users[0], env.users[3], "test-offer-url", jujuparams.OfferAdminAccess
},
expectedError: "failed to unset given access",
expectedAccessLevelOnError: "read",
expectedAccessLevel: "read",
}, {
about: "admin revokes a read user consume access - an error returns (no direct relation to remove)",
about: "admin revokes a read user consume access - user keeps read access",
parameterFunc: func(env *environment) (dbmodel.Identity, dbmodel.Identity, string, jujuparams.OfferAccessPermission) {
return env.users[0], env.users[3], "test-offer-url", jujuparams.OfferConsumeAccess
},
expectedError: "failed to unset given access",
expectedAccessLevelOnError: "read",
expectedAccessLevel: "read",
}, {
about: "admin revokes a read user read access - user has no access",
parameterFunc: func(env *environment) (dbmodel.Identity, dbmodel.Identity, string, jujuparams.OfferAccessPermission) {
return env.users[0], env.users[3], "test-offer-url", jujuparams.OfferReadAccess
},
expectedAccessLevel: "",
}, {
about: "admin tries to revoke access to user that does not have access - an error returns",
about: "admin tries to revoke access to user that does not have access - user continues to have no access",
parameterFunc: func(env *environment) (dbmodel.Identity, dbmodel.Identity, string, jujuparams.OfferAccessPermission) {
return env.users[0], env.users[4], "test-offer-url", jujuparams.OfferReadAccess
},
expectedError: "failed to unset given access",
expectedAccessLevel: "",
}, {
about: "user with consume access cannot revoke access",
parameterFunc: func(env *environment) (dbmodel.Identity, dbmodel.Identity, string, jujuparams.OfferAccessPermission) {
Expand Down
15 changes: 6 additions & 9 deletions internal/openfga/names/names.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ func ConvertTagWithRelation[RT ResourceTagger](t RT, relation cofga.Relation) *T
// ConvertTag converts a resource tag to an OpenFGA tag where the resource tag is limited to
// specific types of tags.
func ConvertTag[RT ResourceTagger](t RT) *Tag {
tag := names.Tag(t)
return ConvertGenericTag(tag)
}

// ConvertGenericTag converts any tag implementing the names.tag interface to an OpenFGA tag.
func ConvertGenericTag(t names.Tag) *Tag {
id := t.Id()
if t.Kind() == names.UserTagKind && id == EveryoneUser {
// A user with ID "*" represents "everyone" in OpenFGA and allows checks like
Expand All @@ -90,15 +96,6 @@ func ConvertTag[RT ResourceTagger](t RT) *Tag {
return tag
}

// ConvertGenericTag converts any tag implementing the names.tag interface to an OpenFGA tag.
func ConvertGenericTag(t names.Tag) *Tag {
tag := &Tag{
ID: t.Id(),
Kind: cofga.Kind(t.Kind()),
}
return tag
}

// BlankKindTag returns a tag of the specified kind with a blank id.
// This function should only be used when removing relations to a specific
// resource (e.g. we want to remove all controller relations to a specific
Expand Down
108 changes: 51 additions & 57 deletions internal/openfga/openfga.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,50 @@ func publicAccessAdaptor(tt cofga.TimestampedTuple) cofga.TimestampedTuple {
return tt
}

// setResourceAccess creates a relation to model the requested resource access.
// Note that the action is idempotent (does not return error if the relation already exists).
func (o *OFGAClient) setResourceAccess(ctx context.Context, object, target names.Tag, relation Relation) error {
if object == nil || target == nil {
return errors.E("missing object or target for relation")
}
err := o.AddRelation(ctx, Tuple{
Object: ofganames.ConvertGenericTag(object),
Relation: relation,
Target: ofganames.ConvertGenericTag(target),
})
if err != nil {
// if the tuple already exist we don't return an error.
// TODO we should opt to check against specific errors via checking their code/metadata.
if strings.Contains(err.Error(), "cannot write a tuple which already exists") {
return nil
}
return errors.E(err)
}
return nil
}

// unsetResourceAccess deletes a relation that corresponds to the requested resource access.
// Note that the action is idempotent (does not return error if the relation does not exist).
func (o *OFGAClient) unsetResourceAccess(ctx context.Context, object, target names.Tag, relation Relation) error {
if object == nil || target == nil {
return errors.E("missing object or target for relation")
}
err := o.RemoveRelation(ctx, Tuple{
Object: ofganames.ConvertGenericTag(object),
Relation: relation,
Target: ofganames.ConvertGenericTag(target),
})
if err != nil {
// if the tuple already exist we don't return an error.
// TODO we should opt to check against specific errors via checking their code/metadata.
if strings.Contains(err.Error(), "cannot delete a tuple which does not exist") {
return nil
}
return errors.E(err)
}
return nil
}

// getRelatedObjects returns all objects where the user has a valid relation to them.
// Such as all the groups a user resides in.
//
Expand Down Expand Up @@ -225,32 +269,12 @@ func (o *OFGAClient) removeTuples(ctx context.Context, tuple Tuple) (err error)

// AddControllerModel adds a relation between a controller and a model.
func (o *OFGAClient) AddControllerModel(ctx context.Context, controller names.ControllerTag, model names.ModelTag) error {
if err := o.AddRelation(
ctx,
Tuple{
Object: ofganames.ConvertTag(controller),
Relation: ofganames.ControllerRelation,
Target: ofganames.ConvertTag(model),
},
); err != nil {
return errors.E(err)
}
return nil
return o.setResourceAccess(ctx, controller, model, ofganames.ControllerRelation)
}

// RemoveControllerModel removes a relation between a controller and a model.
func (o *OFGAClient) RemoveControllerModel(ctx context.Context, controller names.ControllerTag, model names.ModelTag) error {
if err := o.RemoveRelation(
ctx,
Tuple{
Object: ofganames.ConvertTag(controller),
Relation: ofganames.ControllerRelation,
Target: ofganames.ConvertTag(model),
},
); err != nil {
return errors.E(err)
}
return nil
return o.unsetResourceAccess(ctx, controller, model, ofganames.ControllerRelation)
}

// RemoveModel removes a model.
Expand All @@ -268,17 +292,7 @@ func (o *OFGAClient) RemoveModel(ctx context.Context, model names.ModelTag) erro

// AddModelApplicationOffer adds a relation between a model and an application offer.
func (o *OFGAClient) AddModelApplicationOffer(ctx context.Context, model names.ModelTag, offer names.ApplicationOfferTag) error {
if err := o.AddRelation(
ctx,
Tuple{
Object: ofganames.ConvertTag(model),
Relation: ofganames.ModelRelation,
Target: ofganames.ConvertTag(offer),
},
); err != nil {
return errors.E(err)
}
return nil
return o.setResourceAccess(ctx, model, offer, ofganames.ModelRelation)
}

// RemoveApplicationOffer removes an application offer.
Expand All @@ -296,6 +310,7 @@ func (o *OFGAClient) RemoveApplicationOffer(ctx context.Context, offer names.App

// RemoveGroup removes a group.
func (o *OFGAClient) RemoveGroup(ctx context.Context, group jimmnames.GroupTag) error {
// Remove all access to a group. I.e. user->group
if err := o.removeTuples(
ctx,
Tuple{
Expand All @@ -305,6 +320,7 @@ func (o *OFGAClient) RemoveGroup(ctx context.Context, group jimmnames.GroupTag)
); err != nil {
return errors.E(err)
}
// Next remove all access that a group had. I.e. group->model
// We need to loop through all resource types because the OpenFGA Read API does not provide
// means for only specifying a user resource, it must be paired with an object type.
for _, kind := range resourceTypes {
Expand Down Expand Up @@ -340,33 +356,11 @@ func (o *OFGAClient) RemoveCloud(ctx context.Context, cloud names.CloudTag) erro
// AddCloudController adds a controller relation between a controller and
// a cloud.
func (o *OFGAClient) AddCloudController(ctx context.Context, cloud names.CloudTag, controller names.ControllerTag) error {
if err := o.AddRelation(ctx, Tuple{
Object: ofganames.ConvertTag(controller),
Relation: ofganames.ControllerRelation,
Target: ofganames.ConvertTag(cloud),
}); err != nil {
// if the tuple already exist we don't return an error.
if strings.Contains(err.Error(), "cannot write a tuple which already exists") {
return nil
}
return errors.E(err)
}
return nil
return o.setResourceAccess(ctx, controller, cloud, ofganames.ControllerRelation)
}

// AddController adds a controller relation between JIMM and the added controller. Meaning
// JIMM admins also have administrator access to the added controller
func (o *OFGAClient) AddController(ctx context.Context, jimm names.ControllerTag, controller names.ControllerTag) error {
if err := o.AddRelation(ctx, Tuple{
Object: ofganames.ConvertTag(jimm),
Relation: ofganames.ControllerRelation,
Target: ofganames.ConvertTag(controller),
}); err != nil {
// if the tuple already exist we don't return an error.
if strings.Contains(err.Error(), "cannot write a tuple which already exists") {
return nil
}
return errors.E(err)
}
return nil
return o.setResourceAccess(ctx, jimm, controller, ofganames.ControllerRelation)
}
Loading

0 comments on commit 428b2ce

Please sign in to comment.