Ma/elastic role #1139
Ma/elastic role #1139
Conversation
WalkthroughThe pull request introduces several changes across multiple files. The Changes
Possibly related PRs
🪧 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: 3
🧹 Outside diff range and nitpick comments (1)
src/services/email/resend.ts (1)
20-26: Consider type safety improvements for the request bodyThe request body structure should be type-safe to match Resend's API requirements.
Add a type definition for the request body:
interface ResendEmailRequest { from: string; to: string[]; subject: string; html?: string; text?: string; }Then use it in the code:
- const body = { + const body: ResendEmailRequest = { from: `${emailOptions.from.name} <${emailOptions.from.email}>`, to: recipients, subject: emailOptions.subject, html: htmlContent ? htmlContent.value : undefined, text: textContent ? textContent.value : undefined, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/handlers/cleanup.ts(0 hunks)src/helpers/generate-auth-response.ts(1 hunks)src/services/email/resend.ts(1 hunks)test/fixtures/context.ts(0 hunks)
💤 Files with no reviewable changes (2)
- src/handlers/cleanup.ts
- test/fixtures/context.ts
🔇 Additional comments (1)
src/helpers/generate-auth-response.ts (1)
139-142:
Verify compatibility of role format change
Changing the role format from array to string could break existing consumers expecting an array of roles.
Let's verify the impact:
✅ Verification successful
Based on the comprehensive analysis of the codebase, I can now provide the final response:
Role format change is safe to proceed
The change from array to string format for the roles claim in the "sesamy-elastic" client's custom claims is safe because:
- The role is only set for internal Sesamy users (email ending with "@sesamy.com")
- The authentication middleware doesn't validate or use the
rolesclaim for authorization - No direct consumers of the
rolesclaim were found in the codebase - The authorization is primarily based on JWT scopes and permissions, not roles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consumers of the roles claim in the codebase
# Search for role-related patterns
echo "Searching for role array access patterns..."
rg -A 2 'roles\['
echo "Searching for role type definitions..."
ast-grep --pattern 'type $_ = {
$$$
roles: $_[]
$$$
}'
echo "Searching for role assignments..."
rg -A 2 'roles.*=.*\[.*\]'
Length of output: 372
Script:
#!/bin/bash
# Let's try a broader search for roles-related patterns
echo "Searching for roles type annotations..."
ast-grep --pattern 'roles: $_'
echo "Searching for roles usage..."
rg -A 3 'roles[^a-zA-Z]'
echo "Searching for role-based checks..."
rg -A 3 'hasRole|checkRole|userRole|isRole'
echo "Checking the auth response generator history..."
rg -A 5 'roles' src/helpers/generate-auth-response.ts
Length of output: 13331
Script:
#!/bin/bash
# Let's check for JWT token validation and usage patterns
echo "Searching for JWT token validation..."
rg -A 5 'verify.*token|validateToken|decodeToken|parseToken'
echo "Checking JWT related files..."
fd -e ts -e js jwt
echo "Searching for custom claims usage..."
rg -A 3 'customClaims|claims\.roles|token\.roles'
echo "Checking auth middleware..."
fd -e ts -e js middleware
rg -A 5 'middleware.*auth|auth.*middleware'
Length of output: 1865
Script:
#!/bin/bash
# Let's examine the JWT validation and middleware implementation
echo "Checking JWT validation implementation..."
cat src/utils/jwt.ts
echo "Checking authentication middleware..."
cat src/middlewares/authentication.ts
echo "Searching for token role validation..."
rg -A 5 'validateRole|checkScope|verifyScope|validateScope'
Length of output: 5288
| if (!response.ok) { | ||
| throw new Error(`Failed with status: ${response.status}`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with detailed error messages
The current error handling doesn't provide enough context about what went wrong.
Improve error handling by including the response body:
if (!response.ok) {
- throw new Error(`Failed with status: ${response.status}`);
+ const errorBody = await response.text();
+ throw new Error(
+ `Email sending failed with status ${response.status}: ${errorBody}`
+ );
}📝 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.
| if (!response.ok) { | |
| throw new Error(`Failed with status: ${response.status}`); | |
| } | |
| if (!response.ok) { | |
| const errorBody = await response.text(); | |
| throw new Error( | |
| `Email sending failed with status ${response.status}: ${errorBody}` | |
| ); | |
| } |
| export default async function send(emailOptions: EmailOptions, apiKey: string) { | ||
| const apiUrl = `https://api.resend.com/emails`; | ||
|
|
||
| const headers = new Headers(); | ||
| headers.set("Authorization", `Bearer ${apiKey}`); | ||
| headers.set("Content-Type", "application/json"); | ||
|
|
There was a problem hiding this comment.
Add input validation for required parameters
The function should validate the required parameters before making the API call.
Add validation at the start of the function:
export default async function send(emailOptions: EmailOptions, apiKey: string) {
+ if (!apiKey?.trim()) {
+ throw new Error('API key is required');
+ }
+
+ if (!emailOptions?.to?.length) {
+ throw new Error('At least one recipient is required');
+ }
+
+ if (!emailOptions.from?.email) {
+ throw new Error('Sender email is required');
+ }
+
const apiUrl = `https://api.resend.com/emails`;📝 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.
| export default async function send(emailOptions: EmailOptions, apiKey: string) { | |
| const apiUrl = `https://api.resend.com/emails`; | |
| const headers = new Headers(); | |
| headers.set("Authorization", `Bearer ${apiKey}`); | |
| headers.set("Content-Type", "application/json"); | |
| export default async function send(emailOptions: EmailOptions, apiKey: string) { | |
| if (!apiKey?.trim()) { | |
| throw new Error('API key is required'); | |
| } | |
| if (!emailOptions?.to?.length) { | |
| throw new Error('At least one recipient is required'); | |
| } | |
| if (!emailOptions.from?.email) { | |
| throw new Error('Sender email is required'); | |
| } | |
| const apiUrl = `https://api.resend.com/emails`; | |
| const headers = new Headers(); | |
| headers.set("Authorization", `Bearer ${apiKey}`); | |
| headers.set("Content-Type", "application/json"); |
| roles: "sesamy_admin", | ||
| subject_key: user.name || user.email, | ||
| } | ||
| : {}; |
There was a problem hiding this comment.
Strengthen role assignment security
The current implementation assigns admin roles based solely on email domain, which could be vulnerable if email verification is not enforced.
Consider these security improvements:
- Verify email ownership:
const customClaims =
- client.id === "sesamy-elastic" && user?.email.endsWith("sesamy.com")
+ client.id === "sesamy-elastic" &&
+ user?.email.endsWith("sesamy.com") &&
+ user.email_verified === true- Consider maintaining an explicit allowlist of admin users instead of relying on email domain.
Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Refactor
ticketsproperty, improving the clarity of the function's logic.