Conversation
-----------
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.
| export default async function LoginPage() { | ||
| const t = await getTranslations("auth"); | ||
| const ssoEnabled = process.env.SSO_MODE === "oidc"; | ||
| const ssoIsOidc = process.env.SSO_MODE === "oidc"; |
|
|
||
| const hasExistingUsers = await hasUsers(); | ||
| if ((!hasExistingUsers && !ssoEnabled) || (!hasExistingUsers && allowLocal)) { | ||
| if (!hasExistingUsers && (!process.env.SSO_MODE || allowLocal)) { |
There was a problem hiding this comment.
This here means "any SSO_MODE" (including LDAP). I don't know if you prefer some central variable over this env check.
app/_server/actions/auth/index.ts
Outdated
|
|
||
| /** | ||
| * 🧙♂️ | ||
| * �♂️ |
There was a problem hiding this comment.
The wizard must be put back, dunno why your linter decided it couldn't be rendered. I'll fix it when I pull the PR 😆
You may or may not have noticed a bunch of troll comments around the codebase, I get bored and have to leave stupid stuff around. Try searching the code for @fccview here 😆
app/_server/actions/auth/index.ts
Outdated
|
|
||
| const usersFile = path.join(process.cwd(), "data", "users", "users.json"); | ||
| await lock(usersFile); | ||
| // fccview is onto you! |
There was a problem hiding this comment.
I'll sort it out when i finish this up
app/_server/actions/auth/index.ts
Outdated
| } | ||
|
|
||
| // invalid_credentials — increment brute-force counter if user exists locally | ||
| if (user && !bruteforceProtectionDisabled) { |
There was a problem hiding this comment.
There is only bruteforce protection for local users, because we don't write to the ldap.
howto/LDAP.md
Outdated
|
|
||
| ### Required | ||
|
|
||
| - `SSO_MODE=ldap` — Enables LDAP authentication. The standard username/password form is used; the OIDC "Sign in with SSO" button is not shown. |
There was a problem hiding this comment.
I have reused the environment variable, because it was the obvious thing to do, but of course LDAP is not "SSO" strictly speaking, so some general term might have been better.
| const nextConfig = { | ||
| output: "standalone", | ||
| serverExternalPackages: ["ws"], | ||
| serverExternalPackages: ["ws", "libsodium-wrappers-sumo"], |
There was a problem hiding this comment.
This one is apparently neccessary on Linux/arm.
| "dompurify": "^3.3.0", | ||
| "hast-util-to-html": "^9.0.5", | ||
| "hugeicons-react": "^0.3.0", | ||
| "hugeicons-react": "0.3.0", |
There was a problem hiding this comment.
This took me super-long to debug. ^0.3.0 results in 0.3.1 (if not using frozen lockfile), but 0.3.1 is broken and contains no content. 0.4.0 is not broken, but also not compatible 🤯
| "js-beautify": "^1.15.4", | ||
| "js-yaml": "^4.1.0", | ||
| "jsonwebtoken": "^9.0.2", | ||
| "ldapts": "^8.1.7", |
There was a problem hiding this comment.
This is the dependency that actually does the LDAP
| "lodash-es": "^4.17.23", | ||
| "lodash": "^4.17.23", | ||
| "nanoid": "^3.3.8", | ||
| "**/mermaid": "^11.12.1", |
There was a problem hiding this comment.
yarn-4.1 removed this on first start
There was a problem hiding this comment.
I'll look into it, may not be necessary anymore
|
|
||
| - Changing a user's password or deleting a user in jotty's admin panel does **not** propagate to the LDAP server. | ||
| - Changing a password in jotty's personal settings has no effect on LDAP login — the user always authenticates against LDAP. | ||
| - Deleting a user in jotty removes their local notes and checklists, but does not remove them from the directory. |
There was a problem hiding this comment.
I think the above three points are actually sub-optimal UX, but I didn't want to change this, since it's this way for OIDC, too. And I wanted to keep this PR minimal.
If you like, I could prepare another PR at some point which hides the respective options during sessions that are not local.
There was a problem hiding this comment.
We can play with this afterwards, that behaviour always bothered me anyway, but let's get this ready to be merged first
|
Loved the comments, thank you for going through the diffs before I get a chance to do so myself. I agree the naming of SSO_MODE is not great, we may need to change that to AUTH_MODE for future users and leave a fallback to SSO_MODE for legacy, should be easy enough to achieve, we remove SSO_MODE from the documentation and add a I will need to set up LDAP and play around with it all, but this is GREAT stuff overall, thank you so much! |
Glad you appreciate it, and no rush of course! I just had a day off and wanted to see whether I could contribute something :)
👍🏻
I recommend https://github.com/lldap/lldap , it's a lot simpler than most other LDAP servers!
Thank you for publishing this useful service and good luck with the family situation 🍀 |
|
Alright, tomorrow I'll be merging #461 , been working on it all day today! I finally have a bit more time ❤️ |
…p code a little and test it all (spoiler alert, it works like a goddamn charm, amazing work)
|
This is AWESOME! Ldap is pretty damn cool, not gonna lie, and this implementation does make a lot of sense, thank you for taking the time to get the ball rolling. Merging! |
|
Thank you for taking the time to merge this; I am glad it's useful! |
2 participants
Adds LDAP support. Closes #453
I know feelings are mixed about AI contributions, so I accept if this does not get priority.
The individual commits are separate steps and contain the AI instructions in their description, so I recommend reviewing commit-by-commit.
Since I needed to rebase, a bunch of manuel smaller changes were later cherry-picked into the commits, though.
supersedes #471 and #472