NOISSUE - Remove redundant relation check

[WashingtonKK]

Signed-off-by: WashingtonKK washingtonkigan@gmail.com

What type of PR is this?

This is a refactor because it removes a redundant relation check when unassigning users from a domain.

What does this do?

Removes a redundant relation check when unassigning users from a domain.

Which issue(s) does this PR fix/relate to?

No issue

Have you included tests for your changes?

No, the changes are covered by existing tests.

Did you document any new/modified feature?

No, the changes are already documented.

Notes

[arvindh123]

@WashingtonKK

We should not allow a user to have mutiple relationship with domain.

And we also should not allow SuperAdmin to create new relationship with domain .

So i suggesting below

Modification for addPolicyPreCondition in auth/spicedb/policies.go

func (pa *policyAgent) addPolicyPreCondition(ctx context.Context, pr auth.PolicyReq) ([]*v1.Precondition, error) {
	// Checks are required for following  ( -> means adding)
	// 1.) user -> group (both user groups and channels)
	// 2.) user -> thing
	// 3.) group -> group (both for adding parent_group and channels)
	// 4.) group (channel) -> thing
	// 5.) user -> domain

	switch {
	// 1.) user -> group (both user groups and channels)
	// Checks :
	// - USER with ANY RELATION to DOMAIN
	// - GROUP with DOMAIN RELATION to DOMAIN
	case pr.SubjectType == auth.UserType && pr.ObjectType == auth.GroupType:
		return pa.userGroupPreConditions(ctx, pr)

	// 2.) user -> thing
	// Checks :
	// - USER with ANY RELATION to DOMAIN
	// - THING with DOMAIN RELATION to DOMAIN
	case pr.SubjectType == auth.UserType && pr.ObjectType == auth.ThingType:
		return pa.userThingPreConditions(ctx, pr)

	// 3.) group -> group (both for adding parent_group and channels)
	// Checks :
	// - CHILD_GROUP with out PARENT_GROUP RELATION with any GROUP
	case pr.SubjectType == auth.GroupType && pr.ObjectType == auth.GroupType:
		return groupPreConditions(pr)

	// 4.) group (channel) -> thing
	// Checks :
	// - GROUP (channel) with DOMAIN RELATION to DOMAIN
	// - NO GROUP should not have PARENT_GROUP RELATION with GROUP (channel)
	// - THING with DOMAIN RELATION to DOMAIN
	case pr.SubjectType == auth.GroupType && pr.ObjectType == auth.ThingType:
		return channelThingPreCondition(pr)

	// 5.) user -> domain
	// Checks :
	// - User is doesn't have any relation with domain
	case pr.SubjectType == auth.UserType && pr.ObjectType == auth.DomainType:
		return pa.userDomainPreConditions(ctx, pr)

	// Check thing and group not belongs to other domain before adding to domain
	case pr.SubjectType == auth.DomainType && pr.Relation == auth.DomainRelation && (pr.ObjectType == auth.ThingType || pr.ObjectType == auth.GroupType):
		preconds := []*v1.Precondition{
			{
				Operation: v1.Precondition_OPERATION_MUST_NOT_MATCH,
				Filter: &v1.RelationshipFilter{
					ResourceType:       pr.ObjectType,
					OptionalResourceId: pr.Object,
					OptionalRelation:   auth.DomainRelation,
					OptionalSubjectFilter: &v1.SubjectFilter{
						SubjectType: auth.DomainType,
					},
				},
			},
		}
		return preconds, nil
	}
	return nil, nil
}

Addition of new function in in auth/spicedb/policies.go

func (pa *policyAgent) userDomainPreConditions(ctx context.Context, pr auth.PolicyReq) ([]*v1.Precondition, error) {
	var preconds []*v1.Precondition

	if err := pa.CheckPolicy(ctx, auth.PolicyReq{
		Subject:     pr.Subject,
		SubjectType: pr.SubjectType,
		Permission:  auth.AdminPermission,
		Object:      auth.MagistralaObject,
		ObjectType:  auth.PlatformType,
	}); err == nil {
		return preconds, fmt.Errorf("use already exists in domain")
	}

	// user should not have any relation with group
	preconds = append(preconds, &v1.Precondition{
		Operation: v1.Precondition_OPERATION_MUST_NOT_MATCH,
		Filter: &v1.RelationshipFilter{
			ResourceType:       auth.DomainType,
			OptionalResourceId: pr.Object,
			OptionalSubjectFilter: &v1.SubjectFilter{
				SubjectType:       auth.UserType,
				OptionalSubjectId: pr.Subject,
			},
		},
	})

	return preconds, nil
}

Modification of auth/postgres/init.go

// Copyright (c) Abstract Machines
// SPDX-License-Identifier: Apache-2.0

package postgres

import (
	_ "github.com/jackc/pgx/v5/stdlib" // required for SQL access
	migrate "github.com/rubenv/sql-migrate"
)

// Migration of Auth service.
func Migration() *migrate.MemoryMigrationSource {
	return &migrate.MemoryMigrationSource{
		Migrations: []*migrate.Migration{
			{
				Id: "auth_1",
				Up: []string{
					`CREATE TABLE IF NOT EXISTS keys (
                        id          VARCHAR(254) NOT NULL,
                        type        SMALLINT,
                        subject     VARCHAR(254) NOT NULL,
                        issuer_id   VARCHAR(254) NOT NULL,
                        issued_at   TIMESTAMP NOT NULL,
                        expires_at  TIMESTAMP,
                        PRIMARY KEY (id, issuer_id)
                    )`,

					`CREATE TABLE IF NOT EXISTS domains (
                        id          VARCHAR(36) PRIMARY KEY,
                        name        VARCHAR(254),
                        tags        TEXT[],
                        metadata    JSONB,
                        alias       VARCHAR(254) NULL UNIQUE,
                        created_at  TIMESTAMP,
                        updated_at  TIMESTAMP,
                        updated_by  VARCHAR(254),
                        created_by  VARCHAR(254),
                        status      SMALLINT NOT NULL DEFAULT 0 CHECK (status >= 0)
                    );`,
					`CREATE TABLE IF NOT EXISTS policies (
                        subject_type        VARCHAR(254) NOT NULL,
                        subject_id          VARCHAR(254) NOT NULL,
                        subject_relation    VARCHAR(254) NOT NULL,
                        relation            VARCHAR(254) NOT NULL,
                        object_type         VARCHAR(254) NOT NULL,
                        object_id           VARCHAR(254) NOT NULL,
                        CONSTRAINT unique_policy_constraint UNIQUE (subject_type, subject_id, subject_relation, object_type, object_id)
                    );`,
				},
				Down: []string{
					`DROP TABLE IF EXISTS keys`,
				},
			},
		},
	}
}

[arvindh123]

@WashingtonKK

#2197 (comment)

Can we have this kind of implement in this PR ?

[WashingtonKK]

@arvindh123, yes - I think it is possible. Let me work on it, I will let you know in case I encounter any challenges.

[arvindh123]

Unassigned user could not able to assign back again to domain

[arvindh123]

let me explain this here case here for removing users from Domain

Example:

Doman name: domain_1

domain_1 Administrator:
user_101
user_102

domain_1 Editor:
user_1

domain_1 Viewer:
user_2

domain_1 Member:
user_3

user_1 (domain_1 Editor) should able to remove user_2 user_3 without giving relation in request JSON

{ 
    "user_ids": ["user_2", "user_3" ]
} 

if user_1 (domain_1 Editor) tries to remove user_2, user_3 , user_101 (domain administrator) , then the request should fails

{ 
    "user_ids": ["user_2", "user_3","user_100" ]
} 

[WashingtonKK]

if user_1 (domain_1 Editor) tries to remove user_2, user_3 , user_101 (domain administrator) , should it fail or just ignore the admin users and remove the other users?

{ 
    "user_ids": ["user_2", "user_3","user_100" ]
} 

[arvindh123]

if user_1 (domain_1 Editor) tries to remove user_2, user_3 , user_101 (domain administrator) , should it fail or just ignore the admin users and remove the other users?

{ 
    "user_ids": ["user_2", "user_3","user_100" ]
} 

It should fail.

[arvindh123]

Since we are removing all the relation between the user and domain,
So I think we can remove the relation from the Unassign user from domain request

type unassignUsersReq struct {
	token    string
	domainID string
	UserIDs  []string `json:"user_ids"`
-	Relation string   `json:"relation"`
}

[arvindh123]

Instead of using array of relations you can use DeletePoicy

DeletePolicy acutally it works like filter. There is PR to remove this gRPC functiion name from DeletePolicy to DeletePolicyFilter #2218

If you don't provide relation , It will remove all the relation between user id and domain id

Example :

	if err = svc.domains.DeletePolicy(ctx,  PolicyReq{
				Subject:     userID,
				SubjectType: UserType,
				SubjectKind: UsersKind,
				Object:      id,
				ObjectType:  DomainType,
			}); err != nil {
		return errors.Wrap(errRemovePolicies, err)
	}

[arvindh123]

Since DeletePolicy is single operation per user. It difficult to track if any one policy removal fails. It should happens like single operation for all users. So something like below will keep the removal process in policy and repo in single step.

Suggested change

	}
	func (svc service) UnassignUsers(ctx context.Context, token, id string, userIds []string) error {
	pr := PolicyReq{
	Subject: token,
	SubjectType: UserType,
	SubjectKind: TokenKind,
	Object: id,
	ObjectType: DomainType,
	Permission: SharePermission,
	}
	if err := svc.Authorize(ctx, pr); err != nil {
	return err
	}
	relations := []string{MemberRelation, ViewerRelation, EditorRelation}
	pr.Permission = AdminPermission
	if err := svc.Authorize(ctx, pr); err != nil {
	pr.SubjectKind = UsersKind
	// User is not admin.
	for _, userID := range userIds {
	pr.Subject = userID
	if err := svc.Authorize(ctx, pr); err == nil {
	// Non admin attempts to remove admin.
	return errors.Wrap(svcerr.ErrAuthorization, err)
	}
	}
	} else {
	relations = append(relations, AdministratorRelation)
	}
	if err := svc.removeDomainPolicies(ctx, id, relations, userIds...); err != nil {
	return err
	}
	return nil
	}

[arvindh123]

Optimization can be done in seperate PR #2280

[arvindh123]

Please remove the relation here. This makes relation mandatory.
We don't want it as mandotory and not even as optional.
Because we have unique constraint in database, that will prevent creation of mutiple relation for single domain

[arvindh123]

@WashingtonKK
They are not removed from Repo . I could not see repo call for removing users from Database.