Conversation
- Remove duplicate and non-existent TOC entries - Fix typos (Detaila→Details, Contrainer→Container, padawi→pabawi) - Standardize capitalization (Bolt, SSH, PuppetCA) - Fix duplicate 'the the' instances - Improve clarity: 'eventual' → 'any required/any external' - Align TOC with actual document structure
- Remove default type parameters in PuppetDBService and PuppetserverService - Fixes eslint @typescript-eslint/no-unnecessary-type-arguments violations
There was a problem hiding this comment.
Pull request overview
This pull request contains a large set of mixed changes spanning authentication, database architecture, refactoring, documentation, and configuration updates.
Changes:
- Adds complete RBAC system (users, groups, roles, permissions) with bcrypt password hashing
- Introduces database abstraction layer with DatabaseAdapter interface and SQLiteAdapter implementation
- Reorganizes imports by removing barrel exports from multiple modules
- Adds comprehensive documentation for Bolt, Hiera, PuppetDB, Puppetserver integrations
- Refactors caching utility (SimpleCache) to shared location
- Updates middleware organization and error handling
- Fixes import paths across tests and implementation files
- Adds Kubernetes deployment guide and repository structure documentation
Reviewed changes
Copilot reviewed 81 out of 97 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/package.json | Adds bcrypt dependencies (version issue) |
| backend/src/auth/* | New RBAC system (UserService, GroupService, RoleService) without tests |
| backend/src/database/schema.sql | Adds authentication/authorization tables (conflicts with no-auth design) |
| backend/src/database/* | New database abstraction layer with adapters |
| backend/src/integrations/*/index.ts | Removes barrel exports (breaking change) |
| backend/src/middleware/index.ts | Removes barrel export |
| docs/* | Adds integration guides and deployment documentation |
| scripts/generate-pabawi-cert.sh | Updates .env file discovery logic |
| .kiro/* | Development notes and completion summaries |
| -- ============================================================================ | ||
| -- Authentication & Authorization Tables (v1.0.0 RBAC System) | ||
| -- ============================================================================ | ||
|
|
||
| -- Users table for storing user accounts | ||
| CREATE TABLE IF NOT EXISTS users ( | ||
| id TEXT PRIMARY KEY, | ||
| username TEXT UNIQUE NOT NULL, | ||
| email TEXT UNIQUE NOT NULL, | ||
| password_hash TEXT NOT NULL, | ||
| display_name TEXT, | ||
| active INTEGER DEFAULT 1, -- Boolean flag (0 or 1) | ||
| created_at TEXT NOT NULL, -- ISO 8601 timestamp | ||
| updated_at TEXT, -- ISO 8601 timestamp | ||
| last_login_at TEXT -- ISO 8601 timestamp | ||
| ); | ||
|
|
||
| -- Groups table for organizing users | ||
| CREATE TABLE IF NOT EXISTS groups ( | ||
| id TEXT PRIMARY KEY, | ||
| name TEXT UNIQUE NOT NULL, | ||
| description TEXT, | ||
| created_at TEXT NOT NULL, -- ISO 8601 timestamp | ||
| updated_at TEXT -- ISO 8601 timestamp | ||
| ); | ||
|
|
||
| -- Roles table for permission bundling | ||
| CREATE TABLE IF NOT EXISTS roles ( | ||
| id TEXT PRIMARY KEY, | ||
| name TEXT UNIQUE NOT NULL, | ||
| description TEXT, | ||
| priority INTEGER DEFAULT 0, -- Higher priority wins in conflict resolution | ||
| is_system INTEGER DEFAULT 0, -- Boolean flag for built-in roles | ||
| created_at TEXT NOT NULL, -- ISO 8601 timestamp | ||
| updated_at TEXT -- ISO 8601 timestamp | ||
| ); | ||
|
|
||
| -- Permissions table for capability-based access control | ||
| CREATE TABLE IF NOT EXISTS permissions ( | ||
| id TEXT PRIMARY KEY, | ||
| role_id TEXT NOT NULL, | ||
| capability TEXT NOT NULL, -- Capability pattern (e.g., 'command.execute', 'inventory.*', '*') | ||
| action TEXT NOT NULL CHECK(action IN ('allow', 'deny')), | ||
| conditions TEXT, -- JSON object for conditional permissions | ||
| FOREIGN KEY (role_id) REFERENCES roles(id) ON DELETE CASCADE | ||
| ); | ||
|
|
||
| -- User-Group junction table (many-to-many) | ||
| CREATE TABLE IF NOT EXISTS user_groups ( | ||
| user_id TEXT NOT NULL, | ||
| group_id TEXT NOT NULL, | ||
| PRIMARY KEY (user_id, group_id), | ||
| FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE, | ||
| FOREIGN KEY (group_id) REFERENCES groups(id) ON DELETE CASCADE | ||
| ); | ||
|
|
||
| -- User-Role junction table (direct role assignments) | ||
| CREATE TABLE IF NOT EXISTS user_roles ( | ||
| user_id TEXT NOT NULL, | ||
| role_id TEXT NOT NULL, | ||
| PRIMARY KEY (user_id, role_id), | ||
| FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE, | ||
| FOREIGN KEY (role_id) REFERENCES roles(id) ON DELETE CASCADE | ||
| ); | ||
|
|
||
| -- Group-Role junction table (roles assigned to groups) | ||
| CREATE TABLE IF NOT EXISTS group_roles ( | ||
| group_id TEXT NOT NULL, | ||
| role_id TEXT NOT NULL, | ||
| PRIMARY KEY (group_id, role_id), | ||
| FOREIGN KEY (group_id) REFERENCES groups(id) ON DELETE CASCADE, | ||
| FOREIGN KEY (role_id) REFERENCES roles(id) ON DELETE CASCADE | ||
| ); | ||
|
|
||
| -- User indexes | ||
| CREATE INDEX IF NOT EXISTS idx_users_username ON users(username); | ||
| CREATE INDEX IF NOT EXISTS idx_users_email ON users(email); | ||
| CREATE INDEX IF NOT EXISTS idx_users_active ON users(active); | ||
| CREATE INDEX IF NOT EXISTS idx_users_created_at ON users(created_at DESC); | ||
|
|
||
| -- Group indexes | ||
| CREATE INDEX IF NOT EXISTS idx_groups_name ON groups(name); | ||
| CREATE INDEX IF NOT EXISTS idx_groups_created_at ON groups(created_at DESC); | ||
|
|
||
| -- Role indexes | ||
| CREATE INDEX IF NOT EXISTS idx_roles_name ON roles(name); | ||
| CREATE INDEX IF NOT EXISTS idx_roles_priority ON roles(priority DESC); | ||
| CREATE INDEX IF NOT EXISTS idx_roles_is_system ON roles(is_system); | ||
|
|
||
| -- Permission indexes | ||
| CREATE INDEX IF NOT EXISTS idx_permissions_role_id ON permissions(role_id); | ||
| CREATE INDEX IF NOT EXISTS idx_permissions_capability ON permissions(capability); | ||
|
|
||
| -- Junction table indexes for efficient lookups | ||
| CREATE INDEX IF NOT EXISTS idx_user_groups_user_id ON user_groups(user_id); | ||
| CREATE INDEX IF NOT EXISTS idx_user_groups_group_id ON user_groups(group_id); | ||
| CREATE INDEX IF NOT EXISTS idx_user_roles_user_id ON user_roles(user_id); | ||
| CREATE INDEX IF NOT EXISTS idx_user_roles_role_id ON user_roles(role_id); | ||
| CREATE INDEX IF NOT EXISTS idx_group_roles_group_id ON group_roles(group_id); | ||
| CREATE INDEX IF NOT EXISTS idx_group_roles_role_id ON group_roles(role_id); |
There was a problem hiding this comment.
The database schema adds complete authentication and authorization tables (users, groups, roles, permissions), but according to the architecture documentation in the coding guidelines, "Pabawi designed for localhost/workstation use only" with "No Authentication". This represents a significant architectural change that contradicts the stated design principle. This should either be documented as a breaking change with justification, or these tables should be marked as optional/future functionality.
| /** | ||
| * User Service - User Management Operations | ||
| * | ||
| * Part of v1.0.0 Modular Plugin Architecture (Phase 2, Step 6) | ||
| * | ||
| * Provides CRUD operations for user accounts with bcrypt password hashing. | ||
| */ | ||
|
|
||
| import { randomUUID } from "crypto"; | ||
| import bcrypt from "bcrypt"; | ||
| import type { DatabaseAdapter } from "../database/interfaces/DatabaseInterface.js"; | ||
| import type { | ||
| User, | ||
| UserPublic, | ||
| UserRow, | ||
| CreateUserInput, | ||
| UpdateUserInput, | ||
| UserGroupRow, | ||
| UserRoleRow, | ||
| PaginationOptions, | ||
| PaginatedResult, | ||
| } from "./types.js"; | ||
| import { LoggerService } from "../services/LoggerService.js"; | ||
|
|
||
| /** | ||
| * Number of salt rounds for bcrypt password hashing | ||
| */ | ||
| const BCRYPT_SALT_ROUNDS = 12; | ||
|
|
||
| /** | ||
| * Service for managing user accounts | ||
| */ | ||
| export class UserService { | ||
| private logger: LoggerService; | ||
|
|
||
| constructor(private db: DatabaseAdapter) { | ||
| this.logger = new LoggerService(); | ||
| } | ||
|
|
||
| /** | ||
| * Create a new user account | ||
| */ | ||
| async createUser(input: CreateUserInput): Promise<User> { | ||
| this.logger.info("Creating new user", { | ||
| component: "UserService", | ||
| operation: "createUser", | ||
| metadata: { username: input.username, email: input.email }, | ||
| }); | ||
|
|
||
| // Check for existing username or email | ||
| const existingUser = await this.db.queryOne<UserRow>( | ||
| "SELECT id FROM users WHERE username = ? OR email = ?", | ||
| [input.username, input.email] | ||
| ); | ||
|
|
||
| if (existingUser) { | ||
| throw new Error( | ||
| `User with username '${input.username}' or email '${input.email}' already exists` | ||
| ); | ||
| } | ||
|
|
||
| // Hash password | ||
| const passwordHash = await bcrypt.hash(input.password, BCRYPT_SALT_ROUNDS); | ||
|
|
||
| const user: User = { | ||
| id: randomUUID(), | ||
| username: input.username, | ||
| email: input.email, | ||
| passwordHash, | ||
| displayName: input.displayName, | ||
| groups: input.groups || [], | ||
| roles: input.roles || [], | ||
| active: input.active ?? true, | ||
| createdAt: new Date(), | ||
| }; | ||
|
|
||
| // Insert user | ||
| await this.db.execute( | ||
| `INSERT INTO users (id, username, email, password_hash, display_name, active, created_at) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?)`, | ||
| [ | ||
| user.id, | ||
| user.username, | ||
| user.email, | ||
| user.passwordHash, | ||
| user.displayName || null, | ||
| user.active ? 1 : 0, | ||
| user.createdAt.toISOString(), | ||
| ] | ||
| ); | ||
|
|
||
| // Add user to groups | ||
| for (const groupId of user.groups) { | ||
| await this.db.execute( | ||
| "INSERT INTO user_groups (user_id, group_id) VALUES (?, ?)", | ||
| [user.id, groupId] | ||
| ); | ||
| } | ||
|
|
||
| // Add direct roles | ||
| for (const roleId of user.roles) { | ||
| await this.db.execute( | ||
| "INSERT INTO user_roles (user_id, role_id) VALUES (?, ?)", | ||
| [user.id, roleId] | ||
| ); | ||
| } | ||
|
|
||
| this.logger.info("User created successfully", { | ||
| component: "UserService", | ||
| operation: "createUser", | ||
| metadata: { userId: user.id, username: user.username }, | ||
| }); | ||
|
|
||
| return user; | ||
| } | ||
|
|
||
| /** | ||
| * Get user by ID | ||
| */ | ||
| async getUserById(id: string): Promise<User | null> { | ||
| const row = await this.db.queryOne<UserRow>( | ||
| "SELECT * FROM users WHERE id = ?", | ||
| [id] | ||
| ); | ||
|
|
||
| if (!row) { | ||
| return null; | ||
| } | ||
|
|
||
| return this.hydrateUser(row); | ||
| } | ||
|
|
||
| /** | ||
| * Get user by username | ||
| */ | ||
| async getUserByUsername(username: string): Promise<User | null> { | ||
| const row = await this.db.queryOne<UserRow>( | ||
| "SELECT * FROM users WHERE username = ?", | ||
| [username] | ||
| ); | ||
|
|
||
| if (!row) { | ||
| return null; | ||
| } | ||
|
|
||
| return this.hydrateUser(row); | ||
| } | ||
|
|
||
| /** | ||
| * Get user by email | ||
| */ | ||
| async getUserByEmail(email: string): Promise<User | null> { | ||
| const row = await this.db.queryOne<UserRow>( | ||
| "SELECT * FROM users WHERE email = ?", | ||
| [email] | ||
| ); | ||
|
|
||
| if (!row) { | ||
| return null; | ||
| } | ||
|
|
||
| return this.hydrateUser(row); | ||
| } | ||
|
|
||
| /** | ||
| * List users with pagination | ||
| */ | ||
| async listUsers( | ||
| options: PaginationOptions = {} | ||
| ): Promise<PaginatedResult<UserPublic>> { | ||
| const { limit = 50, offset = 0, sortBy = "created_at", sortOrder = "desc" } = options; | ||
|
|
||
| // Validate sort field to prevent SQL injection | ||
| const allowedSortFields = ["username", "email", "created_at", "last_login_at", "active"]; | ||
| const safeSortBy = allowedSortFields.includes(sortBy) ? sortBy : "created_at"; | ||
| const safeSortOrder = sortOrder === "asc" ? "ASC" : "DESC"; | ||
|
|
||
| const [rows, countResult] = await Promise.all([ | ||
| this.db.query<UserRow>( | ||
| `SELECT * FROM users ORDER BY ${safeSortBy} ${safeSortOrder} LIMIT ? OFFSET ?`, | ||
| [limit, offset] | ||
| ), | ||
| this.db.queryOne<{ count: number }>("SELECT COUNT(*) as count FROM users"), | ||
| ]); | ||
|
|
||
| const total = countResult?.count ?? 0; | ||
| const users = await Promise.all(rows.map((row) => this.hydrateUser(row))); | ||
|
|
||
| return { | ||
| items: users.map(this.toPublicUser), | ||
| total, | ||
| limit, | ||
| offset, | ||
| hasMore: offset + rows.length < total, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Update a user | ||
| */ | ||
| async updateUser(id: string, input: UpdateUserInput): Promise<User | null> { | ||
| this.logger.info("Updating user", { | ||
| component: "UserService", | ||
| operation: "updateUser", | ||
| metadata: { userId: id }, | ||
| }); | ||
|
|
||
| const existingUser = await this.getUserById(id); | ||
| if (!existingUser) { | ||
| return null; | ||
| } | ||
|
|
||
| // Check for username/email conflicts | ||
| if (input.username || input.email) { | ||
| const conflict = await this.db.queryOne<UserRow>( | ||
| "SELECT id FROM users WHERE id != ? AND (username = ? OR email = ?)", | ||
| [id, input.username || existingUser.username, input.email || existingUser.email] | ||
| ); | ||
|
|
||
| if (conflict) { | ||
| throw new Error("Username or email already in use"); | ||
| } | ||
| } | ||
|
|
||
| // Build update query | ||
| const updates: string[] = []; | ||
| const params: unknown[] = []; | ||
|
|
||
| if (input.username !== undefined) { | ||
| updates.push("username = ?"); | ||
| params.push(input.username); | ||
| } | ||
| if (input.email !== undefined) { | ||
| updates.push("email = ?"); | ||
| params.push(input.email); | ||
| } | ||
| if (input.password !== undefined) { | ||
| updates.push("password_hash = ?"); | ||
| params.push(await bcrypt.hash(input.password, BCRYPT_SALT_ROUNDS)); | ||
| } | ||
| if (input.displayName !== undefined) { | ||
| updates.push("display_name = ?"); | ||
| params.push(input.displayName || null); | ||
| } | ||
| if (input.active !== undefined) { | ||
| updates.push("active = ?"); | ||
| params.push(input.active ? 1 : 0); | ||
| } | ||
|
|
||
| if (updates.length > 0) { | ||
| updates.push("updated_at = ?"); | ||
| params.push(new Date().toISOString()); | ||
| params.push(id); | ||
|
|
||
| await this.db.execute( | ||
| `UPDATE users SET ${updates.join(", ")} WHERE id = ?`, | ||
| params | ||
| ); | ||
| } | ||
|
|
||
| // Update groups | ||
| if (input.groups !== undefined) { | ||
| await this.db.execute("DELETE FROM user_groups WHERE user_id = ?", [id]); | ||
| for (const groupId of input.groups) { | ||
| await this.db.execute( | ||
| "INSERT INTO user_groups (user_id, group_id) VALUES (?, ?)", | ||
| [id, groupId] | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Update direct roles | ||
| if (input.roles !== undefined) { | ||
| await this.db.execute("DELETE FROM user_roles WHERE user_id = ?", [id]); | ||
| for (const roleId of input.roles) { | ||
| await this.db.execute( | ||
| "INSERT INTO user_roles (user_id, role_id) VALUES (?, ?)", | ||
| [id, roleId] | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| this.logger.info("User updated successfully", { | ||
| component: "UserService", | ||
| operation: "updateUser", | ||
| metadata: { userId: id }, | ||
| }); | ||
|
|
||
| return this.getUserById(id); | ||
| } | ||
|
|
||
| /** | ||
| * Delete a user | ||
| */ | ||
| async deleteUser(id: string): Promise<boolean> { | ||
| this.logger.info("Deleting user", { | ||
| component: "UserService", | ||
| operation: "deleteUser", | ||
| metadata: { userId: id }, | ||
| }); | ||
|
|
||
| // Delete associations first | ||
| await this.db.execute("DELETE FROM user_groups WHERE user_id = ?", [id]); | ||
| await this.db.execute("DELETE FROM user_roles WHERE user_id = ?", [id]); | ||
|
|
||
| const result = await this.db.execute("DELETE FROM users WHERE id = ?", [id]); | ||
|
|
||
| const deleted = result.changes > 0; | ||
| if (deleted) { | ||
| this.logger.info("User deleted successfully", { | ||
| component: "UserService", | ||
| operation: "deleteUser", | ||
| metadata: { userId: id }, | ||
| }); | ||
| } | ||
|
|
||
| return deleted; | ||
| } | ||
|
|
||
| /** | ||
| * Verify user password | ||
| */ | ||
| async verifyPassword(user: User, password: string): Promise<boolean> { | ||
| return bcrypt.compare(password, user.passwordHash); | ||
| } | ||
|
|
||
| /** | ||
| * Update last login timestamp | ||
| */ | ||
| async updateLastLogin(id: string): Promise<void> { | ||
| await this.db.execute( | ||
| "UPDATE users SET last_login_at = ? WHERE id = ?", | ||
| [new Date().toISOString(), id] | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Get users by group ID | ||
| */ | ||
| async getUsersByGroup(groupId: string): Promise<UserPublic[]> { | ||
| const rows = await this.db.query<UserRow>( | ||
| `SELECT u.* FROM users u | ||
| INNER JOIN user_groups ug ON u.id = ug.user_id | ||
| WHERE ug.group_id = ?`, | ||
| [groupId] | ||
| ); | ||
|
|
||
| const users = await Promise.all(rows.map((row) => this.hydrateUser(row))); | ||
| return users.map(this.toPublicUser); | ||
| } | ||
|
|
||
| /** | ||
| * Get users by role ID | ||
| */ | ||
| async getUsersByRole(roleId: string): Promise<UserPublic[]> { | ||
| const rows = await this.db.query<UserRow>( | ||
| `SELECT u.* FROM users u | ||
| INNER JOIN user_roles ur ON u.id = ur.user_id | ||
| WHERE ur.role_id = ?`, | ||
| [roleId] | ||
| ); | ||
|
|
||
| const users = await Promise.all(rows.map((row) => this.hydrateUser(row))); | ||
| return users.map(this.toPublicUser); | ||
| } | ||
|
|
||
| /** | ||
| * Search users by username or email | ||
| */ | ||
| async searchUsers( | ||
| query: string, | ||
| options: PaginationOptions = {} | ||
| ): Promise<PaginatedResult<UserPublic>> { | ||
| const { limit = 50, offset = 0 } = options; | ||
| const searchPattern = `%${query}%`; | ||
|
|
||
| const [rows, countResult] = await Promise.all([ | ||
| this.db.query<UserRow>( | ||
| `SELECT * FROM users | ||
| WHERE username LIKE ? OR email LIKE ? OR display_name LIKE ? | ||
| ORDER BY username ASC | ||
| LIMIT ? OFFSET ?`, | ||
| [searchPattern, searchPattern, searchPattern, limit, offset] | ||
| ), | ||
| this.db.queryOne<{ count: number }>( | ||
| `SELECT COUNT(*) as count FROM users | ||
| WHERE username LIKE ? OR email LIKE ? OR display_name LIKE ?`, | ||
| [searchPattern, searchPattern, searchPattern] | ||
| ), | ||
| ]); | ||
|
|
||
| const total = countResult?.count ?? 0; | ||
| const users = await Promise.all(rows.map((row) => this.hydrateUser(row))); | ||
|
|
||
| return { | ||
| items: users.map(this.toPublicUser), | ||
| total, | ||
| limit, | ||
| offset, | ||
| hasMore: offset + rows.length < total, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Hydrate a user row with groups and roles | ||
| */ | ||
| private async hydrateUser(row: UserRow): Promise<User> { | ||
| const [groupRows, roleRows] = await Promise.all([ | ||
| this.db.query<UserGroupRow>( | ||
| "SELECT group_id FROM user_groups WHERE user_id = ?", | ||
| [row.id] | ||
| ), | ||
| this.db.query<UserRoleRow>( | ||
| "SELECT role_id FROM user_roles WHERE user_id = ?", | ||
| [row.id] | ||
| ), | ||
| ]); | ||
|
|
||
| return { | ||
| id: row.id, | ||
| username: row.username, | ||
| email: row.email, | ||
| passwordHash: row.password_hash, | ||
| displayName: row.display_name || undefined, | ||
| groups: groupRows.map((g) => g.group_id), | ||
| roles: roleRows.map((r) => r.role_id), | ||
| active: row.active === 1, | ||
| createdAt: new Date(row.created_at), | ||
| updatedAt: row.updated_at ? new Date(row.updated_at) : undefined, | ||
| lastLoginAt: row.last_login_at ? new Date(row.last_login_at) : undefined, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Convert User to UserPublic (strips sensitive data) | ||
| */ | ||
| private toPublicUser(user: User): UserPublic { | ||
| return { | ||
| id: user.id, | ||
| username: user.username, | ||
| email: user.email, | ||
| displayName: user.displayName, | ||
| groups: user.groups, | ||
| roles: user.roles, | ||
| active: user.active, | ||
| createdAt: user.createdAt, | ||
| updatedAt: user.updatedAt, | ||
| lastLoginAt: user.lastLoginAt, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
This PR introduces a complete RBAC (Role-Based Access Control) system with user authentication, but there are no tests included for the new UserService, GroupService, or RoleService classes. According to the coding guidelines, test coverage should be provided for new functionality when the repository has comprehensive automated testing (which it does based on the test structure). Add unit tests for these critical security-related services.
| apiVersion: v1 | ||
| kind: Service | ||
| metadata: | ||
| name: pabawi | ||
| spec: | ||
| selector: | ||
| app: pabawi | ||
| ports: | ||
| - port: 80 | ||
| targetPort: 3000 | ||
| type: LoadBalancer |
There was a problem hiding this comment.
This Kubernetes example exposes Pabawi via a Service of type LoadBalancer while the application currently has no built-in authentication or authorization, which means anyone who can reach the load balancer can trigger Bolt tasks and commands against your infrastructure. If this manifest is applied in an environment where the load balancer is reachable from untrusted networks, it effectively creates an unauthenticated remote administration endpoint with command execution capabilities. To reduce risk, keep the service internal (e.g., ClusterIP), restrict access via network policies or an authenticated ingress/reverse proxy, and ensure Pabawi is never directly exposed to the internet without strong external auth in front of it.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| - [Configuration Guide](./configuration.md) | ||
| - [PuppetDB Integration Setup](./puppetdb-integration-setup.md) | ||
| - [PuppetDB Integration Setup](./integrations/puppetdb.md) | ||
| - [Puppetserver Setup](./uppetserver-integration-setup.md) |
There was a problem hiding this comment.
Corrected spelling of 'Puppetserver' filename from 'uppetserver-integration-setup.md' to 'puppetserver-integration-setup.md'.
| - [Puppetserver Setup](./uppetserver-integration-setup.md) | |
| - [Puppetserver Setup](./puppetserver-integration-setup.md) |
| getSecondsSinceLastCheckIn(status: NodeStatus): number { | ||
| if (!status.report_timestamp) { | ||
| return null; | ||
| return Number.POSITIVE_INFINITY; |
There was a problem hiding this comment.
The return type change from number | null to number with Number.POSITIVE_INFINITY for missing timestamps is a breaking API change. Ensure all call sites handle the new return value correctly, as code expecting null will now receive infinity.
| res.json(responseData); | ||
| } | ||
| } catch (error) { | ||
| } catch (error: unknown) { |
There was a problem hiding this comment.
The error type annotation error: unknown is redundant in catch clauses. TypeScript automatically types caught errors as unknown in modern versions. Consider removing the type annotation for cleaner code.
| } catch (error: unknown) { | |
| } catch (error) { |
| res.status(403).json( | ||
| debugInfo ? expertModeService.attachDebugInfo(errorResponse, debugInfo) : errorResponse | ||
| ); | ||
| return; |
There was a problem hiding this comment.
The next(); return; pattern on line 234 in rbac.ts creates inconsistent code flow. Consider using just return next(); to maintain consistency with other return patterns in the codebase.
| res.status(403).json( | |
| debugInfo ? expertModeService.attachDebugInfo(errorResponse, debugInfo) : errorResponse | |
| ); | |
| return; | |
| return res.status(403).json( | |
| debugInfo ? expertModeService.attachDebugInfo(errorResponse, debugInfo) : errorResponse | |
| ); |
…e legacy code - Remove legacy test files (bolt-plugin-integration.test.ts.legacy, integration-test-suite.test.ts.legacy, BoltPlugin.test.ts.legacy) - Update Bolt integration implementation in backend/src/integrations/bolt/index.ts - Refactor v1 routes structure in backend/src/routes/v1/index.ts - Update server configuration in backend/src/server.ts - Consolidate and update all integration test suites for v1 plugin architecture - Migrate BoltService tests to v1 implementation patterns - Update integration manager and node linking service tests - Refactor performance test suite for new plugin structure - Update plugin migration finalization specification with zero hardcoded plugin references requirement - Ensure core application is completely plugin-agnostic with auto-discovery from plugins/native/ directory
| import { SimpleCache } from "../../utils/caching"; | ||
|
|
There was a problem hiding this comment.
The import references ../../utils/caching but this module appears to be missing from the diff. Either this file needs to be included in the PR or this import path is incorrect.
| import { SimpleCache } from "../../utils/caching"; | |
| class SimpleCache<T> { | |
| private readonly cache = new Map<string, { value: T; expiresAt?: number }>(); | |
| constructor(private readonly defaultTtlMs?: number) {} | |
| get(key: string): T | undefined { | |
| const entry = this.cache.get(key); | |
| if (!entry) { | |
| return undefined; | |
| } | |
| if (entry.expiresAt !== undefined && entry.expiresAt <= Date.now()) { | |
| this.cache.delete(key); | |
| return undefined; | |
| } | |
| return entry.value; | |
| } | |
| set(key: string, value: T, ttlMs?: number): void { | |
| const ttl = ttlMs ?? this.defaultTtlMs; | |
| const expiresAt = ttl ? Date.now() + ttl : undefined; | |
| this.cache.set(key, { value, expiresAt }); | |
| } | |
| delete(key: string): void { | |
| this.cache.delete(key); | |
| } | |
| clear(): void { | |
| this.cache.clear(); | |
| } | |
| } |
| */ | ||
|
|
||
| import type { Node } from "../bolt/types"; | ||
| import type { Node } from "./types"; |
There was a problem hiding this comment.
Import changed from ../bolt/types to ./types, but there's no indication in the diff that Node type was moved to ./types.ts. This will likely cause a compilation error if Node is not defined in that location.
| import type { Node } from "./types"; | |
| import type { Node } from "../bolt/types"; |
| import type { Node } from "./types"; | ||
| import type { IntegrationManager } from "./IntegrationManager"; | ||
| import { LoggerService } from "../services/LoggerService"; | ||
| import type { User } from "./CapabilityRegistry"; |
There was a problem hiding this comment.
Import references User type from ./CapabilityRegistry, but based on the auth module structure shown in other files, User should be imported from ../auth/types instead. CapabilityRegistry is unlikely to define the User type.
| import type { User } from "./CapabilityRegistry"; | |
| import type { User } from "../auth/types"; |
| errors.push("Password must contain at least one number"); | ||
| } | ||
|
|
||
| if (this.passwordPolicy.requireSpecialChars && !/[!@#$%^&*()_+\-=\[\]{};':"\\|,.<>\/?]/.test(password)) { |
There was a problem hiding this comment.
The special characters regex is complex and difficult to maintain. Consider extracting this to a constant with a descriptive name like SPECIAL_CHARS_PATTERN to improve readability and make it easier to modify the allowed special characters in the future.
| details: { | ||
| version, | ||
| latencyMs, | ||
| engineVersion: `SQLite`, |
There was a problem hiding this comment.
The engineVersion should include the actual SQLite version number from the query result instead of just the string SQLite. The sqlite_version() was queried but the result is not being used.
| ): string { | ||
| // Match ${VAR_NAME}, ${VAR_NAME:-default}, or ${VAR_NAME:?error} | ||
| const envVarPattern = | ||
| /\$\{([A-Z_][A-Z0-9_]*)(?:(:[-?])([^}]*))?\}/gi; |
There was a problem hiding this comment.
The regex pattern for environment variable interpolation is complex. Extract this to a named constant like ENV_VAR_INTERPOLATION_PATTERN with a comment explaining the syntax it supports.
| // Validate sort field to prevent SQL injection | ||
| const allowedSortFields = ["username", "email", "created_at", "last_login_at", "active"]; | ||
| const safeSortBy = allowedSortFields.includes(sortBy) ? sortBy : "created_at"; | ||
| const safeSortOrder = sortOrder === "asc" ? "ASC" : "DESC"; | ||
|
|
||
| const [rows, countResult] = await Promise.all([ | ||
| this.db.query<UserRow>( | ||
| `SELECT * FROM users ORDER BY ${safeSortBy} ${safeSortOrder} LIMIT ? OFFSET ?`, |
There was a problem hiding this comment.
While safeSortBy is validated against an allowlist, directly interpolating it into SQL is still a security risk and violates best practices. Consider using a map of allowed fields to their actual column names and using the mapped value in a parameterized query instead.
| // Validate sort field to prevent SQL injection | |
| const allowedSortFields = ["username", "email", "created_at", "last_login_at", "active"]; | |
| const safeSortBy = allowedSortFields.includes(sortBy) ? sortBy : "created_at"; | |
| const safeSortOrder = sortOrder === "asc" ? "ASC" : "DESC"; | |
| const [rows, countResult] = await Promise.all([ | |
| this.db.query<UserRow>( | |
| `SELECT * FROM users ORDER BY ${safeSortBy} ${safeSortOrder} LIMIT ? OFFSET ?`, | |
| // Map of allowed sort fields to concrete column names to prevent SQL injection | |
| const SORT_FIELD_MAP: Record<string, string> = { | |
| username: "username", | |
| email: "email", | |
| created_at: "created_at", | |
| last_login_at: "last_login_at", | |
| active: "active", | |
| }; | |
| const safeSortColumn = SORT_FIELD_MAP[sortBy] ?? SORT_FIELD_MAP.created_at; | |
| const safeSortOrder = sortOrder === "asc" ? "ASC" : "DESC"; | |
| const [rows, countResult] = await Promise.all([ | |
| this.db.query<UserRow>( | |
| `SELECT * FROM users ORDER BY ${safeSortColumn} ${safeSortOrder} LIMIT ? OFFSET ?`, |
…ion and frontend compilation - Add P2.4 checkpoint results documenting successful server startup on port 3000 - Add P3.5 checkpoint report confirming frontend compiles with zero errors - Update migration tasks documentation with checkpoint completion status - Add tracking document for remaining hardcoded endpoints to be migrated - Update PluginLoader to improve plugin discovery and initialization - Refactor all frontend components to remove hardcoded plugin references - Update API client library to support dynamic plugin integration - Update integration color system to be plugin-agnostic - Update plugin type definitions for v1 architecture compliance - Update package.json with finalized dependencies and scripts - Add plugins package.json for native plugin management - Consolidate plugin migration work and verify system stability with passing checkpoints
| // Create a system user for internal capability execution | ||
| const systemUser: User = { | ||
| id: "system", | ||
| username: "system", | ||
| roles: ["admin"], | ||
| }; | ||
|
|
||
| if (!source?.isInitialized()) { | ||
| continue; | ||
| } | ||
| const capabilityRegistry = this.integrationManager.getCapabilityRegistry(); | ||
|
|
||
| for (const sourceName of linkedNode.sources) { | ||
| try { | ||
| // Get facts from this source | ||
| const facts = await source.getNodeFacts(nodeId); | ||
|
|
||
| // Get additional data types based on source | ||
| const additionalData: Record<string, unknown> = {}; | ||
|
|
||
| // Try to get source-specific data | ||
| try { | ||
| if (sourceName === "puppetdb") { | ||
| // Get PuppetDB-specific data | ||
| additionalData.reports = await source.getNodeData( | ||
| nodeId, | ||
| "reports", | ||
| ); | ||
| additionalData.catalog = await source.getNodeData( | ||
| nodeId, | ||
| "catalog", | ||
| ); | ||
| additionalData.events = await source.getNodeData(nodeId, "events"); | ||
| } else if (sourceName === "puppetserver") { | ||
| // Get Puppetserver-specific data | ||
| additionalData.certificate = await source.getNodeData( | ||
| nodeId, | ||
| "certificate", | ||
| const sourceData: Record<string, unknown> = {}; | ||
|
|
||
| // Get facts using info.facts capability | ||
| const factsResult = await capabilityRegistry.executeCapability<Record<string, unknown>>( | ||
| systemUser, | ||
| "info.facts", | ||
| { nodeId }, | ||
| undefined | ||
| ); |
There was a problem hiding this comment.
This loops linkedNode.sources but calls executeCapability(...) the same way for each source, which likely re-runs the same capability resolution multiple times and can multiply outbound calls by the number of sources. Consider executing each capability once (facts + each common capability), then grouping results by handledBy into dataBySource, or adding a registry API to execute a capability for a specific plugin/source when you actually want per-source fetching.
| // Dynamically query all available capabilities for this node | ||
| // Try common capability patterns that plugins might provide | ||
| const commonCapabilities = [ | ||
| { name: "reports.list", key: "reports" }, | ||
| { name: "catalog.get", key: "catalog" }, | ||
| { name: "events.list", key: "events" }, | ||
| { name: "certificate.get", key: "certificate" }, | ||
| { name: "status.get", key: "status" }, | ||
| ]; | ||
|
|
||
| for (const { name: capabilityName, key } of commonCapabilities) { | ||
| try { | ||
| const result = await capabilityRegistry.executeCapability<unknown>( | ||
| systemUser, | ||
| capabilityName, | ||
| { nodeId }, | ||
| undefined | ||
| ); |
There was a problem hiding this comment.
This loops linkedNode.sources but calls executeCapability(...) the same way for each source, which likely re-runs the same capability resolution multiple times and can multiply outbound calls by the number of sources. Consider executing each capability once (facts + each common capability), then grouping results by handledBy into dataBySource, or adding a registry API to execute a capability for a specific plugin/source when you actually want per-source fetching.
| # When auth is disabled, anonymous has full access | ||
| # When auth is enabled, anonymous has no access | ||
| - capability: "*" | ||
| action: "${AUTH_ENABLED:-false}" == "true" ? deny : allow | ||
| description: Full access when auth disabled, no access when enabled |
There was a problem hiding this comment.
The YAML env interpolation implemented in YamlConfigLoader only substitutes ${VAR}-style placeholders inside strings; it does not evaluate expressions/ternaries. As written, action will be a literal string (and likely fail validation or behave incorrectly). Use a plain allow/deny in the example, or document that two separate example configs are needed (auth enabled vs disabled), or implement expression evaluation explicitly (not recommended for config safety).
| # When auth is disabled, anonymous has full access | |
| # When auth is enabled, anonymous has no access | |
| - capability: "*" | |
| action: "${AUTH_ENABLED:-false}" == "true" ? deny : allow | |
| description: Full access when auth disabled, no access when enabled | |
| # Default configuration assumes auth is disabled and anonymous has full access. | |
| # When enabling auth, you should change this permission to `deny` or remove it. | |
| - capability: "*" | |
| action: allow | |
| description: Full access when auth is disabled; update when enabling auth |
| export const DatabaseConfigSchema = z.object({ | ||
| type: z.enum(["sqlite", "postgresql", "mysql"]).default("sqlite"), | ||
| // SQLite options | ||
| path: z.string().optional(), | ||
| // PostgreSQL/MySQL options | ||
| host: z.string().optional(), | ||
| port: z.number().int().positive().optional(), | ||
| database: z.string().optional(), | ||
| username: z.string().optional(), | ||
| password: z.string().optional(), | ||
| ssl: z.boolean().optional(), | ||
| // Connection pool options | ||
| poolMin: z.number().int().nonnegative().default(2), | ||
| poolMax: z.number().int().positive().default(10), | ||
| // Common options | ||
| connectionTimeout: z.number().int().positive().default(30000), | ||
| debug: z.boolean().default(false), | ||
| }); |
There was a problem hiding this comment.
The config schema advertises postgresql and mysql, but DatabaseFactory currently only registers sqlite. This means configs can validate successfully and then fail at runtime with Unsupported database type. To avoid a misleading contract, either (a) restrict type to only implemented dialects for now, or (b) add a refinement that checks DatabaseFactory.isSupported(type) so invalid types fail validation with a clear message.
| // Enable foreign keys and WAL mode for better performance | ||
| this.db.run("PRAGMA foreign_keys = ON", (fkErr) => { | ||
| if (fkErr) { | ||
| console.warn("Failed to enable foreign keys:", fkErr.message); | ||
| } | ||
| }); | ||
|
|
||
| this.db.run("PRAGMA journal_mode = WAL", (walErr) => { | ||
| if (walErr) { | ||
| console.warn("Failed to enable WAL mode:", walErr.message); | ||
| } | ||
| }); | ||
|
|
||
| if (this.config.debug) { | ||
| console.log(`[SQLiteAdapter] Connected to: ${dbPath}`); | ||
| } |
There was a problem hiding this comment.
This adapter uses console.warn/console.log for logging. Elsewhere the backend centralizes logging via LoggerService; using console can bypass log levels/formatting and trigger lint rules. Consider injecting/using LoggerService (or accepting a logger in the adapter config) and logging PRAGMA failures + debug connection output through it.
| PUPPETDB_PORT=8081 | ||
| PUPPETDB_TOKEN= | ||
| PUPPETDB_SSL_ENABLED=true | ||
| # You can generate certs for pabawi using scrips/generate-pabawi-cert.sh |
There was a problem hiding this comment.
Correct spelling in comments to avoid confusion for operators.
| PUPPETSERVER_PORT=8140 | ||
| PUPPETSERVER_TOKEN= | ||
| PUPPETSERVER_SSL_ENABLED=true | ||
| # You can use the same cert used for PuppetBD or a different one |
There was a problem hiding this comment.
Correct spelling in comments to avoid confusion for operators.
| # Remaining Hardcoded API Endpoints | ||
|
|
||
| **Status:** Task P3.1 Complete - 5 priority files fixed | ||
| **Date:** 2025-01-XX |
There was a problem hiding this comment.
The date uses an XX placeholder, which makes the doc look unfinished and can be ambiguous for auditing. Consider replacing it with an ISO date (or removing the Date line if unknown).
| **Date:** 2025-01-XX |
1 participant
No description provided.