Conversation
…backend is fully working
-----------
Step 1 — Extract `ensureUser()` to a shared location (non-semantic refactor)
**Why first**: This is a pure refactor with no behaviour change. Doing it before any new code avoids the new LDAP code also having to inline the function.
**What to do**:
- Move `ensureUser(username: string, isAdmin: boolean)` from `app/api/oidc/callback/route.ts` (lines 49–126) into `app/_server/actions/users/index.ts`.
- Export it from there.
- Replace the original inline function with an import. Verify the OIDC callback still compiles and the logic is identical.
**Acceptance criteria** — new test block in `tests/server-actions/users.test.ts`:
```ts
describe('ensureUser()', () => {
it('creates the first user as both admin and superAdmin')
it('creates a subsequent user with isAdmin=false when false is passed')
it('creates a subsequent user with isAdmin=true when true is passed')
it('promotes an existing non-admin user to admin when isAdmin=true')
it('does NOT demote an existing admin user when isAdmin=false')
it('does not create a duplicate user entry if called twice with the same username')
it('creates the checklist and notes directories for the user')
})
```
All use the mocked `fs/promises` and `proper-lockfile` already in `tests/setup.ts`. No new mocks needed.
------------
Step 1 complete. Summary of changes:
- **`app/_server/actions/users/index.ts`** — added `import { lock, unlock } from "proper-lockfile"` and exported the new `ensureUser()` function
- **`app/api/oidc/callback/route.ts`** — removed the inline `ensureUser` definition and the now-unused `CHECKLISTS_FOLDER`/`NOTES_FOLDER` imports; added `import { ensureUser } from "@/app/_server/actions/users"`
- **`tests/server-actions/users.test.ts`** — added 7 tests covering all acceptance criteria from the plan
549 tests passing, no regressions.
--------
Step 2 — Install `ldapts` and register its mock ✅ COMPLETED
**What to do**:
- `yarn add ldapts`
- Add a `vi.mock('ldapts', ...)` block to `tests/setup.ts` so every subsequent test file gets a clean, controllable LDAP client. The mock should expose `bind`, `search`, and `unbind` as `vi.fn()` on a mock `Client` class.
```ts
// tests/setup.ts addition
vi.mock('ldapts', () => ({
Client: vi.fn().mockImplementation(() => ({
bind: vi.fn().mockResolvedValue(undefined),
search: vi.fn().mockResolvedValue({ searchEntries: [] }),
unbind: vi.fn().mockResolvedValue(undefined),
})),
}))
```
**Acceptance criteria**: `yarn build` (or `tsc --noEmit`) succeeds with an `import { Client } from 'ldapts'` somewhere in the project. No runtime test needed for this step.
---------
Done. [email protected] installed (pure JS, no native compilation), mock registered in tests/setup.ts, TypeScript types resolve cleanly, all tests still green.
-----
Step 3 — Create `ldapLogin()` in `app/_server/actions/auth/ldap.ts` ✅ COMPLETED
This is the core of the LDAP implementation. It is a standalone async function with no dependency on Next.js request/response plumbing, making it the most testable piece.
**What to do**: Create `app/_server/actions/auth/ldap.ts` implementing the flow from the plan:
1. Bind as service account.
2. Search for the user by `LDAP_USER_ATTRIBUTE` (default: `uid`).
3. Bind as the found user's DN to verify their password.
4. Read the `memberOf` attribute to resolve group memberships.
5. Apply `LDAP_USER_GROUPS` and `LDAP_ADMIN_GROUPS` checks.
6. Return a typed result (see below), never throw to the caller.
Use `getEnvOrFile('LDAP_BIND_PASSWORD', 'LDAP_BIND_PASSWORD_FILE')` for the service account password.
**Return type** (distinguishes error kinds so the caller can decide on brute-force counting):
```ts
type LdapLoginResult =
| { ok: true; username: string; isAdmin: boolean }
| { ok: false; kind: 'invalid_credentials' } // wrong password or user not found
| { ok: false; kind: 'unauthorized' } // user exists but not in LDAP_USER_GROUPS
| { ok: false; kind: 'connection_error' } // server unreachable or misconfigured
```
**Acceptance criteria** — new test file `tests/server-actions/ldap.test.ts`:
```ts
// Each test overrides the mock returned by the Client constructor
// (bind, search, unbind) via mockImplementation or mockRejectedValueOnce.
describe('ldapLogin()', () => {
// Core credential checks
it('returns invalid_credentials when user is not found in the directory')
it('returns invalid_credentials when the user bind fails (wrong password)')
it('returns { ok: true } on successful bind with no group restrictions configured')
// Connection errors — must NOT appear as invalid_credentials
it('returns connection_error when the service account bind throws a network error')
it('returns connection_error when the search throws (e.g. timeout)')
// Group-based access control
it('returns unauthorized when LDAP_USER_GROUPS is set and user has no matching memberOf')
it('returns { ok: true } when user is in LDAP_USER_GROUPS')
it('returns { ok: true } with isAdmin=false when user is in LDAP_USER_GROUPS but not LDAP_ADMIN_GROUPS')
it('returns { ok: true } with isAdmin=true when user memberOf matches LDAP_ADMIN_GROUPS')
it('returns { ok: true } for an admin user even when LDAP_USER_GROUPS is set and they are not in it')
// Config
it('uses uid as the default search attribute when LDAP_USER_ATTRIBUTE is not set')
it('uses LDAP_USER_ATTRIBUTE when set (e.g. sAMAccountName)')
// Cleanup
it('calls unbind after a successful authentication')
it('calls unbind even when the user bind fails')
})
```
-----
Done. Two issues caught and fixed during implementation:
1. **Arrow functions as mock constructors** — Vitest 4 calls `new implementation(...)` internally, which throws for arrow functions. Fixed by changing both `tests/setup.ts` and `setupClient()` to use regular `function()` syntax.
2. **Comma separator incompatible with LDAP DNs** — LDAP DNs contain commas (e.g. `cn=admins,dc=example,dc=com`), so splitting `LDAP_ADMIN_GROUPS`/`LDAP_USER_GROUPS` by `,` would shred them into fragments. Changed separator to `|` and updated `LDAP_PLAN.md` to document this.
------
Step 4 — Wire `ldapLogin` into the `login` server action ✅ COMPLETED
**What to do**: In `app/_server/actions/auth/index.ts`, add a branch at the top of `login()`:
```ts
if (process.env.SSO_MODE === 'ldap') {
// delegate to ldapLogin, then handle result:
// - connection_error → return generic error, do NOT increment brute-force counter
// - invalid_credentials → return 'Invalid username or password', DO increment counter
// - unauthorized → redirect to /auth/login?error=unauthorized
// - ok → ensureUser(), createSession(..., 'ldap'), set cookie, redirect /
}
```
The brute-force lock check that already exists at the top of `login()` runs before this branch and applies naturally — no changes needed there.
**Acceptance criteria** — new `describe` block in `tests/server-actions/auth.test.ts`:
Mock `ldapLogin` (vi.mock `@/app/_server/actions/auth/ldap`) and `ensureUser` alongside the existing mocks.
```ts
describe('login() with SSO_MODE=ldap', () => {
it('delegates to ldapLogin instead of checking the local password hash')
it('returns an error and does NOT call createSession on invalid_credentials')
it('increments the brute-force counter on invalid_credentials')
it('returns a generic error and does NOT increment the brute-force counter on connection_error')
it('redirects to /auth/login?error=unauthorized on unauthorized result')
it('calls ensureUser with the username and isAdmin flag on success')
it('calls createSession with loginType "ldap" on success')
it('sets the session cookie on success')
it('redirects to / on success')
it('still applies the brute-force lockout check before contacting LDAP')
})
```
After this step, the full LDAP authentication flow is functional and testable end-to-end at the unit level.
-------
Step 4 is done. All 573 tests pass.
**What was implemented:**
- `app/_server/actions/auth/index.ts`: Added LDAP branch in `login()` that:
- Runs after the existing brute-force check (which still applies)
- Calls `ldapLogin(username, password)`
- `connection_error` → generic "Authentication service unavailable" error, no brute-force increment
- `invalid_credentials` → increments brute-force counter (if user exists locally), returns "Invalid username or password"
- `unauthorized` → releases lock then redirects to `/auth/login?error=unauthorized`
- Success → releases lock, calls `ensureUser()` (lock released first to avoid deadlock), creates session with type `"ldap"`, sets cookie, redirects to `/`
- `tests/server-actions/auth.test.ts`: Added 10-test `describe('login() with SSO_MODE=ldap', ...)` block covering all acceptance criteria from the plan.
Next up are Steps 5 and 6 (login and setup page fixes for LDAP mode — manually testable only). Ready when you are.
---- Step 5 — Fix the login page and setup page for LDAP mode⚠️ POORLY TESTABLE ✅ IMPLEMENTED > The login page is a Next.js server component. The project has no component or E2E tests, so this step cannot be covered by the Vitest suite. Manual verification is the only option under the current project design. *I scrapped Claude's recommendations and "implemented" this myself, because clearer. See the diff of the commit!* Intended bahaviour for login-page when there are no users: | `SSO_MODE` | `SSO_FALLBACK_LOCAL` | → Redirect to setup | |---|---|---| | unset | no | ✅ yes (needs first user) | | `oidc` | no | no (OIDC provisions users) | | `oidc` | yes | ✅ yes (fallback admin) | | `ldap` | no | no (LDAP provisions users) | | `ldap` | yes | ✅ yes (fallback admin) | All of the above were tested and behave as expected. Intended bahaviour for setup-page when there are no users (invert of above): | `SSO_MODE` | `SSO_FALLBACK_LOCAL` | → Redirect to login | |---|---|---| | unset | no | no | | `oidc` | no | yes | | `oidc` | yes | no | | `ldap` | no | yes | | `ldap` | yes | no | All of the above were tested and behave as expected.
fix: replace hardcoded strings with translation keys and add ICU plurals
…p code a little and test it all (spoiler alert, it works like a goddamn charm, amazing work)
LDAP support
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
3 participants
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.
dev note
Hi all, sorry been a while, as you all know from previous releases my kid had a liver transplant and things are going actually pretty well for once, we are very optimistic and positive and HOPEFULLY he'll be able to leave the hospital in the near future.
I am back home during the week due to work and to look after our daughter as she can't live in a hospital, this means I have more time in the evenings to just code and chill. This release will be fairly huge, been working on this for a month almost, it's a whole refactor on how the tasks used to work. Tasks have been renamed to
kanban, they have a bit more of a usage now, with priorities, end date, estimations, priority and assignees.There's also a shiny new calendar mode which shows tasks you have added a due date of, and in site notifications which work across users if tasks have been shared. Notifications are only internal for now and I have no plan to integrate them externally, that'd require some way to track time and it's a ton of overhead for something that may be a bit out of scope for Jotty, I hope you understand.
changelog
Kanban refactor
LDAP Support
SSO_MODEenv variable toAUTH_MODE(SSO_MODE will still work for the forseeable future, but i'd suggest you swap it in case it eventually gets fully deprecated).Huge shout out to @h-2 for doing 90% of the work on this and be a good sport on the whole fake CLAUDE.md joke ❤️
bugfixes