-
Notifications
You must be signed in to change notification settings - Fork 445
General project provisioning #694
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
domain: domainSchema.optional(), | ||
}), | ||
onCreate: async ({ auth, data, params }) => { | ||
const oldDomains = auth.tenancy.config.domains; |
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.
Unsafe access to potentially undefined nested properties. The code directly accesses auth.tenancy.config.domains without any null checks or default value. If any part of the chain (auth, tenancy, config, or domains) is undefined, it will cause a runtime error. Should add null checks or use optional chaining.
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
😱 Found 1 issue. Time to roll up your sleeves! 😱 🗒️ View all ignored comments in this repo
|
}), | ||
), | ||
handler: async (req) => { | ||
const tokenResponse = await fetch(new URL("/api/v1/integrations/custom/oauth/idp/token", getEnvVariable("NEXT_PUBLIC_STACK_API_URL")), { |
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.
Consider using a tagged template literal (e.g., url`` or
urlString``) for constructing URLs (e.g., in the fetch call) to ensure proper encoding and consistency with our guidelines.
This comment was generated because it violated a code review rule: mrule_pmzJAgHDlFZgwIwD.
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.
PR Summary
This PR generalizes the project provisioning system by removing Neon-specific implementations in favor of a more flexible integration framework that can support multiple providers.
- Renamed
NeonProvisionedProject
toProvisionedProject
and updated related enums/tables inapps/backend/prisma/migrations/20250520185503_rename_neon/migration.sql
- Added new custom integration routes in
/apps/backend/src/app/api/latest/integrations/custom/
for domains, OAuth, and project management - Moved IDP implementation from Neon-specific to general use in
/apps/backend/src/app/api/latest/integrations/idp.ts
- Renamed
STACK_NEON_INTEGRATION_CLIENTS_CONFIG
toSTACK_INTEGRATION_CLIENTS_CONFIG
in environment files - Added comprehensive test coverage for new custom integration endpoints in
/apps/e2e/tests/backend/endpoints/api/v1/integrations/custom/
31 file(s) reviewed, 25 comment(s)
Edit PR Review Bot Settings | Greptile
onCreate: async ({ auth, data, params }) => { | ||
const oldDomains = auth.tenancy.config.domains; | ||
await projectsCrudHandlers.adminUpdate({ | ||
data: { | ||
config: { | ||
domains: [...oldDomains, { domain: data.domain, handler_path: "/handler" }], | ||
}, | ||
}, | ||
tenancy: auth.tenancy, | ||
allowedErrorTypes: [StatusError], | ||
}); | ||
|
||
return { domain: data.domain }; | ||
}, |
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.
logic: No validation to prevent duplicate domains from being added. Should check if domain already exists in oldDomains before adding.
onCreate: async ({ auth, data, params }) => { | |
const oldDomains = auth.tenancy.config.domains; | |
await projectsCrudHandlers.adminUpdate({ | |
data: { | |
config: { | |
domains: [...oldDomains, { domain: data.domain, handler_path: "/handler" }], | |
}, | |
}, | |
tenancy: auth.tenancy, | |
allowedErrorTypes: [StatusError], | |
}); | |
return { domain: data.domain }; | |
}, | |
onCreate: async ({ auth, data, params }) => { | |
const oldDomains = auth.tenancy.config.domains; | |
if (oldDomains.some(d => d.domain === data.domain)) { | |
throw new StatusError(400, 'Domain already exists'); | |
} | |
await projectsCrudHandlers.adminUpdate({ | |
data: { | |
config: { | |
domains: [...oldDomains, { domain: data.domain, handler_path: "/handler" }], | |
}, | |
}, | |
tenancy: auth.tenancy, | |
allowedErrorTypes: [StatusError], | |
}); | |
return { domain: data.domain }; | |
}, |
onDelete: async ({ auth, params }) => { | ||
const oldDomains = auth.tenancy.config.domains; | ||
await projectsCrudHandlers.adminUpdate({ | ||
data: { | ||
config: { domains: oldDomains.filter((domain) => domain.domain !== params.domain) }, | ||
}, | ||
tenancy: auth.tenancy, | ||
allowedErrorTypes: [StatusError], | ||
}); | ||
}, |
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.
style: No error handling if domain to be deleted doesn't exist in oldDomains. Should return appropriate error response.
onDelete: async ({ auth, params }) => { | |
const oldDomains = auth.tenancy.config.domains; | |
await projectsCrudHandlers.adminUpdate({ | |
data: { | |
config: { domains: oldDomains.filter((domain) => domain.domain !== params.domain) }, | |
}, | |
tenancy: auth.tenancy, | |
allowedErrorTypes: [StatusError], | |
}); | |
}, | |
onDelete: async ({ auth, params }) => { | |
const oldDomains = auth.tenancy.config.domains; | |
if (!oldDomains.some(d => d.domain === params.domain)) { | |
throw new StatusError(404, 'Domain not found'); | |
} | |
await projectsCrudHandlers.adminUpdate({ | |
data: { | |
config: { domains: oldDomains.filter((domain) => domain.domain !== params.domain) }, | |
}, | |
tenancy: auth.tenancy, | |
allowedErrorTypes: [StatusError], | |
}); | |
}, |
response: yupNever(), | ||
handler: async (req) => { | ||
const url = new URL(req.url); | ||
if (url.pathname !== "/api/v1/integrations/custom/oauth/authorize") { |
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.
style: Consider using URL path constants to avoid hardcoding strings that could change
const authorizationCode = generateSecureRandomString(); | ||
await prismaClient.projectWrapperCodes.create({ | ||
data: { | ||
idpId: "stack-preconfigured-idp:integrations/custom", |
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.
style: Consider moving 'stack-preconfigured-idp:integrations/custom' to a constant
it("adds two different domains", async ({ expect }) => { | ||
await Auth.Otp.signIn(); | ||
const { adminAccessToken } = await Project.createAndGetAdminToken(); | ||
|
||
// Add first domain | ||
await niceBackendFetch("/api/v1/integrations/custom/domains", { | ||
accessType: "admin", | ||
headers: { | ||
'x-stack-admin-access-token': adminAccessToken, | ||
}, | ||
method: "POST", | ||
body: { | ||
domain: "https://first-domain.example.com", | ||
}, | ||
}); | ||
|
||
// Add second domain | ||
await niceBackendFetch("/api/v1/integrations/custom/domains", { | ||
accessType: "admin", | ||
headers: { | ||
'x-stack-admin-access-token': adminAccessToken, | ||
}, | ||
method: "POST", | ||
body: { | ||
domain: "https://second-domain.example.com", | ||
}, | ||
}); | ||
|
||
// List domains to verify both were added | ||
const listResponse = await niceBackendFetch("/api/v1/integrations/custom/domains", { | ||
accessType: "admin", | ||
headers: { | ||
'x-stack-admin-access-token': adminAccessToken, | ||
}, | ||
}); | ||
|
||
expect(listResponse).toMatchInlineSnapshot(` | ||
NiceResponse { | ||
"status": 200, | ||
"body": { | ||
"is_paginated": false, | ||
"items": [ | ||
{ "domain": "https://first-domain.example.com" }, | ||
{ "domain": "https://second-domain.example.com" }, | ||
], | ||
}, | ||
"headers": Headers { <some fields may have been hidden> }, | ||
} | ||
`); | ||
}); |
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.
style: Missing test for duplicate domain creation - should verify proper error handling when adding the same domain twice
it("adds two domains and deletes one", async ({ expect }) => { | ||
await Auth.Otp.signIn(); | ||
const { adminAccessToken } = await Project.createAndGetAdminToken(); | ||
|
||
// Add first domain | ||
await niceBackendFetch("/api/v1/integrations/custom/domains", { | ||
accessType: "admin", | ||
headers: { | ||
'x-stack-admin-access-token': adminAccessToken, | ||
}, | ||
method: "POST", | ||
body: { | ||
domain: "https://domain-to-keep.example.com", | ||
}, | ||
}); | ||
|
||
// Add second domain | ||
await niceBackendFetch("/api/v1/integrations/custom/domains", { | ||
accessType: "admin", | ||
headers: { | ||
'x-stack-admin-access-token': adminAccessToken, | ||
}, | ||
method: "POST", | ||
body: { | ||
domain: "https://domain-to-delete.example.com", | ||
}, | ||
}); | ||
|
||
// Delete the second domain | ||
const deleteResponse = await niceBackendFetch(`/api/v1/integrations/custom/domains/${encodeURIComponent("https://domain-to-delete.example.com")}`, { | ||
accessType: "admin", | ||
headers: { | ||
'x-stack-admin-access-token': adminAccessToken, | ||
}, | ||
method: "DELETE", | ||
}); | ||
|
||
expect(deleteResponse).toMatchInlineSnapshot(` | ||
NiceResponse { | ||
"status": 200, | ||
"body": { "success": true }, | ||
"headers": Headers { <some fields may have been hidden> }, | ||
} | ||
`); | ||
|
||
// List domains to verify only one remains | ||
const listResponse = await niceBackendFetch("/api/v1/integrations/custom/domains", { | ||
accessType: "admin", | ||
headers: { | ||
'x-stack-admin-access-token': adminAccessToken, | ||
}, | ||
}); | ||
|
||
expect(listResponse).toMatchInlineSnapshot(` | ||
NiceResponse { | ||
"status": 200, | ||
"body": { | ||
"is_paginated": false, | ||
"items": [{ "domain": "https://domain-to-keep.example.com" }], | ||
}, | ||
"headers": Headers { <some fields may have been hidden> }, | ||
} | ||
`); | ||
}); |
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.
style: Missing test for deleting non-existent domain - should verify proper 404 handling
apps/e2e/tests/backend/endpoints/api/v1/integrations/custom/projects/transfer.test.ts
Outdated
Show resolved
Hide resolved
apps/e2e/tests/backend/endpoints/api/v1/integrations/custom/oauth.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…ack into general-provision
apps/dashboard/src/app/(main)/integrations/custom/projects/transfer/confirm/page.tsx
Show resolved
Hide resolved
<!-- Make sure you've read the CONTRIBUTING.md guidelines: https://github.com/stack-auth/stack-auth/blob/dev/CONTRIBUTING.md --> <!-- ELLIPSIS_HIDDEN --> ---- > [!IMPORTANT] > This pull request adds support for custom integrations alongside Neon, including environment updates, database schema changes, new API endpoints, and comprehensive tests. > > - **Environment**: > - Renamed `STACK_NEON_INTEGRATION_CLIENTS_CONFIG` to `STACK_INTEGRATION_CLIENTS_CONFIG` in `.env.development`. > - Added `custom-local` client configuration to `STACK_INTEGRATION_CLIENTS_CONFIG`. > - **Database**: > - Renamed `NeonProvisionedProject` table to `ProvisionedProject` and updated related constraints and columns in `migration.sql`. > - Updated `schema.prisma` to reflect table and column renames. > - **API Endpoints**: > - Added CRUD operations for custom domains in `domains/`. > - Implemented OAuth authorization and token exchange for custom integrations in `oauth/`. > - Added project provisioning and transfer endpoints for custom integrations in `projects/`. > - **IDP**: > - Updated `createOidcProvider` to handle both Neon and custom integrations in `idp.ts`. > - **Tests**: > - Added e2e tests for custom domain, OAuth, project provisioning, and transfer in `e2e/tests/backend/endpoints/api/v1/integrations/custom/`. > - Updated snapshot serializer to handle custom integration paths in `snapshot-serializer.ts`. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=stack-auth%2Fstack-auth&utm_source=github&utm_medium=referral)<sup> for 7da89c3. You can [customize](https://app.ellipsis.dev/stack-auth/settings/summaries) this summary. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN --> --------- Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Co-authored-by: Konsti Wohlwend <n2d4xc@gmail.com>
Important
This pull request adds support for custom integrations alongside Neon, including environment updates, database schema changes, new API endpoints, and comprehensive tests.
STACK_NEON_INTEGRATION_CLIENTS_CONFIG
toSTACK_INTEGRATION_CLIENTS_CONFIG
in.env.development
.custom-local
client configuration toSTACK_INTEGRATION_CLIENTS_CONFIG
.NeonProvisionedProject
table toProvisionedProject
and updated related constraints and columns inmigration.sql
.schema.prisma
to reflect table and column renames.domains/
.oauth/
.projects/
.createOidcProvider
to handle both Neon and custom integrations inidp.ts
.e2e/tests/backend/endpoints/api/v1/integrations/custom/
.snapshot-serializer.ts
.This description was created by
for 7da89c3. You can customize this summary. It will automatically update as commits are pushed.