Apply suggestions from code review

Co-authored-by: John Schaeffer <[email protected]>
Signed-off-by: Bailin He <[email protected]>

cmd/createrole.go

@@ -23,7 +23,6 @@ const (
 	createRoleFlagResource = "resource"
 	createRoleFlagActions  = "actions"
 	createRoleFlagName     = "name"
-	createRoleFlagIsV2     = "v2"
 )
 
 var createRoleCmd = &cobra.Command{
@@ -42,23 +41,20 @@ func init() {
 	flags.StringSlice(createRoleFlagActions, []string{}, "actions to assign to created role")
 	flags.String(createRoleFlagResource, "", "resource to bind to created role")
 	flags.String(createRoleFlagName, "", "name of role to create")
-	flags.Bool(createRoleFlagIsV2, false, "create a v2 role")
 
 	v := viper.GetViper()
 
 	viperx.MustBindFlag(v, createRoleFlagSubject, flags.Lookup(createRoleFlagSubject))
 	viperx.MustBindFlag(v, createRoleFlagActions, flags.Lookup(createRoleFlagActions))
 	viperx.MustBindFlag(v, createRoleFlagResource, flags.Lookup(createRoleFlagResource))
 	viperx.MustBindFlag(v, createRoleFlagName, flags.Lookup(createRoleFlagName))
-	viperx.MustBindFlag(v, createRoleFlagIsV2, flags.Lookup(createRoleFlagIsV2))
 }
 
 func createRole(ctx context.Context, cfg *config.AppConfig) {
 	subjectIDStr := viper.GetString(createRoleFlagSubject)
 	actions := viper.GetStringSlice(createRoleFlagActions)
 	resourceIDStr := viper.GetString(createRoleFlagResource)
 	name := viper.GetString(createRoleFlagName)
-	v2 := viper.GetBool(createRoleFlagIsV2)
 
 	if subjectIDStr == "" || len(actions) == 0 || resourceIDStr == "" || name == "" {
 		logger.Fatal("invalid config")
@@ -128,35 +124,22 @@ func createRole(ctx context.Context, cfg *config.AppConfig) {
 		logger.Fatalw("error creating subject resource", "error", err)
 	}
 
-	if v2 {
-		role, err := engine.CreateRoleV2(ctx, subjectResource, resource, name, actions)
-		if err != nil {
-			logger.Fatalw("error creating role", "error", err)
-		}
-
-		rbsubj := []types.RoleBindingSubject{{SubjectResource: subjectResource}}
-
-		roleres, err := engine.NewResourceFromID(role.ID)
-		if err != nil {
-			logger.Fatalw("error creating role resource", "error", err)
-		}
-
-		rb, err := engine.CreateRoleBinding(ctx, subjectResource, resource, roleres, rbsubj)
-		if err != nil {
-			logger.Fatalw("error creating role binding", "error", err)
-		}
+	role, err := engine.CreateRoleV2(ctx, subjectResource, resource, name, actions)
+	if err != nil {
+		logger.Fatalw("error creating role", "error", err)
+	}
 
-		logger.Infof("created role %s[%s] and role-binding %s", role.Name, role.ID, rb.ID)
-	} else {
-		role, err := engine.CreateRole(ctx, subjectResource, resource, name, actions)
-		if err != nil {
-			logger.Fatalw("error creating role", "error", err)
-		}
+	rbsubj := []types.RoleBindingSubject{{SubjectResource: subjectResource}}
 
-		if err = engine.AssignSubjectRole(ctx, subjectResource, role); err != nil {
-			logger.Fatalw("error creating role", "error", err)
-		}
+	roleres, err := engine.NewResourceFromID(role.ID)
+	if err != nil {
+		logger.Fatalw("error creating role resource", "error", err)
+	}
 
-		logger.Infow("role successfully created", "role_id", role.ID)
+	rb, err := engine.CreateRoleBinding(ctx, subjectResource, resource, roleres, rbsubj)
+	if err != nil {
+		logger.Fatalw("error creating role binding", "error", err)
 	}
+
+	logger.Infof("created role %s[%s] and role-binding %s", role.Name, role.ID, rb.ID)
 }

internal/api/rolebindings.go

@@ -18,9 +18,8 @@ func resourceToSubject(subjects []types.RoleBindingSubject) []roleBindingSubject
 	resp := make([]roleBindingSubject, len(subjects))
 	for i, subj := range subjects {
 		resp[i] = roleBindingSubject{
-			ID:        subj.SubjectResource.ID,
-			Type:      subj.SubjectResource.Type,
-			Condition: nil,
+			ID:   subj.SubjectResource.ID,
+			Type: subj.SubjectResource.Type,
 		}
 	}
 
@@ -113,7 +112,6 @@ func (r *Router) roleBindingCreate(c echo.Context) error {
 
 func (r *Router) roleBindingsList(c echo.Context) error {
 	resourceIDStr := c.Param("id")
-	roleIDStr := c.QueryParam("role_id")
 
 	ctx, span := tracer.Start(
 		c.Request().Context(), "api.roleBindingList",
@@ -140,23 +138,7 @@ func (r *Router) roleBindingsList(c echo.Context) error {
 		return err
 	}
 
-	roleFilter := (*types.Resource)(nil)
-
-	if roleIDStr != "" {
-		roleID, err := gidx.Parse(roleIDStr)
-		if err != nil {
-			return r.errorResponse("error parsing role ID", fmt.Errorf("%w: %s", ErrInvalidID, err.Error()))
-		}
-
-		roleResource, err := r.engine.NewResourceFromID(roleID)
-		if err != nil {
-			return r.errorResponse("error creating role resource", err)
-		}
-
-		roleFilter = &roleResource
-	}
-
-	rbs, err := r.engine.ListRoleBindings(ctx, resource, roleFilter)
+	rbs, err := r.engine.ListRoleBindings(ctx, resource, nil)
 	if err != nil {
 		return r.errorResponse("error listing role-binding", err)
 	}

internal/api/router.go

@@ -87,10 +87,10 @@ func (r *Router) Routes(rg *echo.Group) {
 		v2.DELETE("/roles/:id", r.roleV2Delete)
 
 		v2.GET("/resources/:id/role-bindings", r.roleBindingsList)
-		v2.GET("/resources/:id/role-bindings/:rb_id", r.roleBindingGet)
+		v2.GET("/role-bindings/:rb_id", r.roleBindingGet)
 		v2.POST("/resources/:id/role-bindings", r.roleBindingCreate)
 		v2.DELETE("/resources/:id/role-bindings/:rb_id", r.roleBindingsDelete)
-		v2.PATCH("/resources/:id/role-bindings/:rb_id", r.roleBindingUpdate)
+		v2.PATCH("/role-bindings/:rb_id", r.roleBindingUpdate)
 
 		v2.GET("/actions", r.listActions)
 	}

internal/api/types.go

@@ -88,12 +88,9 @@ type roleBindingResponseRole struct {
 	Name string          `json:"name"`
 }
 
-type roleBindingSubjectCondition struct{}
-
 type roleBindingSubject struct {
-	ID        gidx.PrefixedID              `json:"id" binding:"required"`
-	Type      string                       `json:"type,omitempty"`
-	Condition *roleBindingSubjectCondition `json:"condition,omitempty"`
+	ID   gidx.PrefixedID `json:"id" binding:"required"`
+	Type string          `json:"type,omitempty"`
 }
 
 type roleBindingRequest struct {

internal/query/relations.go

@@ -1231,3 +1231,59 @@ func (e *engine) NewResourceFromIDString(id string) (types.Resource, error) {
 
 	return subject, nil
 }
+
+// rollbackUpdates is a helper function that rolls back a list of
+// relationship updates on spiceDB.
+func (e *engine) rollbackUpdates(ctx context.Context, updates []*pb.RelationshipUpdate) error {
+	updatesLen := len(updates)
+	rollbacks := make([]*pb.RelationshipUpdate, 0, updatesLen)
+
+	for i := range updates {
+		// reversed order
+		u := updates[updatesLen-i-1]
+
+		if u == nil {
+			continue
+		}
+
+		var op pb.RelationshipUpdate_Operation
+
+		switch u.Operation {
+		case pb.RelationshipUpdate_OPERATION_CREATE:
+			fallthrough
+		case pb.RelationshipUpdate_OPERATION_TOUCH:
+			op = pb.RelationshipUpdate_OPERATION_DELETE
+		case pb.RelationshipUpdate_OPERATION_DELETE:
+			op = pb.RelationshipUpdate_OPERATION_TOUCH
+		default:
+			continue
+		}
+
+		rollbacks = append(rollbacks, &pb.RelationshipUpdate{
+			Operation:    op,
+			Relationship: u.Relationship,
+		})
+	}
+
+	return e.applyUpdates(ctx, rollbacks)
+}
+
+// applyUpdates is a wrapper function around the spiceDB WriteRelationships method
+// it applies the given relationship updates and store the zed token for each resource.
+func (e *engine) applyUpdates(ctx context.Context, updates []*pb.RelationshipUpdate) error {
+	resp, err := e.client.WriteRelationships(ctx, &pb.WriteRelationshipsRequest{Updates: updates})
+	if err != nil {
+		return err
+	}
+
+	t := resp.WrittenAt.Token
+
+	for _, u := range updates {
+		resID := u.Relationship.Resource.ObjectId
+		if err := e.upsertZedToken(ctx, resID, t); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}