Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions internal/api/handlers/swagger_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,14 @@ type UpdateMeRequest struct {
Email *string `json:"email" example:"jane@acme.com"`
}

// CreateUserRequest is the request body for POST /api/v1/users.
type CreateUserRequest struct {
Email string `json:"email" example:"employee@acme.com"`
Password string `json:"password" example:"S3cur3P@ss!"`
FullName *string `json:"full_name" example:"John Doe"`
RoleIDs []string `json:"role_ids" example:"550e8400-e29b-41d4-a716-446655440002"`
}

// UpdateOrgRequest is the request body for PUT /api/v1/organizations/me.
type UpdateOrgRequest struct {
Name string `json:"name" example:"Acme Corporation"`
Expand Down
69 changes: 69 additions & 0 deletions internal/api/handlers/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,75 @@ func (h *UserHandler) UpdateMe(w http.ResponseWriter, r *http.Request) {
writeJSON(w, http.StatusOK, userResponse(user))
}

// CreateUser godoc
// @Summary Create a user in the organization
// @Description Creates a new user inside the caller's organization. The organization is derived from the JWT — no org_id is accepted in the request body. New users always default to non-superuser. Optionally assign initial roles at creation time. Requires the `users:write` permission.
// @Tags Users
// @Accept json
// @Produce json
// @Param body body CreateUserRequest true "User creation payload"
// @Success 201 {object} UserView
// @Failure 400 {object} SwaggerErrorResponse "Validation error (weak password, empty email, invalid role ID)"
// @Failure 401 {object} SwaggerErrorResponse
// @Failure 403 {object} SwaggerErrorResponse "Missing users:write permission"
// @Failure 409 {object} SwaggerErrorResponse "Email already exists in organization"
// @Failure 500 {object} SwaggerErrorResponse
// @Security BearerAuth
// @Router /api/v1/users [post]
func (h *UserHandler) CreateUser(w http.ResponseWriter, r *http.Request) {
actorID, ok := middleware.GetUserID(r.Context())
if !ok {
writeError(w, http.StatusUnauthorized, "unauthorized")
return
}
orgID, ok := middleware.GetOrgID(r.Context())
if !ok {
writeError(w, http.StatusUnauthorized, "unauthorized")
return
}

var req struct {
Email string `json:"email"`
Password string `json:"password"`
FullName *string `json:"full_name"`
RoleIDs []string `json:"role_ids"`
}
if err := decodeJSON(r, &req); err != nil {
writeError(w, http.StatusBadRequest, "invalid request body")
return
}
if req.Email == "" || req.Password == "" {
writeError(w, http.StatusBadRequest, "email and password are required")
return
}

roleIDs := make([]uuid.UUID, 0, len(req.RoleIDs))
for _, idStr := range req.RoleIDs {
id, err := uuid.Parse(idStr)
if err != nil {
writeError(w, http.StatusBadRequest, "invalid role_id: "+idStr)
return
}
roleIDs = append(roleIDs, id)
}

ip := ptrStr(middleware.ClientIP(r))
ua := ptrStr(r.UserAgent())

user, err := h.userSvc.Create(r.Context(), orgID, actorID, service.CreateUserInput{
Email: req.Email,
Password: req.Password,
FullName: req.FullName,
RoleIDs: roleIDs,
}, ip, ua)
if err != nil {
handleServiceError(w, err)
return
}

writeJSON(w, http.StatusCreated, userResponse(user))
}

// ListUsers godoc
// @Summary List users in organization
// @Description Returns a paginated list of all users in the authenticated user's organization. Requires the `users:read` permission.
Expand Down
3 changes: 2 additions & 1 deletion internal/api/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func NewRouter(
webhookSvc := service.NewWebhookService(store, logger)
emailSvc := service.NewEmailService(cfg.Email, logger, cfg.IsDevelopment())
authSvc := service.NewAuthService(store, tokenMaker, redisCache, auditSvc, webhookSvc, emailSvc, cfg.Auth)
userSvc := service.NewUserService(store, auditSvc)
userSvc := service.NewUserService(store, auditSvc, webhookSvc)
orgSvc := service.NewOrgService(store)
rbacSvc := service.NewRBACService(store, auditSvc)
apiKeySvc := service.NewAPIKeyService(store, auditSvc)
Expand Down Expand Up @@ -102,6 +102,7 @@ func NewRouter(
r.Route("/users", func(r chi.Router) {
r.Get("/me", userH.GetMe)
r.Put("/me", userH.UpdateMe)
r.With(middleware.RequirePermission(rbacSvc, domain.PermissionUsersWrite)).Post("/", userH.CreateUser)
r.With(middleware.RequirePermission(rbacSvc, domain.PermissionUsersRead)).Get("/", userH.ListUsers)
r.With(middleware.RequirePermission(rbacSvc, domain.PermissionUsersRead)).Get("/{id}", userH.GetUser)
r.With(middleware.RequirePermission(rbacSvc, domain.PermissionUsersDelete)).Delete("/{id}", userH.DeactivateUser)
Expand Down
65 changes: 45 additions & 20 deletions internal/api/router_authorization_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ func TestRouterRequiresPermissionsOnPrivilegedRoutes(t *testing.T) {
path string
body string
}{
{name: "create user", method: http.MethodPost, path: "/api/v1/users", body: `{"email":"test@example.com","password":"S3cur3P@ss!"}`},
{name: "list users", method: http.MethodGet, path: "/api/v1/users"},
{name: "get user", method: http.MethodGet, path: "/api/v1/users/" + uuid.NewString()},
{name: "deactivate user", method: http.MethodDelete, path: "/api/v1/users/" + uuid.NewString()},
Expand Down Expand Up @@ -121,6 +122,16 @@ func TestRouterAllowsGrantedPermissions(t *testing.T) {
path: func(_ *testing.T, _ context.Context, _ *db.Store, _ uuid.UUID) string { return "/api/v1/users" },
wantStatus: http.StatusOK,
},
{
name: "users write",
permission: domain.PermissionUsersWrite,
method: http.MethodPost,
path: func(_ *testing.T, _ context.Context, _ *db.Store, _ uuid.UUID) string {
return "/api/v1/users"
},
body: `{"email":"create-` + uuid.NewString() + `@example.com","password":"S3cur3P@ss!"}`,
wantStatus: http.StatusCreated,
},
{
name: "users delete",
permission: domain.PermissionUsersDelete,
Expand Down Expand Up @@ -263,42 +274,56 @@ func TestRouterAllowsSuperuserBypass(t *testing.T) {
handler := newTestRouter(store)

tests := []struct {
name string
method string
path func(t *testing.T, ctx context.Context, store *db.Store, orgID uuid.UUID) string
body string
name string
method string
path func(t *testing.T, ctx context.Context, store *db.Store, orgID uuid.UUID) string
body string
wantStatus int
}{
{
name: "users family",
method: http.MethodGet,
name: "users family",
method: http.MethodGet,
path: func(_ *testing.T, _ context.Context, _ *db.Store, _ uuid.UUID) string { return "/api/v1/users" },
wantStatus: http.StatusOK,
},
{
name: "create user",
method: http.MethodPost,
path: func(_ *testing.T, _ context.Context, _ *db.Store, _ uuid.UUID) string { return "/api/v1/users" },
body: `{"email":"superuser-create-` + uuid.NewString() + `@example.com","password":"S3cur3P@ss!"}`,
wantStatus: http.StatusCreated,
},
{
name: "organizations family",
method: http.MethodGet,
path: func(_ *testing.T, _ context.Context, _ *db.Store, _ uuid.UUID) string {
return "/api/v1/organizations/me"
},
wantStatus: http.StatusOK,
},
{
name: "roles family",
method: http.MethodGet,
path: func(_ *testing.T, _ context.Context, _ *db.Store, _ uuid.UUID) string { return "/api/v1/roles" },
name: "roles family",
method: http.MethodGet,
path: func(_ *testing.T, _ context.Context, _ *db.Store, _ uuid.UUID) string { return "/api/v1/roles" },
wantStatus: http.StatusOK,
},
{
name: "api keys family",
method: http.MethodGet,
path: func(_ *testing.T, _ context.Context, _ *db.Store, _ uuid.UUID) string { return "/api/v1/api-keys" },
name: "api keys family",
method: http.MethodGet,
path: func(_ *testing.T, _ context.Context, _ *db.Store, _ uuid.UUID) string { return "/api/v1/api-keys" },
wantStatus: http.StatusOK,
},
{
name: "audit family",
method: http.MethodGet,
path: func(_ *testing.T, _ context.Context, _ *db.Store, _ uuid.UUID) string { return "/api/v1/audit" },
name: "audit family",
method: http.MethodGet,
path: func(_ *testing.T, _ context.Context, _ *db.Store, _ uuid.UUID) string { return "/api/v1/audit" },
wantStatus: http.StatusOK,
},
{
name: "webhooks family",
method: http.MethodGet,
path: func(_ *testing.T, _ context.Context, _ *db.Store, _ uuid.UUID) string { return "/api/v1/webhooks" },
name: "webhooks family",
method: http.MethodGet,
path: func(_ *testing.T, _ context.Context, _ *db.Store, _ uuid.UUID) string { return "/api/v1/webhooks" },
wantStatus: http.StatusOK,
},
}

Expand All @@ -312,8 +337,8 @@ func TestRouterAllowsSuperuserBypass(t *testing.T) {

path := tc.path(t, ctx, store, org.ID)
rec := performAuthedRequest(handler, tc.method, path, tc.body, issueAccessToken(t, superuser.ID, org.ID))
if rec.Code != http.StatusOK {
t.Fatalf("%s %s status = %d, want %d, body=%s", tc.method, path, rec.Code, http.StatusOK, rec.Body.String())
if rec.Code != tc.wantStatus {
t.Fatalf("%s %s status = %d, want %d, body=%s", tc.method, path, rec.Code, tc.wantStatus, rec.Body.String())
}
})
}
Expand Down
1 change: 1 addition & 0 deletions internal/domain/audit.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

// Well-known audit action constants.
const (
AuditActionUserCreated = "user.created"
AuditActionUserRegistered = "user.registered"
AuditActionUserLogin = "user.login"
AuditActionUserLoginFailed = "user.login_failed"
Expand Down
8 changes: 4 additions & 4 deletions internal/service/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ type RegisterOutput struct {

// Register creates a new organization and its first (superuser) user.
func (s *AuthService) Register(ctx context.Context, in RegisterInput, ipAddress, userAgent *string) (*RegisterOutput, error) {
if err := validatePassword(in.Password); err != nil {
if err := ValidatePassword(in.Password); err != nil {
return nil, err
}

Expand Down Expand Up @@ -284,7 +284,7 @@ func (s *AuthService) RequestPasswordReset(ctx context.Context, orgSlug, email s

// ConfirmPasswordReset validates the token and updates the password.
func (s *AuthService) ConfirmPasswordReset(ctx context.Context, rawToken, newPassword string) error {
if err := validatePassword(newPassword); err != nil {
if err := ValidatePassword(newPassword); err != nil {
return err
}

Expand Down Expand Up @@ -318,7 +318,7 @@ func (s *AuthService) ConfirmPasswordReset(ctx context.Context, rawToken, newPas
// ChangePassword updates a user's password after verifying the current one,
// then blacklists the current access token so it is rejected on all instances.
func (s *AuthService) ChangePassword(ctx context.Context, orgID, userID uuid.UUID, currentPassword, newPassword string, accessTokenID uuid.UUID, accessTokenExpiresAt time.Time) error {
if err := validatePassword(newPassword); err != nil {
if err := ValidatePassword(newPassword); err != nil {
return err
}

Expand Down Expand Up @@ -399,7 +399,7 @@ func hashString(s string) string {
return hex.EncodeToString(sum[:])
}

func validatePassword(p string) error {
func ValidatePassword(p string) error {
if len(p) < 8 {
return fmt.Errorf("%w: minimum 8 characters", domain.ErrWeakPassword)
}
Expand Down
4 changes: 2 additions & 2 deletions internal/service/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
"github.com/osama1998h/uniauth/pkg/token"
)

// validatePassword and slugify are package-private functions tested here
// ValidatePassword and slugify are package-private functions tested here
// because they encapsulate security-critical logic.

func TestValidatePassword(t *testing.T) {
Expand Down Expand Up @@ -88,7 +88,7 @@ func TestValidatePassword(t *testing.T) {

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
err := validatePassword(tc.input)
err := ValidatePassword(tc.input)
if tc.wantErr {
if err == nil {
t.Fatalf("expected error, got nil")
Expand Down
71 changes: 67 additions & 4 deletions internal/service/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,86 @@ package service

import (
"context"
"errors"
"fmt"

"github.com/google/uuid"
"github.com/jackc/pgx/v5/pgconn"
"golang.org/x/crypto/bcrypt"

"github.com/osama1998h/uniauth/internal/domain"
db "github.com/osama1998h/uniauth/internal/repository/postgres"
)

// UserService handles user management logic.
type UserService struct {
store *db.Store
auditSvc *AuditService
store *db.Store
auditSvc *AuditService
webhookSvc *WebhookService
}

// NewUserService creates a UserService.
func NewUserService(store *db.Store, auditSvc *AuditService) *UserService {
return &UserService{store: store, auditSvc: auditSvc}
func NewUserService(store *db.Store, auditSvc *AuditService, webhookSvc *WebhookService) *UserService {
return &UserService{store: store, auditSvc: auditSvc, webhookSvc: webhookSvc}
}

// CreateUserInput holds the data required to create a user in an organization.
type CreateUserInput struct {
Email string
Password string
FullName *string
RoleIDs []uuid.UUID
}

// Create creates a new user inside the given organization.
// The new user always defaults to non-superuser.
func (s *UserService) Create(ctx context.Context, orgID, actorID uuid.UUID, in CreateUserInput, ipAddress, userAgent *string) (*domain.User, error) {
if in.Email == "" {
return nil, fmt.Errorf("%w: email is required", domain.ErrInvalidInput)
}

if err := ValidatePassword(in.Password); err != nil {
return nil, err
}

hashed, err := bcrypt.GenerateFromPassword([]byte(in.Password), bcrypt.DefaultCost)
if err != nil {
return nil, fmt.Errorf("hash password: %w", err)
}

user, err := s.store.CreateUser(ctx, orgID, in.Email, string(hashed), in.FullName, false)
if err != nil {
var pgErr *pgconn.PgError
if errors.As(err, &pgErr) && pgErr.Code == "23505" {
return nil, fmt.Errorf("%w: email already exists in this organization", domain.ErrAlreadyExists)
}
return nil, fmt.Errorf("create user: %w", err)
}

for _, roleID := range in.RoleIDs {
if _, err := s.store.GetRoleByID(ctx, orgID, roleID); err != nil {
return nil, fmt.Errorf("%w: role %s not found in organization", domain.ErrInvalidInput, roleID)
}
if err := s.store.AssignRoleToUser(ctx, orgID, user.ID, roleID); err != nil {
return nil, fmt.Errorf("assign role %s: %w", roleID, err)
}
Comment on lines +52 to +67

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Create can persist a user even when role validation fails.

On Line 52, the user is inserted before role validation/assignment. If any role is invalid (Line 62) or assignment fails (Line 65), the method returns an error but leaves the user created, which is a partial-success bug.

💡 Suggested fix (validate roles first, then create/assign)
+	// Validate all requested roles before creating the user to avoid partial success.
+	for _, roleID := range in.RoleIDs {
+		if _, err := s.store.GetRoleByID(ctx, orgID, roleID); err != nil {
+			if errors.Is(err, domain.ErrNotFound) {
+				return nil, fmt.Errorf("%w: role %s not found in organization", domain.ErrInvalidInput, roleID)
+			}
+			return nil, fmt.Errorf("service.UserService.Create: get role %s: %w", roleID, err)
+		}
+	}
+
 	user, err := s.store.CreateUser(ctx, orgID, in.Email, string(hashed), in.FullName, false)
 	if err != nil {
 		var pgErr *pgconn.PgError
 		if errors.As(err, &pgErr) && pgErr.Code == "23505" {
 			return nil, fmt.Errorf("%w: email already exists in this organization", domain.ErrAlreadyExists)
 		}
 		return nil, fmt.Errorf("create user: %w", err)
 	}
 
 	for _, roleID := range in.RoleIDs {
-		if _, err := s.store.GetRoleByID(ctx, orgID, roleID); err != nil {
-			return nil, fmt.Errorf("%w: role %s not found in organization", domain.ErrInvalidInput, roleID)
-		}
 		if err := s.store.AssignRoleToUser(ctx, orgID, user.ID, roleID); err != nil {
-			return nil, fmt.Errorf("assign role %s: %w", roleID, err)
+			return nil, fmt.Errorf("service.UserService.Create: assign role %s: %w", roleID, err)
 		}
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/service/user.go` around lines 52 - 67, Currently CreateUser is
called before role validation/assignment which can leave a user persisted when
role checks or AssignRoleToUser fail; change the flow to validate all role IDs
first by calling GetRoleByID for each role in in.RoleIDs, then create the user
with s.store.CreateUser, and finally assign roles with s.store.AssignRoleToUser.
Prefer doing the create+assign sequence inside a single transactional operation
(use the store's transaction API or a single transactional method) so that
failures during AssignRoleToUser roll back the created user; update error
messages to still wrap errors (e.g., "create user" / "assign role %s") as
before.

}

s.auditSvc.Log(&domain.AuditLog{
OrgID: &orgID,
UserID: &actorID,
Action: domain.AuditActionUserCreated,
ResourceType: strPtr("user"),
ResourceID: strPtr(user.ID.String()),
IPAddress: ipAddress,
UserAgent: userAgent,
})
s.webhookSvc.Dispatch(orgID, domain.AuditActionUserCreated, map[string]any{
"user_id": user.ID,
"email": user.Email,
})

return user, nil
}

// GetByID returns a user by ID within the caller's organization.
Expand Down
Loading
Loading