MultiTenantSaaS/CODE_REVIEW.md

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

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:

// 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

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:

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

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:

const orgExists = await this.orgService.findById(dto.orgId);
if (!orgExists) throw new NotFoundException('Organization');

And separately:

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:

// 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:

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.

// 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.

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

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

console.log(a); // in deleteKey — what is `a`?
console.warn('Redis disconnected');

Use NestJS's built-in Logger:

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

@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

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.

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:

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 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)