-
Notifications
You must be signed in to change notification settings - Fork 663
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
NOISSUE - Remove redundant relation check #2197
base: main
Are you sure you want to change the base?
Conversation
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 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 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 // 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`,
},
},
},
}
} |
50e0e44
to
c0cf4ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arvindh123, yes - I think it is possible. Let me work on it, I will let you know in case I encounter any challenges. |
c0cf4ed
to
af1185f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unassigned user could not able to assign back again to domain
let me explain this here case here for removing users from Example: Doman name: domain_1 Administrator: domain_1 Editor: domain_1 Viewer: domain_1 Member:
{
"user_ids": ["user_2", "user_3" ]
} if {
"user_ids": ["user_2", "user_3","user_100" ]
} |
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?
|
It should fail. |
cb2f8fa
to
ac47938
Compare
75345f9
to
ab97519
Compare
ab97519
to
064faf1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"`
}
auth/service.go
Outdated
for _, rel := range []string{AdministratorRelation, MemberRelation, ViewerRelation, EditorRelation} { | ||
// Remove all policies. | ||
if err := svc.removeDomainPolicies(ctx, id, rel, userIds...); err != nil { | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
}
auth/postgres/init.go
Outdated
{ | ||
Id: "auth_2", | ||
Up: []string{ | ||
`ALTER TABLE policies DROP CONSTRAINT IF EXISTS unique_policy_constraint`, | ||
`ALTER TABLE policies ADD CONSTRAINT unique_policy_constraint UNIQUE (subject_type, subject_id, subject_relation, object_type, object_id)`, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets rollback this constraint, and add back relation
in constraint.
This will prevent to have user id with multiple relations in database.
Signed-off-by: WashingtonKK <[email protected]>
Signed-off-by: WashingtonKK <[email protected]>
Signed-off-by: WashingtonKK <[email protected]>
Signed-off-by: WashingtonKK <[email protected]>
Signed-off-by: WashingtonKK <[email protected]>
Signed-off-by: WashingtonKK <[email protected]>
Signed-off-by: WashingtonKK <[email protected]>
Signed-off-by: WashingtonKK <[email protected]>
Signed-off-by: WashingtonKK <[email protected]>
Signed-off-by: WashingtonKK <[email protected]>
8916990
to
52f7aef
Compare
Signed-off-by: WashingtonKK <[email protected]>
Signed-off-by: WashingtonKK <[email protected]>
@@ -975,37 +961,6 @@ func (svc service) createDomainPolicyRollback(ctx context.Context, userID, domai | |||
return err | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | |
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 | |
} |
Signed-off-by: WashingtonKK [email protected]
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