Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(policy): make MatchSubjectMappings operator agnostic #1658

Merged
merged 28 commits into from
Nov 4, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
b906280
remove operator condition
suchak1 Oct 16, 2024
af489a5
remove value condition
suchak1 Oct 16, 2024
be2e94c
remove duplicate field condition
suchak1 Oct 16, 2024
9b850e7
update db code comments
suchak1 Oct 17, 2024
b4597b0
update proto
suchak1 Oct 17, 2024
2562520
update gen code
suchak1 Oct 17, 2024
b8faf75
change NOT_IN test
suchak1 Oct 17, 2024
bf1035e
don't consider values in multiple test
suchak1 Oct 17, 2024
f2b7beb
add validation to proto
suchak1 Oct 18, 2024
6581f84
remove values from MatchSubjectMappingsRequest
suchak1 Oct 18, 2024
bac4f8f
Revert "remove values from MatchSubjectMappingsRequest"
suchak1 Oct 18, 2024
394e061
add value back to request
suchak1 Oct 18, 2024
cb156ae
make proto-generate
suchak1 Oct 18, 2024
1dd6d34
remove test
suchak1 Oct 19, 2024
cd3a765
remove integration tests, add unit tests
suchak1 Oct 19, 2024
caf0077
convert to sqlc
suchak1 Oct 21, 2024
881ba07
clean up
suchak1 Oct 21, 2024
e01820f
remove unused select fx
suchak1 Oct 21, 2024
4723a71
rewrite unused hydrate fxs
suchak1 Oct 21, 2024
c58881e
delete hydrate fxs - type issues
suchak1 Oct 21, 2024
93c22c6
delete constructMetadata fx
suchak1 Oct 21, 2024
24870d4
Merge branch 'main' into feature/matchsubjectmappings
suchak1 Oct 21, 2024
b7c4788
fix(policy): fix MatchSubjectMappings e2e functionality (#1692)
jakedoublev Oct 29, 2024
21d7610
chore(policy): DSP-241 clean up db table code (#1682)
suchak1 Oct 29, 2024
f64e0cd
Merge branch 'main' into feature/matchsubjectmappings
jakedoublev Nov 4, 2024
394052a
chore(policy): address merge conflict in proto gencode (#1711)
jakedoublev Nov 4, 2024
8516522
re-trigger CI
jakedoublev Nov 4, 2024
4b17a70
Merge branch 'main' into feature/matchsubjectmappings
jakedoublev Nov 4, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/grpc/index.html

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 2 additions & 9 deletions protocol/go/policy/subjectmapping/subject_mapping.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions service/integration/subject_mappings_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,7 @@ func (s *SubjectMappingsSuite) TestGetMatchedSubjectMappings_InOne() {
s.Equal(fixtureScs.ID, sm[0].GetSubjectConditionSet().GetId())
}

func (s *SubjectMappingsSuite) TestGetMatchedSubjectMappings_DoesNotReturnNotInWhenMatches() {
func (s *SubjectMappingsSuite) TestGetMatchedSubjectMappings_ReturnsNotInWhenMatches() {
fixtureScs := s.f.GetSubjectConditionSetKey("subject_condition_simple_not_in")
externalSelectorValue := fixtureScs.Condition.SubjectSets[0].ConditionGroups[0].Conditions[0].SubjectExternalSelectorValue
externalValues := fixtureScs.Condition.SubjectSets[0].ConditionGroups[0].Conditions[0].SubjectExternalValues
Expand All @@ -849,7 +849,7 @@ func (s *SubjectMappingsSuite) TestGetMatchedSubjectMappings_DoesNotReturnNotInW
smList, err := s.db.PolicyClient.GetMatchedSubjectMappings(context.Background(), props)
s.Require().NoError(err)
s.NotZero(smList)
s.Empty(smList)
s.Equal(fixtureScs.ID, smList[0].GetSubjectConditionSet().GetId())
}

func (s *SubjectMappingsSuite) TestGetMatchedSubjectMappings_NotInOneMatch() {
Expand Down Expand Up @@ -1003,7 +1003,7 @@ func (s *SubjectMappingsSuite) TestGetMatchedSubjectMappings_NotInMultiple() {
smList, err := s.db.PolicyClient.GetMatchedSubjectMappings(context.Background(), props)
s.Require().NoError(err)
s.NotZero(smList)
s.Len(smList, 2)
s.Len(smList, 3)
for _, sm := range smList {
if sm.GetSubjectConditionSet().GetId() == fixtureScs.ID {
s.Equal(expectedMappedFixture.ID, sm.GetId())
Expand Down
27 changes: 5 additions & 22 deletions service/policy/db/subject_mappings.go
Original file line number Diff line number Diff line change
Expand Up @@ -527,12 +527,7 @@ func (c PolicyDBClient) DeleteSubjectMapping(ctx context.Context, id string) (*p
// and the JSON structure being SubjectSets -> ConditionGroups -> Conditions.
//
// Unfortunately we must do some slight filtering at the SQL level to avoid extreme and potentially non-rare edge cases. Subject Mappings will
// be returned if there is any condition found among the structures that matches:
// 1. The external field, external value, and an IN operator
// 2. The external field, _no_ external value, and a NOT_IN operator
//
// Without this filtering, if a selector value was something like '.emailAddress' or '.username', every Subject is probably going to relate to that mapping
// in some way or another. This could theoretically be every attribute in the DB if a policy admin has relied heavily on that field.
// be returned if an external selector field matches.
//
// NOTE: if you have any issues, set the log level to 'debug' for more comprehensive context.
func selectMatchedSubjectMappingsSQL(subjectProperties []*policy.SubjectProperty, logger *logger.Logger) (string, []interface{}, error) {
Expand All @@ -554,13 +549,8 @@ func selectMatchedSubjectMappingsSQL(subjectProperties []*policy.SubjectProperty
}

hasField := "each_condition->>'subject_external_selector_value' = '" + sp.GetExternalSelectorValue() + "'"
hasValue := "(each_condition->>'subject_external_values')::jsonb @> '[\"" + sp.GetExternalValue() + "\"]'::jsonb"
hasInOperator := "each_condition->>'operator' = 'SUBJECT_MAPPING_OPERATOR_ENUM_IN'"
hasNotInOperator := "each_condition->>'operator' = 'SUBJECT_MAPPING_OPERATOR_ENUM_NOT_IN'"
// Parses the json and matches the row if either of the following conditions are met:
where += "((" + hasField + " AND " + hasValue + " AND " + hasInOperator + ")" +
" OR " +
"(" + hasField + " AND NOT " + hasValue + " AND " + hasNotInOperator + "))"
// Parses the json and matches the row if the selector exists:
suchak1 marked this conversation as resolved.
Show resolved Hide resolved
where += "(" + hasField + ")"
logger.Debug("current condition filter WHERE clause", slog.String("subject_external_selector_value", sp.GetExternalSelectorValue()), slog.String("subject_external_value", sp.GetExternalValue()), slog.String("where", where))
}
where += ")"
Expand Down Expand Up @@ -589,15 +579,8 @@ func selectMatchedSubjectMappingsSQL(subjectProperties []*policy.SubjectProperty
ToSql()
}

// GetMatchedSubjectMappings liberally returns a list of SubjectMappings based on the provided SubjectProperties. The SubjectMappings are returned
// if there is any single condition found among the structures that matches:
// 1. The external field, external value, and an IN operator
// 2. The external field, _no_ external value, and a NOT_IN operator
//
// Without this filtering, if a field was something like '.emailAddress' or '.username', every Subject is probably going to relate to that mapping
// in some way or another, potentially matching every single attribute in the DB if a policy admin has relied heavily on that field. There is no
// logic applied beyond a single condition within the query to avoid business logic interpreting the supplied conditions beyond the bare minimum
// initial filter.
// GetMatchedSubjectMappings liberally returns a list of SubjectMappings based on the provided SubjectProperties.
// The SubjectMappings are returned if an external selector field matches.
//
// NOTE: This relationship is sometimes called Entitlements or Subject Entitlements.
// NOTE: if you have any issues, set the log level to 'debug' for more comprehensive context.
Expand Down
11 changes: 2 additions & 9 deletions service/policy/subjectmapping/subject_mapping.proto
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,8 @@ import "google/api/annotations.proto";
import "common/common.proto";
import "policy/objects.proto";

// MatchSubjectMappingsRequest liberally returns a list of SubjectMappings based on the provided SubjectProperties. The SubjectMappings are returned
// if there is any single condition found among the structures that matches for one of the provided properties:
// 1. The external selector value, external value, and an IN operator
// 2. The external selector value, _no_ external value, and a NOT_IN operator
//
// Without this filtering, if a selector value was something like '.emailAddress' or '.username', every Subject is probably going to relate to that mapping
// in some way or another, potentially matching every single attribute in the DB if a policy admin has relied heavily on that field. There is no
// logic applied beyond a single condition within the query to avoid business logic interpreting the supplied conditions beyond the bare minimum
// initial filter.
// MatchSubjectMappingsRequest liberally returns a list of SubjectMappings based on the provided SubjectProperties.
// The SubjectMappings are returned if an external selector field matches.
//
// NOTE: if you have any issues, debug logs are available within the service to help identify why a mapping was or was not returned.
message MatchSubjectMappingsRequest {
Expand Down
Loading