CSS-5674 Replace database-stored access with OpenFGA #1051

babakks · 2023-10-02T12:56:07Z

Description

This PR replaces remainings of JIMM.v1 access storage with OpenFGA; namely the following has been updated:

Grant/revoke cloud access.

I've added a lot of test cases. To make the review easier here is the list tests:

GrantCloudAccess

CloudNotFound
Admin grants admin access
Admin grants add-model access
UserNotAuthorized
DialError
APIError

RevokeCloudAccess

CloudNotFound
Admin revokes 'admin' from another admin
Admin revokes 'add-model' from another admin
Admin revokes 'add-model' from a user with 'add-model' access
Admin revokes 'add-model' from a user with no access
Admin revokes 'admin' from a user with no access
Admin revokes 'add-model' access from a user who has separate tuples for all accesses (add-model/admin)
Admin revokes 'admin' access from a user who has separate tuples for all accesses (add-model/admin)
UserNotAuthorized
DialError
APIError

GrantModelAccess

ModelNotFound
Admin grants 'admin' access to a user with no access
Admin grants 'write' access to a user with no access
Admin grants 'read' access to a user with no access
Admin grants 'write' access to a user who already has 'write' access
Admin grants 'read' access to a user who already has 'write' access
Admin grants 'admin' access to themselves
Admin grants 'write' access to themselves
Admin grants 'read' access to themselves
UserNotAuthorized
DialError
APIError

RevokeModelAccess

Admin revokes 'admin' access from another admin
Admin revokes 'write' access from another admin
Admin revokes 'read' access from another admin
Admin revokes 'admin' access from a user who has 'write' access
Admin revokes 'write' access from a user who has 'write' access
Admin revokes 'read' access from a user who has 'write' access
Admin revokes 'admin' access from a user who has 'read' access
Admin revokes 'write' access from a user who has 'read' access
Admin revokes 'read' access from a user who has 'read' access
Admin revokes 'admin' access from themselves
Admin revokes 'write' access from themselves
Admin revokes 'read' access from themselves
Writer revokes 'admin' access from themselves
Writer revokes 'write' access from themselves
Writer revokes 'read' access from themselves
Reader revokes 'admin' access from themselves
Reader revokes 'write' access from themselves
Reader revokes 'read' access from themselves
Admin revokes 'admin' access from a user who has separate tuples for all accesses (read/write/admin)
Admin revokes 'write' access from a user who has separate tuples for all accesses (read/write/admin)
Admin revokes 'read' access from a user who has separate tuples for all accesses (read/write/admin)
UserNotAuthorized
DialError
APIError

Fixes CSS-5674

Change of behavior: `ModifyCloudAccess`

Note that there is a change in behavior for when revoking a users's access to a cloud. In the old implementation, if we revoked the admin access, the user would be granted with the add-model access:

switch access {
case "admin":
	uca.Access = "add-model" // <- Here 
default:
	uca.Access = ""
}

But in the new implementation, we just drop the admin relation and do not create a separate tuple to grant add-model access.

Change of behavior: `ModifyModelAccess`

In the old implementation when we're revoking a user's model access, we'd do it like this (i.e., demoting user access by one level):

switch access {
case "admin":
	uma.Access = "write"
case "write":
	uma.Access = "read"
default:
	uma.Access = ""
}

But in the new implementation, we don't add new relations to lower accesses. We just drop the relation together with all higher accesses. For example, when revoking read access, we drop read, write and admin all together.

Engineering checklist

Check only items that apply

Documentation updated
Covered by unit tests
Covered by integration tests

- removes the need for basic auth from JIMM as it will now use JWTs which Juju controllers trust

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

kian99 · 2023-10-05T12:35:57Z

internal/jimm/cloud.go

-					uca = a
-					break
+		currentRelation := targetOfgaUser.GetCloudAccess(ctx, ct)
+		if currentRelation != ofganames.NoRelation {


Might this be better as a switch statement?

kian99 · 2023-10-05T12:36:47Z

internal/jimm/cloud.go

-			}
-			return nil
-		})
+		if err := api.GrantCloudAccess(ctx, ct, ut, access); err != nil {


Do we need to do this? This is assigning access on the Juju side I believe, but Juju won't be checking it's own access permissions, it will just rely on whatever is in the JWT.

Yeah looking at

jimm/internal/jimm/cloud.go

Line 488 in 5e85687

// TODO (alesstimec) This will no longer be needed.

I'm pretty sure this isn't needed.

agreed.. this should no longer be needed..

kian99 · 2023-10-05T12:41:59Z

internal/jimm/model.go

+			}
+		}
+
+		if err := api.GrantModelAccess(ctx, mt, ut, access); err != nil {


As above, I don't think we need these calls anymore.

kian99 · 2023-10-05T13:27:14Z

internal/openfga/user.go

+			existingRelations[tt.Tuple.Relation] = nil
+		}
+
+		if ct == lastCT {


Shouldn't this come before we range over tts?

kian99 · 2023-10-05T13:28:23Z

internal/openfga/user.go

+
+	lastCT := ""
+	existingRelations := map[Relation]interface{}{}
+	for {


Also what do you think about in a future PR if we create an abstraction over this to have a cleaner way of getting all the results?

As I checked this is the only place we're iterating using continuation tokens. So, at the moment, there's no need to split this part away. But you're right. If we're going to do the pagination again, we should do it in a reusable way.

ale8k

Some questions and potential changes

ale8k · 2023-10-09T09:00:39Z

internal/jimm/cloud.go

+		return errors.E(op, errors.CodeBadRequest, "failed to recognize given access", err)
+	}
+
+	err = j.doCloudAdmin(ctx, user, ct, func(c *dbmodel.Cloud, api API) error {


The callback still takes dbmodel.Cloud, is this needed now? I'd presume not right?

ale8k · 2023-10-09T09:02:03Z

internal/jimm/cloud.go

 		if err != nil {
-			ale.Params["err"] = err.Error()
-			return errors.E(op, err)
+			return errors.E(err, "cannot update OpenFGA record after updating cloud")


Isn't the error more a tuple issue just from ofga?

also, please include op in the error

Since there's no Juju API call anymore, I changed the error message to failed to set cloud access.

ale8k · 2023-10-09T09:02:35Z

internal/jimm/cloud.go

-			}
-			return nil
-		})
+		if err := api.RevokeCloudAccess(ctx, ct, ut, access); err != nil {


Don't think this is needed either, api typically hits the controller/model, we wanna keep this purely in JIMM

ale8k · 2023-10-09T09:03:21Z

internal/jimm/cloud.go

+			return err
+		}
+
+		err = targetOfgaUser.SetCloudAccess(ctx, ct, targetRelation)


Sometime we do err := p(); err != nil, and sometimes two lines, let's try keep it inline if's with errs where we can

ale8k · 2023-10-09T09:04:36Z

internal/jimm/model.go

+		requiredAccess = "read"
+	}
+
+	err = j.doModel(ctx, user, mt, requiredAccess, func(m *dbmodel.Model, api API) error {


Like doCloud, doCloudAdmin, doModelAdmin, we don't need the model passing in anymore do we?

ale8k · 2023-10-09T09:05:34Z

internal/jimm/model.go

+			return err
+		}
+
+		err = targetOfgaUser.UnsetModelAccess(ctx, mt, relationsToRevoke...)


We aren't transactional ATM are we? So this may remove some but not all?

No, at last we decided to go with the transactional approach. To be precise, if we're removing a few tuples, all-or-none will be deleted.

internal/jujuapi/cloud.go

internal/openfga/user.go

ale8k · 2023-10-09T09:36:22Z

internal/openfga/user.go

@@ -318,6 +339,56 @@ func setResourceAccess[T ofganames.ResourceTagger](ctx context.Context, user *Us
 	return nil
 }

+// unsetMultipleResourceAccesses deletes relations that correspond to the requested resource access, atomically.
+// Note that the action is idempotent (i.e., does not return error if any of the relations does not exist).
+func unsetMultipleResourceAccesses[T ofganames.ResourceTagger](ctx context.Context, user *User, resource T, relations []Relation) error {


Honestly I'm a bit lost with ct and tts, tts is targetTuples I'm presuming? and CT is convertTag? Would prefer something like lastCT for lastConvertTag and just be explicit so it's easier on the brain initially

No. CT stands for continuation token and TT(s) is timestamped-tuple(s). But you're right. I've improved the namings.

I think honestly just call it "lastContinuationToken", as I'll probably forget in another 3 months

I've already changed it to just token and lastToken. But can change it to continuationToken and lastContinuationToken if you think so.

I know it's not the "go" way, but we have a lot of context and I feel personally I'll just forget what last token is :D

Will sort it now.

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

alesstimec

LGTM, but please add logging so that we have an easier time debugging issues in the future..

alesstimec · 2023-10-11T09:20:32Z

internal/jimm/cloud.go

-			},
+	targetRelation, err := ToCloudRelation(access)
+	if err != nil {
+		return errors.E(op, errors.CodeBadRequest, "failed to recognize given access", err)


could we, please log the access? might be useful for debugging purposes

Do you think it should be a debug entry? or error? If it's for debugging I think debug should suffice.

alesstimec · 2023-10-11T09:38:03Z

internal/jimm/cloud.go

+	})
+
+	if err != nil {
+		return errors.E(op, err)


please log this error

alesstimec · 2023-10-11T09:38:19Z

internal/jimm/cloud.go

+	})
+
+	if err != nil {
+		return errors.E(op, err)


please log this error

alesstimec · 2023-10-11T09:38:41Z

internal/jimm/model.go

+	})
+
+	if err != nil {
+		return errors.E(op, err)


please the log this error

alesstimec · 2023-10-11T09:39:23Z

internal/jimm/model.go

+	})
+
+	if err != nil {
+		return errors.E(op, err)


please log this error

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

internal/jimm/cloud.go

ale8k

lgtm bar ales' request for log lines

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

babakks · 2023-10-12T10:05:49Z

lgtm bar ales' request for log lines

I've already added them.

babakks changed the base branch from main to feature-rebac October 2, 2023 12:56

alesstimec and others added 16 commits October 2, 2023 14:59

JIMM uses JWT to log in to individual controllers.

0a3479e

- removes the need for basic auth from JIMM as it will now use JWTs which Juju controllers trust

Update godocs to indicate idempotency

b02d546

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Add unsetMultipleResourceAccess

1f0b28b

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Replace database-stored relations with OpenFGA

e0b6310

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Un-comment grant/revoke cloud access tests

72db748

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Use existing ToCloudRelation for mapping accesses to relations

6adda6d

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Merge branch 'feature-rebac' into css-5674/replace-db-with-openfga

a9fbc13

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Add UnsetModelAccess method

1ce4d99

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Assert for tuples that should exist after revoking cloud access

f3fde61

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Update ModifyModelAccess to change state in OpenFGA

9517dc9

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Update GrantModelAccess tests

fdcd5ed

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Update RevokeModelAccess tests

a755c0e

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Improve revoke cloud access tests

46a4957

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Improve revoke model access tests

a090d49

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Add more revoke cloud access cases having all relations separately

3e3b83e

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Add more revoke model access cases having all relations separately

5e85687

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

babakks marked this pull request as ready for review October 4, 2023 13:40

mina1460 requested review from alesstimec, ale8k, kian99 and mina1460 October 4, 2023 14:34

kian99 reviewed Oct 5, 2023

View reviewed changes

ale8k reviewed Oct 9, 2023

View reviewed changes

babakks added 6 commits October 10, 2023 12:58

Change if-statements with switch-statements

b475081

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Remove unnecessary Juju API call to grant/revoke model access

0a467c4

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Remove unnecessary Juju API call to grant/revoke cloud access

f435efe

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Improve failure error message

9a88556

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Change to inline if-err checks where possible

03916b2

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Improve local var naming

543f39d

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

babakks added 3 commits October 10, 2023 13:51

Add op to re-thrown error

e1ade1a

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Remove unnecessary test for idempotent re-granting of cloud access

e43cc66

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Merge branch 'feature-rebac' into css-5674/replace-db-with-openfga

0cf9f3e

alesstimec approved these changes Oct 11, 2023

View reviewed changes

babakks added 2 commits October 11, 2023 11:02

Log errors when granting/revoking access

42a373b

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

Add tests to verify returning unrecognized access error

29ac788

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

ale8k reviewed Oct 11, 2023

View reviewed changes

internal/jimm/cloud.go Show resolved Hide resolved

ale8k approved these changes Oct 12, 2023

View reviewed changes

Improve naming

92b3b71

Signed-off-by: Babak K. Shandiz <babak.k.shandiz@canonical.com>

kian99 approved these changes Oct 12, 2023

View reviewed changes

babakks merged commit 61c1370 into canonical:feature-rebac Oct 12, 2023

babakks deleted the css-5674/replace-db-with-openfga branch October 12, 2023 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSS-5674 Replace database-stored access with OpenFGA #1051

CSS-5674 Replace database-stored access with OpenFGA #1051

babakks commented Oct 2, 2023 •

edited

Loading

kian99 Oct 5, 2023

kian99 Oct 5, 2023

kian99 Oct 5, 2023

alesstimec Oct 10, 2023

kian99 Oct 5, 2023

kian99 Oct 5, 2023

kian99 Oct 5, 2023

babakks Oct 10, 2023

ale8k left a comment

ale8k Oct 9, 2023

ale8k Oct 9, 2023

alesstimec Oct 10, 2023

babakks Oct 10, 2023

ale8k Oct 9, 2023

ale8k Oct 9, 2023

ale8k Oct 9, 2023

ale8k Oct 9, 2023

babakks Oct 10, 2023

ale8k Oct 9, 2023

babakks Oct 10, 2023

ale8k Oct 11, 2023

babakks Oct 11, 2023

ale8k Oct 12, 2023

babakks Oct 12, 2023

alesstimec left a comment

alesstimec Oct 11, 2023

babakks Oct 11, 2023

alesstimec Oct 11, 2023

alesstimec Oct 11, 2023

alesstimec Oct 11, 2023

alesstimec Oct 11, 2023

ale8k left a comment

babakks commented Oct 12, 2023

CSS-5674 Replace database-stored access with OpenFGA #1051

CSS-5674 Replace database-stored access with OpenFGA #1051

Conversation

babakks commented Oct 2, 2023 • edited Loading

Description

GrantCloudAccess

RevokeCloudAccess

GrantModelAccess

RevokeModelAccess

Change of behavior: ModifyCloudAccess

Change of behavior: ModifyModelAccess

Engineering checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ale8k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alesstimec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ale8k left a comment

Choose a reason for hiding this comment

babakks commented Oct 12, 2023

babakks commented Oct 2, 2023 •

edited

Loading

Change of behavior: `ModifyCloudAccess`

Change of behavior: `ModifyModelAccess`