diff --git a/service/integration/attributes_test.go b/service/integration/attributes_test.go index 675e471af1..8c20c88a4a 100644 --- a/service/integration/attributes_test.go +++ b/service/integration/attributes_test.go @@ -66,6 +66,42 @@ func (s *AttributesSuite) Test_CreateAttribute_NoMetadataSucceeds() { s.NotNil(createdAttr) } +func (s *AttributesSuite) Test_CreateAttribute_WithoutValues_DoesNotReturnEmptyValue() { + attr := &attributes.CreateAttributeRequest{ + Name: "test__create_attribute_without_values", + NamespaceId: fixtureNamespaceID, + Rule: policy.AttributeRuleTypeEnum_ATTRIBUTE_RULE_TYPE_ENUM_HIERARCHY, + } + + createdAttr, err := s.db.PolicyClient.CreateAttribute(s.ctx, attr) + s.Require().NoError(err) + s.Require().NotNil(createdAttr) + s.Empty(createdAttr.GetValues()) + + gotAttr, err := s.db.PolicyClient.GetAttribute(s.ctx, createdAttr.GetId()) + s.Require().NoError(err) + s.Require().NotNil(gotAttr) + s.Empty(gotAttr.GetValues()) + + listRsp, err := s.db.PolicyClient.ListAttributes(s.ctx, &attributes.ListAttributesRequest{ + Namespace: fixtureNamespaceID, + State: common.ActiveStateEnum_ACTIVE_STATE_ENUM_ANY, + }) + s.Require().NoError(err) + s.Require().NotNil(listRsp) + + for _, listedAttr := range listRsp.GetAttributes() { + if listedAttr.GetId() != createdAttr.GetId() { + continue + } + + s.Empty(listedAttr.GetValues()) + return + } + + s.Failf("created attribute not found in list response", "attribute_id=%s", createdAttr.GetId()) +} + func (s *AttributesSuite) Test_CreateAttribute_NormalizeName() { name := "NaMe_12_ShOuLdBe-NoRmAlIzEd" attr := &attributes.CreateAttributeRequest{ diff --git a/service/integration/namespaces_test.go b/service/integration/namespaces_test.go index f7c9e8640c..46a1549764 100644 --- a/service/integration/namespaces_test.go +++ b/service/integration/namespaces_test.go @@ -84,6 +84,29 @@ func (s *NamespacesSuite) Test_CreateNamespace_NormalizeCasing() { s.Equal(strings.ToLower(name), got.GetName(), createdNamespace.GetName()) } +func (s *NamespacesSuite) Test_CreateNamespace_WithoutPublicKeys_DoesNotReturnKeys() { + name := fmt.Sprintf("no-namespace-children-%d.com", time.Now().UnixNano()) + createdNamespace, err := s.db.PolicyClient.CreateNamespace(s.ctx, &namespaces.CreateNamespaceRequest{Name: name}) + s.Require().NoError(err) + s.Require().NotNil(createdNamespace) + defer func() { + _, err := s.db.PolicyClient.UnsafeDeleteNamespace(s.ctx, createdNamespace, createdNamespace.GetFqn()) + s.Require().NoError(err) + }() + + s.Empty(createdNamespace.GetKasKeys()) + + gotByID, err := s.db.PolicyClient.GetNamespace(s.ctx, createdNamespace.GetId()) + s.Require().NoError(err) + s.Require().NotNil(gotByID) + s.Empty(gotByID.GetKasKeys()) + + gotByFQN, err := s.db.PolicyClient.GetNamespace(s.ctx, &namespaces.GetNamespaceRequest_Fqn{Fqn: createdNamespace.GetName()}) + s.Require().NoError(err) + s.Require().NotNil(gotByFQN) + s.Empty(gotByFQN.GetKasKeys()) +} + func (s *NamespacesSuite) Test_GetNamespace() { testData := s.getActiveNamespaceFixtures() diff --git a/service/integration/obligations_test.go b/service/integration/obligations_test.go index 72cc1c240c..f233a055c6 100644 --- a/service/integration/obligations_test.go +++ b/service/integration/obligations_test.go @@ -103,6 +103,44 @@ func (s *ObligationsSuite) Test_CreateObligation_Succeeds() { s.deleteObligations([]string{obl.GetId()}) } +func (s *ObligationsSuite) Test_CreateObligation_WithoutValues_DoesNotReturnValues() { + namespaceID, namespaceFQN, namespace := s.getNamespaceData(nsExampleCom) + name := fmt.Sprintf("%s-no-values-%d", oblName, time.Now().UnixNano()) + + createdObl := s.createObligation(namespaceID, name, nil) + defer s.deleteObligations([]string{createdObl.GetId()}) + + s.assertObligationBasics(createdObl, name, namespaceID, namespace.Name, namespaceFQN) + s.Empty(createdObl.GetValues()) + + gotObl, err := s.db.PolicyClient.GetObligation(s.ctx, &obligations.GetObligationRequest{ + Id: createdObl.GetId(), + }) + s.Require().NoError(err) + s.Require().NotNil(gotObl) + s.assertObligationBasics(gotObl, name, namespaceID, namespace.Name, namespaceFQN) + s.Empty(gotObl.GetValues()) + + oblList, _, err := s.db.PolicyClient.ListObligations(s.ctx, &obligations.ListObligationsRequest{ + NamespaceId: namespaceID, + }) + s.Require().NoError(err) + s.Require().NotNil(oblList) + + found := false + for _, obl := range oblList { + if obl.GetId() != createdObl.GetId() { + continue + } + + found = true + s.assertObligationBasics(obl, name, namespaceID, namespace.Name, namespaceFQN) + s.Empty(obl.GetValues()) + break + } + s.True(found) +} + func (s *ObligationsSuite) Test_CreateObligation_Fails() { // Invalid namespace ID obl, err := s.db.PolicyClient.CreateObligation(s.ctx, &obligations.CreateObligationRequest{ diff --git a/service/internal/access/v2/pdp_test.go b/service/internal/access/v2/pdp_test.go index 0601d4ed28..4c873351c5 100644 --- a/service/internal/access/v2/pdp_test.go +++ b/service/internal/access/v2/pdp_test.go @@ -871,6 +871,30 @@ func (s *PDPTestSuite) TestNewPolicyDecisionPoint() { } } +func (s *PDPTestSuite) TestNewPolicyDecisionPoint_AllowsAttributeDefinitionsWithoutValues() { + f := s.fixtures + emptyAttrFQN := createAttrFQN(testSecondaryNamespace, "empty") + emptyAttr := &policy.Attribute{ + Fqn: emptyAttrFQN, + Rule: policy.AttributeRuleTypeEnum_ATTRIBUTE_RULE_TYPE_ENUM_ANY_OF, + } + + pdp, err := NewPolicyDecisionPoint( + s.T().Context(), + s.logger, + []*policy.Attribute{f.classificationAttr, emptyAttr}, + []*policy.SubjectMapping{f.secretMapping}, + nil, + allowDirectEntitlements, + ) + + s.Require().NoError(err) + s.Require().NotNil(pdp) + + s.Require().Contains(pdp.allAttributesByDefinitionFQN, emptyAttrFQN) + s.Require().NotContains(pdp.allEntitleableAttributesByValueFQN, emptyAttrFQN) +} + // Test_GetDecision_MultipleResources tests the GetDecision method with some generalized scenarios for multiple resources func (s *PDPTestSuite) Test_GetDecision_MultipleResources() { f := s.fixtures diff --git a/service/internal/access/v2/validators.go b/service/internal/access/v2/validators.go index b10515b7e2..12cfe54285 100644 --- a/service/internal/access/v2/validators.go +++ b/service/internal/access/v2/validators.go @@ -93,8 +93,10 @@ func validateSubjectMapping(subjectMapping *policy.SubjectMapping) error { // // - must not be nil // - must have a non-empty FQN -// - must have non-empty values -// - must have non-empty values FQNs +// - if values are present, they must have non-empty values FQNs +// +// Attribute definitions may legitimately exist before any values have been created +// for them, so an empty values list is allowed here. func validateAttribute(attribute *policy.Attribute) error { if attribute == nil { return fmt.Errorf("attribute is nil: %w", ErrInvalidAttributeDefinition) @@ -102,9 +104,6 @@ func validateAttribute(attribute *policy.Attribute) error { if attribute.GetFqn() == "" { return fmt.Errorf("attribute FQN is empty: %w", ErrInvalidAttributeDefinition) } - if len(attribute.GetValues()) == 0 { - return fmt.Errorf("attribute values are empty: %w", ErrInvalidAttributeDefinition) - } for _, value := range attribute.GetValues() { if value == nil { return fmt.Errorf("attribute value is nil: %w", ErrInvalidAttributeDefinition) diff --git a/service/internal/access/v2/validators_test.go b/service/internal/access/v2/validators_test.go index 67c6d7557c..4da9a23036 100644 --- a/service/internal/access/v2/validators_test.go +++ b/service/internal/access/v2/validators_test.go @@ -217,22 +217,22 @@ func TestValidateAttribute(t *testing.T) { wantErr: ErrInvalidAttributeDefinition, }, { - name: "Empty attribute values", + name: "Empty attribute values are allowed", attribute: &policy.Attribute{ Fqn: "https://example.org/attr/name", Rule: policy.AttributeRuleTypeEnum_ATTRIBUTE_RULE_TYPE_ENUM_ANY_OF, Values: []*policy.Value{}, }, - wantErr: ErrInvalidAttributeDefinition, + wantErr: nil, }, { - name: "Nil attribute values", + name: "Nil attribute values are allowed", attribute: &policy.Attribute{ Fqn: "https://example.org/attr/name", Rule: policy.AttributeRuleTypeEnum_ATTRIBUTE_RULE_TYPE_ENUM_HIERARCHY, Values: nil, }, - wantErr: ErrInvalidAttributeDefinition, + wantErr: nil, }, { name: "Nil value in attribute values", diff --git a/service/policy/db/attributes.sql.go b/service/policy/db/attributes.sql.go index fa0eac15c4..af3df5a78b 100644 --- a/service/policy/db/attributes.sql.go +++ b/service/policy/db/attributes.sql.go @@ -98,7 +98,7 @@ SELECT 'active', avt.active, 'fqn', CONCAT(fqns.fqn, '/value/', avt.value) ) ORDER BY ARRAY_POSITION(ad.values_order, avt.id) - ) AS values, + ) FILTER (WHERE avt.id IS NOT NULL) AS values, JSONB_AGG( DISTINCT JSONB_BUILD_OBJECT( 'id', kas.id, @@ -185,7 +185,7 @@ type getAttributeRow struct { // 'active', avt.active, // 'fqn', CONCAT(fqns.fqn, '/value/', avt.value) // ) ORDER BY ARRAY_POSITION(ad.values_order, avt.id) -// ) AS values, +// ) FILTER (WHERE avt.id IS NOT NULL) AS values, // JSONB_AGG( // DISTINCT JSONB_BUILD_OBJECT( // 'id', kas.id, @@ -774,7 +774,7 @@ SELECT 'active', avt.active, 'fqn', CONCAT(fqns.fqn, '/value/', avt.value) ) ORDER BY ARRAY_POSITION(ad.values_order, avt.id) - ) AS values, + ) FILTER (WHERE avt.id IS NOT NULL) AS values, fqns.fqn, COUNT(*) OVER() AS total FROM attribute_definitions ad @@ -841,7 +841,7 @@ type listAttributesDetailRow struct { // 'active', avt.active, // 'fqn', CONCAT(fqns.fqn, '/value/', avt.value) // ) ORDER BY ARRAY_POSITION(ad.values_order, avt.id) -// ) AS values, +// ) FILTER (WHERE avt.id IS NOT NULL) AS values, // fqns.fqn, // COUNT(*) OVER() AS total // FROM attribute_definitions ad diff --git a/service/policy/db/queries/attributes.sql b/service/policy/db/queries/attributes.sql index a2c8e48ef5..f698640686 100644 --- a/service/policy/db/queries/attributes.sql +++ b/service/policy/db/queries/attributes.sql @@ -19,7 +19,7 @@ SELECT 'active', avt.active, 'fqn', CONCAT(fqns.fqn, '/value/', avt.value) ) ORDER BY ARRAY_POSITION(ad.values_order, avt.id) - ) AS values, + ) FILTER (WHERE avt.id IS NOT NULL) AS values, fqns.fqn, COUNT(*) OVER() AS total FROM attribute_definitions ad @@ -307,7 +307,7 @@ SELECT 'active', avt.active, 'fqn', CONCAT(fqns.fqn, '/value/', avt.value) ) ORDER BY ARRAY_POSITION(ad.values_order, avt.id) - ) AS values, + ) FILTER (WHERE avt.id IS NOT NULL) AS values, JSONB_AGG( DISTINCT JSONB_BUILD_OBJECT( 'id', kas.id,