Skip to content

Commit

Permalink
aspects: consider access when matching rules (canonical#13447)
Browse files Browse the repository at this point in the history 
Instead of matching a request to rules and failing if those rules
support the requested operation, ignore rules that don't and get/set
those that do. If no rule matches the request string and the required
access type, return the usual "no matching (read|write) rule".

Signed-off-by: Miguel Pires <[email protected]>
  • Loading branch information
@miguelpires
miguelpires authored Jan 11, 2024
1 parent 0238dca commit 5c92351
Show file tree
Hide file tree
Showing 7 changed files with 53 additions and 128 deletions.
40 changes: 12 additions & 28 deletions aspects/aspects.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,24 +63,26 @@ type NotFoundError struct {
Account string
BundleName string
Aspect string
Operation string
Request string
Cause string
}

func (e *NotFoundError) Error() string {
return fmt.Sprintf("cannot find value for %q in aspect %s/%s/%s: %s", e.Request, e.Account, e.BundleName, e.Aspect, e.Cause)
return fmt.Sprintf("cannot %s %q in aspect %s/%s/%s: %s", e.Operation, e.Request, e.Account, e.BundleName, e.Aspect, e.Cause)
}

func (e *NotFoundError) Is(err error) bool {
_, ok := err.(*NotFoundError)
return ok
}

func notFoundErrorFrom(a *Aspect, request, errMsg string) *NotFoundError {
func notFoundErrorFrom(a *Aspect, op, request, errMsg string) *NotFoundError {
return &NotFoundError{
Account: a.bundle.Account,
BundleName: a.bundle.Name,
Aspect: a.Name,
Operation: op,
Request: request,
Cause: errMsg,
}
Expand Down Expand Up @@ -115,24 +117,6 @@ func badRequestErrorFrom(a *Aspect, operation, request, errMsg string, v ...inte
}
}

// InvalidAccessError represents a failure to perform a read or write due to the
// aspect's access control.
type InvalidAccessError struct {
RequestedAccess accessType
FieldAccess accessType
Field string
}

func (e *InvalidAccessError) Error() string {
return fmt.Sprintf("cannot %s field %q: only supports %s access",
accessTypeStrings[e.RequestedAccess], e.Field, accessTypeStrings[e.FieldAccess])
}

func (e *InvalidAccessError) Is(err error) bool {
_, ok := err.(*InvalidAccessError)
return ok
}

// DataBag controls access to the aspect data storage.
type DataBag interface {
Get(path string) (interface{}, error)
Expand Down Expand Up @@ -348,7 +332,7 @@ func (a *Aspect) Set(databag DataBag, request string, value interface{}) error {
}

if !accessPatt.isWriteable() {
return &InvalidAccessError{RequestedAccess: write, FieldAccess: accessPatt.access, Field: request}
continue
}

path, err := accessPatt.storagePath(placeholders)
Expand All @@ -360,7 +344,7 @@ func (a *Aspect) Set(databag DataBag, request string, value interface{}) error {
}

if len(matches) == 0 {
return notFoundErrorFrom(a, request, "no matching write rule")
return notFoundErrorFrom(a, "set", request, "no matching write rule")
}

// sort less nested paths before more nested ones so that writes aren't overwritten
Expand Down Expand Up @@ -474,7 +458,7 @@ func (a *Aspect) Get(databag DataBag, request string) (map[string]interface{}, e
}

if merged == nil {
return nil, notFoundErrorFrom(a, request, "matching rules don't map to any values")
return nil, notFoundErrorFrom(a, "get", request, "matching rules don't map to any values")
}

// the top level maps the request to the remaining namespace
Expand Down Expand Up @@ -540,13 +524,17 @@ func (a *Aspect) matchGetRequest(request string) (matches []requestMatch, err er
}

if !accessPatt.isReadable() {
return nil, &InvalidAccessError{RequestedAccess: read, FieldAccess: accessPatt.access, Field: request}
continue
}

m := requestMatch{storagePath: path, suffixParts: restSuffix}
matches = append(matches, m)
}

if len(matches) == 0 {
return nil, notFoundErrorFrom(a, "get", request, "no matching read rule")
}

// sort matches by namespace (unmatched suffix) to ensure that nested matches
// are read after
sort.Slice(matches, func(x, y int) bool {
Expand All @@ -563,10 +551,6 @@ func (a *Aspect) matchGetRequest(request string) (matches []requestMatch, err er
return len(xNamespace) < len(yNamespace)
})

if len(matches) == 0 {
return nil, notFoundErrorFrom(a, request, "no matching read rule")
}

return matches, nil
}

Expand Down
66 changes: 26 additions & 40 deletions aspects/aspects_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,22 +194,22 @@ func (s *aspectSuite) TestAspectNotFound(c *C) {

_, err = aspect.Get(databag, "missing")
c.Assert(err, testutil.ErrorIs, &aspects.NotFoundError{})
c.Assert(err, ErrorMatches, `cannot find value for "missing" in aspect acc/foo/bar: no matching read rule`)
c.Assert(err, ErrorMatches, `cannot get "missing" in aspect acc/foo/bar: no matching read rule`)

err = aspect.Set(databag, "missing", "thing")
c.Assert(err, testutil.ErrorIs, &aspects.NotFoundError{})
c.Assert(err, ErrorMatches, `cannot find value for "missing" in aspect acc/foo/bar: no matching write rule`)
c.Assert(err, ErrorMatches, `cannot set "missing" in aspect acc/foo/bar: no matching write rule`)

_, err = aspect.Get(databag, "top-level")
c.Assert(err, testutil.ErrorIs, &aspects.NotFoundError{})
c.Assert(err, ErrorMatches, `cannot find value for "top-level" in aspect acc/foo/bar: matching rules don't map to any values`)
c.Assert(err, ErrorMatches, `cannot get "top-level" in aspect acc/foo/bar: matching rules don't map to any values`)

err = aspect.Set(databag, "nested", "thing")
c.Assert(err, IsNil)

_, err = aspect.Get(databag, "other-nested")
c.Assert(err, testutil.ErrorIs, &aspects.NotFoundError{})
c.Assert(err, ErrorMatches, `cannot find value for "other-nested" in aspect acc/foo/bar: matching rules don't map to any values`)
c.Assert(err, ErrorMatches, `cannot get "other-nested" in aspect acc/foo/bar: matching rules don't map to any values`)
}

func (s *aspectSuite) TestAspectBadRead(c *C) {
Expand All @@ -231,52 +231,48 @@ func (s *aspectSuite) TestAspectBadRead(c *C) {
}

func (s *aspectSuite) TestAspectsAccessControl(c *C) {
aspectBundle, err := aspects.NewAspectBundle("acc", "bundle", map[string]interface{}{
"foo": []map[string]string{
{"request": "default", "storage": "path.default"},
{"request": "read-write", "storage": "path.read-write", "access": "read-write"},
{"request": "read-only", "storage": "path.read-only", "access": "read"},
{"request": "write-only", "storage": "path.write-only", "access": "write"},
},
}, aspects.NewJSONSchema())
c.Assert(err, IsNil)

aspect := aspectBundle.Aspect("foo")

for _, t := range []struct {
request string
getErr string
setErr string
access string
getErr string
setErr string
}{
{
request: "read-write",
access: "read-write",
},
{
// defaults to "read-write"
request: "default",
access: "",
},
{
request: "read-only",
// unrelated error
getErr: `cannot find value for "read-only" in aspect acc/bundle/foo: matching rules don't map to any values`,
setErr: `cannot write field "read-only": only supports read access`,
access: "read",
// non-access control error, access ok
getErr: `cannot get "foo" in aspect acc/bundle/foo: matching rules don't map to any values`,
setErr: `cannot set "foo" in aspect acc/bundle/foo: no matching write rule`,
},
{
request: "write-only",
getErr: `cannot read field "write-only": only supports write access`,
access: "write",
getErr: `cannot get "foo" in aspect acc/bundle/foo: no matching read rule`,
},
} {
cmt := Commentf("sub-test %q failed", t.request)
cmt := Commentf("sub-test with %q access failed", t.access)
databag := aspects.NewJSONDataBag()
aspectBundle, err := aspects.NewAspectBundle("acc", "bundle", map[string]interface{}{
"foo": []map[string]string{
{"request": "foo", "storage": "foo", "access": t.access},
},
}, aspects.NewJSONSchema())
c.Assert(err, IsNil)

aspect := aspectBundle.Aspect("foo")

err := aspect.Set(databag, t.request, "thing")
err = aspect.Set(databag, "foo", "thing")
if t.setErr != "" {
c.Assert(err.Error(), Equals, t.setErr, cmt)
} else {
c.Assert(err, IsNil, cmt)
}

_, err = aspect.Get(databag, t.request)
_, err = aspect.Get(databag, "foo")
if t.getErr != "" {
c.Assert(err.Error(), Equals, t.getErr, cmt)
} else {
Expand Down Expand Up @@ -579,16 +575,6 @@ func (s *aspectSuite) TestAspectUnsetAlreadyUnsetEntry(c *C) {
c.Assert(err, IsNil)
}

func (s *aspectSuite) TestInvalidAccessError(c *C) {
err := &aspects.InvalidAccessError{RequestedAccess: 1, FieldAccess: 2, Field: "foo"}
c.Assert(err, testutil.ErrorIs, &aspects.InvalidAccessError{})
c.Assert(err, ErrorMatches, `cannot read field "foo": only supports write access`)

err = &aspects.InvalidAccessError{RequestedAccess: 2, FieldAccess: 1, Field: "foo"}
c.Assert(err, testutil.ErrorIs, &aspects.InvalidAccessError{})
c.Assert(err, ErrorMatches, `cannot write field "foo": only supports read access`)
}

func (s *aspectSuite) TestJSONDataBagCopy(c *C) {
bag := aspects.NewJSONDataBag()
err := bag.Set("foo", "bar")
Expand Down
6 changes: 0 additions & 6 deletions daemon/api_aspects.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,12 +128,6 @@ func toAPIError(err error) *apiError {
case errors.Is(err, &aspects.BadRequestError{}):
return BadRequest(err.Error())

case errors.Is(err, &aspects.InvalidAccessError{}):
return &apiError{
Status: 403,
Message: err.Error(),
}

default:
return InternalError(err.Error())
}
Expand Down
39 changes: 2 additions & 37 deletions daemon/api_aspects_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ func (s *aspectsSuite) TestGetAspectNoFieldsFound(c *C) {

func (s *aspectsSuite) TestAspectGetDatabagNotFound(c *C) {
restore := daemon.MockAspectstateGet(func(_ aspects.DataBag, _, _, _, _ string) (interface{}, error) {
return nil, &aspects.NotFoundError{Account: "foo", BundleName: "network", Aspect: "wifi-setup", Request: "ssid", Cause: "mocked"}
return nil, &aspects.NotFoundError{Account: "foo", BundleName: "network", Aspect: "wifi-setup", Operation: "get", Request: "ssid", Cause: "mocked"}
})
defer restore()

Expand All @@ -188,7 +188,7 @@ func (s *aspectsSuite) TestAspectGetDatabagNotFound(c *C) {

rspe := s.errorReq(c, req, nil)
c.Check(rspe.Status, Equals, 404)
c.Check(rspe.Message, Equals, `cannot find value for "ssid" in aspect foo/network/wifi-setup: mocked`)
c.Check(rspe.Message, Equals, `cannot get "ssid" in aspect foo/network/wifi-setup: mocked`)
}

func (s *aspectsSuite) TestAspectSetManyWithExistingState(c *C) {
Expand Down Expand Up @@ -289,7 +289,6 @@ func (s *aspectsSuite) TestGetAspectError(c *C) {
for _, t := range []test{
{name: "aspect not found", err: &aspects.NotFoundError{}, code: 404},
{name: "internal", err: errors.New("internal"), code: 500},
{name: "invalid access", err: &aspects.InvalidAccessError{RequestedAccess: 1, FieldAccess: 2, Field: "foo"}, code: 403},
} {
restore := daemon.MockAspectstateGet(func(_ aspects.DataBag, _, _, _, _ string) (interface{}, error) {
return nil, t.err
Expand Down Expand Up @@ -433,7 +432,6 @@ func (s *aspectsSuite) TestSetAspectError(c *C) {
for _, t := range []test{
{name: "not found", err: &aspects.NotFoundError{}, code: 404},
{name: "internal", err: errors.New("internal"), code: 500},
{name: "invalid access", err: &aspects.InvalidAccessError{}, code: 403},
} {
restore := daemon.MockAspectstateSet(func(aspects.DataBag, string, string, string, string, interface{}) error {
return t.err
Expand Down Expand Up @@ -477,39 +475,6 @@ func (s *aspectsSuite) TestSetAspectBadRequest(c *C) {
c.Check(rspe.Message, Equals, "cannot decode aspect request body: unexpected EOF")
}

func (s *aspectsSuite) TestSetAspectNotAllowed(c *C) {
restore := daemon.MockAspectstateSet(func(_ aspects.DataBag, acc, bundleName, aspect, field string, val interface{}) error {
return &aspects.InvalidAccessError{RequestedAccess: 2, FieldAccess: 1, Field: "foo"}
})
defer restore()

buf := bytes.NewBufferString(`{"foo": "bar"}`)
req, err := http.NewRequest("PUT", "/v2/aspects/system/network/wifi-setup", buf)
c.Assert(err, IsNil)
req.Header.Set("Content-Type", "application/json")

rspe := s.errorReq(c, req, nil)
c.Check(rspe.Status, Equals, 403)
c.Check(rspe.Message, Equals, `cannot write field "foo": only supports read access`)
c.Check(rspe.Kind, Equals, client.ErrorKind(""))
}

func (s *aspectsSuite) TestGetAspectNotAllowed(c *C) {
restore := daemon.MockAspectstateGet(func(_ aspects.DataBag, acc, bundleName, aspect, field string) (interface{}, error) {
return nil, &aspects.InvalidAccessError{RequestedAccess: 1, FieldAccess: 2, Field: "foo"}
})
defer restore()

req, err := http.NewRequest("GET", "/v2/aspects/system/network/wifi-setup?fields=foo", &bytes.Buffer{})
req.Header.Set("Content-Type", "application/json")
c.Assert(err, IsNil)

rspe := s.errorReq(c, req, nil)
c.Check(rspe.Status, Equals, 403)
c.Check(rspe.Message, Equals, `cannot read field "foo": only supports write access`)
c.Check(rspe.Kind, Equals, client.ErrorKind(""))
}

func (s *aspectsSuite) TestGetBadRequest(c *C) {
restore := daemon.MockAspectstateGet(func(_ aspects.DataBag, acc, bundleName, aspect, field string) (interface{}, error) {
return nil, &aspects.BadRequestError{
Expand Down
2 changes: 2 additions & 0 deletions overlord/aspectstate/aspectstate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ func SetAspect(databag aspects.DataBag, account, bundleName, aspect, field strin
Account: account,
BundleName: bundleName,
Aspect: aspect,
Operation: "set",
Request: field,
Cause: "aspect not found",
}
Expand Down Expand Up @@ -73,6 +74,7 @@ func GetAspect(databag aspects.DataBag, account, bundleName, aspect, field strin
Account: account,
BundleName: bundleName,
Aspect: aspect,
Operation: "get",
Request: field,
Cause: "aspect not found",
}
Expand Down
16 changes: 5 additions & 11 deletions overlord/aspectstate/aspectstate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,17 @@ func (s *aspectTestSuite) TestGetNotFound(c *C) {

res, err := aspectstate.GetAspect(databag, "system", "network", "other-aspect", "ssid")
c.Assert(err, FitsTypeOf, &aspects.NotFoundError{})
c.Assert(err, ErrorMatches, `cannot find value for "ssid" in aspect system/network/other-aspect: aspect not found`)
c.Assert(err, ErrorMatches, `cannot get "ssid" in aspect system/network/other-aspect: aspect not found`)
c.Check(res, IsNil)

res, err = aspectstate.GetAspect(databag, "system", "network", "wifi-setup", "ssid")
c.Assert(err, FitsTypeOf, &aspects.NotFoundError{})
c.Assert(err, ErrorMatches, `cannot find value for "ssid" in aspect system/network/wifi-setup: matching rules don't map to any values`)
c.Assert(err, ErrorMatches, `cannot get "ssid" in aspect system/network/wifi-setup: matching rules don't map to any values`)
c.Check(res, IsNil)

res, err = aspectstate.GetAspect(databag, "system", "network", "wifi-setup", "other-field")
c.Assert(err, FitsTypeOf, &aspects.NotFoundError{})
c.Assert(err, ErrorMatches, `cannot find value for "other-field" in aspect system/network/wifi-setup: no matching read rule`)
c.Assert(err, ErrorMatches, `cannot get "other-field" in aspect system/network/wifi-setup: no matching read rule`)
c.Check(res, IsNil)
}

Expand All @@ -84,17 +84,11 @@ func (s *aspectTestSuite) TestSetNotFound(c *C) {
databag := aspects.NewJSONDataBag()
err := aspectstate.SetAspect(databag, "system", "network", "wifi-setup", "foo", "bar")
c.Assert(err, FitsTypeOf, &aspects.NotFoundError{})
c.Assert(err, ErrorMatches, `cannot find value for "foo" in aspect system/network/wifi-setup: no matching write rule`)
c.Assert(err, ErrorMatches, `cannot set "foo" in aspect system/network/wifi-setup: no matching write rule`)

err = aspectstate.SetAspect(databag, "system", "network", "other-aspect", "foo", "bar")
c.Assert(err, FitsTypeOf, &aspects.NotFoundError{})
c.Assert(err, ErrorMatches, `cannot find value for "foo" in aspect system/network/other-aspect: aspect not found`)
}

func (s *aspectTestSuite) TestSetAccessError(c *C) {
databag := aspects.NewJSONDataBag()
err := aspectstate.SetAspect(databag, "system", "network", "wifi-setup", "status", "foo")
c.Assert(err, ErrorMatches, `cannot write field "status": only supports read access`)
c.Assert(err, ErrorMatches, `cannot set "foo" in aspect system/network/other-aspect: aspect not found`)
}

func (s *aspectTestSuite) TestUnsetAspect(c *C) {
Expand Down
Loading

0 comments on commit 5c92351

Please sign in to comment.