-
-
Notifications
You must be signed in to change notification settings - Fork 358
feat: add domain-based access control for API keys #224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| -- AlterTable | ||
| ALTER TABLE "ApiKey" ADD COLUMN "domainId" INTEGER; | ||
|
|
||
| -- AddForeignKey | ||
| ALTER TABLE "ApiKey" ADD CONSTRAINT "ApiKey_domainId_fkey" FOREIGN KEY ("domainId") REFERENCES "Domain"("id") ON DELETE SET NULL ON UPDATE CASCADE; |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -33,3 +33,26 @@ export const checkIsValidEmailId = async (emailId: string, teamId: number) => { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new UnsendApiError({ code: "NOT_FOUND", message: "Email not found" }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export const checkIsValidEmailIdWithDomainRestriction = async ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| emailId: string, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| teamId: number, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| apiKeyDomainId?: number | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const whereClause: { id: string; teamId: number; domainId?: number } = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: emailId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| teamId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (apiKeyDomainId !== undefined) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| whereClause.domainId = apiKeyDomainId; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const email = await db.email.findUnique({ where: whereClause }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!email) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new UnsendApiError({ code: "NOT_FOUND", message: "Email not found" }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+47
to
+55
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Critical: null domainId narrows the query unexpectedly (unrestricted keys break); also use findFirst for composite filters When Apply: -export const checkIsValidEmailIdWithDomainRestriction = async (
- emailId: string,
- teamId: number,
- apiKeyDomainId?: number
-) => {
- const whereClause: { id: string; teamId: number; domainId?: number } = {
- id: emailId,
- teamId,
- };
-
- if (apiKeyDomainId !== undefined) {
- whereClause.domainId = apiKeyDomainId;
- }
-
- const email = await db.email.findUnique({ where: whereClause });
+export const checkIsValidEmailIdWithDomainRestriction = async (
+ emailId: string,
+ teamId: number,
+ apiKeyDomainId?: number | null
+) => {
+ const email = await db.email.findFirst({
+ where: {
+ id: emailId,
+ teamId,
+ ...(apiKeyDomainId != null ? { domainId: apiKeyDomainId } : {}),
+ },
+ });
if (!email) {
throw new UnsendApiError({ code: "NOT_FOUND", message: "Email not found" });
}
return email;
};📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return email; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,5 @@ | ||||||||||||||||||||||||||||||||||||||||
| import { createRoute, z } from "@hono/zod-openapi"; | ||||||||||||||||||||||||||||||||||||||||
| import { PublicAPIApp } from "~/server/public-api/hono"; | ||||||||||||||||||||||||||||||||||||||||
| import { getTeamFromToken } from "~/server/public-api/auth"; | ||||||||||||||||||||||||||||||||||||||||
| import { db } from "~/server/db"; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| const route = createRoute({ | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -26,15 +25,70 @@ const route = createRoute({ | |||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| description: "Create a new domain", | ||||||||||||||||||||||||||||||||||||||||
| description: "Verify domain", | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| 403: { | ||||||||||||||||||||||||||||||||||||||||
| content: { | ||||||||||||||||||||||||||||||||||||||||
| "application/json": { | ||||||||||||||||||||||||||||||||||||||||
| schema: z.object({ | ||||||||||||||||||||||||||||||||||||||||
| error: z.string(), | ||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| description: "Forbidden - API key doesn't have access to this domain", | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| 404: { | ||||||||||||||||||||||||||||||||||||||||
| content: { | ||||||||||||||||||||||||||||||||||||||||
| "application/json": { | ||||||||||||||||||||||||||||||||||||||||
| schema: z.object({ | ||||||||||||||||||||||||||||||||||||||||
| error: z.string(), | ||||||||||||||||||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| description: "Domain not found", | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+28
to
49
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Docs/code mismatch for 403 vs 404 OpenAPI declares a 403, but the handler always returns 404 on denied access. Return 403 when the API key is restricted to another domain. 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| function verifyDomain(app: PublicAPIApp) { | ||||||||||||||||||||||||||||||||||||||||
| app.openapi(route, async (c) => { | ||||||||||||||||||||||||||||||||||||||||
| const team = c.var.team; | ||||||||||||||||||||||||||||||||||||||||
| const domainId = c.req.valid("param").id; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // Check if API key has access to this domain | ||||||||||||||||||||||||||||||||||||||||
| let domain = null; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| if (team.apiKey.domainId) { | ||||||||||||||||||||||||||||||||||||||||
| // If API key is restricted to a specific domain, verify the requested domain matches | ||||||||||||||||||||||||||||||||||||||||
| if (domainId === team.apiKey.domainId) { | ||||||||||||||||||||||||||||||||||||||||
| domain = await db.domain.findFirst({ | ||||||||||||||||||||||||||||||||||||||||
| where: { | ||||||||||||||||||||||||||||||||||||||||
| teamId: team.id, | ||||||||||||||||||||||||||||||||||||||||
| id: domainId | ||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| // If domainId doesn't match the API key's restriction, domain remains null | ||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||
| // API key has access to all team domains | ||||||||||||||||||||||||||||||||||||||||
| domain = await db.domain.findFirst({ | ||||||||||||||||||||||||||||||||||||||||
| where: { | ||||||||||||||||||||||||||||||||||||||||
| teamId: team.id, | ||||||||||||||||||||||||||||||||||||||||
| id: domainId | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| if (!domain) { | ||||||||||||||||||||||||||||||||||||||||
| return c.json({ | ||||||||||||||||||||||||||||||||||||||||
| error: team.apiKey.domainId | ||||||||||||||||||||||||||||||||||||||||
| ? "API key doesn't have access to this domain" | ||||||||||||||||||||||||||||||||||||||||
| : "Domain not found" | ||||||||||||||||||||||||||||||||||||||||
| }, 404); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+82
to
+88
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Return correct status for access denial Send 403 when the API key is restricted to a different domain; 404 when the domain doesn't exist for the team. - if (!domain) {
- return c.json({
- error: team.apiKey.domainId
- ? "API key doesn't have access to this domain"
- : "Domain not found"
- }, 404);
- }
+ if (!domain) {
+ const isForbidden =
+ Boolean(team.apiKey.domainId) && domainId !== team.apiKey.domainId;
+ return c.json(
+ {
+ error: isForbidden
+ ? "API key doesn't have access to this domain"
+ : "Domain not found",
+ },
+ isForbidden ? 403 : 404,
+ );
+ }📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| await db.domain.update({ | ||||||||||||||||||||||||||||||||||||||||
| where: { id: c.req.valid("param").id }, | ||||||||||||||||||||||||||||||||||||||||
| where: { id: domainId }, | ||||||||||||||||||||||||||||||||||||||||
| data: { isVerifying: true }, | ||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Map service error to typed TRPC error
addApiKey throws "DOMAIN_NOT_FOUND". Convert to TRPC NOT_FOUND so clients get a proper 404-ish signal instead of 500.
📝 Committable suggestion
🤖 Prompt for AI Agents