diff --git a/api/roles/role_test.go b/api/roles/role_test.go index ee24a9afd..dfa4ac292 100644 --- a/api/roles/role_test.go +++ b/api/roles/role_test.go @@ -91,19 +91,19 @@ func TestCustom(t *testing.T) { assert.Empty(updatedRole.Item.Principals) version++ - updatedRole, err = rc.AddGrants(tc.Context(), updatedRole.Item.Id, updatedRole.Item.Version, []string{"id=*;actions=read"}) + updatedRole, err = rc.AddGrants(tc.Context(), updatedRole.Item.Id, updatedRole.Item.Version, []string{"id=*;type=*;actions=read"}) require.NoError(err) assert.EqualValues(updatedRole.Item.Version, version) - assert.Contains(updatedRole.Item.GrantStrings, "id=*;actions=read") + assert.Contains(updatedRole.Item.GrantStrings, "id=*;type=*;actions=read") version++ - updatedRole, err = rc.SetGrants(tc.Context(), updatedRole.Item.Id, updatedRole.Item.Version, []string{"id=*;actions=*"}) + updatedRole, err = rc.SetGrants(tc.Context(), updatedRole.Item.Id, updatedRole.Item.Version, []string{"id=*;type=*;actions=*"}) require.NoError(err) assert.EqualValues(updatedRole.Item.Version, version) - assert.Contains(updatedRole.Item.GrantStrings, "id=*;actions=*") + assert.Contains(updatedRole.Item.GrantStrings, "id=*;type=*;actions=*") version++ - updatedRole, err = rc.RemoveGrants(tc.Context(), updatedRole.Item.Id, updatedRole.Item.Version, []string{"id=*;actions=*"}) + updatedRole, err = rc.RemoveGrants(tc.Context(), updatedRole.Item.Id, updatedRole.Item.Version, []string{"id=*;type=*;actions=*"}) require.NoError(err) assert.EqualValues(updatedRole.Item.Version, version) assert.Empty(updatedRole.Item.Grants) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 364f86862..bce094e3c 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -385,7 +385,7 @@ func (v verifier) performAuthCheck() (aclResults perms.ACLResults, userId string } parsedGrants = make([]perms.Grant, 0, len(grantPairs)) for _, pair := range grantPairs { - parsed, err := perms.Parse(pair.ScopeId, userId, pair.Grant) + parsed, err := perms.Parse(pair.ScopeId, userId, pair.Grant, true) if err != nil { retErr = fmt.Errorf("perform auth check: failed to parse grant %#v: %w", pair.Grant, err) return diff --git a/internal/cmd/base/initial_resources.go b/internal/cmd/base/initial_resources.go index a30291f61..145339457 100644 --- a/internal/cmd/base/initial_resources.go +++ b/internal/cmd/base/initial_resources.go @@ -132,7 +132,7 @@ func (b *Server) CreateInitialAuthMethod(ctx context.Context) (*password.AuthMet if err != nil { return nil, nil, fmt.Errorf("error creating role for default generated grants: %w", err) } - if _, err := iamRepo.AddRoleGrants(cancelCtx, defPermsRole.PublicId, defPermsRole.Version, []string{"id=*;actions=*"}); err != nil { + if _, err := iamRepo.AddRoleGrants(cancelCtx, defPermsRole.PublicId, defPermsRole.Version, []string{"id=*;type=*;actions=*"}); err != nil { return nil, nil, fmt.Errorf("error creating grant for default generated grants: %w", err) } if _, err := iamRepo.AddPrincipalRoles(cancelCtx, defPermsRole.PublicId, defPermsRole.Version+1, []string{u.GetPublicId()}, nil); err != nil { diff --git a/internal/cmd/commands/authenticate/password.go b/internal/cmd/commands/authenticate/password.go index 9ff2678a1..f629ea64d 100644 --- a/internal/cmd/commands/authenticate/password.go +++ b/internal/cmd/commands/authenticate/password.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/boundary/api" "github.com/hashicorp/boundary/api/authmethods" + "github.com/hashicorp/boundary/api/authtokens" "github.com/hashicorp/boundary/internal/cmd/base" "github.com/hashicorp/vault/sdk/helper/password" "github.com/mitchellh/cli" @@ -135,7 +136,7 @@ func (c *PasswordCommand) Run(args []string) int { return 2 } - token := result.Item + token := result.GetItem().(*authtokens.AuthToken) switch base.Format(c.UI) { case "table": c.UI.Output(base.WrapForHelpText([]string{ @@ -149,7 +150,7 @@ func (c *PasswordCommand) Run(args []string) int { })) case "json": - jsonOut, err := base.JsonFormatter{}.Format(result) + jsonOut, err := base.JsonFormatter{}.Format(token) if err != nil { c.UI.Error(fmt.Errorf("Error formatting as JSON: %w", err).Error()) return 1 diff --git a/internal/cmd/commands/roles/funcs.go b/internal/cmd/commands/roles/funcs.go index fab7b010e..10bd31f59 100644 --- a/internal/cmd/commands/roles/funcs.go +++ b/internal/cmd/commands/roles/funcs.go @@ -117,6 +117,7 @@ func generateRoleTableOutput(in *roles.Role) string { if len(in.Principals) > 0 { ret = append(ret, + "", fmt.Sprintf(" Principals: %s", ""), ) } @@ -135,7 +136,6 @@ func generateRoleTableOutput(in *roles.Role) string { } for _, grant := range in.Grants { ret = append(ret, - "", fmt.Sprintf(" %s", grant.Canonical), ) diff --git a/internal/cmd/commands/roles/role.go b/internal/cmd/commands/roles/role.go index e34ab02a0..adf63b0f7 100644 --- a/internal/cmd/commands/roles/role.go +++ b/internal/cmd/commands/roles/role.go @@ -182,7 +182,7 @@ func (c *Command) Run(args []string) int { if len(grants) > 0 { for _, grant := range grants { - _, err := perms.Parse("global", "", grant) + _, err := perms.Parse("global", "", grant, false) if err != nil { c.UI.Error(fmt.Errorf("Grant %q could not be parsed successfully: %w", grant, err).Error()) return 1 diff --git a/internal/db/migrations/postgres.gen.go b/internal/db/migrations/postgres.gen.go index 78122210c..1c8e35e37 100644 --- a/internal/db/migrations/postgres.gen.go +++ b/internal/db/migrations/postgres.gen.go @@ -1103,7 +1103,7 @@ insert into iam_role (public_id, name, description, scope_id) values('r_default', 'Default Grants', 'Default role created on first instantiation of Boundary. It is meant to provide enough permissions for users to successfully authenticate via various client types.', 'global'); insert into iam_role_grant (role_id, canonical_grant, raw_grant) values - ('r_default', 'id=*;type=scope;actions=list', 'id=*;type=scope;actions=list'), + ('r_default', 'type=scope;actions=list', 'type=scope;actions=list'), ('r_default', 'id=*;type=auth-method;actions=authenticate,list', 'id=*;type=auth-method;actions=authenticate,list'); insert into iam_user_role (role_id, principal_id) values diff --git a/internal/db/migrations/postgres/06_iam.up.sql b/internal/db/migrations/postgres/06_iam.up.sql index bca7d4813..76a05aee7 100644 --- a/internal/db/migrations/postgres/06_iam.up.sql +++ b/internal/db/migrations/postgres/06_iam.up.sql @@ -585,7 +585,7 @@ insert into iam_role (public_id, name, description, scope_id) values('r_default', 'Default Grants', 'Default role created on first instantiation of Boundary. It is meant to provide enough permissions for users to successfully authenticate via various client types.', 'global'); insert into iam_role_grant (role_id, canonical_grant, raw_grant) values - ('r_default', 'id=*;type=scope;actions=list', 'id=*;type=scope;actions=list'), + ('r_default', 'type=scope;actions=list', 'type=scope;actions=list'), ('r_default', 'id=*;type=auth-method;actions=authenticate,list', 'id=*;type=auth-method;actions=authenticate,list'); insert into iam_user_role (role_id, principal_id) values diff --git a/internal/iam/repository_role_grant.go b/internal/iam/repository_role_grant.go index 3707a27c8..5ee317d75 100644 --- a/internal/iam/repository_role_grant.go +++ b/internal/iam/repository_role_grant.go @@ -162,7 +162,7 @@ func (r *Repository) DeleteRoleGrants(ctx context.Context, roleId string, roleVe deleteRoleGrants := make([]interface{}, 0, len(grants)) for _, grant := range grants { // Use a fake scope, just want to get out a canonical string - perm, err := perms.Parse("o_abcd1234", "", grant) + perm, err := perms.Parse("o_abcd1234", "", grant, true) if err != nil { return fmt.Errorf("delete role grants: error parsing grant string: %w", err) } @@ -253,7 +253,7 @@ func (r *Repository) SetRoleGrants(ctx context.Context, roleId string, roleVersi deleteRoleGrants := make([]interface{}, 0, len(grants)) for _, grant := range grants { // Use a fake scope, just want to get out a canonical string - perm, err := perms.Parse("o_abcd1234", "", grant) + perm, err := perms.Parse("o_abcd1234", "", grant, true) if err != nil { return nil, db.NoRowsAffected, fmt.Errorf("set role grants: error parsing grant string: %w", err) } diff --git a/internal/iam/repository_role_test.go b/internal/iam/repository_role_test.go index 8672e8cf7..0a17729f8 100644 --- a/internal/iam/repository_role_test.go +++ b/internal/iam/repository_role_test.go @@ -386,7 +386,7 @@ func TestRepository_UpdateRole(t *testing.T) { if tt.wantDup { r := TestRole(t, conn, org.PublicId) _ = TestUserRole(t, conn, r.GetPublicId(), u.GetPublicId()) - _ = TestRoleGrant(t, conn, r.GetPublicId(), "id=*;actions=*") + _ = TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") r.Name = tt.args.name _, _, _, _, err := repo.UpdateRole(context.Background(), r, r.Version, tt.args.fieldMaskPaths, tt.args.opt...) assert.NoError(err) @@ -409,7 +409,7 @@ func TestRepository_UpdateRole(t *testing.T) { princRole.RoleScopeId = tt.newScopeId princRole.ScopedPrincipalId = fmt.Sprintf("%s:%s", org.PublicId, ur.PrincipalId) } - rGrant := TestRoleGrant(t, conn, r.GetPublicId(), "id=*;actions=*") + rGrant := TestRoleGrant(t, conn, r.GetPublicId(), "id=*;type=*;actions=*") updateRole := allocRole() updateRole.PublicId = r.PublicId diff --git a/internal/iam/repository_scope.go b/internal/iam/repository_scope.go index 0d4a50a3a..388c84102 100644 --- a/internal/iam/repository_scope.go +++ b/internal/iam/repository_scope.go @@ -214,7 +214,7 @@ func (r *Repository) CreateScope(ctx context.Context, s *Scope, userId string, o msgs = append(msgs, &roleOplogMsg) - roleGrant, err := NewRoleGrant(adminRolePublicId, "id=*;actions=*") + roleGrant, err := NewRoleGrant(adminRolePublicId, "id=*;type=*;actions=*") if err != nil { return fmt.Errorf("unable to create in memory role grant: %w", err) } @@ -279,7 +279,7 @@ func (r *Repository) CreateScope(ctx context.Context, s *Scope, userId string, o // Grants { grants := []interface{}{} - roleGrant, err := NewRoleGrant(defaultRolePublicId, "id=*;type=scope;actions=list") + roleGrant, err := NewRoleGrant(defaultRolePublicId, "type=scope;actions=list") if err != nil { return fmt.Errorf("unable to create in memory role grant: %w", err) } diff --git a/internal/iam/role_grant.go b/internal/iam/role_grant.go index 869295f85..0de8d18ef 100644 --- a/internal/iam/role_grant.go +++ b/internal/iam/role_grant.go @@ -34,7 +34,7 @@ func NewRoleGrant(roleId string, grant string, opt ...Option) (*RoleGrant, error // Validate that the grant parses successfully. Note that we fake the scope // here to avoid a lookup as the scope is only relevant at actual ACL // checking time and we just care that it parses correctly. - perm, err := perms.Parse("o_abcd1234", "", grant) + perm, err := perms.Parse("o_abcd1234", "", grant, false) if err != nil { return nil, fmt.Errorf("new role grant: error parsing grant string: %w", err) } @@ -73,7 +73,7 @@ func (g *RoleGrant) VetForWrite(ctx context.Context, r db.Reader, opType db.OpTy // checking time and we just care that it parses correctly. We may have // already done this in NewRoleGrant, but we re-check and set it here // anyways because it should still be part of the vetting process. - perm, err := perms.Parse("o_abcd1234", "", g.RawGrant) + perm, err := perms.Parse("o_abcd1234", "", g.RawGrant, false) if err != nil { return fmt.Errorf("vet role grant for writing: error parsing grant string: %w", err) } diff --git a/internal/iam/role_grant_test.go b/internal/iam/role_grant_test.go index 9787007a7..5fd80c51d 100644 --- a/internal/iam/role_grant_test.go +++ b/internal/iam/role_grant_test.go @@ -38,7 +38,7 @@ func TestRoleGrant_Create(t *testing.T) { name: "nil-role", args: args{ roleId: "", - grant: "id=*;actions=*", + grant: "id=*;type=*;actions=*", }, wantErr: true, wantIsErr: db.ErrInvalidParameter, @@ -57,13 +57,13 @@ func TestRoleGrant_Create(t *testing.T) { name: "valid", args: args{ roleId: projRole.PublicId, - grant: "id=*;actions=*", + grant: "id=*;type=*;actions=*", }, want: func() *RoleGrant { g := allocRoleGrant() g.RoleId = projRole.PublicId - g.RawGrant = "id=*;actions=*" - g.CanonicalGrant = "id=*;actions=*" + g.RawGrant = "id=*;type=*;actions=*" + g.CanonicalGrant = "id=*;type=*;actions=*" return &g }(), create: true, @@ -72,13 +72,13 @@ func TestRoleGrant_Create(t *testing.T) { name: "valid-reversed-grant", args: args{ roleId: projRole.PublicId, - grant: "actions=*;id=*", + grant: "type=*;actions=*;id=*", }, want: func() *RoleGrant { g := allocRoleGrant() g.RoleId = projRole.PublicId - g.RawGrant = "actions=*;id=*" - g.CanonicalGrant = "id=*;actions=*" + g.RawGrant = "type=*;actions=*;id=*" + g.CanonicalGrant = "id=*;type=*;actions=*" return &g }(), create: true, @@ -125,9 +125,9 @@ func TestRoleGrant_Update(t *testing.T) { t.Run("updates not allowed", func(t *testing.T) { assert, require := assert.New(t), require.New(t) r := TestRole(t, conn, org.PublicId) - roleGrant := TestRoleGrant(t, conn, r.PublicId, "id=*;actions=*") + roleGrant := TestRoleGrant(t, conn, r.PublicId, "id=*;type=*;actions=*") updateRoleGrant := roleGrant.Clone().(*RoleGrant) - updateRoleGrant.RawGrant = "actions=*;id=*" + updateRoleGrant.RawGrant = "type=*;actions=*;id=*" updatedRows, err := rw.Update(context.Background(), updateRoleGrant, []string{"RawGrant"}, nil) require.Error(err) assert.Equal(0, updatedRows) @@ -159,7 +159,7 @@ func TestRoleGrant_Delete(t *testing.T) { name: "nil-role", args: args{ roleId: "", - grant: "id=*;actions=*", + grant: "id=*;type=*;actions=*", }, wantErr: true, wantsErrStr: "primary key is not set", @@ -230,11 +230,11 @@ func TestRoleGrant_Clone(t *testing.T) { s := testOrg(t, repo, "", "") role := TestRole(t, conn, s.PublicId) - g, err := NewRoleGrant(role.PublicId, "id=*;actions=*") + g, err := NewRoleGrant(role.PublicId, "id=*;type=*;actions=*") assert.NoError(err) assert.NotNil(g) assert.Equal(g.RoleId, role.PublicId) - assert.Equal(g.RawGrant, "id=*;actions=*") + assert.Equal(g.RawGrant, "id=*;type=*;actions=*") cp := g.Clone() assert.True(proto.Equal(cp.(*RoleGrant).RoleGrant, g.RoleGrant)) @@ -244,11 +244,11 @@ func TestRoleGrant_Clone(t *testing.T) { s := testOrg(t, repo, "", "") role := TestRole(t, conn, s.PublicId) - g, err := NewRoleGrant(role.PublicId, "id=*;actions=*") + g, err := NewRoleGrant(role.PublicId, "id=*;type=*;actions=*") assert.NoError(err) assert.NotNil(g) assert.Equal(g.RoleId, role.PublicId) - assert.Equal(g.RawGrant, "id=*;actions=*") + assert.Equal(g.RawGrant, "id=*;type=*;actions=*") g2, err := NewRoleGrant(role.PublicId, "id=foo;actions=read") assert.NoError(err) diff --git a/internal/iam/testing_test.go b/internal/iam/testing_test.go index f1e499fa8..a397a97af 100644 --- a/internal/iam/testing_test.go +++ b/internal/iam/testing_test.go @@ -118,11 +118,11 @@ func Test_TestRoleGrant(t *testing.T) { _, proj := TestScopes(t, repo) projRole := TestRole(t, conn, proj.PublicId) - grant := TestRoleGrant(t, conn, projRole.PublicId, "actions=*;id=*") + grant := TestRoleGrant(t, conn, projRole.PublicId, "type=*;actions=*;id=*") require.NotNil(grant) require.Equal(projRole.PublicId, grant.RoleId) - require.Equal("actions=*;id=*", grant.RawGrant) - require.Equal("id=*;actions=*", grant.CanonicalGrant) + require.Equal("type=*;actions=*;id=*", grant.RawGrant) + require.Equal("id=*;type=*;actions=*", grant.CanonicalGrant) } func Test_TestUserRole(t *testing.T) { diff --git a/internal/perms/acl.go b/internal/perms/acl.go index 0a061e242..0f09f1693 100644 --- a/internal/perms/acl.go +++ b/internal/perms/acl.go @@ -6,13 +6,8 @@ https://hashicorp.atlassian.net/wiki/spaces/ICU/pages/866976600/API+Actions+and+ speaking of which: TODO: put that chart in public docs. Anyways, from that page you can see that there are really only a few patterns of -ACLs that are ever allowed: - -* id=*;type=;actions= -* id=;actions= -* id=;type=;actions= - -and of course a matching scope. +ACLs that are ever allowed; see the Allowed function for a description along +with the code. This makes it actually quite simple to perform the ACL checking. Much of ACL construction is thus synthesizing something reasonable from a set of Grants. @@ -81,26 +76,65 @@ func (a ACL) Allowed(r Resource, aType action.Type) (results ACLResults) { continue } switch { - // id=*;type=;actions= - case grant.id == "*" && - (grant.typ == r.Type || grant.typ == resource.All): + // type=;actions= when action is list or create. + // If create, must be a top-level collection; otherwise an ID or "*" + // must be specified (as a pin format string, below). + case grant.id == "" && + r.Id == "" && + grant.typ == r.Type && + grant.typ != resource.Unknown && + (aType == action.List || + (topLevelType(r.Type) && aType == action.Create)): + results.Allowed = true return - // id=;actions= - case grant.id != "" && - (grant.id == r.Id || grant.id == "*") && + // id=;actions= where ID cannot be a wildcard + case grant.id == r.Id && + grant.id != "" && + grant.id != "*" && grant.typ == resource.Unknown: + results.Allowed = true return - // id=;type=;actions= + // id=*;type=;actions= where type cannot be + // unknown but can be a wildcard to allow any resource at all + case grant.id == "*" && + grant.typ != resource.Unknown && + (grant.typ == r.Type || + grant.typ == resource.All): + + results.Allowed = true + return + + // id=;type=;actions= where type can be a + // wildcard and this this is operating on a non-top-level type case grant.id != "" && grant.id == r.Pin && - (grant.typ == r.Type || grant.typ == resource.All): + grant.typ != resource.Unknown && + (grant.typ == r.Type || grant.typ == resource.All) && + !topLevelType(r.Type): + results.Allowed = true return } } return } + +func topLevelType(typ resource.Type) bool { + switch typ { + case resource.AuthMethod, + resource.AuthToken, + resource.Group, + resource.HostCatalog, + resource.Role, + resource.Scope, + resource.Session, + resource.Target, + resource.User: + return true + } + return false +} diff --git a/internal/perms/acl_test.go b/internal/perms/acl_test.go index 6afff7996..54fe40219 100644 --- a/internal/perms/acl_test.go +++ b/internal/perms/acl_test.go @@ -34,7 +34,8 @@ func Test_ACLAllowed(t *testing.T) { scope: "o_a", grants: []string{ "id=a_bar;actions=read,update", - "id=a_foo;type=host-catalog;actions=update,delete", + "type=host-catalog;actions=create", + "type=host;actions=list", "id=*;type=host-set;actions=list,create", }, }, @@ -42,14 +43,14 @@ func Test_ACLAllowed(t *testing.T) { scope: "o_b", grants: []string{ "id=*;type=host-set;actions=list,create", - "id=*;type=host;actions=*", - "id=*;actions=authenticate", + "id=mypin;type=host;actions=*", + "id=*;type=*;actions=authenticate", }, }, { scope: "o_c", grants: []string{ - "id={{user.id }};actions=create,update", + "id={{user.id }};actions=read,update", }, }, { @@ -80,9 +81,18 @@ func Test_ACLAllowed(t *testing.T) { {action: action.Read}, }, }, + { + name: "top level create with type only", + resource: Resource{ScopeId: "o_a", Type: resource.HostCatalog}, + scopeGrants: commonGrants, + actionsAllowed: []actionAllowed{ + {action: action.Create, allowed: true}, + {action: action.Delete}, + }, + }, { name: "matching scope and id no matching action", - resource: Resource{ScopeId: "o_a", Id: "a_foo"}, + resource: Resource{ScopeId: "o_a", Id: "a_foo", Type: resource.Role}, scopeGrants: commonGrants, actionsAllowed: []actionAllowed{ {action: action.Update}, @@ -100,8 +110,8 @@ func Test_ACLAllowed(t *testing.T) { }, }, { - name: "matching scope and id and all action", - resource: Resource{ScopeId: "o_b", Type: resource.Host}, + name: "matching scope and type and all action with valid pin", + resource: Resource{ScopeId: "o_b", Pin: "mypin", Type: resource.Host}, scopeGrants: commonGrants, actionsAllowed: []actionAllowed{ {action: action.Read, allowed: true}, @@ -109,6 +119,26 @@ func Test_ACLAllowed(t *testing.T) { {action: action.Delete, allowed: true}, }, }, + { + name: "matching scope and type and all action but bad pin", + resource: Resource{ScopeId: "o_b", Pin: "notmypin", Type: resource.Host}, + scopeGrants: commonGrants, + actionsAllowed: []actionAllowed{ + {action: action.Read}, + {action: action.Update}, + {action: action.Delete}, + }, + }, + { + name: "matching scope and id and some action", + resource: Resource{ScopeId: "o_b", Id: "myhost", Type: resource.HostSet}, + scopeGrants: commonGrants, + actionsAllowed: []actionAllowed{ + {action: action.List, allowed: true}, + {action: action.Create, allowed: true}, + {action: action.AddHosts}, + }, + }, { name: "matching scope and id and all action but bad specifier", resource: Resource{ScopeId: "o_b", Id: "id_g"}, @@ -138,16 +168,6 @@ func Test_ACLAllowed(t *testing.T) { {action: action.Delete}, }, }, - { - name: "matching scope, type, action, bad pin", - resource: Resource{ScopeId: "o_a", Id: "a_foo", Type: resource.HostCatalog}, - scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.Update}, - {action: action.Delete}, - {action: action.Read}, - }, - }, { name: "matching scope, type, action, random id and bad pin", resource: Resource{ScopeId: "o_a", Id: "anything", Type: resource.HostCatalog, Pin: "a_bar"}, @@ -158,16 +178,6 @@ func Test_ACLAllowed(t *testing.T) { {action: action.Read}, }, }, - { - name: "matching scope, type, action, random id and good pin", - resource: Resource{ScopeId: "o_a", Id: "anything", Type: resource.HostCatalog, Pin: "a_foo"}, - scopeGrants: commonGrants, - actionsAllowed: []actionAllowed{ - {action: action.Update, allowed: true}, - {action: action.Delete, allowed: true}, - {action: action.Read}, - }, - }, { name: "wrong scope and matching type", resource: Resource{ScopeId: "o_bad", Type: resource.HostSet}, @@ -204,7 +214,7 @@ func Test_ACLAllowed(t *testing.T) { resource: Resource{ScopeId: "o_c", Id: "u_abcd1234"}, scopeGrants: commonGrants, actionsAllowed: []actionAllowed{ - {action: action.Create, allowed: true}, + {action: action.Read, allowed: true}, {action: action.Update, allowed: true}, }, userId: "u_abcd1234", @@ -226,7 +236,7 @@ func Test_ACLAllowed(t *testing.T) { var grants []Grant for _, sg := range test.scopeGrants { for _, g := range sg.grants { - grant, err := Parse(sg.scope, test.userId, g) + grant, err := Parse(sg.scope, test.userId, g, false) require.NoError(t, err) grants = append(grants, grant) } diff --git a/internal/perms/grants.go b/internal/perms/grants.go index 42a5802a3..8dcf8f8bd 100644 --- a/internal/perms/grants.go +++ b/internal/perms/grants.go @@ -230,7 +230,7 @@ func (g *Grant) unmarshalText(grantString string) error { // // The scope must be the org and project where this grant originated, not the // request. -func Parse(scopeId, userId, grantString string) (Grant, error) { +func Parse(scopeId, userId, grantString string, skipFinalValidation bool) (Grant, error) { if len(grantString) == 0 { return Grant{}, errors.New("grant string is empty") } @@ -279,10 +279,6 @@ func Parse(scopeId, userId, grantString string) (Grant, error) { } } - if grant.id == "" { - return Grant{}, errors.New(`"id" cannot be empty, perhaps "*" was meant`) - } - if err := grant.validateType(); err != nil { return Grant{}, err } @@ -291,6 +287,30 @@ func Parse(scopeId, userId, grantString string) (Grant, error) { return Grant{}, err } + if !skipFinalValidation { + // Validate the grant. Create a dummy resource and pass it through + // Allowed and ensure that we get allowed. + acl := NewACL(grant) + r := Resource{ + ScopeId: scopeId, + Id: grant.id, + Type: grant.typ, + } + if !topLevelType(grant.typ) { + r.Pin = grant.id + } + var allowed bool + for k := range grant.actions { + results := acl.Allowed(r, k) + if results.Allowed { + allowed = true + } + } + if !allowed { + return Grant{}, errors.New("parsed grant string would not result in any action being authorized") + } + } + return grant, nil } diff --git a/internal/perms/grants_test.go b/internal/perms/grants_test.go index 84773a928..e01b20858 100644 --- a/internal/perms/grants_test.go +++ b/internal/perms/grants_test.go @@ -349,43 +349,69 @@ func Test_Parse(t *testing.T) { { name: "empty id and type", input: "actions=create", - err: `"id" cannot be empty, perhaps "*" was meant`, + err: `parsed grant string would not result in any action being authorized`, }, { - name: "good json", - input: `{"id":"foobar","type":"host-catalog","actions":["create","read"]}`, + name: "good json type", + input: `{"type":"host-catalog","actions":["create"]}`, expected: Grant{ scope: Scope{ Id: "o_scope", Type: scope.Org, }, - id: "foobar", typ: resource.HostCatalog, actions: map[action.Type]bool{ action.Create: true, - action.Read: true, }, }, }, { - name: "good text", - input: `id=foobar;type=host-catalog;actions=create,read`, + name: "good json id", + input: `{"id":"foobar","actions":["read"]}`, expected: Grant{ scope: Scope{ Id: "o_scope", Type: scope.Org, }, id: "foobar", + typ: resource.Unknown, + actions: map[action.Type]bool{ + action.Read: true, + }, + }, + }, + { + name: "good text type", + input: `type=host-catalog;actions=create`, + expected: Grant{ + scope: Scope{ + Id: "o_scope", + Type: scope.Org, + }, typ: resource.HostCatalog, actions: map[action.Type]bool{ action.Create: true, - action.Read: true, + }, + }, + }, + { + name: "good text id", + input: `id=foobar;actions=read`, + expected: Grant{ + scope: Scope{ + Id: "o_scope", + Type: scope.Org, + }, + id: "foobar", + typ: resource.Unknown, + actions: map[action.Type]bool{ + action.Read: true, }, }, }, { name: "default project scope", - input: `id=foobar;type=host-catalog;actions=create,read`, + input: `id=foobar;actions=read`, scopeOverride: "p_1234", expected: Grant{ scope: Scope{ @@ -393,16 +419,15 @@ func Test_Parse(t *testing.T) { Type: scope.Project, }, id: "foobar", - typ: resource.HostCatalog, + typ: resource.Unknown, actions: map[action.Type]bool{ - action.Create: true, - action.Read: true, + action.Read: true, }, }, }, { name: "default org scope", - input: `id=foobar;type=host-catalog;actions=create,read`, + input: `id=foobar;actions=read`, scopeOverride: "o_1234", expected: Grant{ scope: Scope{ @@ -410,16 +435,15 @@ func Test_Parse(t *testing.T) { Type: scope.Org, }, id: "foobar", - typ: resource.HostCatalog, + typ: resource.Unknown, actions: map[action.Type]bool{ - action.Create: true, - action.Read: true, + action.Read: true, }, }, }, { name: "default global scope", - input: `id=foobar;type=host-catalog;actions=create,read`, + input: `id=foobar;actions=read`, scopeOverride: "global", expected: Grant{ scope: Scope{ @@ -427,10 +451,9 @@ func Test_Parse(t *testing.T) { Type: scope.Global, }, id: "foobar", - typ: resource.HostCatalog, + typ: resource.Unknown, actions: map[action.Type]bool{ - action.Create: true, - action.Read: true, + action.Read: true, }, }, }, @@ -458,11 +481,11 @@ func Test_Parse(t *testing.T) { }, } - _, err := Parse("", "", "") + _, err := Parse("", "", "", false) require.Error(t, err) assert.Equal(t, "grant string is empty", err.Error()) - _, err = Parse("", "", "{}") + _, err = Parse("", "", "{}", false) require.Error(t, err) assert.Equal(t, "no scope ID provided", err.Error()) @@ -474,10 +497,10 @@ func Test_Parse(t *testing.T) { if test.scopeOverride != "" { scope = test.scopeOverride } - grant, err := Parse(scope, test.userId, test.input) + grant, err := Parse(scope, test.userId, test.input, false) if test.err != "" { require.Error(err) - assert.True(strings.Contains(err.Error(), test.err)) + assert.True(strings.Contains(err.Error(), test.err), err.Error()) } else { require.NoError(err) assert.Equal(test.expected, grant) diff --git a/internal/servers/controller/handlers/roles/role_service.go b/internal/servers/controller/handlers/roles/role_service.go index 1a195204a..16a2b0ffe 100644 --- a/internal/servers/controller/handlers/roles/role_service.go +++ b/internal/servers/controller/handlers/roles/role_service.go @@ -538,7 +538,7 @@ func toProto(in *iam.Role, principals []iam.PrincipalRole, grants []*iam.RoleGra } for _, g := range grants { out.GrantStrings = append(out.GrantStrings, g.GetRawGrant()) - parsed, err := perms.Parse(in.GetGrantScopeId(), "", g.GetRawGrant()) + parsed, err := perms.Parse(in.GetGrantScopeId(), "", g.GetRawGrant(), false) if err != nil { // This should never happen as we validate on the way in, but let's // return what we can since we are still returning the raw grant diff --git a/internal/servers/controller/handlers/roles/role_service_test.go b/internal/servers/controller/handlers/roles/role_service_test.go index 279e3b3a0..b21cb1cf1 100644 --- a/internal/servers/controller/handlers/roles/role_service_test.go +++ b/internal/servers/controller/handlers/roles/role_service_test.go @@ -503,8 +503,8 @@ func TestCreate(t *testing.T) { } func TestUpdate(t *testing.T) { - grantString := "id=*;actions=*" - g, err := perms.Parse("global", "", grantString) + grantString := "id=*;type=*;actions=*" + g, err := perms.Parse("global", "", grantString, false) require.NoError(t, err) _, actions := g.Actions() grant := &pb.Grant{ @@ -1339,7 +1339,7 @@ func checkEqualGrants(t *testing.T, expected []string, got *pb.Role) { require.Equal(len(expected), len(got.GrantStrings)) require.Equal(len(expected), len(got.Grants)) for i, v := range expected { - parsed, err := perms.Parse("o_abc123", "", v) + parsed, err := perms.Parse("o_abc123", "", v, false) require.NoError(err) assert.Equal(expected[i], got.GrantStrings[i]) assert.Equal(expected[i], got.Grants[i].GetRaw()) @@ -1372,19 +1372,19 @@ func TestAddGrants(t *testing.T) { }{ { name: "Add grant on empty role", - add: []string{"id=*;actions=delete"}, - result: []string{"id=*;actions=delete"}, + add: []string{"id=*;type=*;actions=delete"}, + result: []string{"id=*;type=*;actions=delete"}, }, { name: "Add grant on role with grant", existing: []string{"id=1;actions=read"}, - add: []string{"id=*;actions=delete"}, - result: []string{"id=1;actions=read", "id=*;actions=delete"}, + add: []string{"id=*;type=*;actions=delete"}, + result: []string{"id=1;actions=read", "id=*;type=*;actions=delete"}, }, { name: "Add grant matching existing grant", - existing: []string{"id=1;actions=read", "id=*;actions=delete"}, - add: []string{"id=*;actions=delete"}, + existing: []string{"id=1;actions=read", "id=*;type=*;actions=delete"}, + add: []string{"id=*;type=*;actions=delete"}, wantErr: true, }, } @@ -1429,7 +1429,7 @@ func TestAddGrants(t *testing.T) { name: "Bad Version", req: &pbs.AddRoleGrantsRequest{ Id: role.GetPublicId(), - GrantStrings: []string{"id=*;actions=create"}, + GrantStrings: []string{"id=*;type=*;actions=create"}, Version: role.GetVersion() + 2, }, err: handlers.ApiErrorWithCode(codes.Internal), @@ -1438,7 +1438,7 @@ func TestAddGrants(t *testing.T) { name: "Bad Role Id", req: &pbs.AddRoleGrantsRequest{ Id: "bad id", - GrantStrings: []string{"id=*;actions=create"}, + GrantStrings: []string{"id=*;type=*;actions=create"}, Version: role.GetVersion(), }, err: handlers.ApiErrorWithCode(codes.InvalidArgument), @@ -1476,24 +1476,24 @@ func TestSetGrants(t *testing.T) { }{ { name: "Set grant on empty role", - set: []string{"id=*;actions=delete"}, - result: []string{"id=*;actions=delete"}, + set: []string{"id=*;type=*;actions=delete"}, + result: []string{"id=*;type=*;actions=delete"}, }, { name: "Set grant on role with grant", existing: []string{"id=1;actions=read"}, - set: []string{"id=*;actions=delete"}, - result: []string{"id=*;actions=delete"}, + set: []string{"id=*;type=*;actions=delete"}, + result: []string{"id=*;type=*;actions=delete"}, }, { name: "Set grant matching existing grant", - existing: []string{"id=1;actions=read", "id=*;actions=delete"}, - set: []string{"id=*;actions=delete"}, - result: []string{"id=*;actions=delete"}, + existing: []string{"id=1;actions=read", "id=*;type=*;actions=delete"}, + set: []string{"id=*;type=*;actions=delete"}, + result: []string{"id=*;type=*;actions=delete"}, }, { name: "Set empty on role", - existing: []string{"id=1;actions=read", "id=*;actions=delete"}, + existing: []string{"id=1;actions=read", "id=*;type=*;actions=delete"}, set: nil, result: nil, }, @@ -1541,7 +1541,7 @@ func TestSetGrants(t *testing.T) { name: "Bad Version", req: &pbs.SetRoleGrantsRequest{ Id: role.GetPublicId(), - GrantStrings: []string{"id=*;actions=create"}, + GrantStrings: []string{"id=*;type=*;actions=create"}, Version: role.GetVersion() + 2, }, err: handlers.ApiErrorWithCode(codes.Internal), @@ -1550,7 +1550,7 @@ func TestSetGrants(t *testing.T) { name: "Bad Role Id", req: &pbs.SetRoleGrantsRequest{ Id: "bad id", - GrantStrings: []string{"id=*;actions=create"}, + GrantStrings: []string{"id=*;type=*;actions=create"}, Version: role.GetVersion(), }, err: handlers.ApiErrorWithCode(codes.InvalidArgument),