diff --git a/interfaces/prompting/patterns/render.go b/interfaces/prompting/patterns/render.go index 90fba610597..89db0a952d5 100644 --- a/interfaces/prompting/patterns/render.go +++ b/interfaces/prompting/patterns/render.go @@ -80,9 +80,9 @@ func renderAllVariants(n renderNode, observe func(index int, variant PatternVari if err != nil { // This should never occur logger.Noticef("patterns: cannot parse pattern variant '%s': %v", variant, err) - continue + } else { + observe(i, variant) } - observe(i, variant) length, lengthUnchanged, moreRemain = v.NextVariant() } } diff --git a/interfaces/prompting/patterns/render_internal_test.go b/interfaces/prompting/patterns/render_internal_test.go index 2252515f335..0a77f67a26e 100644 --- a/interfaces/prompting/patterns/render_internal_test.go +++ b/interfaces/prompting/patterns/render_internal_test.go @@ -95,6 +95,57 @@ func (s *renderSuite) TestRenderAllVariants(c *C) { c.Check(variants, HasLen, parsed.NumVariants()) } +func (s *renderSuite) TestRenderAllVariantsConflicts(c *C) { + // all variants are the exact same path + for _, testCase := range []struct { + pattern string + expectedVariants []string + }{ + { + "/{fo{obar},foo{b{ar,ar},bar,{ba}r}}", + []string{"/foobar"}, + }, + { + "/{{foo/{bar,baz},123},{123,foo{/bar,/baz}}}", + []string{ + "/foo/bar", + "/foo/baz", + "/123", + "/123", + "/foo/bar", + "/foo/baz", + }, + }, + } { + scanned, err := scan(testCase.pattern) + c.Assert(err, IsNil) + parsed, err := parse(scanned) + c.Assert(err, IsNil) + variants := make([]string, 0, len(testCase.expectedVariants)) + renderAllVariants(parsed, func(index int, variant PatternVariant) { + variants = append(variants, variant.String()) + }) + c.Check(variants, DeepEquals, testCase.expectedVariants) + c.Check(variants, HasLen, parsed.NumVariants()) + } +} + +func (s *renderSuite) TestRenderAllVariantsError(c *C) { + badNodes := seq{ + literal("/"), + alt{ + literal("good"), + literal("bad[path]"), + literal("great"), + }, + } + variants := make([]string, 0, 1) + renderAllVariants(badNodes, func(index int, variant PatternVariant) { + variants = append(variants, variant.String()) + }) + c.Check(variants, DeepEquals, []string{"/good", "/great"}) +} + func (s *renderSuite) TestNextVariant(c *C) { pattern := "/{,usr/}lib{,32,64,x32}/{,@{multiarch}/{,atomics/}}ld{-*,64}.so*" scanned, err := scan(pattern) diff --git a/interfaces/prompting/requestrules/requestrules.go b/interfaces/prompting/requestrules/requestrules.go index f6d43a5a3b1..f314f394ee3 100644 --- a/interfaces/prompting/requestrules/requestrules.go +++ b/interfaces/prompting/requestrules/requestrules.go @@ -465,6 +465,12 @@ func (rdb *RuleDB) addRulePermissionToTree(rule *Rule, permission string) []prom expiredRules[conflictingVariantEntry.RuleID] = true newVariantEntries[variantStr] = newEntry default: + // XXX: If we ever switch to adding variants directly to the real + // variant entries map instead of a new map, check that the + // "conflicting" entry does not belong to the same rule that we're + // in the process of adding, which is possible if the rule's path + // pattern can render to duplicate variants. Ignore self-conflicts. + // Exists and is not expired, so there's a conflict conflicts = append(conflicts, prompting_errors.RuleConflict{ Permission: permission, @@ -540,9 +546,15 @@ func (rdb *RuleDB) removeRulePermissionFromTree(rule *Rule, permission string) [ err := fmt.Errorf("internal error: no rules in the rule tree for user %d, snap %q, interface %q, permission %q", rule.User, rule.Snap, rule.Interface, permission) return []error{err} } + seenVariants := make(map[string]bool, rule.Constraints.PathPattern.NumVariants()) var errs []error removeVariant := func(index int, variant patterns.PatternVariant) { - variantEntry, exists := permVariants.VariantEntries[variant.String()] + variantStr := variant.String() + if seenVariants[variantStr] { + return + } + seenVariants[variantStr] = true + variantEntry, exists := permVariants.VariantEntries[variantStr] if !exists { // Database was left inconsistent, should not occur errs = append(errs, fmt.Errorf(`internal error: path pattern variant not found in the rule tree: %q`, variant)) @@ -550,7 +562,7 @@ func (rdb *RuleDB) removeRulePermissionFromTree(rule *Rule, permission string) [ // Database was left inconsistent, should not occur errs = append(errs, fmt.Errorf(`internal error: path pattern variant maps to different rule ID: %q: %s`, variant, variantEntry.RuleID.String())) } else { - delete(permVariants.VariantEntries, variant.String()) + delete(permVariants.VariantEntries, variantStr) } } rule.Constraints.PathPattern.RenderAllVariants(removeVariant) diff --git a/interfaces/prompting/requestrules/requestrules_test.go b/interfaces/prompting/requestrules/requestrules_test.go index 77ff288ed30..e96bdea2c5e 100644 --- a/interfaces/prompting/requestrules/requestrules_test.go +++ b/interfaces/prompting/requestrules/requestrules_test.go @@ -617,6 +617,53 @@ func addRuleFromTemplate(c *C, rdb *requestrules.RuleDB, template *addRuleConten return rdb.AddRule(partial.User, partial.Snap, partial.Interface, constraints, partial.Outcome, partial.Lifespan, partial.Duration) } +func (s *requestrulesSuite) TestAddRuleRemoveRuleDuplicateVariants(c *C) { + rdb, err := requestrules.New(s.defaultNotifyRule) + c.Assert(err, IsNil) + + ruleContents := &addRuleContents{ + User: s.defaultUser, + Snap: "nextcloud", + Interface: "home", + PathPattern: "/home/test/{{foo/{bar,baz},123},{123,foo{/bar,/baz}}}", + Permissions: []string{"read"}, + Outcome: prompting.OutcomeAllow, + Lifespan: prompting.LifespanForever, + Duration: "", + } + + // Test that rule with a pattern which renders into duplicate variants does + // not conflict with itself while adding + var addedRules []*requestrules.Rule + rule, err := addRuleFromTemplate(c, rdb, ruleContents, ruleContents) + c.Check(err, IsNil) + c.Check(rule, NotNil) + addedRules = append(addedRules, rule) + s.checkWrittenRuleDB(c, addedRules) + s.checkNewNoticesSimple(c, nil, rule) + + // Test that the rule exists + found, err := rdb.RuleWithID(rule.User, rule.ID) + c.Assert(err, IsNil) + c.Check(found, DeepEquals, rule) + // Test that the rule's path pattern really renders to duplicate variants + variantList := make([]string, 0, found.Constraints.PathPattern.NumVariants()) + variantSet := make(map[string]int, found.Constraints.PathPattern.NumVariants()) + found.Constraints.PathPattern.RenderAllVariants(func(index int, variant patterns.PatternVariant) { + variantStr := variant.String() + variantList = append(variantList, variantStr) + variantSet[variantStr] += 1 + }) + c.Check(variantSet, Not(HasLen), len(variantList), Commentf("variant list: %q\nvariant set: %q", variantList, variantSet)) + + // Test that rule with a pattern which renders into duplicate variants does + // not cause an error while removing by trying to remove the same variant + // twice and finding it already removed the second time + removed, err := rdb.RemoveRule(rule.User, rule.ID) + c.Assert(err, IsNil) + c.Check(removed, DeepEquals, rule) +} + func (s *requestrulesSuite) TestAddRuleErrors(c *C) { rdb, err := requestrules.New(s.defaultNotifyRule) c.Assert(err, IsNil)