From ef4c1e6135c9f0e6b228b85f40ef62cedd0ab887 Mon Sep 17 00:00:00 2001 From: flyinghermit Date: Mon, 28 Oct 2024 13:03:03 -0400 Subject: [PATCH 1/2] temporarily remove app label checker for saml_idp_service_provider resources --- lib/auth/auth_with_roles.go | 79 +---- lib/auth/auth_with_roles_test.go | 490 ++++++++++++++++--------------- 2 files changed, 258 insertions(+), 311 deletions(-) diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 15c186f54e0c2..c896a74a4b346 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -1263,6 +1263,11 @@ func (c *resourceAccess) checkAccess(resource types.ResourceWithLabels, filter s return false, nil } + // KindSAMLIdPServiceProvider does not support label matcher + if resourceKind == types.KindSAMLIdPServiceProvider { + return true, nil + } + // check access normally if base checker doesnt exist if c.baseAuthChecker == nil { if err := c.accessChecker.CanAccess(resource); err != nil { @@ -6773,38 +6778,13 @@ func (a *ServerWithRoles) ListReleases(ctx context.Context) ([]*types.Release, e return a.authServer.releaseService.ListReleases(ctx) } -// TODO(sshah): set MFARequired for SAML IdP admin actions? -func (a *ServerWithRoles) checkAccessToSAMLIdPServiceProvider(sp types.SAMLIdPServiceProvider) error { - return a.context.Checker.CheckAccess( - sp, - // MFA is not required for operations on SAML resources but - // will be enforced at the connection time. - services.AccessState{}) -} - // ListSAMLIdPServiceProviders returns a paginated list of SAML IdP service provider resources. func (a *ServerWithRoles) ListSAMLIdPServiceProviders(ctx context.Context, pageSize int, nextToken string) ([]types.SAMLIdPServiceProvider, string, error) { if err := a.action(apidefaults.Namespace, types.KindSAMLIdPServiceProvider, types.VerbList); err != nil { return nil, "", trace.Wrap(err) } - sps, nextKey, err := a.authServer.ListSAMLIdPServiceProviders(ctx, pageSize, nextToken) - if err != nil { - return nil, "", trace.Wrap(err) - } - - // Filter out service providers the caller doesn't have access to. - var filtered []types.SAMLIdPServiceProvider - for _, sp := range sps { - err := a.checkAccessToSAMLIdPServiceProvider(sp) - if err != nil && !trace.IsAccessDenied(err) { - return nil, "", trace.Wrap(err) - } else if err == nil { - filtered = append(filtered, sp) - } - - } - return filtered, nextKey, nil + return a.authServer.ListSAMLIdPServiceProviders(ctx, pageSize, nextToken) } // GetSAMLIdPServiceProvider returns the specified SAML IdP service provider resources. @@ -6813,16 +6793,7 @@ func (a *ServerWithRoles) GetSAMLIdPServiceProvider(ctx context.Context, name st return nil, trace.Wrap(err) } - sp, err := a.authServer.GetSAMLIdPServiceProvider(ctx, name) - if err != nil { - return nil, trace.Wrap(err) - } - - if err = a.checkAccessToSAMLIdPServiceProvider(sp); err != nil { - return nil, trace.Wrap(err) - } - - return sp, nil + return a.authServer.GetSAMLIdPServiceProvider(ctx, name) } // CreateSAMLIdPServiceProvider creates a new SAML IdP service provider resource. @@ -6860,10 +6831,6 @@ func (a *ServerWithRoles) CreateSAMLIdPServiceProvider(ctx context.Context, sp t return trace.Wrap(err) } - if err = a.checkAccessToSAMLIdPServiceProvider(sp); err != nil { - return trace.Wrap(err) - } - if err := services.ValidateSAMLIdPACSURLAndRelayStateInputs(sp); err != nil { return trace.Wrap(err) } @@ -6913,17 +6880,6 @@ func (a *ServerWithRoles) UpdateSAMLIdPServiceProvider(ctx context.Context, sp t return trace.Wrap(err) } - existingSP, err := a.authServer.GetSAMLIdPServiceProvider(ctx, sp.GetName()) - if err != nil { - return trace.Wrap(err) - } - if err = a.checkAccessToSAMLIdPServiceProvider(existingSP); err != nil { - return trace.Wrap(err) - } - if err = a.checkAccessToSAMLIdPServiceProvider(sp); err != nil { - return trace.Wrap(err) - } - if err := services.ValidateSAMLIdPACSURLAndRelayStateInputs(sp); err != nil { return trace.Wrap(err) } @@ -6984,10 +6940,6 @@ func (a *ServerWithRoles) DeleteSAMLIdPServiceProvider(ctx context.Context, name return trace.Wrap(err) } - if err = a.checkAccessToSAMLIdPServiceProvider(sp); err != nil { - return trace.Wrap(err) - } - name = sp.GetName() entityID = sp.GetEntityID() @@ -7026,23 +6978,6 @@ func (a *ServerWithRoles) DeleteAllSAMLIdPServiceProviders(ctx context.Context) return trace.Wrap(err) } - var startKey string - for { - sps, nextKey, err := a.authServer.ListSAMLIdPServiceProviders(ctx, apidefaults.DefaultChunkSize, startKey) - if err != nil { - return trace.Wrap(err) - } - for _, sp := range sps { - if err := a.checkAccessToSAMLIdPServiceProvider(sp); err != nil { - return trace.Wrap(err) - } - } - if nextKey == "" { - break - } - startKey = nextKey - } - err = a.authServer.DeleteAllSAMLIdPServiceProviders(ctx) return trace.Wrap(err) } diff --git a/lib/auth/auth_with_roles_test.go b/lib/auth/auth_with_roles_test.go index a040e444083da..cfdf12ca155e6 100644 --- a/lib/auth/auth_with_roles_test.go +++ b/lib/auth/auth_with_roles_test.go @@ -3073,29 +3073,11 @@ func TestListSAMLIdPServiceProviderAndListResources(t *testing.T) { clt, err := srv.NewClient(identity) require.NoError(t, err) - // permit user to get the first service provider - role.SetAppLabels(types.Allow, types.Labels{"name": {testServiceProviders[0].GetName()}}) - _, err = srv.Auth().UpsertRole(ctx, role) - require.NoError(t, err) - sps, _, err := clt.ListSAMLIdPServiceProviders(ctx, 0, "") require.NoError(t, err) - require.Len(t, sps, 1) - require.Empty(t, cmp.Diff(testServiceProviders[0:1], sps)) - resp, err := clt.ListResources(ctx, listRequest) - require.NoError(t, err) - require.Len(t, resp.Resources, 1) - require.Empty(t, cmp.Diff(testResources[0:1], resp.Resources)) - - // permit user to get all service providers - role.SetAppLabels(types.Allow, types.Labels{types.Wildcard: {types.Wildcard}}) - _, err = srv.Auth().UpsertRole(ctx, role) - require.NoError(t, err) - sps, _, err = clt.ListSAMLIdPServiceProviders(ctx, 0, "") - require.NoError(t, err) require.EqualValues(t, len(testServiceProviders), len(sps)) require.Empty(t, cmp.Diff(testServiceProviders, sps)) - resp, err = clt.ListResources(ctx, listRequest) + resp, err := clt.ListResources(ctx, listRequest) require.NoError(t, err) require.Len(t, resp.Resources, len(testResources)) require.Empty(t, cmp.Diff(testResources, resp.Resources)) @@ -3107,14 +3089,6 @@ func TestListSAMLIdPServiceProviderAndListResources(t *testing.T) { ResourceType: types.KindSAMLIdPServiceProvider, } - // list only service providers with label - withLabels := baseRequest - withLabels.Labels = map[string]string{"name": testServiceProviders[0].GetName()} - resp, err = clt.ListResources(ctx, withLabels) - require.NoError(t, err) - require.Len(t, resp.Resources, 1) - require.Empty(t, cmp.Diff(testResources[0:1], resp.Resources)) - // Test search keywords match. withSearchKeywords := baseRequest withSearchKeywords.SearchKeywords = []string{"name", testServiceProviders[0].GetName()} @@ -3131,29 +3105,19 @@ func TestListSAMLIdPServiceProviderAndListResources(t *testing.T) { require.Len(t, resp.Resources, 1) require.Empty(t, cmp.Diff(testResources[0:1], resp.Resources)) - // deny user to get the first service provider - role.SetAppLabels(types.Deny, types.Labels{"name": {testServiceProviders[0].GetName()}}) - _, err = srv.Auth().UpsertRole(ctx, role) - require.NoError(t, err) - sps, _, err = clt.ListSAMLIdPServiceProviders(ctx, 0, "") - require.NoError(t, err) - require.EqualValues(t, len(testServiceProviders[1:]), len(sps)) - require.Empty(t, cmp.Diff(testServiceProviders[1:], sps)) - resp, err = clt.ListResources(ctx, listRequest) - require.NoError(t, err) - require.Len(t, resp.Resources, len(testResources[1:])) - require.Empty(t, cmp.Diff(testResources[1:], resp.Resources)) - // deny user to get all service providers - role.SetAppLabels(types.Deny, types.Labels{types.Wildcard: {types.Wildcard}}) + role.SetRules(types.Deny, []types.Rule{ + { + Resources: []string{types.KindSAMLIdPServiceProvider}, + Verbs: []string{types.VerbList}, + }, + }) + // role.SetAppLabels(types.Deny, types.Labels{types.Wildcard: {types.Wildcard}}) _, err = srv.Auth().UpsertRole(ctx, role) require.NoError(t, err) sps, _, err = clt.ListSAMLIdPServiceProviders(ctx, 0, "") - require.NoError(t, err) + require.ErrorContains(t, err, `access denied to perform action "list"`) require.Empty(t, sps) - resp, err = clt.ListResources(ctx, listRequest) - require.NoError(t, err) - require.Empty(t, resp.Resources) } // TestApps verifies RBAC is applied to app resources. @@ -6471,76 +6435,187 @@ func TestGetLicensePermissions(t *testing.T) { } } -const errSAMLAppLabelsDenied = "access to saml_idp_service_provider denied" - -func TestCreateSAMLIdPServiceProvider(t *testing.T) { +func TestGetSAMLIdPServiceProvider(t *testing.T) { ctx := context.Background() srv := newTestTLSServer(t) - const errCreateVerbDenied = `access denied to perform action "create"` + const errReadVerbDenied = `access denied to perform action "read"` + + sp, err := types.NewSAMLIdPServiceProvider( + types.Metadata{ + Name: "sp1", + Labels: map[string]string{ + "env": "dev", + }, + }, + types.SAMLIdPServiceProviderSpecV1{ + EntityDescriptor: newEntityDescriptor("sp1"), + EntityID: "sp1", + }) + require.NoError(t, err) + require.NoError(t, srv.Auth().CreateSAMLIdPServiceProvider(ctx, sp)) tt := []struct { - name string - metadata types.Metadata - spec types.SAMLIdPServiceProviderSpecV1 - allowRule types.RoleConditions - denyRule types.RoleConditions - eventCode string + name string + allowRule types.RoleConditions + denyRule types.RoleConditions + errAssertion require.ErrorAssertionFunc }{ { - name: "with VerbCreate and wildcard app_labels", - metadata: types.Metadata{ - Name: "sp1", - }, - spec: types.SAMLIdPServiceProviderSpecV1{ - EntityDescriptor: newEntityDescriptor("sp1"), - EntityID: "sp1", + name: "without VerbRead but a matching app_labels", + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}), + errAssertion: func(t require.TestingT, err error, i ...interface{}) { + require.ErrorContains(t, err, errReadVerbDenied) }, - allowRule: samlIdPRoleCondition(types.Labels{"*": []string{"*"}}, types.VerbCreate), - eventCode: events.SAMLIdPServiceProviderCreateCode, - errAssertion: require.NoError, }, + { - name: "with VerbCreate and a matching app_labels", - metadata: types.Metadata{ - Name: "sp2", - Labels: map[string]string{ - "env": "dev", - }, + name: "with allow VerbRead and deny VerbRead", + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbRead), + denyRule: samlIdPRoleCondition(types.Labels{}, types.VerbRead), + errAssertion: func(t require.TestingT, err error, i ...interface{}) { + require.ErrorContains(t, err, errReadVerbDenied) }, - spec: types.SAMLIdPServiceProviderSpecV1{ - EntityDescriptor: newEntityDescriptor("sp2"), - EntityID: "sp2", + }, + { + name: "without any permissions", + errAssertion: func(t require.TestingT, err error, i ...interface{}) { + require.ErrorContains(t, err, errReadVerbDenied) }, - allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbCreate), - eventCode: events.SAMLIdPServiceProviderCreateCode, - errAssertion: require.NoError, }, { - name: "without VerbCreate but a matching app_labels", - metadata: types.Metadata{ - Name: "sp1", - Labels: map[string]string{ - "env": "dev", - }, + name: "with VerbRead and a matching app_labels", + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbRead), + }, + { + name: "with VerbRead but a non-matching app_labels", + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"prod"}}, types.VerbRead), + }, + { + name: "with VerbRead but a deny app_labels", + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbRead), + denyRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}), + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + + user := createSAMLIdPTestUser(t, srv.Auth(), types.RoleSpecV6{Allow: tc.allowRule, Deny: tc.denyRule}) + + client, err := srv.NewClient(TestUser(user)) + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, client.Close()) + }) + spFromBackend, err := client.GetSAMLIdPServiceProvider(ctx, sp.GetName()) + if tc.errAssertion != nil { + tc.errAssertion(t, err) + } else { + require.Empty(t, cmp.Diff(sp, spFromBackend, + cmpopts.IgnoreFields(types.Metadata{}, "Revision"), + )) + } + }) + } +} + +func TestListSAMLIdPServiceProvider(t *testing.T) { + ctx := context.Background() + srv := newTestTLSServer(t) + + const errListVerbDenied = `access denied to perform action "list"` + + createSAMLIdPServiceProvider(t, ctx, srv.Auth(), "sp1", map[string]string{"env": "dev"}) + createSAMLIdPServiceProvider(t, ctx, srv.Auth(), "sp2", map[string]string{"env": "prod"}) + + tt := []struct { + name string + allowRule types.RoleConditions + denyRule types.RoleConditions + + errAssertion require.ErrorAssertionFunc + }{ + { + name: "without VerbList but a matching app_labels", + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}), + errAssertion: func(t require.TestingT, err error, i ...interface{}) { + require.ErrorContains(t, err, errListVerbDenied) }, - spec: types.SAMLIdPServiceProviderSpecV1{ - EntityDescriptor: newEntityDescriptor("sp1"), - EntityID: "sp1", + }, + + { + name: "with allow VerbList and deny VerbList", + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbList), + denyRule: samlIdPRoleCondition(types.Labels{}, types.VerbList), + errAssertion: func(t require.TestingT, err error, i ...interface{}) { + require.ErrorContains(t, err, errListVerbDenied) }, - allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}), - eventCode: events.SAMLIdPServiceProviderCreateFailureCode, + }, + + { + name: "without any permissions", errAssertion: func(t require.TestingT, err error, i ...interface{}) { - require.ErrorContains(t, err, errCreateVerbDenied) + require.ErrorContains(t, err, errListVerbDenied) }, }, { - name: "with VerbCreate but a non-matching app_labels", + name: "with VerbList and a matching app_labels", + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbList), + }, + { + name: "with VerbList but a non-matching app_labels", + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"prod"}}, types.VerbList), + }, + { + name: "with VerbList but a deny app_labels", + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbList), + denyRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}), + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + user := createSAMLIdPTestUser(t, srv.Auth(), types.RoleSpecV6{Allow: tc.allowRule, Deny: tc.denyRule}) + + client, err := srv.NewClient(TestUser(user)) + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, client.Close()) + }) + sps, _, err := client.ListSAMLIdPServiceProviders(ctx, 10, "") + if tc.errAssertion != nil { + tc.errAssertion(t, err) + } else { + require.Len(t, sps, 2) + } + }) + } +} + +func TestCreateSAMLIdPServiceProvider(t *testing.T) { + ctx := context.Background() + srv := newTestTLSServer(t) + + const errCreateVerbDenied = `access denied to perform action "create"` + + tt := []struct { + name string + metadata types.Metadata + spec types.SAMLIdPServiceProviderSpecV1 + allowRule types.RoleConditions + denyRule types.RoleConditions + eventCode string + errAssertion require.ErrorAssertionFunc + }{ + + { + name: "with allow VerbCreate and deny VerbCreate", metadata: types.Metadata{ Name: "sp1", Labels: map[string]string{ - "env": "prod", + "env": "dev", }, }, spec: types.SAMLIdPServiceProviderSpecV1{ @@ -6548,25 +6623,21 @@ func TestCreateSAMLIdPServiceProvider(t *testing.T) { EntityID: "sp1", }, allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbCreate), + denyRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbCreate), eventCode: events.SAMLIdPServiceProviderCreateFailureCode, errAssertion: func(t require.TestingT, err error, i ...interface{}) { - require.ErrorContains(t, err, errSAMLAppLabelsDenied) + require.ErrorContains(t, err, errCreateVerbDenied) }, }, { - name: "with allow VerbCreate and deny VerbCreate", + name: "without any permissions", metadata: types.Metadata{ Name: "sp1", - Labels: map[string]string{ - "env": "dev", - }, }, spec: types.SAMLIdPServiceProviderSpecV1{ EntityDescriptor: newEntityDescriptor("sp1"), EntityID: "sp1", }, - allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbCreate), - denyRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbCreate), eventCode: events.SAMLIdPServiceProviderCreateFailureCode, errAssertion: func(t require.TestingT, err error, i ...interface{}) { require.ErrorContains(t, err, errCreateVerbDenied) @@ -6584,26 +6655,26 @@ func TestCreateSAMLIdPServiceProvider(t *testing.T) { EntityDescriptor: newEntityDescriptor("sp1"), EntityID: "sp1", }, - allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbCreate), - denyRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}), - eventCode: events.SAMLIdPServiceProviderCreateFailureCode, - errAssertion: func(t require.TestingT, err error, i ...interface{}) { - require.ErrorContains(t, err, errSAMLAppLabelsDenied) - }, + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbCreate), + denyRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}), + eventCode: events.SAMLIdPServiceProviderCreateCode, + errAssertion: require.NoError, }, { - name: "without any permissions", + name: "with VerbCreate but a non-matching app_labels", metadata: types.Metadata{ - Name: "sp1", + Name: "sp2", + Labels: map[string]string{ + "env": "prod", + }, }, spec: types.SAMLIdPServiceProviderSpecV1{ - EntityDescriptor: newEntityDescriptor("sp1"), - EntityID: "sp1", - }, - eventCode: events.SAMLIdPServiceProviderCreateFailureCode, - errAssertion: func(t require.TestingT, err error, i ...interface{}) { - require.ErrorContains(t, err, errCreateVerbDenied) + EntityDescriptor: newEntityDescriptor("sp2"), + EntityID: "sp2", }, + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbCreate), + eventCode: events.SAMLIdPServiceProviderCreateCode, + errAssertion: require.NoError, }, } @@ -6631,21 +6702,7 @@ func TestUpdateSAMLIdPServiceProvider(t *testing.T) { const errUpdateVerbDenied = `access denied to perform action "update"` - sp := &types.SAMLIdPServiceProviderV1{ - ResourceHeader: types.ResourceHeader{ - Metadata: types.Metadata{ - Name: "sp1", - Labels: map[string]string{ - "env": "dev", - }, - }, - }, - Spec: types.SAMLIdPServiceProviderSpecV1{ - EntityDescriptor: newEntityDescriptor("sp1"), - EntityID: "sp1", - }, - } - require.NoError(t, srv.Auth().CreateSAMLIdPServiceProvider(ctx, sp)) + createSAMLIdPServiceProvider(t, ctx, srv.Auth(), "sp1", map[string]string{"env": "dev"}) tt := []struct { name string @@ -6657,36 +6714,18 @@ func TestUpdateSAMLIdPServiceProvider(t *testing.T) { errAssertion require.ErrorAssertionFunc }{ { - name: "with VerbUpdate and wildcard app_labels", + name: "without any permissions", metadata: types.Metadata{ Name: "sp1", - Labels: map[string]string{ - "env": "dev", - }, }, spec: types.SAMLIdPServiceProviderSpecV1{ - EntityDescriptor: newEntityDescriptor("sp2"), - EntityID: "sp2", - }, - allowRule: samlIdPRoleCondition(types.Labels{"*": []string{"*"}}, types.VerbUpdate), - eventCode: events.SAMLIdPServiceProviderUpdateCode, - errAssertion: require.NoError, - }, - { - name: "with VerbUpdate and a matching app_labels", - metadata: types.Metadata{ - Name: "sp1", - Labels: map[string]string{ - "env": "dev", - }, + EntityDescriptor: newEntityDescriptor("sp1"), + EntityID: "sp1", }, - spec: types.SAMLIdPServiceProviderSpecV1{ - EntityDescriptor: newEntityDescriptor("sp3"), - EntityID: "sp3", + eventCode: events.SAMLIdPServiceProviderUpdateFailureCode, + errAssertion: func(t require.TestingT, err error, i ...interface{}) { + require.ErrorContains(t, err, errUpdateVerbDenied) }, - allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbUpdate), - eventCode: events.SAMLIdPServiceProviderUpdateCode, - errAssertion: require.NoError, }, { name: "without VerbUpdate but a matching app_labels", @@ -6706,24 +6745,6 @@ func TestUpdateSAMLIdPServiceProvider(t *testing.T) { require.ErrorContains(t, err, errUpdateVerbDenied) }, }, - { - name: "with VerbUpdate but a non-matching app_labels", - metadata: types.Metadata{ - Name: "sp1", - Labels: map[string]string{ - "env": "prod", - }, - }, - spec: types.SAMLIdPServiceProviderSpecV1{ - EntityDescriptor: newEntityDescriptor("sp2"), - EntityID: "sp2", - }, - allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbUpdate), - eventCode: events.SAMLIdPServiceProviderUpdateFailureCode, - errAssertion: func(t require.TestingT, err error, i ...interface{}) { - require.ErrorContains(t, err, errSAMLAppLabelsDenied) - }, - }, { name: "with allow VerbUpdate and deny VerbUpdate", metadata: types.Metadata{ @@ -6744,7 +6765,7 @@ func TestUpdateSAMLIdPServiceProvider(t *testing.T) { }, }, { - name: "with VerbUpdate but a deny app_labels", + name: "with VerbUpdate but a non-matching app_labels", metadata: types.Metadata{ Name: "sp1", Labels: map[string]string{ @@ -6755,26 +6776,26 @@ func TestUpdateSAMLIdPServiceProvider(t *testing.T) { EntityDescriptor: newEntityDescriptor("sp1"), EntityID: "sp1", }, - allowRule: samlIdPRoleCondition(types.Labels{}, types.VerbUpdate), - denyRule: samlIdPRoleCondition(types.Labels{"env": []string{"prod"}}), - eventCode: events.SAMLIdPServiceProviderUpdateFailureCode, - errAssertion: func(t require.TestingT, err error, i ...interface{}) { - require.ErrorContains(t, err, errSAMLAppLabelsDenied) - }, + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbUpdate), + eventCode: events.SAMLIdPServiceProviderUpdateCode, + errAssertion: require.NoError, }, { - name: "without any permissions", + name: "with VerbUpdate but a deny app_labels", metadata: types.Metadata{ Name: "sp1", + Labels: map[string]string{ + "env": "prod", + }, }, spec: types.SAMLIdPServiceProviderSpecV1{ EntityDescriptor: newEntityDescriptor("sp1"), EntityID: "sp1", }, - eventCode: events.SAMLIdPServiceProviderUpdateFailureCode, - errAssertion: func(t require.TestingT, err error, i ...interface{}) { - require.ErrorContains(t, err, errUpdateVerbDenied) - }, + allowRule: samlIdPRoleCondition(types.Labels{}, types.VerbUpdate), + denyRule: samlIdPRoleCondition(types.Labels{"env": []string{"prod"}}), + eventCode: events.SAMLIdPServiceProviderUpdateCode, + errAssertion: require.NoError, }, } @@ -6987,25 +7008,9 @@ func TestDeleteSAMLIdPServiceProvider(t *testing.T) { const errDeleteVerbDenied = `access denied to perform action "delete"` - sp := &types.SAMLIdPServiceProviderV1{ - ResourceHeader: types.ResourceHeader{ - Metadata: types.Metadata{ - Name: "sp1", - Labels: map[string]string{ - "env": "dev", - }, - }, - }, - Spec: types.SAMLIdPServiceProviderSpecV1{ - EntityDescriptor: newEntityDescriptor("sp1"), - EntityID: "sp1", - }, - } - require.NoError(t, srv.Auth().CreateSAMLIdPServiceProvider(ctx, sp)) - + const serviceProviderName = "sp1" tt := []struct { name string - spName string allowRule types.RoleConditions denyRule types.RoleConditions eventCode string @@ -7013,25 +7018,14 @@ func TestDeleteSAMLIdPServiceProvider(t *testing.T) { }{ { name: "without VerbDelete but a matching app_labels", - spName: "sp1", allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}), eventCode: events.SAMLIdPServiceProviderDeleteFailureCode, errAssertion: func(t require.TestingT, err error, i ...interface{}) { require.ErrorContains(t, err, errDeleteVerbDenied) }, }, - { - name: "with VerbDelete but a non-matching app_labels", - spName: "sp1", - allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"prod"}}, types.VerbDelete), - eventCode: events.SAMLIdPServiceProviderDeleteFailureCode, - errAssertion: func(t require.TestingT, err error, i ...interface{}) { - require.ErrorContains(t, err, errSAMLAppLabelsDenied) - }, - }, { name: "with allow VerbDelete and deny VerbDelete", - spName: "sp1", allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbDelete), denyRule: samlIdPRoleCondition(types.Labels{}, types.VerbDelete), eventCode: events.SAMLIdPServiceProviderDeleteFailureCode, @@ -7039,19 +7033,8 @@ func TestDeleteSAMLIdPServiceProvider(t *testing.T) { require.ErrorContains(t, err, errDeleteVerbDenied) }, }, - { - name: "with VerbDelete but a deny app_labels", - spName: "sp1", - allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbDelete), - denyRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}), - eventCode: events.SAMLIdPServiceProviderDeleteFailureCode, - errAssertion: func(t require.TestingT, err error, i ...interface{}) { - require.ErrorContains(t, err, errSAMLAppLabelsDenied) - }, - }, { name: "without any permissions", - spName: "sp1", eventCode: events.SAMLIdPServiceProviderDeleteFailureCode, errAssertion: func(t require.TestingT, err error, i ...interface{}) { require.ErrorContains(t, err, errDeleteVerbDenied) @@ -7059,15 +7042,32 @@ func TestDeleteSAMLIdPServiceProvider(t *testing.T) { }, { name: "with VerbDelete and a matching app_labels", - spName: "sp1", allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbDelete), eventCode: events.SAMLIdPServiceProviderDeleteCode, errAssertion: require.NoError, }, + { + name: "with VerbDelete but a non-matching app_labels", + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"prod"}}, types.VerbDelete), + eventCode: events.SAMLIdPServiceProviderDeleteCode, + errAssertion: require.NoError, + }, + { + name: "with VerbDelete but a deny app_labels", + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbDelete), + denyRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}), + eventCode: events.SAMLIdPServiceProviderDeleteCode, + errAssertion: require.NoError, + }, } for _, tc := range tt { t.Run(tc.name, func(t *testing.T) { + _, err := srv.Auth().GetSAMLIdPServiceProvider(ctx, serviceProviderName) + if trace.IsNotFound(err) { + createSAMLIdPServiceProvider(t, ctx, srv.Auth(), serviceProviderName, map[string]string{"env": "dev"}) + } + user := createSAMLIdPTestUser(t, srv.Auth(), types.RoleSpecV6{Allow: tc.allowRule, Deny: tc.denyRule}) client, err := srv.NewClient(TestUser(user)) @@ -7077,7 +7077,7 @@ func TestDeleteSAMLIdPServiceProvider(t *testing.T) { }) modifyAndWaitForEvent(t, tc.errAssertion, srv, tc.eventCode, func() error { - return client.DeleteSAMLIdPServiceProvider(ctx, tc.spName) + return client.DeleteSAMLIdPServiceProvider(ctx, serviceProviderName) }) }) } @@ -7118,14 +7118,6 @@ func TestDeleteAllSAMLIdPServiceProviders(t *testing.T) { require.ErrorContains(t, err, errDeleteVerbDenied) }, }, - { - name: "with VerbDelete but a non-matching app_labels", - allowRule: samlIdPRoleCondition(types.Labels{}, types.VerbDelete), - eventCode: events.SAMLIdPServiceProviderDeleteAllFailureCode, - errAssertion: func(t require.TestingT, err error, i ...interface{}) { - require.ErrorContains(t, err, errSAMLAppLabelsDenied) - }, - }, { name: "with allow VerbDelete and deny VerbDelete", allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbDelete), @@ -7135,15 +7127,6 @@ func TestDeleteAllSAMLIdPServiceProviders(t *testing.T) { require.ErrorContains(t, err, errDeleteVerbDenied) }, }, - { - name: "with VerbDelete but a deny app_labels", - allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbDelete), - denyRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}), - eventCode: events.SAMLIdPServiceProviderDeleteAllFailureCode, - errAssertion: func(t require.TestingT, err error, i ...interface{}) { - require.ErrorContains(t, err, errSAMLAppLabelsDenied) - }, - }, { name: "without any permissions", eventCode: events.SAMLIdPServiceProviderDeleteAllFailureCode, @@ -7151,6 +7134,20 @@ func TestDeleteAllSAMLIdPServiceProviders(t *testing.T) { require.ErrorContains(t, err, errDeleteVerbDenied) }, }, + { + name: "with VerbDelete but a non-matching app_labels", + allowRule: samlIdPRoleCondition(types.Labels{}, types.VerbDelete), + eventCode: events.SAMLIdPServiceProviderDeleteAllCode, + errAssertion: require.NoError, + }, + { + name: "with VerbDelete but a deny app_labels", + allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbDelete), + denyRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}), + eventCode: events.SAMLIdPServiceProviderDeleteAllCode, + errAssertion: require.NoError, + }, + { name: "with VerbDelete and a matching app_labels", allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbDelete), @@ -7176,6 +7173,21 @@ func TestDeleteAllSAMLIdPServiceProviders(t *testing.T) { } } +func createSAMLIdPServiceProvider(t *testing.T, ctx context.Context, authServer *Server, name string, labels map[string]string) { + t.Helper() + sp, err := types.NewSAMLIdPServiceProvider( + types.Metadata{ + Name: name, + Labels: labels, + }, + types.SAMLIdPServiceProviderSpecV1{ + EntityDescriptor: newEntityDescriptor(name), + EntityID: name, + }) + require.NoError(t, err) + require.NoError(t, authServer.CreateSAMLIdPServiceProvider(ctx, sp)) +} + func createSAMLIdPTestUser(t *testing.T, server *Server, userRole types.RoleSpecV6) string { ctx := context.Background() From 990e2d41553cae11388725c0100a870dd6a9a0d9 Mon Sep 17 00:00:00 2001 From: flyinghermit Date: Fri, 1 Nov 2024 01:02:07 -0400 Subject: [PATCH 2/2] add todo for role V8, update test to accomodate default implicit RO rule --- lib/auth/auth_with_roles.go | 1 + lib/auth/auth_with_roles_test.go | 21 ++++----------------- 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index c896a74a4b346..92a435bb4b912 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -1264,6 +1264,7 @@ func (c *resourceAccess) checkAccess(resource types.ResourceWithLabels, filter s } // KindSAMLIdPServiceProvider does not support label matcher + // TODO(sshah): remove this exclusion once we introduce role V8. if resourceKind == types.KindSAMLIdPServiceProvider { return true, nil } diff --git a/lib/auth/auth_with_roles_test.go b/lib/auth/auth_with_roles_test.go index cfdf12ca155e6..c533a554959d0 100644 --- a/lib/auth/auth_with_roles_test.go +++ b/lib/auth/auth_with_roles_test.go @@ -6463,13 +6463,13 @@ func TestGetSAMLIdPServiceProvider(t *testing.T) { errAssertion require.ErrorAssertionFunc }{ { - name: "without VerbRead but a matching app_labels", + name: "with deny VerbRead but a matching app_labels", allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}), + denyRule: samlIdPRoleCondition(types.Labels{}, types.VerbRead), errAssertion: func(t require.TestingT, err error, i ...interface{}) { require.ErrorContains(t, err, errReadVerbDenied) }, }, - { name: "with allow VerbRead and deny VerbRead", allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbRead), @@ -6478,12 +6478,6 @@ func TestGetSAMLIdPServiceProvider(t *testing.T) { require.ErrorContains(t, err, errReadVerbDenied) }, }, - { - name: "without any permissions", - errAssertion: func(t require.TestingT, err error, i ...interface{}) { - require.ErrorContains(t, err, errReadVerbDenied) - }, - }, { name: "with VerbRead and a matching app_labels", allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbRead), @@ -6538,13 +6532,13 @@ func TestListSAMLIdPServiceProvider(t *testing.T) { errAssertion require.ErrorAssertionFunc }{ { - name: "without VerbList but a matching app_labels", + name: "With deny VerbList but a matching app_labels", allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}), + denyRule: samlIdPRoleCondition(types.Labels{}, types.VerbList), errAssertion: func(t require.TestingT, err error, i ...interface{}) { require.ErrorContains(t, err, errListVerbDenied) }, }, - { name: "with allow VerbList and deny VerbList", allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbList), @@ -6553,13 +6547,6 @@ func TestListSAMLIdPServiceProvider(t *testing.T) { require.ErrorContains(t, err, errListVerbDenied) }, }, - - { - name: "without any permissions", - errAssertion: func(t require.TestingT, err error, i ...interface{}) { - require.ErrorContains(t, err, errListVerbDenied) - }, - }, { name: "with VerbList and a matching app_labels", allowRule: samlIdPRoleCondition(types.Labels{"env": []string{"dev"}}, types.VerbList),