feat: allows user to recover soft-deleted namespaces #2872
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@harshsbhat is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe changes introduce logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant DB
participant Audit
Client->>Server: Request to create namespace (name X)
Server->>DB: Query for soft-deleted namespace with name X
alt Soft-deleted namespace exists
DB-->>Server: Return soft-deleted record
Server->>DB: Begin transaction to restore namespace (set deletedAt = null)
DB-->>Server: Confirm update success
Server->>Audit: Log restoration event
Server-->>Client: Return restored namespace
else Namespace not found or active
Server->>DB: Create new namespace
DB-->>Server: Return new namespace record
Server->>Audit: Log creation event
Server-->>Client: Return new namespace
end
Assessment against linked issues
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Bug
|
Thank you for following the naming conventions for pull request titles! 🙏 |
harshsbhat
requested review from
perkinsjr,
chronark,
mcstepp and
ogzhanolguncu
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts (2)
82-98: Remove commented-out codeThere is a block of commented-out code that appears to be unnecessary. It's generally a good practice to remove unused code to keep the codebase clean and maintainable. If this code is needed for future reference, consider adding a comment explaining its purpose.
165-165: Typo in error messageThe error message has a typo: "We are unable to create namspace." The word "namespace" is misspelled.
Apply this diff to correct the typo:
message: - "We are unable to create namspace. Please try again or contact support@unkey.dev", + "We are unable to create namespace. Please try again or contact support@unkey.dev",
| const isSoftDeleted = await db.query.ratelimitNamespaces | ||
| .findFirst({ | ||
| where: (table, { and, eq }) => and(eq(table.name, input.name)), | ||
| }) |
There was a problem hiding this comment.
Security Issue: Scope namespace queries and updates to the workspace
The current implementation does not scope the namespace queries and updates to the current workspace. This could potentially allow users to access or restore namespaces from other workspaces, leading to a security vulnerability. Please ensure that all namespace queries and updates include a condition to match the workspaceId to ws.id.
Apply these diffs to scope the namespace queries and updates to the workspace:
Lines 36-39:
const isSoftDeleted = await db.query.ratelimitNamespaces
.findFirst({
- where: (table, { and, eq }) => and(eq(table.name, input.name)),
+ where: (table, { and, eq }) => and(eq(table.name, input.name), eq(table.workspaceId, ws.id)),
})
.catch((_err) => {Lines 50-53:
const namespace = await db.query.ratelimitNamespaces
.findFirst({
- where: eq(schema.ratelimitNamespaces.name, input.name),
+ where: and(eq(schema.ratelimitNamespaces.name, input.name), eq(schema.ratelimitNamespaces.workspaceId, ws.id)),
})
.catch((_err) => {Lines 69-73:
await tx
.update(schema.ratelimitNamespaces)
.set({
deletedAt: null,
})
- .where(eq(schema.ratelimitNamespaces.name, input.name))
+ .where(and(eq(schema.ratelimitNamespaces.name, input.name), eq(schema.ratelimitNamespaces.workspaceId, ws.id)))
.catch((_err) => {Also applies to: 50-53, 69-73
| const isSoftDeleted = await db.query.ratelimitNamespaces | ||
| .findFirst({ | ||
| where: (table, { and, eq }) => and(eq(table.name, input.name)), | ||
| }) | ||
| .catch((_err) => { | ||
| throw new TRPCError({ | ||
| code: "INTERNAL_SERVER_ERROR", | ||
| message: | ||
| "We are unable to create a new namespace. Please try again or contact support@unkey.dev", | ||
| }); | ||
| }); | ||
|
|
||
| if (isSoftDeleted) { | ||
| const namespace = await db.query.ratelimitNamespaces | ||
| .findFirst({ | ||
| where: eq(schema.ratelimitNamespaces.name, input.name), | ||
| }) | ||
| .catch((_err) => { | ||
| throw new TRPCError({ | ||
| code: "INTERNAL_SERVER_ERROR", | ||
| message: | ||
| "We were unable to find the namespace. Please try again or contact support@unkey.dev.", | ||
| }); | ||
| }); | ||
| if (!namespace) { | ||
| throw new TRPCError({ | ||
| code: "NOT_FOUND", | ||
| message: | ||
| "We are unable to find the correct namespace. Please try again or contact support@unkey.dev.", | ||
| }); | ||
| } | ||
| await db.transaction(async (tx) => { | ||
| await tx | ||
| .update(schema.ratelimitNamespaces) | ||
| .set({ | ||
| deletedAt: null, | ||
| }) | ||
| .where(eq(schema.ratelimitNamespaces.name, input.name)) | ||
| .catch((_err) => { | ||
| throw new TRPCError({ | ||
| code: "INTERNAL_SERVER_ERROR", | ||
| message: | ||
| "We were unable to update the namespace. Please try again or contact support@unkey.dev.", | ||
| }); | ||
| }); | ||
|
|
||
| // const overrides = await tx.query.ratelimitOverrides.findMany({ | ||
| // where: (table, { eq }) => eq(table.namespaceId, namespace.id), | ||
| // columns: { id: true }, | ||
| // }); | ||
| // if (overrides.length > 0) { | ||
| // await tx | ||
| // .update(schema.ratelimitOverrides) | ||
| // .set({ deletedAt: null }) | ||
| // .where(eq(schema.ratelimitOverrides.namespaceId, namespace.id)) | ||
| // .catch((_err) => { | ||
| // throw new TRPCError({ | ||
| // code: "INTERNAL_SERVER_ERROR", | ||
| // message: | ||
| // "We are unable to recover the namespaces. Please try again or contact support@unkey.dev", | ||
| // }); | ||
| // }); | ||
| // } | ||
|
|
||
| await insertAuditLogs(tx, { | ||
| workspaceId: ws.id, | ||
| actor: { | ||
| type: "user", | ||
| id: ctx.user.id, | ||
| }, | ||
| event: "ratelimitNamespace.create", | ||
| description: `Restored ${namespace.id}`, | ||
| resources: [ | ||
| { | ||
| type: "ratelimitNamespace", | ||
| id: namespace.id, | ||
| }, | ||
| ], | ||
| context: { | ||
| location: ctx.audit.location, | ||
| userAgent: ctx.audit.userAgent, | ||
| }, | ||
| }); | ||
| }); | ||
| return { | ||
| id: namespace.id, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Logic Error: Handle existing namespaces that are not deleted
The current implementation restores any namespace with the given name, without checking if it is already active (i.e., not soft-deleted). This can lead to unexpected behavior if a namespace with the same name already exists and is not deleted. Before attempting to restore a soft-deleted namespace, you should check if an active namespace with the same name already exists in the current workspace. If it does, throw a PRECONDITION_FAILED error indicating that the namespace name is already in use.
Apply this diff to correct the logic:
Modify the initial namespace lookup to include the workspace ID and fetch the deletedAt field:
Lines 36-39:
const existingNamespace = await db.query.ratelimitNamespaces
.findFirst({
+ where: (table, { and, eq }) => and(eq(table.name, input.name), eq(table.workspaceId, ws.id)),
+ columns: {
+ id: true,
+ deletedAt: true,
+ },
})
.catch((_err) => {Then, adjust the logic to handle different cases:
Lines 48-123:
if (existingNamespace) {
+ if (existingNamespace.deletedAt === null) {
+ throw new TRPCError({
+ code: "PRECONDITION_FAILED",
+ message: "Duplicate namespace name. Please use a unique name for each namespace.",
+ });
+ }
- const namespace = await db.query.ratelimitNamespaces
- .findFirst({
- where: eq(schema.ratelimitNamespaces.name, input.name),
- })
- .catch((_err) => {
- throw new TRPCError({
- code: "INTERNAL_SERVER_ERROR",
- message:
- "We were unable to find the namespace. Please try again or contact support@unkey.dev.",
- });
- });
- if (!namespace) {
- throw new TRPCError({
- code: "NOT_FOUND",
- message:
- "We are unable to find the correct namespace. Please try again or contact support@unkey.dev.",
- });
- }
await db.transaction(async (tx) => {
await tx
.update(schema.ratelimitNamespaces)
.set({
deletedAt: null,
})
.where(
and(
eq(schema.ratelimitNamespaces.name, input.name),
eq(schema.ratelimitNamespaces.workspaceId, ws.id)
)
)
.catch((_err) => {
throw new TRPCError({
code: "INTERNAL_SERVER_ERROR",
message:
"We were unable to update the namespace. Please try again or contact support@unkey.dev.",
});
});
await insertAuditLogs(tx, {
workspaceId: ws.id,
actor: {
type: "user",
id: ctx.user.id,
},
event: "ratelimitNamespace.create",
- description: `Restored ${namespace.id}`,
+ description: `Restored ${existingNamespace.id}`,
resources: [
{
type: "ratelimitNamespace",
- id: namespace.id,
+ id: existingNamespace.id,
},
],
context: {
location: ctx.audit.location,
userAgent: ctx.audit.userAgent,
},
});
});
return {
- id: namespace.id,
+ id: existingNamespace.id,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isSoftDeleted = await db.query.ratelimitNamespaces | |
| .findFirst({ | |
| where: (table, { and, eq }) => and(eq(table.name, input.name)), | |
| }) | |
| .catch((_err) => { | |
| throw new TRPCError({ | |
| code: "INTERNAL_SERVER_ERROR", | |
| message: | |
| "We are unable to create a new namespace. Please try again or contact support@unkey.dev", | |
| }); | |
| }); | |
| if (isSoftDeleted) { | |
| const namespace = await db.query.ratelimitNamespaces | |
| .findFirst({ | |
| where: eq(schema.ratelimitNamespaces.name, input.name), | |
| }) | |
| .catch((_err) => { | |
| throw new TRPCError({ | |
| code: "INTERNAL_SERVER_ERROR", | |
| message: | |
| "We were unable to find the namespace. Please try again or contact support@unkey.dev.", | |
| }); | |
| }); | |
| if (!namespace) { | |
| throw new TRPCError({ | |
| code: "NOT_FOUND", | |
| message: | |
| "We are unable to find the correct namespace. Please try again or contact support@unkey.dev.", | |
| }); | |
| } | |
| await db.transaction(async (tx) => { | |
| await tx | |
| .update(schema.ratelimitNamespaces) | |
| .set({ | |
| deletedAt: null, | |
| }) | |
| .where(eq(schema.ratelimitNamespaces.name, input.name)) | |
| .catch((_err) => { | |
| throw new TRPCError({ | |
| code: "INTERNAL_SERVER_ERROR", | |
| message: | |
| "We were unable to update the namespace. Please try again or contact support@unkey.dev.", | |
| }); | |
| }); | |
| // const overrides = await tx.query.ratelimitOverrides.findMany({ | |
| // where: (table, { eq }) => eq(table.namespaceId, namespace.id), | |
| // columns: { id: true }, | |
| // }); | |
| // if (overrides.length > 0) { | |
| // await tx | |
| // .update(schema.ratelimitOverrides) | |
| // .set({ deletedAt: null }) | |
| // .where(eq(schema.ratelimitOverrides.namespaceId, namespace.id)) | |
| // .catch((_err) => { | |
| // throw new TRPCError({ | |
| // code: "INTERNAL_SERVER_ERROR", | |
| // message: | |
| // "We are unable to recover the namespaces. Please try again or contact support@unkey.dev", | |
| // }); | |
| // }); | |
| // } | |
| await insertAuditLogs(tx, { | |
| workspaceId: ws.id, | |
| actor: { | |
| type: "user", | |
| id: ctx.user.id, | |
| }, | |
| event: "ratelimitNamespace.create", | |
| description: `Restored ${namespace.id}`, | |
| resources: [ | |
| { | |
| type: "ratelimitNamespace", | |
| id: namespace.id, | |
| }, | |
| ], | |
| context: { | |
| location: ctx.audit.location, | |
| userAgent: ctx.audit.userAgent, | |
| }, | |
| }); | |
| }); | |
| return { | |
| id: namespace.id, | |
| }; | |
| } | |
| const existingNamespace = await db.query.ratelimitNamespaces | |
| .findFirst({ | |
| where: (table, { and, eq }) => and(eq(table.name, input.name), eq(table.workspaceId, ws.id)), | |
| columns: { | |
| id: true, | |
| deletedAt: true, | |
| }, | |
| }) | |
| .catch((_err) => { | |
| throw new TRPCError({ | |
| code: "INTERNAL_SERVER_ERROR", | |
| message: | |
| "We are unable to create a new namespace. Please try again or contact support@unkey.dev", | |
| }); | |
| }); | |
| if (existingNamespace) { | |
| if (existingNamespace.deletedAt === null) { | |
| throw new TRPCError({ | |
| code: "PRECONDITION_FAILED", | |
| message: "Duplicate namespace name. Please use a unique name for each namespace.", | |
| }); | |
| } | |
| await db.transaction(async (tx) => { | |
| await tx | |
| .update(schema.ratelimitNamespaces) | |
| .set({ | |
| deletedAt: null, | |
| }) | |
| .where( | |
| and( | |
| eq(schema.ratelimitNamespaces.name, input.name), | |
| eq(schema.ratelimitNamespaces.workspaceId, ws.id) | |
| ) | |
| ) | |
| .catch((_err) => { | |
| throw new TRPCError({ | |
| code: "INTERNAL_SERVER_ERROR", | |
| message: | |
| "We were unable to update the namespace. Please try again or contact support@unkey.dev.", | |
| }); | |
| }); | |
| await insertAuditLogs(tx, { | |
| workspaceId: ws.id, | |
| actor: { | |
| type: "user", | |
| id: ctx.user.id, | |
| }, | |
| event: "ratelimitNamespace.create", | |
| description: `Restored ${existingNamespace.id}`, | |
| resources: [ | |
| { | |
| type: "ratelimitNamespace", | |
| id: existingNamespace.id, | |
| }, | |
| ], | |
| context: { | |
| location: ctx.audit.location, | |
| userAgent: ctx.audit.userAgent, | |
| }, | |
| }); | |
| }); | |
| return { | |
| id: existingNamespace.id, | |
| }; | |
| } |
|
Don’t think we can do it like that Now we reuse the id, which shows up in audit logs and logs Rather than recovering it, we should have a way to permanently delete it |
|
That’s why I asked about your approach before you tried to implement it. |
|
It's alright. I was having a hard time explaining the approach. So decided to create PR so that we can have something visual to discuss. By permanent delete you mean deleting it completely instead of keeping track of the soft delete using |
|
@harshsbhat yes, we should return an error explianing why there's a conflict and offer a solution
I'm not yet sure how I'd like to let users do it themselves, we're open to suggestions. |
What does this PR do?
Fixes #2869
It allows users to recover the soft-deleted rate limit namespace without manual intervention. It does so by simply allowing users to create a namespace with the same name that was deleted previously.
https://www.loom.com/share/5ac200da139b4812b4cd3d43c85cb28b?sid=a9a427c0-0867-4061-bf2a-b7361a8ce813
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes