From 993a6f1680ffde0cebde78c9f3595265a4feabb4 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 23 Jun 2020 11:28:44 -0400 Subject: [PATCH] Remove "projects" from grant strings (#143) This removes the ability for a grant string to switch projects, in accordance with our updated design. It mostly removes code, with a few test fixes, and also adds one additional check (and test) to ensure a grant errors rather than do-nothing succeeds if both id and type are empty. --- internal/perms/acl_test.go | 24 +----- internal/perms/grants.go | 51 ++--------- internal/perms/grants_test.go | 154 ++++++---------------------------- 3 files changed, 34 insertions(+), 195 deletions(-) diff --git a/internal/perms/acl_test.go b/internal/perms/acl_test.go index d2ffb26339..0ba39533b3 100644 --- a/internal/perms/acl_test.go +++ b/internal/perms/acl_test.go @@ -40,7 +40,7 @@ func Test_ACLAllowed(t *testing.T) { { scope: Scope{Type: iam.OrganizationScope, Id: "org_b"}, grants: []string{ - "project=proj_x;type=host-set;actions=list,create", + "type=host-set;actions=list,create", "type=host;actions=*", "id=*;actions=authen", }, @@ -171,29 +171,9 @@ func Test_ACLAllowed(t *testing.T) { {action: iam.ActionDelete}, }, }, - { - name: "cross project, bad project", - resource: Resource{ScopeId: "proj_y", Type: "host-set"}, - scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: iam.ActionList}, - {action: iam.ActionCreate}, - {action: iam.ActionDelete}, - }, - }, - { - name: "cross project, good project", - resource: Resource{ScopeId: "proj_x", Type: "host-set"}, - scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: iam.ActionList, allowed: true}, - {action: iam.ActionCreate, allowed: true}, - {action: iam.ActionDelete}, - }, - }, { name: "any id", - resource: Resource{ScopeId: "org_b", Type: "host-set"}, + resource: Resource{ScopeId: "org_b", Type: "auth-method"}, scopeGrants: commonGrants, actionsAllowed: []actionAllowed{ {action: iam.ActionList}, diff --git a/internal/perms/grants.go b/internal/perms/grants.go index 4c6a8129fa..f34739d1f0 100644 --- a/internal/perms/grants.go +++ b/internal/perms/grants.go @@ -38,9 +38,6 @@ type Grant struct { // The scope ID, which will be a project ID or an org ID scope Scope - // Project, if defined - project string - // The ID in the grant, if provided. id string @@ -57,10 +54,9 @@ type Grant struct { func (g Grant) clone() *Grant { ret := &Grant{ - scope: g.scope, - project: g.project, - id: g.id, - typ: g.typ, + scope: g.scope, + id: g.id, + typ: g.typ, } if g.actionsBeingParsed != nil { ret.actionsBeingParsed = append(ret.actionsBeingParsed, g.actionsBeingParsed...) @@ -77,9 +73,6 @@ func (g Grant) clone() *Grant { // CanonicalString returns the canonical representation of the grant func (g Grant) CanonicalString() string { var builder []string - if g.project != "" { - builder = append(builder, fmt.Sprintf("project=%s", g.project)) - } if g.id != "" { builder = append(builder, fmt.Sprintf("id=%s", g.id)) @@ -104,9 +97,6 @@ func (g Grant) CanonicalString() string { // MarshalJSON provides a custom marshaller for grants func (g Grant) MarshalJSON() ([]byte, error) { res := make(map[string]interface{}, 4) - if g.project != "" { - res["project"] = g.project - } if g.id != "" { res["id"] = g.id } @@ -132,13 +122,6 @@ func (g *Grant) unmarshalJSON(data []byte) error { if err := json.Unmarshal(data, &raw); err != nil { return err } - if rawProj, ok := raw["project"]; ok { - project, ok := rawProj.(string) - if !ok { - return fmt.Errorf("unable to interpret %q as string", "project") - } - g.project = strings.ToLower(project) - } if rawId, ok := raw["id"]; ok { id, ok := rawId.(string) if !ok { @@ -192,9 +175,6 @@ func (g *Grant) unmarshalText(grantString string) error { } switch kv[0] { - case "project": - g.project = strings.ToLower(kv[1]) - case "id": g.id = strings.ToLower(kv[1]) @@ -225,15 +205,6 @@ func (g *Grant) unmarshalText(grantString string) error { // // The scope must be the org and project where this grant originated, not the // request. -// -// WARNING: It is the responsibility of the caller to validate that a returned -// Grant matches the incoming scope and if not that the relationship is valid. -// Specifically, if a project is specified as part of a grant, the grant's -// returned scope will be a project scope with the associated project ID. The -// caller must validate that the project ID is valid and that its enclosing -// organization is the original organization scope. Likely this can be done in a -// centralized helper context; however it's not done here to avoid reaching into -// the database from within this package. func Parse(scope Scope, userId, grantString string) (Grant, error) { if len(grantString) == 0 { return Grant{}, errors.New("grant string is empty") @@ -278,8 +249,8 @@ func Parse(scope Scope, userId, grantString string) (Grant, error) { } } - if err := grant.validateAndModifyProject(); err != nil { - return Grant{}, err + if grant.id == "" && grant.typ == "" { + return Grant{}, errors.New(`"id" and "type" cannot both be empty`) } if err := grant.validateType(); err != nil { @@ -293,18 +264,6 @@ func Parse(scope Scope, userId, grantString string) (Grant, error) { return grant, nil } -func (g *Grant) validateAndModifyProject() error { - if g.project == "" { - return nil - } - if g.scope.Type != iam.OrganizationScope { - return errors.New("cannot specify a project in the grant when the scope is not an organization") - } - g.scope.Type = iam.ProjectScope - g.scope.Id = g.project - return nil -} - func (g Grant) validateType() error { switch g.typ { case TypeNone, diff --git a/internal/perms/grants_test.go b/internal/perms/grants_test.go index b114773293..671d5116ae 100644 --- a/internal/perms/grants_test.go +++ b/internal/perms/grants_test.go @@ -130,72 +130,6 @@ func Test_ValidateType(t *testing.T) { } } -func Test_ValidateProject(t *testing.T) { - t.Parallel() - - type input struct { - name string - input Grant - output Grant - errResult string - } - - tests := []input{ - { - name: "no project", - input: Grant{ - scope: Scope{ - Type: iam.OrganizationScope, - }, - }, - output: Grant{ - scope: Scope{ - Type: iam.OrganizationScope, - }, - }, - }, - { - name: "project, organization scope", - input: Grant{ - project: "foobar", - scope: Scope{ - Type: iam.OrganizationScope, - }, - }, - output: Grant{ - project: "foobar", - scope: Scope{ - Type: iam.ProjectScope, - Id: "foobar", - }, - }, - }, - { - name: "project, non-organization scope", - input: Grant{ - project: "foobar", - scope: Scope{ - Type: iam.ProjectScope, - }, - }, - errResult: "cannot specify a project in the grant when the scope is not an organization", - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - err := test.input.validateAndModifyProject() - if test.errResult == "" { - require.NoError(t, err) - assert.Equal(t, test.output, test.input) - } else { - require.Error(t, err) - assert.Equal(t, test.errResult, err.Error()) - } - }) - } -} - func Test_MarshallingAndCloning(t *testing.T) { t.Parallel() @@ -218,46 +152,21 @@ func Test_MarshallingAndCloning(t *testing.T) { canonicalString: ``, }, { - name: "project", - input: Grant{ - project: "foobar", - scope: Scope{ - Type: iam.OrganizationScope, - }, - }, - jsonOutput: `{"project":"foobar"}`, - canonicalString: `project=foobar`, - }, - { - name: "project and type", - input: Grant{ - project: "foobar", - scope: Scope{ - Type: iam.ProjectScope, - }, - typ: TypeGroup, - }, - jsonOutput: `{"project":"foobar","type":"group"}`, - canonicalString: `project=foobar;type=group`, - }, - { - name: "project, type, and id", + name: "type and id", input: Grant{ - id: "baz", - project: "foobar", + id: "baz", scope: Scope{ Type: iam.ProjectScope, }, typ: TypeGroup, }, - jsonOutput: `{"id":"baz","project":"foobar","type":"group"}`, - canonicalString: `project=foobar;id=baz;type=group`, + jsonOutput: `{"id":"baz","type":"group"}`, + canonicalString: `id=baz;type=group`, }, { name: "everything", input: Grant{ - id: "baz", - project: "foobar", + id: "baz", scope: Scope{ Type: iam.ProjectScope, }, @@ -268,8 +177,8 @@ func Test_MarshallingAndCloning(t *testing.T) { }, actionsBeingParsed: []string{"create", "read"}, }, - jsonOutput: `{"actions":["create","read"],"id":"baz","project":"foobar","type":"group"}`, - canonicalString: `project=foobar;id=baz;type=group;actions=create,read`, + jsonOutput: `{"actions":["create","read"],"id":"baz","type":"group"}`, + canonicalString: `id=baz;type=group;actions=create,read`, }, } @@ -310,19 +219,11 @@ func Test_Unmarshaling(t *testing.T) { jsonErr: "invalid character 'w' looking for beginning of value", }, { - name: "good project", - expected: Grant{ - project: "foobar", - }, - jsonInput: `{"project":"foobar"}`, - textInput: `project=foobar`, - }, - { - name: "bad project", - jsonInput: `{"project":true}`, - jsonErr: `unable to interpret "project" as string`, - textInput: `project=`, - textErr: `segment "project=" not formatted correctly, missing value`, + name: "bad segment", + jsonInput: `{"id":true}`, + jsonErr: `unable to interpret "id" as string`, + textInput: `id=`, + textErr: `segment "id=" not formatted correctly, missing value`, }, { name: "good id", @@ -450,17 +351,21 @@ func Test_Parse(t *testing.T) { input: "id=foobar;type=host-catalog;actions=createread", err: `unknown action "createread"`, }, + { + name: "empty id and type", + input: "actions=create", + err: `"id" and "type" cannot both be empty`, + }, { name: "good json", - input: `{"project":"proj","id":"foobar","type":"host-catalog","actions":["create","read"]}`, + input: `{"id":"foobar","type":"host-catalog","actions":["create","read"]}`, expected: Grant{ scope: Scope{ - Id: "proj", - Type: iam.ProjectScope, + Id: "scope", + Type: iam.OrganizationScope, }, - project: "proj", - id: "foobar", - typ: "host-catalog", + id: "foobar", + typ: "host-catalog", actions: map[iam.Action]bool{ iam.ActionCreate: true, iam.ActionRead: true, @@ -469,15 +374,14 @@ func Test_Parse(t *testing.T) { }, { name: "good text", - input: `project=proj;id=foobar;type=host-catalog;actions=create,read`, + input: `id=foobar;type=host-catalog;actions=create,read`, expected: Grant{ scope: Scope{ - Id: "proj", - Type: iam.ProjectScope, + Id: "scope", + Type: iam.OrganizationScope, }, - project: "proj", - id: "foobar", - typ: "host-catalog", + id: "foobar", + typ: "host-catalog", actions: map[iam.Action]bool{ iam.ActionCreate: true, iam.ActionRead: true, @@ -523,10 +427,6 @@ func Test_Parse(t *testing.T) { require.Error(err) assert.Equal("no scope ID provided", err.Error()) - _, err = Parse(Scope{Id: "foobar", Type: iam.ProjectScope}, "", `project=foobar`) - require.Error(err) - assert.Equal("cannot specify a project in the grant when the scope is not an organization", err.Error()) - for _, test := range tests { t.Run(test.name, func(t *testing.T) { grant, err := Parse(Scope{Id: "scope", Type: iam.OrganizationScope}, test.userId, test.input)