Add Multi-User Functionality#238
Conversation
thomasb202
commented
May 11, 2026
thomasb202
commented
- Added/Changed Database tables (user and project-membership tables, rls-policies, new DB-Role)
- Changed Backend (jwt, Route protection)
- Minimal Login/Registration page
- Tests
Issue 222
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Added ProjectMembership table Added RLS policies Added new rdb_app role
Added ProjectMembership table Added RLS policies Added new rdb_app role
Implementation of withUserContext (Setting the role in the db to rdb_app for RLS-policies to fire)
# Conflicts: # src/backend/auth/auth-middleware.ts # src/backend/auth/auth-routes.ts # src/backend/db/db.sql # src/index.ts
Adding multi-user functionality
smarr
left a comment
smarr
left a comment
There was a problem hiding this comment.
Thanks! Overall this looks pretty good!
Added some initial thoughts for our discussion.
| import { Next, ParameterizedContext } from 'koa'; | ||
| import jwt from 'jsonwebtoken'; | ||
|
|
||
| const JWT_SECRET = process.env.JWT_SECRET || ''; |
There was a problem hiding this comment.
It would be good if we do not scatter accesses to process.env throughout the codebase.
I'd prefer if we could make it part of the siteConfig, I think:
https://github.com/smarr/ReBenchDB/blob/master/src/backend/util.ts#L103
This isn't super principled at the moment, some things are just exported properties. But appId and githubPrivateKey data go into it, so it seems as if this would be a good fit.
There was a problem hiding this comment.
This is a good idea, I will implement it like this.
| const JWT_SECRET = process.env.JWT_SECRET || ''; | ||
| const BCRYPT_ROUNDS = 12; |
There was a problem hiding this comment.
Both of these, I think should be in the siteConfig
| return; | ||
| } | ||
|
|
||
| if (typeof password !== 'string' || password.length < 8) { |
There was a problem hiding this comment.
Since you use bcrypt, we should also check for the largest supported passwords. It's 72 bytes according to https://en.wikipedia.org/wiki/Bcrypt
| const token = jwt.sign( | ||
| { sub: user.id, username: user.username }, | ||
| JWT_SECRET, | ||
| { expiresIn: '24h' } |
There was a problem hiding this comment.
Ok, I think that's probably a useful default.
Though, I think we had a discussion of how we may want to handle submitting data from benchmark servers. There we probably want a token that is valid for a long time, possibly until it is revoked. I believe you said you want to use the same approach? I wonder whether this require some extra support here, or whether that's going to be handled separately.
| -- ============================================================ | ||
|
|
||
| -- Project: direct membership check | ||
| CREATE POLICY project_access ON Project |
There was a problem hiding this comment.
camelCase also for the various other names please, for consistency
| @@ -0,0 +1,89 @@ | |||
| BEGIN; | |||
There was a problem hiding this comment.
hm, why do you have multiple migration scripts here?
There was a problem hiding this comment.
I had a first script that does not enforce RLS-policies to not completely break the application while implementing. But would be good now to combine them into one script.
| foreign key (criterion) references Criterion (id) | ||
| ); | ||
|
|
||
| -- ============================================================ |
There was a problem hiding this comment.
Please use a style that matches the rest of the file, so no ==...== lines, just plain -- comment comments. And camelCase for consistency please.
Not sure whether there's a linter and/or code formatter, but would be nice to also use whitespace consistent with the existing code.
| * check in policies — use only for internal/background operations). | ||
| */ | ||
| public abstract withUserContext<T>( | ||
| userId: number | null, |
There was a problem hiding this comment.
hm, why do you allow null for userId? Isn't that against the idea?
|
|
||
| public async withUserContext<T>( | ||
| _userId: number | null, | ||
| fn: () => Promise<T> | ||
| ): Promise<T> { | ||
| return fn(); | ||
| } |
There was a problem hiding this comment.
Hm, doesn't this mean that we do not test the security stuff?
2 participants
