Update demo #8
Update demo #8
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@markusahlstrand has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (7)
apps/demo/src/helpers/encryption.ts (2)
59-77: RenameconvertPKCS7ToPemto accurately reflect functionalityThe function
convertPKCS7ToPemconverts keys to PEM format but does not handle PKCS#7 structures. This may cause confusion, as PKCS#7 is associated with cryptographic message syntax rather than key encoding.Rename the function for clarity:
-export function convertPKCS7ToPem( +export function convertKeyToPem( keyType: "PRIVATE" | "PUBLIC", binaryData: ArrayBuffer, ) { // ... }Update references to the function:
- const pkcs7 = convertPKCS7ToPem("PRIVATE", privateKey); + const pemPrivateKey = convertKeyToPem("PRIVATE", privateKey);
69-71: Replace deprecatedsubstrwithsliceThe
substrmethod is deprecated. Useslicefor better compatibility and to avoid potential issues.Update the code as follows:
- pemCert += base64Cert.substr(nextIndex, 64) + "\r\n"; + pemCert += base64Cert.slice(nextIndex, nextIndex + 64) + "\r\n"; - pemCert += base64Cert.substr(nextIndex) + "\r\n"; + pemCert += base64Cert.slice(nextIndex) + "\r\n";apps/demo/src/bun.ts (1)
23-39: Enhance initialization logging and validationThe initialization block could benefit from improved logging and validation:
- No validation of the certificate creation result
- Basic console.log for initialization status
- No logging for existing database state
Consider enhancing the initialization block:
const keys = await dataAdapter.keys.list(); if (keys.length === 0) { + console.log('No signing keys found, initializing database...'); const signingKey = await createX509Certificate({ name: `CN=demo`, }); + if (!signingKey) { + throw new Error('Failed to create X509 certificate'); + } await dataAdapter.keys.create(signingKey); await dataAdapter.tenants.create({ id: "default", name: "Default Tenant", audience: "https://example.com", sender_email: "[email protected]", sender_name: "SenderName", }); - console.log("Initiated database"); + console.log("Successfully initialized database with default tenant and signing key"); +} else { + console.log(`Database already initialized with ${keys.length} signing keys`); }apps/demo/src/app.ts (1)
37-39: Validate init responseThe destructuring of
init()response assumes bothmanagementAppandoauthAppwill be present.Add validation:
- const { managementApp, oauthApp } = init({ + const result = init({ dataAdapter, }); + if (!result.managementApp || !result.oauthApp) { + throw new Error('Failed to initialize apps'); + } + const { managementApp, oauthApp } = result;packages/authhero/src/management-app.ts (1)
Line range hint
16-19: Clean up unused codeThe following items appear to be unused after the refactoring:
CreateAuthParamsinterfaceDataAdaptersimportApply this diff to clean up unused code:
-import { DataAdapters } from "@authhero/adapter-interfaces"; import { createAuthMiddleware } from "./middlewares/authentication"; import { emailProviderRoutes } from "./routes/management-api/emails"; -export interface CreateAuthParams { - dataAdapter: DataAdapters; -}Also applies to: 1-1
apps/demo/CHANGELOG.md (1)
3-12: Enhance changelog entry clarityThe changelog entry could be more descriptive to help users understand the impact of these changes:
- Where were the hooks moved to?
- Is this a breaking change for existing implementations?
Consider expanding the changelog entry like this:
## 0.3.0 ### Minor Changes -- moved the init of the hooks +- Moved hook initialization from management-app to [new location] + - This change may require updates to existing implementations ### Patch Changes - Updated dependencies - [email protected]packages/authhero/CHANGELOG.md (1)
3-8: Enhance changelog entry with migration detailsThe changelog entry should provide more context about this architectural change:
- Where were the hooks moved to?
- How should users migrate their code?
- What's the relationship with changes in the demo package?
Consider expanding the changelog entry like this:
## 0.18.0 ### Minor Changes -- moved the init of the hooks +- Architectural Change: Moved hook initialization + - Relocated from management-app to [new location] + - Breaking: Removed dataAdapter parameter from management-app create() + - Migration: Update your initialization code to use [new approach] + - Related: This change is coordinated with @authhero/[email protected]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
apps/.DS_Storeis excluded by!**/.DS_Storepnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
apps/demo/CHANGELOG.md(1 hunks)apps/demo/package.json(1 hunks)apps/demo/src/app.ts(2 hunks)apps/demo/src/bun.ts(2 hunks)apps/demo/src/helpers/encryption.ts(1 hunks)apps/demo/src/types/Bindings.ts(1 hunks)packages/authhero/CHANGELOG.md(1 hunks)packages/authhero/package.json(1 hunks)packages/authhero/src/authentication-flows/common.ts(1 hunks)packages/authhero/src/index.ts(1 hunks)packages/authhero/src/management-app.ts(1 hunks)packages/authhero/src/oauth-app.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/authhero/package.json
- packages/authhero/src/authentication-flows/common.ts
🔇 Additional comments (6)
apps/demo/src/types/Bindings.ts (1)
1-9: Addition of data property to Bindings is appropriate
The inclusion of data: DataAdapters in Bindings correctly extends the bindings to include data adapters, which are essential for data management within the application.
packages/authhero/src/index.ts (2)
12-17: Middleware correctly integrates data hooks
The middleware function adds data hooks to the context environment using addDataHooks, ensuring that ctx.env.data is properly initialized with the provided dataAdapter. This setup is appropriate for extending the application's capabilities.
25-28: Integration of OAuth routes appears correct
The application now routes OAuth endpoints through createOauthApi(), replacing previous configurations. This update should modularize and streamline OAuth functionalities.
apps/demo/src/bun.ts (1)
44-45: LGTM: DataAdapter integration
The integration of dataAdapter into the server's fetch context is clean and follows the pattern established in the Bindings type.
packages/authhero/src/oauth-app.ts (1)
9-11:
Unused interface parameter
The CreateAuthParams interface is defined but the create function doesn't accept any parameters.
Update the function signature to use the interface:
-export default function create() {
+export default function create(params: CreateAuthParams) {
const app = new OpenAPIHono<{
Bindings: Bindings;
Variables: Variables;
}>();Likely invalid or redundant comment.
apps/demo/src/app.ts (1)
60-61: Verify route precedence
The order of route registration might affect request handling. The OAuth routes at root ("/") might intercept requests intended for other endpoints.
Let's verify the route definitions:
✅ Verification successful
Let me check the implementation of oauthApp and managementApp to understand their route definitions.
Let me try a different approach to find the route definitions by searching for files that might contain these apps.
Let me try one final search to find the actual implementations of these apps to understand their route structures.
Route precedence appears to be correctly configured
Based on the route definitions found, the order is appropriate:
/api/v2/*routes are registered first, ensuring they take precedence for their specific path- The root route
/is registered last, which is correct as it should only handle requests not matched by more specific routes
The current order prevents any unintended interception of API requests by the OAuth routes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential route conflicts
rg -A 2 "\.route\(|\.get\(|\.post\(|\.put\(|\.delete\(" apps/demo/src/
Length of output: 875
Script:
#!/bin/bash
# Check oauth and management app route definitions
rg -A 5 "const oauthApp|const managementApp" apps/demo/src/
Length of output: 61
Script:
#!/bin/bash
# Find relevant files and their route definitions
fd -e ts -e js . apps/demo/src/ --exec grep -l "oauthApp\|managementApp" {} \;
# Then examine their content
echo "=== Route Definitions ==="
rg "app\.(get|post|put|delete|route)\(" $(fd -e ts -e js . apps/demo/src/ --exec grep -l "oauthApp\|managementApp" {} \;)
Length of output: 449
Script:
#!/bin/bash
# Search for the oauth and management app implementations
echo "=== OAuth App Implementation ==="
rg -B 2 -A 5 "export (const|function) oauthApp" apps/demo/src/
echo "=== Management App Implementation ==="
rg -B 2 -A 5 "export (const|function) managementApp" apps/demo/src/
Length of output: 294
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/authhero/src/index.ts (1)
5-5: Ensure consistent naming for default exportsIn
oauth-app.ts, the default exported function is namedcreate, but inindex.ts, it is imported ascreateOauthApi. For clarity and maintainability, consider renaming the function inoauth-app.tsto match the import name.Suggested change in
oauth-app.ts:- export default function create() { + export default function createOauthApi() {This will align the function name with its usage throughout the codebase.
Also applies to: 12-12
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/authhero/src/index.ts(1 hunks)packages/authhero/src/oauth-app.ts(1 hunks)
🔇 Additional comments (1)
packages/authhero/src/oauth-app.ts (1)
27-33: Remove duplicate OpenAPI documentation
As previously noted, the oauthApp.doc("/spec", {...}) method is called twice with different titles but on the same path /spec. This can cause conflicts or overwrite documentation definitions.
Please consolidate the OpenAPI documentation into a single doc call, merging the information as needed.
Also applies to: 37-43
| export default function create() { | ||
| const app = new OpenAPIHono<{ | ||
| Bindings: Bindings; | ||
| Variables: Variables; | ||
| }>(); | ||
|
|
There was a problem hiding this comment.
Unused app instance after middleware application
The app instance is created and middleware is applied using app.use(createAuthMiddleware(app));, but app is not used afterward. Instead, a new oauthApp instance is created and returned.
Consider either integrating the routes into app and returning it, or removing app if it's unnecessary.
Option 1: Integrate routes into app and return app:
export default function create() {
const app = new OpenAPIHono<{
Bindings: Bindings;
Variables: Variables;
}>();
app.use(createAuthMiddleware(app));
- const oauthApp = new OpenAPIHono<{
- Bindings: Bindings;
- Variables: Variables;
- }>()
- .route("/.well-known", wellKnownRoutes)
- .route("/oauth/token", tokenRoutes);
+ app.route("/.well-known", wellKnownRoutes)
+ .route("/oauth/token", tokenRoutes);
app.doc("/spec", {
openapi: "3.0.0",
info: {
version: "1.0.0",
title: "OAuth endpoints",
},
});
registerComponent(app);
return app;
}Option 2: Remove app if it's not needed and apply middleware to oauthApp:
export default function create() {
- const app = new OpenAPIHono<{
- Bindings: Bindings;
- Variables: Variables;
- }>();
- app.use(createAuthMiddleware(app));
const oauthApp = new OpenAPIHono<{
Bindings: Bindings;
Variables: Variables;
}>()
+ .use(createAuthMiddleware)
.route("/.well-known", wellKnownRoutes)
.route("/oauth/token", tokenRoutes);
oauthApp.doc("/spec", {
openapi: "3.0.0",
info: {
version: "1.0.0",
title: "OAuth endpoints",
},
});
registerComponent(oauthApp);
return oauthApp;
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
The issues is getting the types right. The middleware will return a normal hono router and not the openapi-version
There was a problem hiding this comment.
@markusahlstrand, thank you for the clarification. I understand that the issue is with the types—since the middleware returns a normal Hono router and not the OpenAPIHono version, integrating it directly affects type compatibility. That makes sense.
✏️ Learnings added
Learnt from: markusahlstrand
PR: markusahlstrand/authhero#8
File: packages/authhero/src/oauth-app.ts:12-17
Timestamp: 2024-12-09T10:49:39.560Z
Learning: In `packages/authhero/src/oauth-app.ts`, the middleware returns a normal Hono router instead of an `OpenAPIHono` instance, which can lead to type compatibility issues when integrating with `OpenAPIHono`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Added init for demo
Summary by CodeRabbit
Release Notes
New Features
Updates
0.3.0for the@authhero/demopackage and to0.18.0for theauthheropackage.@authhero/demopackage.Bug Fixes
@authhero/demoandauthheropackages.Refactor