feat(java): dashboard session auth, CSRF, RBAC#387
Merged
Conversation
KV-backed users/sessions (PBKDF2 600k, byte-compatible with the other SDKs), double-submit CSRF, admin/viewer RBAC, first-run setup and env-admin bootstrap, plus a legacy shared-token fallback. Restructures the dashboard into layered support/routing/store/auth/api packages behind a route table. The no-token default now requires auth instead of serving open.
📝 WalkthroughWalkthroughThis PR replaces the dashboard's single-token authentication with a session/CSRF-based auth system supporting RBAC, PBKDF2 password hashing, and OAuth user provisioning, alongside a legacy shared-token fallback mode. It introduces new auth, routing, API-handler, and support packages, refactors DashboardServer around a Router, and adds corresponding tests. ChangesDashboard auth and routing rework
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
sdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardClient.java (1)
82-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded password creates implicit test coupling.
seedUseralways creates the user with"password123".DashboardAuthTest.changePasswordRotatesCredentialrelies on this literal asold_password, so a future edit to this default would silently break that test with a confusing failure far from the root cause.♻️ Suggested overload to make the coupling explicit
static Session seedUser(Taskito queue, String username, String role) { + return seedUser(queue, username, role, "password123"); + } + + static Session seedUser(Taskito queue, String username, String role, String password) { AuthStore store = store(queue); - store.createUser(username, "password123", role); + store.createUser(username, password, role); return store.createSession(username, role); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardClient.java` around lines 82 - 90, The hardcoded default password in DashboardClient.seedUser creates hidden coupling with tests that assume the same literal, so make the dependency explicit by introducing a shared constant or an overload that accepts the initial password and use that from seedAdmin/seedUser. Update DashboardAuthTest.changePasswordRotatesCredential to reference the shared default instead of an inline literal, so future password changes only need to happen in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/AuthStore.java`:
- Around line 64-85: AuthStore.createUser() still performs a non-atomic
read-modify-write on auth:users, so concurrent calls can overwrite each other
and lose a newly created user. Update the user-creation flow to use an atomic
write path in AuthStore, centering the fix around createUser(), rawUsers(), and
saveUsers() so the existence check and insert happen under a single
lock/CAS-style update. Also review setup() and countUsers() to ensure they do
not rely on a precheck that can race with createUser().
- Around line 106-117: updatePassword in AuthStore only changes password_hash,
so existing sessions remain valid after a password change. After saving the new
password, also revoke all sessions for the affected username using the existing
session/auth session management helpers in this module, and make sure the caller
flow re-issues a fresh session for the current user if needed. Use the
updatePassword method and related AuthStore session handling code to locate the
fix.
In `@sdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/Cookies.java`:
- Around line 63-65: The legacy auth cookie path is hardcoding the Secure
attribute to false, so secure-cookie configuration is not being honored. Update
Cookies.legacyTokenCookie to accept a secureCookies flag and pass it into format
instead of always using false, then thread that flag through
TokenAuth.openCookie so callers can propagate the dashboard’s secureCookies
setting when issuing the legacy token cookie.
In
`@sdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/PasswordHasher.java`:
- Around line 45-79: Update PasswordHasher.verify so it performs the same dummy
PBKDF2 work before returning false for null or OAUTH_PREFIX values, instead of
exiting immediately. Reuse the existing dummyVerify/password derivation pattern
(or an equivalent call into verify/pbkdf2) so OAuth-only and unknown-user cases
have comparable cost, while keeping the final result false.
In `@sdks/java/src/main/java/org/byteveda/taskito/dashboard/DashboardServer.java`:
- Around line 121-128: The generic exception handler in
DashboardServer.handleRequest is returning RuntimeException/IOException messages
directly to clients, which can leak internal details. Change the catch block to
send a generic 500 response body for unexpected errors instead of using
e.getMessage(), and keep any specific details only in server-side logging if
needed. Use the handleRequest method and the safeRespond call in DashboardServer
to locate the fix.
- Around line 111-120: The dispatch routing in DashboardServer is missing
explicit handling for /readiness and /metrics, so they currently fall through to
serveStatic() instead of returning their intended responses. Update
dispatch(HttpExchange exchange) to route these paths before the static fallback,
alongside /health and /api/*, and ensure the corresponding handlers are invoked
from DashboardServer rather than relying on the SPA fallback. If these endpoints
are not meant to be publicly exposed, remove them from Policy.PUBLIC_PATHS
instead.
In
`@sdks/java/src/test/java/org/byteveda/taskito/dashboard/auth/AuthStoreTest.java`:
- Around line 154-162: The test changePasswordInvalidatesOldCredential in
AuthStoreTest only checks that the old password stops working, but it does not
verify session revocation after AuthStore.updatePassword. Create a session
before calling updatePassword, then assert that the previously created session
can no longer be resolved afterward while the new password still authenticates
successfully; use the existing AuthStore, createUser, updatePassword,
authenticate, and getSession paths to lock in the session invalidation behavior.
---
Nitpick comments:
In `@sdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardClient.java`:
- Around line 82-90: The hardcoded default password in DashboardClient.seedUser
creates hidden coupling with tests that assume the same literal, so make the
dependency explicit by introducing a shared constant or an overload that accepts
the initial password and use that from seedAdmin/seedUser. Update
DashboardAuthTest.changePasswordRotatesCredential to reference the shared
default instead of an inline literal, so future password changes only need to
happen in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3604b6ca-3e40-455d-9177-e01748a24573
📒 Files selected for processing (31)
sdks/java/src/main/java/org/byteveda/taskito/cli/Cli.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/DashboardServer.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/api/Contract.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/api/CoreHandlers.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/api/package-info.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/AuthHandlers.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/AuthStore.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/Cookies.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/PasswordHasher.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/Policy.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/RequestContext.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/Session.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/TokenAuth.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/Tokens.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/User.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/package-info.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/routing/Handler.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/routing/Req.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/routing/Router.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/routing/package-info.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/store/SettingsAccess.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/store/package-info.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/support/DashboardError.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/support/Http.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/support/Json.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/support/package-info.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardAuthTest.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardClient.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/DashboardTest.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/auth/AuthStoreTest.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/auth/PasswordHasherTest.java
Addresses PR review: revoke a user's sessions on password change; return a generic error code (not raw exception text) on 500 and log the detail; pay a dummy PBKDF2 derivation for OAuth/unknown hashes to equalise timing; thread the secure-cookie flag through the legacy token cookie; synchronize auth:users mutations against in-process lost updates; drop the unimplemented /readiness+/metrics from the public-path set.
Collaborator
Author
|
Addressed all review comments in e18f57a:
Resolving the threads. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdks/java/src/main/java/org/byteveda/taskito/dashboard/DashboardServer.java (1)
91-101: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExecutor is never shut down on
close()and is unbounded.
Executors.newCachedThreadPool()is created here but only assigned to the server;close()(Line 109) callsserver.stop(0), which does not terminate a caller-supplied executor. Its (non-daemon) threads linger and theExecutorServiceis never shut down, so repeated create/close cycles (e.g. across tests) leak threads. The cached pool is also unbounded, which allows thread exhaustion under a request burst.Store the executor as a field and shut it down in
close(), and consider a bounded pool.🔒️ Proposed fix
private final HttpServer server; private final Taskito queue; + private final ExecutorService executor;HttpServer server = HttpServer.create(new InetSocketAddress(port), 0); - DashboardServer dashboard = new DashboardServer(server, queue, dir, secureCookies, token); + ExecutorService executor = Executors.newCachedThreadPool(); + DashboardServer dashboard = new DashboardServer(server, queue, dir, secureCookies, token, executor); // Seed an env admin before serving so no request races the open setup endpoint. if (token == null) { dashboard.authStore.bootstrapAdminFromEnv(); } server.createContext("/", dashboard::dispatch); - server.setExecutor(Executors.newCachedThreadPool()); + server.setExecutor(executor); server.start();`@Override` public void close() { server.stop(0); + executor.shutdownNow(); }(constructor updated to accept and assign the
executorfield.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/java/src/main/java/org/byteveda/taskito/dashboard/DashboardServer.java` around lines 91 - 101, The DashboardServer startup path creates an ExecutorService with Executors.newCachedThreadPool() and hands it to HttpServer, but close() only stops the server and never shuts the executor down. Update DashboardServer to keep the executor as a field, initialize it in the create/startup flow, and explicitly shut it down in close() alongside server.stop(0). Also replace the unbounded cached pool with a bounded ExecutorService to avoid thread exhaustion under load.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@sdks/java/src/main/java/org/byteveda/taskito/dashboard/DashboardServer.java`:
- Around line 91-101: The DashboardServer startup path creates an
ExecutorService with Executors.newCachedThreadPool() and hands it to HttpServer,
but close() only stops the server and never shuts the executor down. Update
DashboardServer to keep the executor as a field, initialize it in the
create/startup flow, and explicitly shut it down in close() alongside
server.stop(0). Also replace the unbounded cached pool with a bounded
ExecutorService to avoid thread exhaustion under load.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33163d2a-9707-45e1-89ea-6a7a3ee3a88d
📒 Files selected for processing (7)
sdks/java/src/main/java/org/byteveda/taskito/dashboard/DashboardServer.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/AuthStore.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/Cookies.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/PasswordHasher.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/Policy.javasdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/TokenAuth.javasdks/java/src/test/java/org/byteveda/taskito/dashboard/auth/AuthStoreTest.java
🚧 Files skipped from review as they are similar to previous changes (6)
- sdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/Policy.java
- sdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/Cookies.java
- sdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/TokenAuth.java
- sdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/PasswordHasher.java
- sdks/java/src/test/java/org/byteveda/taskito/dashboard/auth/AuthStoreTest.java
- sdks/java/src/main/java/org/byteveda/taskito/dashboard/auth/AuthStore.java
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
1 participant
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.
Brings the Java SDK dashboard to auth parity with the other SDKs. This is the first of a stacked series that ports the full dashboard + auth/OAuth surface (ops endpoints, native reads, workflows, OAuth, Spring, webhooks) — each subsequent PR stacks on this one.
What this adds
admin/viewer. All writes are admin-only except self-service (logout, change-password); all reads are allowed for any authenticated user.setup_required503 gate until the first admin is created viaPOST /api/auth/setup; env bootstrap viaTASKITO_DASHBOARD_ADMIN_USER/TASKITO_DASHBOARD_ADMIN_PASSWORD.taskito_session(HttpOnly) +taskito_csrf,SameSite=Strict;--insecure-cookiesdropsSecurefor local HTTP.tokenkeeps the previous behaviour (fixed admin identity, no users/sessions), now also acceptingAuthorization: BearerandX-Taskito-Token.The monolithic
DashboardServeris restructured into layered, acyclic subpackages —support(http/json/errors),routing(route table),store(settings KV seam),auth,api— behind a route table.Behaviour change
The no-token default now requires session auth (first-run setup) instead of serving the API open. Existing
--tokendeployments are unaffected.Tests
36 dashboard tests: PBKDF2 golden vector (verified against an externally-produced 600k hash), auth store, all auth endpoints, RBAC, CSRF, setup gate, cookies. Checkstyle + Spotless clean.
Summary by CodeRabbit
--insecure-cookiesoption for local dashboard use.