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

Check should detect and break on cycles #1990

Open
ihasdapie opened this issue Jan 29, 2025 · 0 comments
Open

Check should detect and break on cycles #1990

ihasdapie opened this issue Jan 29, 2025 · 0 comments

Comments

@ihasdapie
Copy link
Contributor

Check currently will recurse indefinitely until MAX_DEPTH is reached if no access can be found for an access check. Instead, permify should track if a cycle is found, and if so, deny the check request.

Cycles are bad, and they shouldn't happen, but checking permissions when one exists should fail gracefully and not take excessive compute.

It would also be useful if these cycles can be reported (maybe just via a log) for downstream diagnostics.

Reproducible via this test under internal/engines

package engines

import (
	"context"
	"testing"

	"github.com/Permify/permify/internal/config"
	"github.com/Permify/permify/internal/factories"
	"github.com/Permify/permify/internal/invoke"
	"github.com/Permify/permify/pkg/database"
	base "github.com/Permify/permify/pkg/pb/base/v1"
	"github.com/Permify/permify/pkg/token"
	"github.com/Permify/permify/pkg/tuple"
	. "github.com/onsi/ginkgo/v2"
	. "github.com/onsi/gomega"
)

func TestCheckRec(t *testing.T) {
	RegisterFailHandler(Fail)

	schema := `

entity user {
    relation viewers @user
    relation owners @user

    action read = viewers or owner
    action readwrite = owner
    action readwriteshare = owner
    action owner = owners
}

entity document {
    relation owning_entity @user
    relation parent @document

    relation readers @user @group
    relation readwriters @user @group
    relation readwritesharers @user @group

    action read = readers or parent.read or readers.read or owning_entity.read or owner
    action owner = owning_entity
}


	`
	db, err := factories.DatabaseFactory(
		config.Database{
			Engine: "memory",
		},
	)

	Expect(err).ShouldNot(HaveOccurred())

	conf, err := newSchema(schema)
	Expect(err).ShouldNot(HaveOccurred())

	schemaWriter := factories.SchemaWriterFactory(db)
	err = schemaWriter.WriteSchema(context.Background(), conf)

	Expect(err).ShouldNot(HaveOccurred())

	type check struct {
		entity     string
		subject    string
		assertions map[string]base.CheckResult
	}

	tests := struct {
		relationships []string
		checks        []check
	}{
		relationships: []string{
			"document:0#parent@document:1",
			"document:0#owning_entity@user:1",
			"document:1#parent@document:0",
			"document:1#owning_entity@user:1",
		},
		checks: []check{
			{
				// works correctly as expected
				entity:  "document:1",
				subject: "user:1",
				assertions: map[string]base.CheckResult{
					"read": base.CheckResult_CHECK_RESULT_ALLOWED,
				},
			},
			{
				// problem: there can be an infinite loop during the no-access case
				// which only terminates when the depth is reached
				entity:  "document:0",
				subject: "user:2",
				assertions: map[string]base.CheckResult{
					"read": base.CheckResult_CHECK_RESULT_ALLOWED,
				},
			},
		},
	}

	schemaReader := factories.SchemaReaderFactory(db)
	dataReader := factories.DataReaderFactory(db)
	dataWriter := factories.DataWriterFactory(db)

	checkEngine := NewCheckEngine(schemaReader, dataReader)

	invoker := invoke.NewDirectInvoker(
		schemaReader,
		dataReader,
		checkEngine,
		nil,
		nil,
		nil,
	)

	checkEngine.SetInvoker(invoker)

	var tuples []*base.Tuple

	for _, relationship := range tests.relationships {
		t, err := tuple.Tuple(relationship)
		Expect(err).ShouldNot(HaveOccurred())
		tuples = append(tuples, t)
	}

	_, err = dataWriter.Write(context.Background(), "t1", database.NewTupleCollection(tuples...), database.NewAttributeCollection())
	Expect(err).ShouldNot(HaveOccurred())

	for _, check := range tests.checks {
		entity, err := tuple.E(check.entity)
		Expect(err).ShouldNot(HaveOccurred())

		ear, err := tuple.EAR(check.subject)
		Expect(err).ShouldNot(HaveOccurred())

		subject := &base.Subject{
			Type:     ear.GetEntity().GetType(),
			Id:       ear.GetEntity().GetId(),
			Relation: ear.GetRelation(),
		}

		for permission, res := range check.assertions {
			println("asserting permission: ", permission)

			response, err := invoker.Check(context.Background(), &base.PermissionCheckRequest{
				TenantId:   "t1",
				Entity:     entity,
				Subject:    subject,
				Permission: permission,
				Metadata: &base.PermissionCheckRequestMetadata{
					SnapToken:     token.NewNoopToken().Encode().String(),
					SchemaVersion: "",
					Depth:         100,
				},
			})
			/* println("---------------")
			fmt.Printf("err: %+v\n", err)
			fmt.Printf("response: %+v\n", response)
			println("---------------") */

			Expect(err).ShouldNot(HaveOccurred())
			Expect(res).Should(Equal(response.GetCan()))
		}
	}

}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant