refactor: enhance RBAC with validation and error handling for role and permission creation

Author: SalmaElsoly

go.mod

@@ -1,3 +1,5 @@
 module github.com/codescalers/rbac
 
 go 1.24.6
+
+require github.com/google/uuid v1.6.0

go.sum

@@ -0,0 +1,2 @@
+github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=
+github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=

pkg/errors.go

@@ -1,8 +1,9 @@
 package rbac
 
 var (
-	ErrNotFound      = errorString("not found")
-	ErrAlreadyExists = errorString("already exists")
+	ErrAlreadyExists           = errorString("already exists")
+	ErrInvalidResourceOrAction = errorString("invalid resource or action")
+	ErrInvalidName             = errorString("invalid name")
 )
 
 type errorString string

pkg/rbac.go

@@ -3,52 +3,98 @@ package rbac
 import (
 	"context"
 	"strings"
+
+	"github.com/google/uuid"
 )
 
 type RBAC struct {
 	store Store
 }
 
-func New(s Store) *RBAC {
-	return &RBAC{store: s}
+func New(ctx context.Context, s Store, opts ...Option) (*RBAC, error) {
+	r := &RBAC{store: s}
+	for _, opt := range opts {
+		if err := opt(ctx, r); err != nil {
+			return nil, err
+		}
+	}
+	return r, nil
+}
+
+type Option func(ctx context.Context, r *RBAC) error
+
+func WithSeed(roles []Role) Option {
+	return func(ctx context.Context, r *RBAC) error {
+		return r.initFromSeed(ctx, roles)
+	}
+}
+
+func (r *RBAC) initFromSeed(ctx context.Context, roles []Role) error {
+	for _, role := range roles {
+		if err := r.store.CreateRole(ctx, role); err != nil {
+			return err
+		}
+	}
+	return nil
 }
 
-func (r *RBAC) CreateRole(ctx context.Context, role Role) error {
-	role.Name = strings.ToLower(role.Name)
+func (r *RBAC) CreateRole(ctx context.Context, name, description string) error {
+	n := strings.ToLower(strings.TrimSpace(name))
+	if n == "" {
+		return ErrInvalidName
+	}
+	role := Role{ID: uuid.New().String(), Name: n, Description: description}
 	return r.store.CreateRole(ctx, role)
 }
 
 func (r *RBAC) RemoveRole(ctx context.Context, roleID string) error {
+	if err := validateUUIDs(roleID); err != nil {
+		return err
+	}
 	return r.store.RemoveRole(ctx, roleID)
 }
 
-func (r *RBAC) CreatePermission(ctx context.Context, p Permission) error {
-	p.Resource = strings.ToLower(p.Resource)
-	p.Action = strings.ToLower(p.Action)
+func (r *RBAC) CreatePermission(ctx context.Context, resource, action string) error {
+	res := strings.ToLower(strings.TrimSpace(resource))
+	a := strings.ToLower(strings.TrimSpace(action))
+	if res == "" || a == "" {
+		return ErrInvalidResourceOrAction
+	}
+	p := Permission{ID: uuid.New().String(), Resource: res, Action: a}
 	return r.store.CreatePermission(ctx, p)
 }
 
-func (r *RBAC) RemovePermission(ctx context.Context, id string) error {
-	return r.store.RemovePermission(ctx, id)
+func (r *RBAC) RemovePermission(ctx context.Context, permID string) error {
+	if err := validateUUIDs(permID); err != nil {
+		return err
+	}
+	return r.store.RemovePermission(ctx, permID)
 }
 
 func (r *RBAC) AssignRole(ctx context.Context, subjectID, roleID string) error {
+	if err := validateUUIDs(roleID); err != nil {
+		return err
+	}
 	return r.store.AssignRole(ctx, subjectID, roleID)
 }
 
 func (r *RBAC) RevokeRole(ctx context.Context, subjectID, roleID string) error {
+	if err := validateUUIDs(roleID); err != nil {
+		return err
+	}
 	return r.store.RevokeRole(ctx, subjectID, roleID)
 }
 
-func (r *RBAC) AddPermissionToRole(ctx context.Context, roleID string, permID string) error {
+func (r *RBAC) AddPermissionToRole(ctx context.Context, roleID, permID string) error {
+	if err := validateUUIDs(roleID, permID); err != nil {
+		return err
+	}
 	role, err := r.store.GetRole(ctx, roleID)
 	if err != nil {
 		return err
 	}
-	for _, p := range role.Permissions {
-		if p.ID == permID {
-			return ErrAlreadyExists
-		}
+	if r.roleHasPermission(&role, permID) {
+		return ErrAlreadyExists
 	}
 	p, err := r.store.GetPermission(ctx, permID)
 	if err != nil {
@@ -58,31 +104,34 @@ func (r *RBAC) AddPermissionToRole(ctx context.Context, roleID string, permID st
 	return r.store.UpdateRole(ctx, role)
 }
 
-func (r *RBAC) RemovePermissionFromRole(ctx context.Context, roleID string, permID string) error {
+func (r *RBAC) RemovePermissionFromRole(ctx context.Context, roleID, permID string) error {
+	if err := validateUUIDs(roleID, permID); err != nil {
+		return err
+	}
 	role, err := r.store.GetRole(ctx, roleID)
 	if err != nil {
 		return err
 	}
-	filtered := role.Permissions[:0]
-	for _, p := range role.Permissions {
-		if p.ID != permID {
-			filtered = append(filtered, p)
-		}
-
-	}
-	role.Permissions = filtered
+	role.Permissions = r.filterOutPermission(role.Permissions, permID)
 	return r.store.UpdateRole(ctx, role)
 }
 
 // Direct grants
-func (r *RBAC) GrantSubject(ctx context.Context, subjectID string, grant Grant) error {
-
-	grant.Resource = strings.ToLower(grant.Resource)
-	grant.Action = strings.ToLower(grant.Action)
+func (r *RBAC) GrantSubject(ctx context.Context, subjectID string, resource, action, resourceID string) error {
+	res := strings.ToLower(strings.TrimSpace(resource))
+	act := strings.ToLower(strings.TrimSpace(action))
+	if res == "" || act == "" {
+		return ErrInvalidResourceOrAction
+	}
+	rid := strings.ToLower(strings.TrimSpace(resourceID))
+	grant := Grant{ID: uuid.New().String(), Resource: res, Action: act, ResourceID: rid}
 	return r.store.GrantSubject(ctx, subjectID, grant)
 }
 
 func (r *RBAC) RevokeSubjectGrant(ctx context.Context, subjectID, grantID string) error {
+	if err := validateUUIDs(grantID); err != nil {
+		return err
+	}
 	return r.store.RevokeSubjectGrant(ctx, subjectID, grantID)
 }
 
@@ -97,18 +146,21 @@ func (r *RBAC) Can(ctx context.Context, subjectID, action, resource string, reso
 	}
 
 	id := ""
-	if len(resourceID) > 0 {
+	if len(resourceID) > 0 && resourceID[0] != "" {
 		id = resourceID[0]
 	}
-	res := strings.ToLower(resource)
-	act := strings.ToLower(action)
+	res := strings.ToLower(strings.TrimSpace(resource))
+	act := strings.ToLower(strings.TrimSpace(action))
+	if res == "" || act == "" {
+		return false, ErrInvalidResourceOrAction
+	}
 
 	//direct grants
 	for _, g := range grants {
-		if strings.ToLower(g.Resource) != res {
+		if g.Resource != res {
 			continue
 		}
-		if strings.ToLower(g.Action) != act {
+		if g.Action != act {
 			continue
 		}
 		if g.ResourceID != id && g.ResourceID != "*" {
@@ -120,10 +172,10 @@ func (r *RBAC) Can(ctx context.Context, subjectID, action, resource string, reso
 	//role permissions
 	for _, role := range roles {
 		for _, p := range role.Permissions {
-			if strings.ToLower(p.Resource) != res {
+			if p.Resource != res {
 				continue
 			}
-			if strings.ToLower(p.Action) != act {
+			if p.Action != act {
 				continue
 			}
 			return true, nil
@@ -132,3 +184,31 @@ func (r *RBAC) Can(ctx context.Context, subjectID, action, resource string, reso
 
 	return false, nil
 }
+
+func (r *RBAC) roleHasPermission(role *Role, permID string) bool {
+	for _, p := range role.Permissions {
+		if p.ID == permID {
+			return true
+		}
+	}
+	return false
+}
+
+func (r *RBAC) filterOutPermission(perms []Permission, permID string) []Permission {
+	filtered := perms[:0]
+	for _, p := range perms {
+		if p.ID != permID {
+			filtered = append(filtered, p)
+		}
+	}
+	return filtered
+}
+
+func validateUUIDs(ids ...string) error {
+	for _, id := range ids {
+		if _, err := uuid.Parse(id); err != nil {
+			return err
+		}
+	}
+	return nil
+}