Merge pull request #36 from superfly/better-resset-errors

resset: better error messages
superfly · Sep 10, 2024 · 6e96925 · 6e96925
2 parents 43a0ea3 + 92aad12
commit 6e96925
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 27 deletions.
diff --git a/flyio/caveats.go b/flyio/caveats.go
@@ -102,7 +102,7 @@ func (c *Apps) Prohibits(a macaroon.Access) error {
 	if !isFlyioAccess {
 		return fmt.Errorf("%w: access isnt AppIDGetter", macaroon.ErrInvalidAccess)
 	}
-	return c.Apps.Prohibits(f.GetAppID(), f.GetAction())
+	return c.Apps.Prohibits(f.GetAppID(), f.GetAction(), "app")
 }
 
 type Volumes struct {
@@ -118,7 +118,7 @@ func (c *Volumes) Prohibits(a macaroon.Access) error {
 	if !isFlyioAccess {
 		return fmt.Errorf("%w: access isnt VolumeGetter", macaroon.ErrInvalidAccess)
 	}
-	return c.Volumes.Prohibits(f.GetVolume(), f.GetAction())
+	return c.Volumes.Prohibits(f.GetVolume(), f.GetAction(), "volume")
 }
 
 type Machines struct {
@@ -134,7 +134,7 @@ func (c *Machines) Prohibits(a macaroon.Access) error {
 	if !isFlyioAccess {
 		return fmt.Errorf("%w: access isnt MachineGetter", macaroon.ErrInvalidAccess)
 	}
-	return c.Machines.Prohibits(f.GetMachine(), f.GetAction())
+	return c.Machines.Prohibits(f.GetMachine(), f.GetAction(), "machine")
 }
 
 type MachineFeatureSet struct {
@@ -150,7 +150,7 @@ func (c *MachineFeatureSet) Prohibits(a macaroon.Access) error {
 	if !isFlyioAccess {
 		return fmt.Errorf("%w: access isnt MachineFeatureGetter", macaroon.ErrInvalidAccess)
 	}
-	return c.Features.Prohibits(f.GetMachineFeature(), f.GetAction())
+	return c.Features.Prohibits(f.GetMachineFeature(), f.GetAction(), "machine feature")
 }
 
 // FeatureSet is a collection of organization-level "features" that are managed
@@ -171,7 +171,7 @@ func (c *FeatureSet) Prohibits(a macaroon.Access) error {
 	if !isFlyioAccess {
 		return fmt.Errorf("%w: access isnt FeatureGetter", macaroon.ErrInvalidAccess)
 	}
-	return c.Features.Prohibits(f.GetFeature(), f.GetAction())
+	return c.Features.Prohibits(f.GetFeature(), f.GetAction(), "org feature")
 }
 
 // Mutations is a set of GraphQL mutations allowed by this token.
@@ -238,7 +238,7 @@ func (c *Clusters) Prohibits(a macaroon.Access) error {
 		return fmt.Errorf("%w: access isnt ClusterGetter", macaroon.ErrInvalidAccess)
 	}
 
-	return c.Clusters.Prohibits(f.GetCluster(), f.GetAction())
+	return c.Clusters.Prohibits(f.GetCluster(), f.GetAction(), "cluster")
 }
 
 // Role is used by the AllowedRoles and IsMember caveats.
@@ -401,7 +401,7 @@ func (c *AppFeatureSet) Prohibits(a macaroon.Access) error {
 	if !isFlyioAccess {
 		return fmt.Errorf("%w: access isnt AppFeatureGetter", macaroon.ErrInvalidAccess)
 	}
-	return c.Features.Prohibits(f.GetAppFeature(), f.GetAction())
+	return c.Features.Prohibits(f.GetAppFeature(), f.GetAction(), "app feature")
 }
 
 // StorageObjects limits what storage objects can be accessed. Objects are
@@ -425,5 +425,5 @@ func (c *StorageObjects) Prohibits(a macaroon.Access) error {
 	if !isFlyioAccess {
 		return fmt.Errorf("%w: access isnt StorageObjectGetter", macaroon.ErrInvalidAccess)
 	}
-	return c.Prefixes.Prohibits(f.GetStorageObject(), f.GetAction())
+	return c.Prefixes.Prohibits(f.GetStorageObject(), f.GetAction(), "storage object")
 }
diff --git a/resset/example_test.go b/resset/example_test.go
@@ -39,7 +39,7 @@ func (c *Widgets) Prohibits(f macaroon.Access) error {
 		return macaroon.ErrInvalidAccess
 	}
 
-	return c.Widgets.Prohibits(wf.WidgetName, wf.Action)
+	return c.Widgets.Prohibits(wf.WidgetName, wf.Action, "widget")
 }
 
 // implements macaroon.Access; describes an attempt to access a widget

diff --git a/resset/resource_set.go b/resset/resource_set.go
@@ -45,38 +45,42 @@ func New[I ID](p Action, ids ...I) ResourceSet[I] {
 	return ret
 }
 
-func (rs ResourceSet[I]) Prohibits(id *I, action Action) error {
+func (rs ResourceSet[I]) Prohibits(id *I, action Action, resourceType string) error {
 	if err := rs.validate(); err != nil {
 		return err
 	}
 	if id == nil {
-		return fmt.Errorf("%w resource", ErrResourceUnspecified)
+		return fmt.Errorf("%w %s", ErrResourceUnspecified, resourceType)
 	}
 
 	var (
-		foundPerm = false
-		perm      = ActionAll
-		zeroID    I
+		foundPerm  = false
+		perm       = ActionAll
+		zeroID     I
+		allowedIDs []I
 	)
 
 	if zeroPerm, hasZero := rs[zeroID]; hasZero {
 		perm &= zeroPerm
 		foundPerm = true
+		allowedIDs = append(allowedIDs, zeroID)
 	}
 
 	for entryID, entryPerm := range rs {
+		allowedIDs = append(allowedIDs, entryID)
+
 		if match(entryID, *id) {
 			perm &= entryPerm
 			foundPerm = true
 		}
 	}
 
 	if !foundPerm {
-		return fmt.Errorf("%w %v", ErrUnauthorizedForResource, *id)
+		return fmt.Errorf("%w %s %v (only %v)", ErrUnauthorizedForResource, resourceType, *id, allowedIDs)
 	}
 
 	if !action.IsSubsetOf(perm) {
-		return fmt.Errorf("%w access %s (%s not allowed)", ErrUnauthorizedForAction, action, action.Remove(perm))
+		return fmt.Errorf("%w access %s on %s (%s not allowed)", ErrUnauthorizedForAction, action, resourceType, action.Remove(perm))
 	}
 
 	return nil

diff --git a/resset/resource_set_test.go b/resset/resource_set_test.go
@@ -18,24 +18,24 @@ func TestResourceSet(t *testing.T) {
 		"bar": ActionWrite,
 	}
 
-	assert.NoError(t, rs.Prohibits(ptr("foo"), ActionRead|ActionWrite))
-	assert.NoError(t, rs.Prohibits(ptr("bar"), ActionWrite))
-	assert.True(t, errors.Is(rs.Prohibits(nil, ActionWrite), ErrResourceUnspecified))
-	assert.True(t, errors.Is(rs.Prohibits(ptr("baz"), ActionWrite), ErrUnauthorizedForResource))
-	assert.True(t, errors.Is(rs.Prohibits(ptr(zero), ActionWrite), ErrUnauthorizedForResource))
-	assert.True(t, errors.Is(rs.Prohibits(ptr("foo"), ActionAll), ErrUnauthorizedForAction))
+	assert.NoError(t, rs.Prohibits(ptr("foo"), ActionRead|ActionWrite, "test resource"))
+	assert.NoError(t, rs.Prohibits(ptr("bar"), ActionWrite, "test resource"))
+	assert.True(t, errors.Is(rs.Prohibits(nil, ActionWrite, "test resource"), ErrResourceUnspecified))
+	assert.True(t, errors.Is(rs.Prohibits(ptr("baz"), ActionWrite, "test resource"), ErrUnauthorizedForResource))
+	assert.True(t, errors.Is(rs.Prohibits(ptr(zero), ActionWrite, "test resource"), ErrUnauthorizedForResource))
+	assert.True(t, errors.Is(rs.Prohibits(ptr("foo"), ActionAll, "test resource"), ErrUnauthorizedForAction))
 }
 
 func TestZeroID(t *testing.T) {
 	zero := ZeroID[string]()
 	rs := &ResourceSet[string]{zero: ActionRead}
 
-	assert.NoError(t, rs.Prohibits(ptr("foo"), ActionRead))
-	assert.NoError(t, rs.Prohibits(ptr(zero), ActionRead))
+	assert.NoError(t, rs.Prohibits(ptr("foo"), ActionRead, "test resource"))
+	assert.NoError(t, rs.Prohibits(ptr(zero), ActionRead, "test resource"))
 
-	assert.True(t, errors.Is(rs.Prohibits(nil, ActionRead), ErrResourceUnspecified))
-	assert.True(t, errors.Is(rs.Prohibits(ptr("foo"), ActionWrite), ErrUnauthorizedForAction))
-	assert.True(t, errors.Is(rs.Prohibits(ptr(zero), ActionWrite), ErrUnauthorizedForAction))
+	assert.True(t, errors.Is(rs.Prohibits(nil, ActionRead, "test resource"), ErrResourceUnspecified))
+	assert.True(t, errors.Is(rs.Prohibits(ptr("foo"), ActionWrite, "test resource"), ErrUnauthorizedForAction))
+	assert.True(t, errors.Is(rs.Prohibits(ptr(zero), ActionWrite, "test resource"), ErrUnauthorizedForAction))
 
 	rs = &ResourceSet[string]{
 		zero:  ActionRead | ActionWrite,