Feat/auth provider interface#266
Conversation
release 1.0.2
…voir purgeProject
Documents the AuthProvider interface, ProviderRegistry, database schema, session model, and a worked end-to-end example of a minimal custom provider. Closes logtide-dev#215.
|
Giustino Gragnaniello seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Resolves conflicts in projects restore flow: incorporates upstream name/slug conflict pre-check in restoreProject() and corresponding 409 error handlers in the restore route.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Polliog
left a comment
There was a problem hiding this comment.
Two findings from reviewing the auth-provider routing changes. The first is a security regression I'd like fixed before merge; the second is a minor robustness note.
1. User enumeration: disabled-account status is now disclosed without a valid password (blocking).
2. Non-atomic registration (minor, non-blocking).
Inline details below.
| error: 'Internal server error', | ||
| }); | ||
| } | ||
| const { user, session } = await authenticationService.authenticateWithProvider('local', parsedBody); |
There was a problem hiding this comment.
Routing local login through LocalProvider.authenticate changes the order of the security checks and introduces a user-enumeration issue.
usersService.login verified the password (service.ts:173) before checking disabled (service.ts:179), so a disabled account with a wrong password returned Invalid email or password, indistinguishable from a non-existent user. LocalProvider.authenticate checks disabled (local-provider.ts:65) before bcrypt.compare (local-provider.ts:74), so a disabled account now returns This account has been disabled for any password. That message is sent to the client at routes.ts:162 and shown verbatim in the login UI, so anyone can probe which emails belong to disabled accounts without knowing the password. Skipping bcrypt for disabled accounts also adds a timing side-channel.
Two things that make this easy to miss: usersService.login no longer has any production callers after this change (the safe ordering now lives only in dead code), and the existing disabled-account tests (users-service.test.ts:258, local-provider.test.ts:114) both use the correct password, so they pass under either ordering.
Please restore the old ordering in LocalProvider.authenticate (run bcrypt.compare before the password_hash/disabled checks); the post-auth disabled re-check in findOrCreateUser (authentication-service.ts:260) still prevents a disabled user from getting a session. Please also add a test for 'disabled account + wrong password -> Invalid email or password' to lock the behavior in.
| const session = await usersService.login({ | ||
| // Automatically log in the new user through the provider registry | ||
| // This also creates the user_identities link for the local provider | ||
| const { session } = await authenticationService.authenticateWithProvider('local', { |
There was a problem hiding this comment.
Minor / non-blocking: createUser commits the user in its own transaction (service.ts:115) and does not create the user_identities row; the identity is created only here, during auto-login, which runs outside that transaction. If this call fails for a transient reason (DB or provider-cache error), the user row is already committed but /register returns a 500 (the catch only handles ZodError and already exists), with no session returned. The account is recoverable via a later /login (the identity is backfilled on the next successful auth), so severity is low, but consider catching an auto-login failure here and returning 201 with the created user and no session instead of propagating a 500.
For the record, disabling the local provider is not a viable trigger for this: it is explicitly blocked at provider-service.ts:250.
Summary
Tenant safety
LogTide is multi-tenant. Confirm the following for any new/changed query, endpoint,
or background job (see
docs/security/tenant-isolation-audit.md):organization_id(andproject_idwhere relevant).npm run check:tenant-scopingpasses (run frompackages/backend).Testing