Repository navigation
Conversation
- Trim verbose sections (TOC, feature list, project structure, long install/config walkthroughs) into tighter prose so the README scans faster and stays easier to maintain. - Document the Checkmk integration alongside existing integrations (CHECKMK_* config, integrations doc link) and add the Entra ID SSO doc link.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues remain in deployment documentation, image-build guidance, and smoke-script compatibility.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Releases Pabawi 1.5.1 with core and batteries images, multi-architecture publishing, supply-chain validation, and refreshed deployment metadata.
Changes:
- Adds core/batteries Docker profiles, smoke tests, SBOM scanning, and vulnerability gates.
- Publishes amd64/arm64 images to Docker Hub and GHCR.
- Updates versions, CI/CD, scripts, Compose, Helm, and documentation.
File summaries
| File | Description |
|---|---|
scripts/supply-chain/trivy-batteries-ignore.yaml |
Batteries vulnerability exception |
scripts/supply-chain/scan-image.sh |
Optional Trivy ignore-file support |
scripts/supply-chain/image-smoke.sh |
Profile-specific smoke tests |
scripts/setup.sh |
Batteries setup default |
scripts/documentation/contracts.test.mjs |
Release metadata checks |
scripts/docker-run.sh |
Batteries runtime default |
scripts/docker-run.ps1 |
Batteries runtime default |
scripts/docker-build-multiarch.sh |
Profile-aware builds |
scripts/deployment/docker-images.test.mjs |
Docker contract tests |
README.md |
Installation and deployment documentation |
package.json |
Version 1.5.1 |
package-lock.json |
Locked workspace versions |
frontend/src/pages/SetupPage.svelte |
Bootstrap token guidance |
frontend/src/components/Navigation.svelte |
Displayed version |
frontend/package.json |
Frontend version |
docs/upgrading.md |
Batteries upgrade instructions |
docs/openapi.yaml |
API version |
docs/internal/dockerhub.md |
Docker image profiles |
docs/deployment/supply-chain.md |
Supply-chain workflow |
docs/deployment/docker.md |
Docker profile documentation |
Dockerfile.ubuntu |
Image version metadata |
Dockerfile.alpine |
Image version metadata |
Dockerfile |
Core and batteries stages |
docker-compose.yml |
Batteries image selection |
charts/pabawi/values.yaml |
Helm image guidance |
charts/pabawi/Chart.yaml |
Chart and app versions |
CHANGELOG.md |
Release notes |
backend/src/server.ts |
API version |
backend/package.json |
Backend version |
.github/workflows/publish.yml |
Multi-architecture registry publishing |
.github/workflows/ci.yml |
Image validation matrix |
Review details
Suppressed comments (6)
README.md:73
- This quick-start command mounts the host directory at
/pabawi, but the image stores its database and runtime data under/opt/pabawi(DATABASE_PATH=/opt/pabawi/data/pabawi.db). As written, the database is created in the container layer and is lost on recreation; mount the persistent host data directory at/opt/pabawi/datainstead.
-v "$(pwd)/pabawi:/pabawi" --env-file ".env" \
charts/pabawi/values.yaml:10
- The chart's default tag still resolves to the core image, but the standalone Kubernetes deployment guide continues to use example42/pabawi:latest while mounting a Bolt project. After this release, that documented deployment has no bolt executable, so Bolt-enabled configurations will fail; update that example to batteries- or explicitly document the core-only limitation.
# Use batteries-<version> when Bolt, Ansible, Puppet/OpenVox, or OpenSSH
# commands must run inside the pod.
docker-compose.yml:3
- This comment says the Compose example mounts both Bolt and Ansible projects, but the Ansible volume remains commented out below; only the Bolt project is mounted by default. The explanation for selecting the batteries image is therefore inaccurate.
# This example mounts Bolt and Ansible projects, so it uses the toolchain image.
scripts/docker-build-multiarch.sh:142
- The new
--targetselection is not reflected in the success hints below: when--profile batteriesis used without--pushor--load, the printed follow-updocker buildx buildcommands omit--targetand therefore rebuild the default core stage while tagging it as batteries. Include the selected target in both emitted commands (or reuse the constructed build command).
BUILD_CMD="$BUILD_CMD --target $PROFILE"
scripts/docker-build-multiarch.sh:27
- The default tag is profile-dependent:
batteriesresolves tobatteries, notlatest. This help text is therefore wrong when--profile batteriesis used and can lead users to assume the wrong image tag.
echo " -t, --tag TAG Image tag (default: latest)"
scripts/supply-chain/image-smoke.sh:3
- The first argument's usage text is now incomplete:
profileis also required, so invoking the script without an image reports a command that can never be valid. Include the profile in this usage string.
image=${1:?Usage: image-smoke.sh IMAGE}
- Files reviewed: 30/31 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
Author
|
@copilot-pull-request-reviewer Fixing. Will be in the next commit |
1 similar comment
Member
Author
|
@copilot-pull-request-reviewer Fixing. Will be in the next commit |
LifetimeCount accumulated for the process lifetime while closed, so sparse failures on long-running instances tripped the breaker even when PuppetDB was healthy. Clear the streak on success, and log grouping query errors. Closes #59 Co-authored-by: Cursor <cursoragent@cursor.com>
- Reuse an existing mcp-service user or MCP Service role instead of throwing when provisioning was interrupted (user created, role or assignment missing), so startup recovers on restart. - Only newly created roles get the default read permissions; existing roles keep their permissions since admins may have customized them. - Make RoleService.getRoleByName public for the role lookup. - Add unit and persisted-SQLite tests for interrupted provisioning, and document the behavior in docs/mcp.md.
- Locked-out admins had no way back in without editing the database by hand. The CLI resets a local user's password from the shell on the host that owns the database. - The password is read from a hidden prompt, stdin or --generate, and is never accepted as an argument, so it stays out of shell history. - It applies the normal password policy, clears lockouts and failed-login counters, revokes all existing sessions, and writes a cli_password_reset audit entry that omits the password. - Disabled accounts are only reactivated when --activate is given. - Document the command in the README, troubleshooting guide and changelog, and add an npm script plus unit tests.
- Add migration 031 for SQLite and PostgreSQL: identities are now unique on (provider, issuer, subject), and users get a local_auth_enabled scope, nullable non-unique external email, and a display_name field. - Make federated role grants identity-owned with source keys, so Entra sync only removes its own grants. Preserve legacy grants and detached ones as manual assignments. - Add LDAP directory state, group mapping, validation and lease tables, with triggers that withdraw grants and invalidate authorization state when mappings change. - Replace ambiguous findByEmail with findLocalByEmail and findAllByEmail. Entra provisioning still rejects any email collision without linking. - Reserve the "ldap:" username prefix and reject local passwords for LDAP identities. - Render null email and displayName safely in the frontend and update the API and OpenAPI docs. - Add upgrade, rollback and concurrency tests on both adapters, and the LDAP implementation plan (package 1 of 7).
- Replace UserService.createFederatedUser(claims) with a provider-neutral, idempotent method taking an explicit FederatedProvisioningInput. It owns its transaction and serializes writers via the default-role config row. - LDAP accounts get a deterministic "ldap:<sha256>" username derived from (provider, issuer, subject). They never link by email or login, get no default role, and issue no token; validation state starts unvalidated. - Returning LDAP identities refresh profile and locators while keeping suspension, admin status, manual grants and sessions untouched. - EntraIdService now derives the username itself and maps shared IDENTITY_COLLISION errors to its existing error code. - Add requireLocalProfileOwnership: directory-owned email, name and password writes return 403 DIRECTORY_ACCOUNT_READ_ONLY via user update and change-password, including for admins. Check runs before password verification so lockout counters are not touched. - Reject linking LDAP identities to existing accounts. - Add SQLite/PostgreSQL provisioning and HTTP ownership tests, update Entra tests, API/OpenAPI docs and the LDAP plan handoff.
- Add disabled-by-default LDAP config (LDAP_* variables) parsed and validated at startup, with OpenLDAP and AD presets. Construction opens no network connection, so a directory outage cannot block local access. - Add LdapDirectoryService with authenticate, revalidate, searchGroups and resolveGroup. Each call has bounded admission and a single deadline, uses isolated lookup and user-bind connections, and returns only safe reason codes. - Require TLS 1.2+, CA and hostname verification for LDAPS and StartTLS, with no plaintext fallback. Failover stops before any credential bind. - Key identities on immutable UUID/objectGUID values and re-read them after the user bind to reject replaced or moved objects. - Fail closed on referrals, truncated results, traversal limits and unreadable AD policy attributes. Validate configured filters and preserve binary and UTF-8 assertion bytes, working around ldapts parsing quirks. - Pin ldapts 9.2.0 and @ldapjs/dn 1.1.0 (the latter is flagged decommissioned upstream; noted for dependency review). - Add a reproducible OpenLDAP fixture and unit, config and integration tests; document the settings. Public LDAP login stays unavailable.
- Add LdapMappingService for role mappings keyed by immutable group subject, with revision-fenced transactions and atomic audit records. - Add /api/auth/ldap routes (group search, mapping CRUD, diagnostics) gated by rbac:admin, rate limited, with opaque bounded cursors. - Add LdapDirectoryService.diagnose for staged TLS, lookup, identity, eligibility and group checks without a user bind or profile output. - Search groups by lookup or display-name attribute. - Document the API in api.md and openapi.yaml, and record package 4 handoff in the LDAP plan. Public LDAP login stays unavailable.
- Update the version in the root, backend and frontend package.json files. - Update the Dockerfile image labels (core, alpine, ubuntu) and the Helm chart appVersion. - Update the version string in the backend health endpoint and the navigation bar.
- Access and refresh JWTs now carry a signed authContext (local or entra-id with the exact federated identity ID). Missing, malformed, extra-field or LDAP contexts are rejected, so provider is never inferred from enrollment, email or username. - assertSessionEligible is the single account/provider check used at issuance, verification and refresh. Removing an identity ends its Entra session while independent local sessions keep working. - Refresh preserves the authenticating context for accounts enrolled in several providers. - createAuthMiddleware takes a configured AuthenticationService and exposes req.authContext. Routers, server, setup, MCP and stream revalidation resolve one shared instance via the DI container (resolveAuthenticationService) instead of building their own. - Migration 032 (sqlite and postgres) rotates all session versions and expires pending OAuth state and auth codes once, because provider ownership of legacy sessions cannot be inferred. Users must sign in again; account data, identities and grants are untouched. - Tests updated for the explicit context argument; new coverage for provider-bound sessions on both adapters, upgrade invalidation and shared service wiring. API docs and the LDAP plan are updated. LDAP login and token issuance stay unavailable until package 5B.
- Set the 1.6.0 release date in CHANGELOG.md. - Update the OpenAPI spec and package-lock.json versions to 1.6.0. - Refresh the secrets baseline for the new resetUserPassword CLI and its test.
- Add POST /api/auth/ldap/login and advertise LDAP in provider discovery. Login is explicit, never falls through to another provider, and issues no tokens without an explicit role grant. - Add LdapAuthenticationService: atomic account, grant and freshness publication, cross-process leases with fencing, and ordered decisions so a stale read cannot override a newer revocation. - Bind JWTs to an exact LDAP context (identity and directory), and force directory revalidation on every refresh. Transient outages block stale access but keep grants and tokens; confirmed ineligibility revokes sessions permanently. - Enforce eligibility on REST, execution and journal SSE, MCP and console websockets. Journal streams previously never rechecked auth per delivery. - Persist console auth context (migration 034) so legacy rows fail closed; add decision ordering state (migration 033). - Scope failed-login lockouts by provider, directory and login. - Harden LdapDirectoryService: fail over only on transient transport errors, treat ambiguous revalidation as incomplete evidence, and report rejected lookup credentials as unavailable. - Update API, OpenAPI, configuration docs and the implementation plan.
- Login page discovers providers and lets users explicitly pick Local or the configured directory; a rejected LDAP attempt never falls through to another provider. - Role Management gains LDAP group search, mapping and restricted diagnostics, gated by the admin page guard plus rbac:admin. - User list and detail expose source and profile-ownership metadata and per-role provenance, so removing a manual role keeps mapped LDAP grants intact. - Temporary refresh failures (429, 503, network) keep the session credentials and show a warning instead of logging the user out. - Add the LDAP operator guide, OpenAPI/API/RBAC doc updates and an isolated loopback Playwright fixture for the UI contracts.
- Add backend/src/config/ldapDn.ts: bounded (8192 bytes) parser with escaped UTF-8, OID descriptors, unordered multi-valued RDNs and opaque BER hex values; scope checks compare parsed suffixes. Replaces the decommissioned @ldapjs/dn and its type shim, plus lockfile entries. - Qualify the failed-login counter upsert so PostgreSQL no longer rejects it as ambiguous, which skipped temporary lockout for local and LDAP. - Add live OpenLDAP rollout, real-connection outage transport cases, separate-process validation worker tests and backup/restore/upgrade rehearsals on SQLite and PostgreSQL. - Add npm run test:ldap acceptance runner and acceptance evidence docs; real AD interoperability remains an open release gate.
- Add ldap-acceptance CI job running the live LDAP and recovery matrix against PostgreSQL 15 and 17, with the browser contracts and synthetic evidence upload (keys, CA and fixture downloads excluded). - Fail acceptance when required LDAP suites are missing, skipped or failed, since a runner can exit zero with skipped suites. - Verify the PostgreSQL major version when --postgres-major is given. - Run browser contracts in focused mode via --browser; emit HTML, JUnit and trace artifacts for the dedicated Playwright config. - Add an opt-in Microsoft AD DS disposable-lab harness with strict config validation, private out-of-repo secret files and an example config. It only validates offline unless --execute is passed.
- Add an AD lab harness that runs LDAPS and StartTLS cases against an operator-attested Microsoft AD DS lab: binary GUID lookup, rename and recreation, account states and lockout, nested groups, large ranged membership, and controller failover. - Restrict all writes to run-owned objects under a generated OU. Each object is tracked by objectGUID and a description marker, and cleanup refuses to delete anything that changed owner. Cancellation blocks new writes but still cleans up. - Unit-test the mutation boundaries with a mocked client, including lost-reply creations, marker changes and cancellation. - Test AD config validation, secret file permission and symlink checks, and the explicit "unavailable"/"not-run" result when no lab is configured, so a missing lab can never report as verified. - Test the LDAP acceptance runner with shimmed docker, npm and clamscan: failure and SIGINT/SIGTERM paths must stop fixtures, kill descendants and retain evidence. Also check the live and browser report verifiers. - Test that the CI job runs on PostgreSQL 15 and 17 and always uploads synthetic evidence without key files.
The openvox8-release-debian12.deb package changed upstream, so the pinned sha256 no longer matched and `sha256sum -c` failed the image build. Pin the new digest to restore the build while keeping the integrity check.
| "dev": "tsx watch src/server.ts", | ||
| "build": "node ../scripts/compilation/backend.mjs", | ||
| "start": "node dist/server.js", | ||
| "reset-password": "tsx src/cli/reset-password.ts", |
| { | ||
| "name": "pabawi", | ||
| "version": "1.5.0", | ||
| "version": "1.6.0", |
The runner can fail readiness with either 'exceeded command deadline' or 'deadline exceeded' depending on timing; match both. Lockfile update from npm audit fix clears the critical proxy-addr advisory. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Remaining high advisories are dev/build tooling only; npm audit --omit=dev reports 0 vulnerabilities. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Comment on lines
+83
to
+95
| await userService.updateUser(user.id, { | ||
| password: options.password, | ||
| ...(reactivate ? { isActive: true } : {}), | ||
| }); | ||
|
|
||
| // Unlock first, then clear the cumulative counters the unlock does not touch | ||
| await authService.unlockAccount(user.username); | ||
| await db.execute('DELETE FROM login_attempt_counters WHERE username = ?', [user.username]); | ||
|
|
||
| // updateUser does not rotate the session version; do it explicitly | ||
| await authService.revokeAllUserTokens(user.id); | ||
|
|
||
| await auditLogger.logAdminAction('cli_password_reset', user.id, { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Images
example42/pabawi:1.5.1,1.5,1,latest: coreexample42/pabawi:batteries-1.5.1,batteries-1.5,batteries-1,batteries: batteries includedghcr.io/example42/pabawi