feat: User operations on join org
This commit is contained in:
SauravDhakal
389
CODE_REVIEW.md
Normal file
389
CODE_REVIEW.md
Normal file
@@ -0,0 +1,389 @@
|
||||
# Code Review — Kaa Khane (Multi-Tenant SaaS Backend)
|
||||
|
||||
## What This Is
|
||||
|
||||
A NestJS backend for a multi-tenant SaaS platform. It models the core of any team-based product: users belong to organizations, organizations have roles (`owner`, `admin`, `member`), and membership is managed through a request/invitation flow. Stack: NestJS + Prisma + PostgreSQL + Redis (via Keyv). This is a learning project but the ambition is real — you've touched AsyncLocalStorage, operation-based authorization, cache resilience, and transactional writes. That's not beginner stuff.
|
||||
|
||||
---
|
||||
|
||||
## What You're Doing Well
|
||||
|
||||
**1. AsyncLocalStorage for request context**
|
||||
This is a legitimately good pattern. Instead of threading `req.user` through every function signature, you store it in ALS and services pull it from context. This is how production NestJS apps at scale handle per-request state. Most people learning NestJS never get here.
|
||||
|
||||
**2. Cache-aside with graceful degradation**
|
||||
`CacheService.getOrSet()` is well thought out. It tracks `redisAvailable`, silently falls back to DB, and doesn't let a Redis outage crash the app. The pattern is correct. The placement of `console.warn` in a service is slightly off (more on that below) but the intent is solid.
|
||||
|
||||
**3. Parallel DB checks with `Promise.all`**
|
||||
You're doing this in multiple places — running independent queries concurrently instead of awaiting them in sequence. This is the right instinct and most junior devs miss it.
|
||||
|
||||
**4. Operation-based authorization**
|
||||
`AuthorizationService.canPerformOperation()` with a switch statement and named operations (`INVITE_USERS`, `DELETE_ORGANIZATION`) is cleaner than scattering raw role checks everywhere. It's a solid foundation.
|
||||
|
||||
**5. Transactions where they matter**
|
||||
`createNewOrganization` and `userOrganiaztionRequestAction` use `$transaction` to ensure atomicity. You understand when to use them.
|
||||
|
||||
**6. DTO validation layer**
|
||||
Using `class-validator` + `@nestjs/swagger` together is standard and correct. `@AtLeastOneField()` is a good custom validator — you're not reaching for a library when a simple decorator does the job.
|
||||
|
||||
**7. Global `AuthGuard` with `@Public()` opt-out**
|
||||
Secure by default. Making all routes protected and requiring explicit `@Public()` to open them is the right model. A lot of apps do it backwards.
|
||||
|
||||
---
|
||||
|
||||
## Issues and What to Improve
|
||||
|
||||
Organized by severity: `[critical]`, `[high]`, `[medium]`, `[low]`.
|
||||
|
||||
---
|
||||
|
||||
### 1. Hardcoded JWT secret `[critical]`
|
||||
|
||||
**File:** `src/auth/auth.service.ts:60`, `src/auth/guards/auth.guard.ts:38`
|
||||
|
||||
```ts
|
||||
secret: 'demo';
|
||||
```
|
||||
|
||||
You know this is wrong (you left a TODO). But let me explain _why_ it's critical: if this ever reaches a staging environment and someone rotates the secret, every user is logged out. Worse, the secret is in version control. Fix this before you add anything else.
|
||||
|
||||
**Fix:**
|
||||
|
||||
```ts
|
||||
// in JwtModule registration (auth.module.ts)
|
||||
JwtModule.registerAsync({
|
||||
inject: [ConfigService],
|
||||
useFactory: (config: ConfigService) => ({
|
||||
secret: config.getOrThrow<string>('JWT_SECRET'),
|
||||
signOptions: { expiresIn: '15m' },
|
||||
}),
|
||||
}),
|
||||
```
|
||||
|
||||
Then remove the `secret` override from every `signAsync` / `verifyAsync` call. The module-level secret is used automatically.
|
||||
|
||||
---
|
||||
|
||||
### 2. `userOrganiaztionRequestAction` uses `this.prisma` inside a `$transaction` `[critical]`
|
||||
|
||||
**File:** `src/organization-membership/organization-membership.service.ts:226`
|
||||
|
||||
```ts
|
||||
return await this.prisma.$transaction(async (tx) => {
|
||||
await tx.organizationJoinRequest.update(...) // ✅ uses tx
|
||||
|
||||
if (userAction === ORGANIZATION_JOIN_REQUEST.ACCEPTED)
|
||||
await this.prisma.organizationUserJoinTable.create(...) // ❌ uses this.prisma — NOT the tx
|
||||
})
|
||||
```
|
||||
|
||||
The `create` inside the transaction is using `this.prisma` (the global client) instead of `tx` (the transaction client). If the second write fails, the first write is NOT rolled back. The whole point of wrapping in `$transaction` is broken here.
|
||||
|
||||
**Fix:**
|
||||
|
||||
```ts
|
||||
await tx.organizationUserJoinTable.create({ ... })
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 3. Two flows conflated into one method — and the semantics are wrong `[high]`
|
||||
|
||||
**File:** `src/organization-membership/organization-membership.service.ts:180`
|
||||
|
||||
`userOrganiaztionRequestAction` is currently called by the user (the invitee) to accept or reject an invitation. But based on your plan, you also want an org admin to accept/reject a join request. These are fundamentally different actors:
|
||||
|
||||
| Flow | Actor | What they act on |
|
||||
| -------------------------- | ---------------------- | ---------------------------------- |
|
||||
| Accept/reject invitation | The invited **user** | Their own pending `INVITED` record |
|
||||
| Accept/reject join request | An **org admin/owner** | Someone else's `REQUESTED` record |
|
||||
|
||||
Right now the method doesn't distinguish by `requestType`. An org admin could use it to act on _their own_ data when they should be acting on _another user's_ data. They need to be two methods:
|
||||
|
||||
- `userRespondToInvitation(userId, dto)` — user accepts/rejects where `requestType = INVITED`
|
||||
- `adminRespondToJoinRequest(adminUserId, targetUserId, orgId, action)` — admin acts on `requestType = REQUESTED`, with a `canPerformOperation` check first
|
||||
|
||||
This is not a minor naming issue. It's a modeling issue. The current code would let a user "accept" their own join request.
|
||||
|
||||
---
|
||||
|
||||
### 4. `organizationExists` vs `findById` — two methods doing nearly the same thing `[high]`
|
||||
|
||||
**File:** `src/organization/organization.service.ts:121`
|
||||
|
||||
```ts
|
||||
async organizationExists(orgId: string) {
|
||||
return await this.prisma.organization.findUnique({
|
||||
where: { id: orgId },
|
||||
select: { id: true }, // minimal select — just checking existence
|
||||
});
|
||||
}
|
||||
|
||||
async findById(orgId: string) {
|
||||
return await this.cacheService.getOrSet(orgId, async () =>
|
||||
await this.prisma.organization.findUnique({ where: { id: orgId } }),
|
||||
);
|
||||
// returns full org object, goes through cache
|
||||
}
|
||||
```
|
||||
|
||||
These have different purposes (one is a lightweight existence check, one returns full data with caching) but the distinction is opaque from the call site. `OrganizationMembershipService` calls both, inconsistently.
|
||||
|
||||
More importantly: `organizationExists` bypasses the cache entirely. If you call `findById` first (which caches), then call `organizationExists`, you've now made an extra DB round-trip for no reason.
|
||||
|
||||
**What to do:**
|
||||
|
||||
- Consolidate. Use `findById` (cached) everywhere.
|
||||
- If you need a boolean check, add a thin wrapper: `async assertOrganizationExists(orgId)` that calls `findById` and throws `NotFoundException` if null. Then services call that one line instead of the two-line check-then-throw pattern you repeat everywhere.
|
||||
- Delete `organizationExists` or make it call `findById` internally.
|
||||
|
||||
---
|
||||
|
||||
### 5. Repeated guard pattern — check existence, throw — everywhere `[high]`
|
||||
|
||||
You do this in almost every method:
|
||||
|
||||
```ts
|
||||
const orgExists = await this.orgService.findById(dto.orgId);
|
||||
if (!orgExists) throw new NotFoundException('Organization');
|
||||
```
|
||||
|
||||
And separately:
|
||||
|
||||
```ts
|
||||
const canUserDeleteOrganization = await this.authorization.canPerformOperation(...);
|
||||
if (!canUserDeleteOrganization) throw new ForbiddenException('Not enough permission');
|
||||
```
|
||||
|
||||
This is the "guard clause" pattern and the repetition itself isn't wrong — guard clauses are good. What's wrong is that the _authorization check_ is manually wired in every service method instead of being declarative. This will become painful as you add more operations.
|
||||
|
||||
**Better pattern — make `canPerformOperation` throw directly:**
|
||||
|
||||
```ts
|
||||
// authorization.service.ts
|
||||
async assertCanPerformOperation(
|
||||
userId: string,
|
||||
orgId: string,
|
||||
operation: USER_ORGANIZATION_OPERATIONS,
|
||||
): Promise<void> {
|
||||
const allowed = await this.canPerformOperation(userId, orgId, operation);
|
||||
if (!allowed) throw new ForbiddenException('Insufficient permissions');
|
||||
}
|
||||
```
|
||||
|
||||
Now the call site is one line with no `if`:
|
||||
|
||||
```ts
|
||||
await this.authorization.assertCanPerformOperation(userId, orgId, INVITE_USERS);
|
||||
```
|
||||
|
||||
Clean. Unambiguous. And you can add logging, metrics, or audit trail in one place.
|
||||
|
||||
---
|
||||
|
||||
### 6. `isAdmin()` is dead code `[medium]`
|
||||
|
||||
**File:** `src/authorization/authorization.service.ts:49`
|
||||
|
||||
`isAdmin()` is defined but never called. Either use it in `canInvite` (which currently re-implements the same logic inline) or delete it.
|
||||
|
||||
```ts
|
||||
// current canInvite re-implements admin check inline:
|
||||
return (
|
||||
isUserPartOfOrganization.role === ORG_ROLE.admin ||
|
||||
isUserPartOfOrganization.role === ORG_ROLE.owner
|
||||
);
|
||||
|
||||
// isAdmin() does the same thing but only for admin role.
|
||||
// Clean it up or wire it in.
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 7. `AuthorizationService` hits the DB every time — no caching `[medium]`
|
||||
|
||||
**File:** `src/authorization/authorization.service.ts:61`
|
||||
|
||||
Every call to `canPerformOperation` does a `findUnique` on `OrganizationUserJoinTable`. In `inviteUserToOrg` alone, you've already called `orgService.findById` (which hits cache), but then `canPerformOperation` hits DB cold. Membership data is relatively stable — it changes when someone joins, leaves, or is promoted.
|
||||
|
||||
**Fix:** Cache the membership record in `CacheService` using a compound key like `membership:{userId}:{orgId}`. Invalidate it when a member joins, leaves, or their role changes.
|
||||
|
||||
```ts
|
||||
const key = `membership:${userId}:${orgId}`;
|
||||
return this.cacheService.getOrSet(key, () =>
|
||||
this.prisma.organizationUserJoinTable.findUnique(...)
|
||||
);
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 8. `deleteAnOrganization` has broken cascade logic `[medium]`
|
||||
|
||||
**File:** `src/organization/organization.service.ts:99`
|
||||
|
||||
```ts
|
||||
await tx.organization.delete({ where: { id: orgId } });
|
||||
|
||||
await tx.organizationUserJoinTable.delete({
|
||||
where: { userId_orgId: { userId, orgId } }, // only deletes the OWNER's row
|
||||
});
|
||||
```
|
||||
|
||||
This only deletes the owner's row from the join table. All other members' rows are left orphaned (or fail with a FK constraint — depending on your Prisma schema cascade setting). Also, `OrganizationJoinRequest` records for this org are not cleaned up.
|
||||
|
||||
You should either:
|
||||
|
||||
- Set `onDelete: Cascade` in the Prisma schema on those relations (so deleting the org auto-cleans everything), or
|
||||
- Manually `deleteMany` all join rows and requests in the transaction before deleting the org
|
||||
|
||||
Option 1 (schema cascade) is cleaner for this case.
|
||||
|
||||
---
|
||||
|
||||
### 9. `console.log` / `console.warn` in production code `[medium]`
|
||||
|
||||
**Files:** `src/cache/cache.service.ts:13,18`, `src/cache/cache.service.ts:62`, `src/prisma/prisma.service.ts:40`
|
||||
|
||||
```ts
|
||||
console.log(a); // in deleteKey — what is `a`?
|
||||
console.warn('Redis disconnected');
|
||||
```
|
||||
|
||||
Use NestJS's built-in `Logger`:
|
||||
|
||||
```ts
|
||||
import { Logger } from '@nestjs/common';
|
||||
|
||||
private readonly logger = new Logger(CacheService.name);
|
||||
|
||||
this.logger.warn('Redis disconnected');
|
||||
this.logger.error('Prisma connection failed', err.stack);
|
||||
```
|
||||
|
||||
The NestJS logger respects log levels, outputs structured logs, and can be replaced with a production logger (Pino, Winston) without touching service code.
|
||||
|
||||
---
|
||||
|
||||
### 10. Controller stubs are wired wrong — copy-paste bug `[medium]`
|
||||
|
||||
**File:** `src/organization-membership/organization-membership.controller.ts:54`
|
||||
|
||||
```ts
|
||||
@Get('organization/:id/invitations')
|
||||
async getOrganizationInvitations(@Param('id') orgId: string) {
|
||||
return await this.orgMemService.getMemebersOfOrganization(orgId); // ❌ wrong method
|
||||
}
|
||||
```
|
||||
|
||||
`getOrganizationInvitations` is calling `getMemebersOfOrganization`. Both endpoints return the same data. This is a copy-paste leftover.
|
||||
|
||||
---
|
||||
|
||||
### 11. `userOrganizationJoinRequestList` does manual enum validation `[low]`
|
||||
|
||||
**File:** `src/organization-membership/organization-membership.service.ts:135`
|
||||
|
||||
```ts
|
||||
async userOrganizationJoinRequestList(userId: string, requestType: string) {
|
||||
const joinReqType: ORGANIZATION_JOIN_REQUEST_TYPE | undefined =
|
||||
ORGANIZATION_JOIN_REQUEST_TYPE[requestType];
|
||||
if (!joinReqType) throw new BadRequestException('Invalid request type');
|
||||
```
|
||||
|
||||
This manual enum validation in the service layer exists because `requestType` comes in as a raw `string`. The fix is to validate it in the DTO/query params using `@IsEnum(ORGANIZATION_JOIN_REQUEST_TYPE)`. Then the service never receives an invalid value — NestJS's validation pipe rejects it before it gets there. The service layer should not be doing input validation.
|
||||
|
||||
---
|
||||
|
||||
### 12. `BaseException` exists but is never used `[low]`
|
||||
|
||||
**File:** `common/exceptions/custom-exceptions.ts`
|
||||
|
||||
You built a `BaseException` class with `code`, `message`, and `HttpStatus`. Nothing uses it. Either start using it for domain-specific errors (e.g., `OrganizationNotFoundError extends BaseException`) or delete it. Dead abstractions add noise.
|
||||
|
||||
---
|
||||
|
||||
### 13. Typos in method and variable names `[low]`
|
||||
|
||||
- `userOrganiaztionRequestAction` → `userOrganizationRequestAction`
|
||||
- `getMemebersOfOrganization` → `getMembersOfOrganization`
|
||||
|
||||
These propagate to controller method names, controller routes, and service interface. Fix them now while the surface area is small.
|
||||
|
||||
---
|
||||
|
||||
## General Guidelines to Internalize
|
||||
|
||||
### On DRY vs. "right abstraction"
|
||||
|
||||
DRY (Don't Repeat Yourself) is often misapplied. The goal isn't zero duplication — it's avoiding duplication of _knowledge_ (the same decision made in two places). Two methods that both check `if (!org) throw NotFoundException` are DRY violations only if they're checking the same _thing_. The right fix is `assertOrganizationExists()` — a single function that owns the "org must exist" rule. The callers delegate to it.
|
||||
|
||||
### On service layer responsibilities
|
||||
|
||||
A service method should do one thing: execute a business operation. Input validation belongs in DTOs + pipes. Authorization belongs in guards or a dedicated authorization service (you're halfway there). Existence checks belong in helper methods on the service. What remains in the method body should read like a business narrative: "check user is allowed → do the thing → return result."
|
||||
|
||||
### On the transaction client (`this.prisma.client`)
|
||||
|
||||
You built the ALS-based `prisma.client` getter for shared transactions across services. It's a good idea — but it only works if services call `this.prisma.client.xxx` instead of `this.prisma.xxx`. Right now no service uses it. Either commit to the pattern (use `client` everywhere) or remove the getter to avoid confusion. Half-implemented patterns are worse than no pattern — they create false confidence.
|
||||
|
||||
### On authorization: where you are vs. where to go
|
||||
|
||||
**Where you are:** operation-based (`canPerformOperation` switch). This is fine for 3-5 operations. As the app grows, the switch becomes unmaintainable and you end up with logic spread across the switch, `isOwner`, `canInvite`, etc.
|
||||
|
||||
**Where to go next:** a role-permission matrix.
|
||||
|
||||
```ts
|
||||
const PERMISSIONS: Record<ORG_ROLE, USER_ORGANIZATION_OPERATIONS[]> = {
|
||||
[ORG_ROLE.owner]: [
|
||||
UPDATE_ORGANIZATION,
|
||||
DELETE_ORGANIZATION,
|
||||
INVITE_USERS,
|
||||
REMOVE_MEMBERS,
|
||||
],
|
||||
[ORG_ROLE.admin]: [INVITE_USERS, REMOVE_MEMBERS],
|
||||
[ORG_ROLE.member]: [],
|
||||
};
|
||||
```
|
||||
|
||||
Then `canPerformOperation` becomes:
|
||||
|
||||
```ts
|
||||
async canPerformOperation(userId, orgId, operation) {
|
||||
const membership = await this.getMembership(userId, orgId);
|
||||
if (!membership) return false;
|
||||
return PERMISSIONS[membership.role].includes(operation);
|
||||
}
|
||||
```
|
||||
|
||||
Adding a new operation is one line in the matrix. No new DB queries, no new switch case.
|
||||
|
||||
**Beyond that:** [CASL](https://casl.js.org/) is the standard Node.js authorization library for more complex scenarios (attribute-level permissions, "can user edit _this specific_ resource"). You don't need it now but it's worth knowing it exists.
|
||||
|
||||
### On caching: what to cache, what to invalidate
|
||||
|
||||
Cache data that is: (a) read frequently, (b) expensive to recompute, (c) stable. Organization data and membership data fit all three. The rule is simple: **every write must invalidate or update the relevant cache key**. Right now you invalidate on org delete but not on org update. When you cache membership, you must invalidate on role change, member removal, and member addition.
|
||||
|
||||
Use structured cache keys: `org:{orgId}`, `membership:{userId}:{orgId}`. Bare IDs as keys (like you're doing now with just `orgId`) work but collide if you ever cache different types under the same store.
|
||||
|
||||
### On the two-flow membership design (your plan)
|
||||
|
||||
Your instinct to split into two separate flows is correct. Model it explicitly:
|
||||
|
||||
- `POST /organization-membership/invite` — org admin invites a user (creates `INVITED` record)
|
||||
- `POST /organization-membership/request` — user requests to join (creates `REQUESTED` record)
|
||||
- `PATCH /organization-membership/user/respond` — **user** accepts/rejects their own `INVITED` record
|
||||
- `PATCH /organization-membership/organization/:id/respond` — **admin** accepts/rejects a `REQUESTED` record
|
||||
|
||||
The actor and the target record type are different in each case. Wire your service methods to match this — each method should assert `requestType` before acting.
|
||||
|
||||
### On what to build next (priority order)
|
||||
|
||||
1. Fix the `$transaction` bug (item 2 above) — data integrity issue
|
||||
2. Split the two membership flows into two methods
|
||||
3. Move JWT secret to `ConfigService`
|
||||
4. Add `assertOrganizationExists()` / `assertCanPerformOperation()` helpers — this cleans up every service method
|
||||
5. Fix the cascade on org delete
|
||||
6. Wire up the remaining controller stubs
|
||||
7. Add pagination to `getOrganizations()`
|
||||
8. Replace `console.log` with `Logger`
|
||||
9. Cache membership in `AuthorizationService`
|
||||
10. Delete dead code (`isAdmin`, `BaseException`, commented-out `acceptInvitation`)
|
||||
Reference in New Issue
Block a user