i/prompting: render path patterns variants using recursive descent pa…

…rser (#14059) * i/prompting: implement path pattern expansion Path patterns may include arbitrary nested groups, delimited by '{' and '}', up to a limit on the total number of groups. In order to compare the precedence of path patterns which match a given path, these path patterns must be expanded until no groups remain, and thus the particular group-free patterns which was resolved from the original patterns when matching the path can be compared. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting: add PathPattern type for pattern validation and expansion Rather than separately validate and expand path patterns, storing the result as a list of expanded patterns, parse a pattern into a PathPattern type, which can dynamically render expanded path patterns as needed with minimal overhead. When path patterns are received from prompting clients, path patterns can be unmarshalled and automatically validated, and any future use of the pattern in-memory can use the pre-parsed PathPattern to iterate through expanded path patterns without needing to explicitly expand and store all path patterns. Additionally, the new PathPattern type should be used in Constraints in place of the old path pattern string. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting: refactor path pattern parsing Rather than keep separate stacks for the sequences and paths which the parser is currently inside, instead keep a single stack, to which the existing sequence and a new group is added whenever a '{' rune is encountered. Then there is no need to no need for a variable to hold the current group, peeking the stack yields the most recent group, to which the current sequence can be added whenever a ',' or '}' is encountered. When a '}' is encountered, the most recent group is popped off the stack, the current sequence is added to it (completing the group), and then the previous sequence is popped off the stack and the completed group is added to it. From there, that previous sequence is now considered the current sequence until another '{' is encountered. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting: use stack instead of non-temp current sequence variable Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting: improve error message prefixes Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting: moved patterns to dedicated subpackage of prompting Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: add scanner, parser, and renderer for path patterns Co-authored-by: Oliver Calder <oliver.calder@canonical.com> Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: add minimal tests for scan and render Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: replace parser in path pattern struct Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: add recursion depth check for nested groups Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: adjusted error messages Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: preserve escape characters in expanded patterns Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: scanner detects invalid chars and returns error Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting: fix formatting Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: add helper for converting read runes into text token Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: consolidate render node types into render.go Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: only re-render differences from previous configuration Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: remove GoString functions from render config types Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting{,/patterns}: added dedicated Match method to PathPattern Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: unexport all internal types and interfaces Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: rename renderConfig to variantState Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: unexported internal renderAllVariants Also improved naming and documentation. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: renamed local variables to match new variantState naming Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: variantState has Render method and renderNode Add a reference to the `renderNode` used to generate a given `variantState` to that state itself. This allows methods on `variantState` to be called without needing to pass as a parameter the same `renderNode` which was used to generate the `variantState`. Also, move the `Render` function to be a method on `variantState` instead of `renderNode`. This makes sense semantically, since we render particular variants, rather than nodes themselves, and makes sense ergonomically since we now have a reference to the `renderNode` within each `variantState`, so there is no need to pass parameters around for nodes and variants which are required to be associated anyway. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: consolidate optimize and fix nodeEqual Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: add tests for tokenType.String Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: use dedicated flag to tell when all seq variants are exhausted Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: use ..._internal_test.go for non-exported test files Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: fix growing of render buffer, unexport peek Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: merge literalVariant into literal Add comment as such to the `literal` type definition, and have `literal.NextVariant` return length 0 to make it consistent with other `variantState` types. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: preallocate render buffer for initial variant Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: improve error handling Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: moved simple bad pattern checks to scanner Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: return length along with initial variant Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/prompting/patterns: simplify check if more variants remain when rendering Signed-off-by: Oliver Calder <oliver.calder@canonical.com> --------- Signed-off-by: Oliver Calder <oliver.calder@canonical.com> Co-authored-by: Zygmunt Krynicki <me@zygoon.pl>
canonical · Jun 28, 2024 · 8fc33dd · 8fc33dd
1 parent 140e7a7
commit 8fc33dd
Show file tree

Hide file tree

Showing 13 changed files with 2,219 additions and 475 deletions.
diff --git a/interfaces/prompting/constraints.go b/interfaces/prompting/constraints.go
@@ -23,6 +23,7 @@ import (
 	"errors"
 	"fmt"
 
+	"github.com/snapcore/snapd/interfaces/prompting/patterns"
 	"github.com/snapcore/snapd/sandbox/apparmor/notify"
 	"github.com/snapcore/snapd/strutil"
 )
@@ -34,14 +35,20 @@ var (
 )
 
 type Constraints struct {
-	PathPattern string   `json:"path-pattern,omitempty"`
-	Permissions []string `json:"permissions,omitempty"`
+	PathPattern *patterns.PathPattern `json:"path-pattern,omitempty"`
+	Permissions []string              `json:"permissions,omitempty"`
 }
 
 // ValidateForInterface returns nil if the constraints are valid for the given
 // interface, otherwise returns an error.
 func (c *Constraints) ValidateForInterface(iface string) error {
-	return c.validatePermissions(iface)
+	if c.PathPattern == nil {
+		return fmt.Errorf("invalid constraints: no path pattern")
+	}
+	if err := c.validatePermissions(iface); err != nil {
+		return fmt.Errorf("invalid constraints: %w", err)
+	}
+	return nil
 }
 
 // validatePermissions checks that the permissions for the given constraints
@@ -77,7 +84,14 @@ func (c *Constraints) validatePermissions(iface string) error {
 //
 // If the constraints or path are invalid, returns an error.
 func (c *Constraints) Match(path string) (bool, error) {
-	return PathPatternMatch(c.PathPattern, path)
+	if c.PathPattern == nil {
+		return false, fmt.Errorf("invalid constraints: no path pattern")
+	}
+	match, err := c.PathPattern.Match(path)
+	if err != nil {
+		return false, fmt.Errorf("invalid constraints: %w", err)
+	}
+	return match, nil
 }
 
 // RemovePermission removes every instance of the given permission from the

diff --git a/interfaces/prompting/constraints_test.go b/interfaces/prompting/constraints_test.go
@@ -24,9 +24,8 @@ import (
 
 	. "gopkg.in/check.v1"
 
-	doublestar "github.com/bmatcuk/doublestar/v4"
-
 	"github.com/snapcore/snapd/interfaces/prompting"
+	"github.com/snapcore/snapd/interfaces/prompting/patterns"
 	"github.com/snapcore/snapd/sandbox/apparmor/notify"
 )
 
@@ -35,33 +34,54 @@ type constraintsSuite struct{}
 var _ = Suite(&constraintsSuite{})
 
 func (s *constraintsSuite) TestConstraintsValidateForInterface(c *C) {
+	validPathPattern, err := patterns.ParsePathPattern("/path/to/foo")
+	c.Assert(err, IsNil)
+
+	// Happy
+	constraints := &prompting.Constraints{
+		PathPattern: validPathPattern,
+		Permissions: []string{"read"},
+	}
+	err = constraints.ValidateForInterface("home")
+	c.Check(err, IsNil)
+
+	// Bad interface or permissions
 	cases := []struct {
-		iface   string
-		pattern string
-		perms   []string
-		errStr  string
+		iface  string
+		perms  []string
+		errStr string
 	}{
 		{
 			"foo",
-			"/invalid/path",
 			[]string{"read"},
-			"unsupported interface.*",
+			"invalid constraints: unsupported interface.*",
 		},
 		{
 			"home",
-			"/valid/path",
 			[]string{},
-			fmt.Sprintf("%v", prompting.ErrPermissionsListEmpty),
+			fmt.Sprintf("invalid constraints: %v", prompting.ErrPermissionsListEmpty),
+		},
+		{
+			"home",
+			[]string{"access"},
+			fmt.Sprintf("invalid constraints: unsupported permission for home interface.*"),
 		},
 	}
 	for _, testCase := range cases {
 		constraints := &prompting.Constraints{
-			PathPattern: testCase.pattern,
+			PathPattern: validPathPattern,
 			Permissions: testCase.perms,
 		}
-		err := constraints.ValidateForInterface(testCase.iface)
+		err = constraints.ValidateForInterface(testCase.iface)
 		c.Check(err, ErrorMatches, testCase.errStr)
 	}
+
+	// Check missing path pattern
+	constraints = &prompting.Constraints{
+		Permissions: []string{"read"},
+	}
+	err = constraints.ValidateForInterface("home")
+	c.Check(err, ErrorMatches, "invalid constraints: no path pattern")
 }
 
 func (s *constraintsSuite) TestValidatePermissionsHappy(c *C) {
@@ -150,8 +170,10 @@ func (*constraintsSuite) TestConstraintsMatch(c *C) {
 		},
 	}
 	for _, testCase := range cases {
+		pattern, err := patterns.ParsePathPattern(testCase.pattern)
+		c.Check(err, IsNil)
 		constraints := &prompting.Constraints{
-			PathPattern: testCase.pattern,
+			PathPattern: pattern,
 			Permissions: []string{"read"},
 		}
 		result, err := constraints.Match(testCase.path)
@@ -161,13 +183,12 @@ func (*constraintsSuite) TestConstraintsMatch(c *C) {
 }
 
 func (s *constraintsSuite) TestConstraintsMatchUnhappy(c *C) {
-	badPath := `bad\pattern\`
+	badPath := `bad\path\`
 	badConstraints := &prompting.Constraints{
-		PathPattern: badPath,
 		Permissions: []string{"read"},
 	}
 	matches, err := badConstraints.Match(badPath)
-	c.Check(err, Equals, doublestar.ErrBadPattern)
+	c.Check(err, ErrorMatches, "invalid constraints: no path pattern")
 	c.Check(matches, Equals, false)
 }
 
@@ -228,11 +249,13 @@ func (s *constraintsSuite) TestConstraintsRemovePermission(c *C) {
 		},
 	}
 	for _, testCase := range cases {
+		pathPattern, err := patterns.ParsePathPattern("/path/to/foo")
+		c.Check(err, IsNil)
 		constraints := &prompting.Constraints{
-			PathPattern: "/path/to/foo",
+			PathPattern: pathPattern,
 			Permissions: testCase.initial,
 		}
-		err := constraints.RemovePermission(testCase.remove)
+		err = constraints.RemovePermission(testCase.remove)
 		c.Check(err, Equals, testCase.err)
 		c.Check(constraints.Permissions, DeepEquals, testCase.final)
 	}
@@ -286,8 +309,10 @@ func (s *constraintsSuite) TestConstraintsContainPermissions(c *C) {
 		},
 	}
 	for _, testCase := range cases {
+		pathPattern, err := patterns.ParsePathPattern("/arbitrary")
+		c.Check(err, IsNil)
 		constraints := &prompting.Constraints{
-			PathPattern: "arbitrary",
+			PathPattern: pathPattern,
 			Permissions: testCase.constPerms,
 		}
 		contained := constraints.ContainPermissions(testCase.queryPerms)

diff --git a/interfaces/prompting/patterns.go b/interfaces/prompting/patterns.go
diff --git a/interfaces/prompting/patterns/parse.go b/interfaces/prompting/patterns/parse.go
@@ -0,0 +1,132 @@
+// -*- Mode: Go; indent-tabs-mode: t -*-
+
+/*
+ * Copyright (C) 2024 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+package patterns
+
+import (
+	"errors"
+	"fmt"
+)
+
+func parse(tokens []token) (renderNode, error) {
+	tr := tokenReader{
+		tokens: tokens,
+	}
+	return parseSeq(&tr)
+}
+
+func parseSeq(tr *tokenReader) (renderNode, error) {
+	var s seq
+seqLoop:
+	for {
+		t := tr.peek()
+
+		switch t.tType {
+		case tokEOF:
+			break seqLoop
+		case tokBraceOpen:
+			inner, err := parseAlt(tr)
+			if err != nil {
+				return nil, err
+			}
+
+			s = append(s, inner)
+		case tokBraceClose:
+			if tr.depth > 0 {
+				break seqLoop
+			}
+			tr.token()
+			return nil, errors.New("unmatched '}' character")
+		case tokText:
+			tr.token()
+			s = append(s, literal(t.text))
+		case tokComma:
+			if tr.depth > 0 {
+				break seqLoop
+			}
+			tr.token() // discard, we get called in a loop
+			s = append(s, literal(","))
+		}
+	}
+
+	return s.optimize()
+}
+
+func parseAlt(tr *tokenReader) (renderNode, error) {
+	var a alt
+
+	if t := tr.token(); t.tType != tokBraceOpen {
+		// Should not occur, caller should call parseAlt on peeking '{'
+		return nil, fmt.Errorf("internal error: expected '{' at start of alt, but got %v", t)
+	}
+
+	tr.depth++
+	defer func() {
+		tr.depth--
+	}()
+	if tr.depth >= maxExpandedPatterns {
+		return nil, fmt.Errorf("nested group depth exceeded maximum number of expanded path patterns (%d)", maxExpandedPatterns)
+	}
+
+altLoop:
+	for {
+		item, err := parseSeq(tr)
+		if err != nil {
+			return nil, err
+		}
+
+		a = append(a, item)
+
+		switch t := tr.token(); t.tType {
+		case tokBraceClose:
+			break altLoop
+		case tokComma:
+			continue
+		case tokEOF:
+			return nil, errors.New("unmatched '{' character")
+		default:
+			return nil, fmt.Errorf("unexpected token %v when parsing alt", t)
+		}
+	}
+
+	return a.optimize()
+}
+
+type tokenReader struct {
+	tokens []token
+	depth  int
+}
+
+func (tr tokenReader) peek() token {
+	if len(tr.tokens) == 0 {
+		return token{tType: tokEOF}
+	}
+
+	return tr.tokens[0]
+}
+
+func (tr *tokenReader) token() token {
+	t := tr.peek()
+
+	if t.tType != tokEOF {
+		tr.tokens = tr.tokens[1:]
+	}
+
+	return t
+}