-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
CSS-5674 Replace database-stored access with OpenFGA #1051
Changes from 16 commits
0a3479e
b02d546
1f0b28b
e0b6310
72db748
6adda6d
a9fbc13
1ce4d99
f3fde61
9517dc9
fdcd5ed
a755c0e
46a4957
a090d49
3e3b83e
5e85687
b475081
0a467c4
f435efe
9a88556
03916b2
543f39d
e1ade1a
e43cc66
0cf9f3e
42a373b
29ac788
92b3b71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -552,124 +552,114 @@ func (j *JIMM) doCloudAdmin(ctx context.Context, u *openfga.User, ct names.Cloud | |||
// given user. If the cloud is not found then an error with the code | ||||
// CodeNotFound is returned. If the authenticated user does not have admin | ||||
// access to the cloud then an error with the code CodeUnauthorized is | ||||
// returned. If the ModifyCloudAccess API call retuns an error the error | ||||
// code is not masked. | ||||
func (j *JIMM) GrantCloudAccess(ctx context.Context, u *dbmodel.User, ct names.CloudTag, ut names.UserTag, access string) error { | ||||
// returned. If the ModifyCloudAccess API call returns an error, the error is | ||||
// returned with the error code unmasked. | ||||
func (j *JIMM) GrantCloudAccess(ctx context.Context, user *openfga.User, ct names.CloudTag, ut names.UserTag, access string) error { | ||||
const op = errors.Op("jimm.GrantCloudAccess") | ||||
|
||||
// TODO (alesstimec) granting and revoking access tbd in a followup | ||||
return errors.E(errors.CodeNotImplemented) | ||||
/* | ||||
ale := dbmodel.AuditLogEntry{ | ||||
Time: time.Now().UTC().Round(time.Millisecond), | ||||
Tag: ct.String(), | ||||
UserTag: u.Tag().String(), | ||||
Action: "grant", | ||||
Params: dbmodel.StringMap{ | ||||
"user": ut.String(), | ||||
"access": access, | ||||
}, | ||||
targetRelation, err := ToCloudRelation(access) | ||||
if err != nil { | ||||
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 { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The callback still takes dbmodel.Cloud, is this needed now? I'd presume not right? |
||||
targetUser := &dbmodel.User{} | ||||
targetUser.SetTag(ut) | ||||
err := j.Database.GetUser(ctx, targetUser) | ||||
if err != nil { | ||||
return err | ||||
} | ||||
defer j.addAuditLogEntry(&ale) | ||||
targetOfgaUser := openfga.NewUser(targetUser, j.OpenFGAClient) | ||||
|
||||
err := j.doCloudAdmin(ctx, u, ct, func(c *dbmodel.Cloud, api API) error { | ||||
targetUser := dbmodel.User{ | ||||
Username: ut.Id(), | ||||
} | ||||
if err := j.Database.GetUser(ctx, &targetUser); err != nil { | ||||
return err | ||||
} | ||||
if err := api.GrantCloudAccess(ctx, ct, ut, access); err != nil { | ||||
return err | ||||
} | ||||
var uca dbmodel.UserCloudAccess | ||||
for _, a := range c.Users { | ||||
if a.Username == targetUser.Username { | ||||
uca = a | ||||
break | ||||
currentRelation := targetOfgaUser.GetCloudAccess(ctx, ct) | ||||
if currentRelation != ofganames.NoRelation { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might this be better as a switch statement? |
||||
if targetRelation == ofganames.CanAddModelRelation { | ||||
return nil | ||||
} else if targetRelation == ofganames.AdministratorRelation { | ||||
if currentRelation == ofganames.AdministratorRelation { | ||||
return nil | ||||
} | ||||
} | ||||
uca.User = targetUser | ||||
uca.Cloud = *c | ||||
uca.Access = access | ||||
} | ||||
|
||||
if err := j.Database.UpdateUserCloudAccess(ctx, &uca); err != nil { | ||||
return errors.E(op, err, "cannot update database after updating controller") | ||||
} | ||||
return nil | ||||
}) | ||||
if err := api.GrantCloudAccess(ctx, ct, ut, access); err != nil { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah looking at Line 488 in 5e85687
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed.. this should no longer be needed.. |
||||
return err | ||||
} | ||||
|
||||
err = targetOfgaUser.SetCloudAccess(ctx, ct, targetRelation) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sometime we do err := p(); err != nil, and sometimes two lines, let's try keep it inline if's with errs where we can |
||||
if err != nil { | ||||
ale.Params["err"] = err.Error() | ||||
return errors.E(op, err) | ||||
return errors.E(err, "cannot update OpenFGA record after updating cloud") | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't the error more a tuple issue just from ofga? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, please include op in the error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since there's no Juju API call anymore, I changed the error message to |
||||
} | ||||
ale.Success = true | ||||
return nil | ||||
*/ | ||||
}) | ||||
|
||||
if err != nil { | ||||
return errors.E(op, err) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please log this error |
||||
} | ||||
return nil | ||||
} | ||||
|
||||
// RevokeCloudAccess revokes the given access level on the given cloud from | ||||
// the given user. If the cloud is not found then an error with the code | ||||
// CodeNotFound is returned. If the authenticated user does not have admin | ||||
// access to the cloud then an error with the code CodeUnauthorized is | ||||
// returned. If the ModifyCloudAccess API call retuns an error the error | ||||
// code is not masked. | ||||
func (j *JIMM) RevokeCloudAccess(ctx context.Context, u *dbmodel.User, ct names.CloudTag, ut names.UserTag, access string) error { | ||||
// returned. If the ModifyCloudAccess API call returns an error, the error | ||||
// is returned with the code unmasked. | ||||
func (j *JIMM) RevokeCloudAccess(ctx context.Context, user *openfga.User, ct names.CloudTag, ut names.UserTag, access string) error { | ||||
const op = errors.Op("jimm.RevokeCloudAccess") | ||||
|
||||
// TODO (alesstimec) granting and revoking access tbd in a followup | ||||
return errors.E(errors.CodeNotImplemented) | ||||
|
||||
/* | ||||
ale := dbmodel.AuditLogEntry{ | ||||
Time: time.Now().UTC().Round(time.Millisecond), | ||||
Tag: ct.String(), | ||||
UserTag: u.Tag().String(), | ||||
Action: "revoke", | ||||
Params: dbmodel.StringMap{ | ||||
"user": ut.String(), | ||||
"access": access, | ||||
}, | ||||
targetRelation, err := ToCloudRelation(access) | ||||
if err != nil { | ||||
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 { | ||||
targetUser := &dbmodel.User{} | ||||
targetUser.SetTag(ut) | ||||
err := j.Database.GetUser(ctx, targetUser) | ||||
if err != nil { | ||||
return err | ||||
} | ||||
defer j.addAuditLogEntry(&ale) | ||||
targetOfgaUser := openfga.NewUser(targetUser, j.OpenFGAClient) | ||||
|
||||
err := j.doCloudAdmin(ctx, u, ct, func(c *dbmodel.Cloud, api API) error { | ||||
targetUser := dbmodel.User{ | ||||
Username: ut.Id(), | ||||
} | ||||
if err := j.Database.GetUser(ctx, &targetUser); err != nil { | ||||
return err | ||||
} | ||||
if err := api.RevokeCloudAccess(ctx, ct, ut, access); err != nil { | ||||
return err | ||||
currentRelation := targetOfgaUser.GetCloudAccess(ctx, ct) | ||||
if currentRelation == ofganames.NoRelation { | ||||
return nil | ||||
} | ||||
|
||||
var relationsToRevoke []openfga.Relation | ||||
if targetRelation == ofganames.CanAddModelRelation { | ||||
// If we're revoking "add-model" access, in addition to the "add-model" relation, we should also revoke the | ||||
// "admin" relation. That's because having an "admin" relation indirectly grants the "add-model" permission | ||||
// to the user. | ||||
relationsToRevoke = []openfga.Relation{ | ||||
ofganames.CanAddModelRelation, | ||||
ofganames.AdministratorRelation, | ||||
} | ||||
var uca dbmodel.UserCloudAccess | ||||
for _, a := range c.Users { | ||||
if a.Username == targetUser.Username { | ||||
uca = a | ||||
break | ||||
} | ||||
} else if targetRelation == ofganames.AdministratorRelation { | ||||
if currentRelation == ofganames.CanAddModelRelation { | ||||
return nil | ||||
} | ||||
uca.User = targetUser | ||||
uca.Cloud = *c | ||||
switch access { | ||||
case "admin": | ||||
uca.Access = "add-model" | ||||
default: | ||||
uca.Access = "" | ||||
relationsToRevoke = []openfga.Relation{ | ||||
ofganames.AdministratorRelation, | ||||
} | ||||
} | ||||
|
||||
if err := j.Database.UpdateUserCloudAccess(ctx, &uca); err != nil { | ||||
return errors.E(op, err, "cannot update database after updating controller") | ||||
} | ||||
return nil | ||||
}) | ||||
if err := api.RevokeCloudAccess(ctx, ct, ut, access); err != nil { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't think this is needed either, api typically hits the controller/model, we wanna keep this purely in JIMM |
||||
return err | ||||
} | ||||
|
||||
err = targetOfgaUser.UnsetCloudAccess(ctx, ct, relationsToRevoke...) | ||||
if err != nil { | ||||
ale.Params["err"] = err.Error() | ||||
return errors.E(op, err) | ||||
return errors.E(err, "cannot update OpenFGA record after updating cloud") | ||||
} | ||||
ale.Success = true | ||||
return nil | ||||
*/ | ||||
}) | ||||
|
||||
if err != nil { | ||||
return errors.E(op, err) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please log this error |
||||
} | ||||
return nil | ||||
} | ||||
|
||||
// RemoveCloud removes the given cloud from JAAS If the cloud is not found | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we, please log the
access
? might be useful for debugging purposesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it should be a
debug
entry? orerror
? If it's for debugging I thinkdebug
should suffice.