feat: premissions revamp#2398
Conversation
🔧 Configuration Changes DetectedThis PR contains changes that will affect the Helm chart configuration. A draft infrastructure PR has been automatically created to preview these changes: 📋 Draft PR: https://github.com/theopenlane/openlane-infra/pull/928 Changes Preview:✅ Updated ConfigMap template
The draft infrastructure PR will be closed automatically after this core PR is merged. |
Signed-off-by: Sarah Funkhouser <147884153+golanglemonade@users.noreply.github.com>
Signed-off-by: Sarah Funkhouser <147884153+golanglemonade@users.noreply.github.com>
b0b52b4 to
de58a11
Compare
Signed-off-by: Sarah Funkhouser <147884153+golanglemonade@users.noreply.github.com>
Signed-off-by: Sarah Funkhouser <147884153+golanglemonade@users.noreply.github.com>
Signed-off-by: Sarah Funkhouser <147884153+golanglemonade@users.noreply.github.com>
Signed-off-by: Sarah Funkhouser <147884153+golanglemonade@users.noreply.github.com>
Signed-off-by: Sarah Funkhouser <147884153+golanglemonade@users.noreply.github.com>
|
| _, scopesModified = m.AppendedScopes() | ||
| } | ||
|
|
||
| if scopesModified && m.Op().Is(ent.OpUpdateOne) { |
There was a problem hiding this comment.
the hook triggers on OpUpdate and OpUpdateOne but you only have logic for OpUpdateOne
| @@ -39,7 +37,9 @@ func HookObjectOwnedTuples(parents []string, ownerRelation string, skipCreateUse | |||
|
|
|||
| var addTuples []fgax.TupleKey | |||
|
|
|||
| if skip := skipCreateUserPermissions(ctx, m); !skip { | |||
There was a problem hiding this comment.
is something changing with the order of operations that allows us to safely remove this?
| ). | ||
| Order( | ||
| // order by personal orgs last so that if there is another org available it will be set as the default instead of the personal org | ||
| organization.ByPersonalOrg(sql.OrderAsc()), |
There was a problem hiding this comment.
why would we need to perform ordering when the Where condition already precludes the previously set organization ID ?
| caller, ok := auth.CallerFromContext(ctx) | ||
| if !ok || caller == nil || caller.IsAnonymous() { | ||
| logx.FromContext(ctx).Info().Msg("unable to get caller from context") | ||
| caller, ok := auth.CallerFromContext(ctx) |
There was a problem hiding this comment.
it might be easier to do GetAuthzSubjectType or one of the other helpers here
| // allow member to invite members | ||
| if strings.EqualFold(role.String(), fgax.MemberRelation) { | ||
| switch strings.ToLower(role.String()) { | ||
| case strings.ToLower(fgax.MemberRelation): |
There was a problem hiding this comment.
the fgax constants are all already lower, no real reason to have strings.ToLower prefixing all these
| expected string | ||
| }{ | ||
| {name: "view query", objectType: "Control", relation: "can_view", expected: "can_view_control"}, | ||
| {name: "update op", objectType: "Task", relation: "can_view", op: ent.OpUpdate, expected: "can_edit_task"}, |
There was a problem hiding this comment.
update operation checks can_view but on OpUpdate and expected can edit?
| {name: "view query", objectType: "Control", relation: "can_view", expected: "can_view_control"}, | ||
| {name: "update op", objectType: "Task", relation: "can_view", op: ent.OpUpdate, expected: "can_edit_task"}, | ||
| {name: "edit relation", objectType: "Evidence", relation: "can_edit", expected: "can_edit_evidence"}, | ||
| {name: "delete op", objectType: "File", relation: "can_edit", op: ent.OpDeleteOne, expected: "can_delete_file"}, |
| return privacy.Allow | ||
| } | ||
|
|
||
| if auth.IsAPITokenAuthentication(ctx) { |
There was a problem hiding this comment.
isn't this duplicative of the check that's at the top of the function already?
|
|
||
| // scopedRelation returns the scoped relation based on the object type, relation, and operation. A operation is checked for for create, update, delete. If instead a specific relation should be checked, that should be passed instead of the operation | ||
| func scopedRelation(objectType string, relation string, op *ent.Op) string { | ||
| object := strcase.SnakeCase(objectType) |
There was a problem hiding this comment.
why manipulate the object type input like this?
| func AllowIfTokenHasMutationScope() privacy.MutationRuleFunc { | ||
| return privacy.MutationRuleFunc(func(ctx context.Context, m ent.Mutation) error { | ||
| objectType := m.Type() | ||
| if objectType == "" { |
There was a problem hiding this comment.
is it possible for m.Type() to return empty?


This PR contains revamped changes to permissions constructs and requires data migration.
fga.modfile in order to break up permissions into more manageable files. These are grouped based on functionality (e.g.compliance,trust center,regsitry, etc.crudpermissions are now generated based on the ent schema and are located infga/model/generated/crud.fga. This contains all thecan_<action>_<object>permissions that are used for both API Token scopes as well as base sets of permissionsroles.fgafile that contains more defined role types, e.g.compliance_manager,policy_manager,risk_manager- this will allow for easier designation of roles to user. These can be added on top of organization roles. For example, amembercan be givenpolicy_mangerrole which will give them full access to manage all policies in the organization.@inherit: This inherits all the same permissions as the specified roles, e.g.compliance_managerinherits the permissions given tocan_manage_compliance, can_manage_policies, can_manage_registry, can_manage_risk@crud: This specifies which objects the role provides crud access toSuper Admin: Users with this role will have the same permissions as the owner with the exception of being able to delete the organization. Organizations can have 0->many super adminsAuditor: This will be a mostly read-only role users can give to their auditor and in the future be able to specifiy what they have access tocan_view,can_edit,can_delete,group_managerscopes in favor of new scopes. THE UI NEEDS TO BE UPDATED TO ACCOMODATE THIS CHANGEparent_contextrelation which replaces theparentrelation fororganizationlevel checks for all objects with the exception offiles. This requires a migration ofparentrelations fororganizationobjects toparent_context. We cannot use contextual tuples because of parent hierarchy, e.g. this fails when you get to something likecontrol_implementationwhere the access is based on the control and the control is based on the organization - we'd have to pass the full hierachy for this to work with the structure we have today.Other Changes:
emailwhencheckAllowedEmailDomainfails so we know the user attempted to be added to the organizationentitlement reconciliation failedlogs when a constraint error to an warn log so test logs aren't full of these errors but we still see the warning in production. All other errors are kept as error level logs.t.Parallel()Deps:
Depends on: feat: disable parent by default iam#508