Adding monetization support at Devportal #604
Conversation
WalkthroughThis pull request introduces a comprehensive billing and monetization system enabling API subscription payments via Stripe. Changes include database schema extensions for pricing and billing fields, new controllers and services for checkout and billing management, Stripe and Moesif integrations, encryption utilities for secure key storage, billing dashboard UI with usage and invoice tracking, and client-side scripts orchestrating checkout flows and payment workflows. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Browser / Client
participant DevPortal as DevPortal Server
participant Stripe as Stripe API
participant DB as Database
participant Moesif as Moesif Service
User->>Client: Click "Subscribe & Pay"
Client->>DevPortal: POST /organizations/:orgId/monetization/checkout<br/>(apiId, policyId, applicationID)
DevPortal->>DB: Check API, Policy, and create PENDING subscription
DB-->>DevPortal: Subscription created (ID, status=PENDING)
DevPortal->>Stripe: Create embedded checkout session<br/>(customerId, priceId, returnUrl, metadata)
Stripe-->>DevPortal: { clientSecret, publishableKey, sessionId }
DevPortal-->>Client: { subId, sessionId, clientSecret, publishableKey }
Client->>Stripe: Stripe.js embedded checkout UI<br/>(clientSecret)
User->>Stripe: Enter payment details
Stripe-->>Client: Payment confirmed, redirect to returnUrl<br/>with session_id, org_id, dp_sub_id
Client->>DevPortal: GET /billing/return<br/>(session_id, org_id, dp_sub_id)
DevPortal->>Stripe: Verify checkout session<br/>(session_id)
Stripe-->>DevPortal: Session details + subscription
DevPortal->>DB: Update subscription<br/>(PAYMENT_STATUS=ACTIVE, billing IDs, CHECKOUT_SESSION_ID)
DB-->>DevPortal: ✓
DevPortal->>Moesif: Sync customer & subscription<br/>(companyId, userId, subscriptionId)
Moesif-->>DevPortal: ✓ (non-blocking on error)
DevPortal-->>Client: Redirect to success page
Client-->>User: Show success message
rect rgb(200, 220, 255)
Note over DevPortal, DB: Billing Portal Flow
User->>Client: Click "Manage Billing"
Client->>DevPortal: POST /organizations/:orgId/billing-portal<br/>(returnUrl)
DevPortal->>Stripe: Create customer portal session<br/>(customerId, returnUrl)
Stripe-->>DevPortal: { url }
DevPortal-->>Client: { url }
Client->>Stripe: Redirect to customer portal
User->>Stripe: Manage payment methods & subscriptions
Stripe-->>Client: Redirect back to returnUrl
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/scripts/common.js (3)
527-531: Same inconsistent selector pattern.Line 527 uses
.common-btn-primarywhile other updated sections use.subscribe-btn.
822-825: Same inconsistent selector pattern in subscription cards section.Line 822 uses
.common-btn-primarywhile line 648 and 736 in the same section use.subscribe-btn.
436-439: Standardize subscribe button selector to.subscribe-btn.This code uses
.common-btn-primary[disabled](line 436), while other parts of the same file (lines 238, 309, 346, 648, 736) use.subscribe-btn. Both selectors work because the button has both classes in the templates (class="common-btn-primary subscribe-btn"), but the inconsistency creates unnecessary fragility and maintenance burden. Use.subscribe-btnthroughout since it's more specific to the subscribe button purpose. Also update lines 527, 616, and 822 which use.common-btn-primaryalone.
🟠 Major comments (9)
src/defaultContent/layout/main.hbs-15-26 (1)
15-26: Remove debug console.log statements before production release.The Stripe load verification logs (
console.log/console.error) are useful for development but should be removed or guarded behind a debug flag for production builds.🔎 Proposed fix
<script src="https://js.stripe.com/v3/"></script> - <script> - // Verify Stripe loads - window.addEventListener('load', function() { - if (window.Stripe) { - console.log('✅ Stripe.js loaded successfully'); - } else { - console.error('❌ Stripe.js failed to load'); - } - }); - </script>src/middlewares/billingAuth.js-28-31 (1)
28-31: Avoid logging PII (user email) even at debug level.Logging
userEmailmay violate data privacy policies (GDPR/CCPA) and create compliance risks. Consider logging a non-PII identifier like a user ID hash or removing the email from logs entirely.🔎 Proposed fix
logger.debug('Billing auth check passed', { - userEmail: req.user.email, + userId: req.user.id || req.user.sub, hasAccessToken: !!req.user.accessToken });src/controllers/authController.js-266-272 (1)
266-272: Reconsider using orgName as orgId fallback.When
orgIdlookup fails, the code falls back to usingorgNameas theorgId(line 271). This could lead to authorization bypass or data leakage if billing logic relies on theorgIdbeing a valid UUID. Consider returning an error instead.🔎 Proposed fix
// Get orgId from the orgName in the URL let orgId; try { orgId = await adminDao.getOrgId(orgName); + if (!orgId) { + throw new Error('Organization not found'); + } } catch (err) { - logger.error('Organization not found, using orgName as orgId', { orgName }); - orgId = orgName; // Use orgName as fallback if org not in database + logger.error('Organization not found', { + orgName, + error: err.message + }); + return res.status(404).send('Organization not found'); }src/controllers/authController.js-252-314 (1)
252-314: Add input validation for route parameters.The function should validate
orgNameandviewNameparameters to prevent injection attacks or invalid data processing, similar to other handlers in this controller.🔎 Proposed fix
Add validation at the beginning of the function:
const renderBillingPage = async (req, res) => { try { + const rules = util.validateRequestParameters(); + for (let validation of rules) { + await validation.run(req); + } + const errors = validationResult(req); + if (!errors.isEmpty()) { + return res.status(400).json(util.getErrors(errors)); + } + const orgName = req.params.orgName; const viewName = req.params.viewName || 'default';Committable suggestion skipped: line range outside the PR's diff.
src/services/orgBillingKeyService.js-14-14 (1)
14-14: Use a constant for billing engine name.The hardcoded string
'stripe'should be replaced with a constant for maintainability and consistency across the codebase.🔎 Proposed refactor
+const constants = require('../utils/constants'); + /** * Fetches and decrypts Stripe keys for a given orgId. * @param {string} orgId - The organization ID * @returns {Promise<{secretKey: string, publishableKey: string, webhookSecret: string}>} */ async function getDecryptedStripeKeysForOrg(orgId) { if (!orgId) throw new Error('orgId is required to fetch Stripe keys'); const record = await BillingEngineKey.findOne({ where: { ORG_ID: orgId, - BILLING_ENGINE: 'stripe', + BILLING_ENGINE: constants.BILLING_ENGINE.STRIPE, }, });Committable suggestion skipped: line range outside the PR's diff.
src/services/orgBillingKeyService.js-9-23 (1)
9-23: Add error handling and audit logging for key decryption.This function handles sensitive cryptographic operations and should include:
- Explicit error handling for decryption failures (e.g., corrupted data, wrong keys)
- Audit logging for security monitoring
- More specific error messages
🔎 Proposed improvements
+const logger = require('../config/logger'); + async function getDecryptedStripeKeysForOrg(orgId) { if (!orgId) throw new Error('orgId is required to fetch Stripe keys'); + + logger.info('Fetching Stripe keys for organization', { orgId }); + const record = await BillingEngineKey.findOne({ where: { ORG_ID: orgId, BILLING_ENGINE: 'stripe', }, }); - if (!record) throw new Error('Stripe keys not found for this org'); + + if (!record) { + logger.warn('Stripe keys not found for organization', { orgId }); + throw new Error(`Stripe keys not found for organization: ${orgId}`); + } + + try { - return { - secretKey: decrypt(record.SECRET_KEY_ENC), - publishableKey: decrypt(record.PUBLISHABLE_KEY_ENC), - webhookSecret: decrypt(record.WEBHOOK_SECRET_ENC), - }; + const keys = { + secretKey: decrypt(record.SECRET_KEY_ENC), + publishableKey: decrypt(record.PUBLISHABLE_KEY_ENC), + webhookSecret: decrypt(record.WEBHOOK_SECRET_ENC), + }; + logger.info('Successfully decrypted Stripe keys', { orgId }); + return keys; + } catch (error) { + logger.error('Failed to decrypt Stripe keys', { + orgId, + error: error.message + }); + throw new Error('Failed to decrypt Stripe keys for organization'); + } }src/defaultContent/pages/apis/partials/api-listing.hbs-207-236 (1)
207-236: Remove duplicate closing tag on line 236.Line 236 contains concatenated closing tags
{{/if}} {{/if}}which will cause rendering issues. The first{{/if}}correctly closes the condition from line 207, but the second is extraneous and should be removed. This appears to be a merge artifact.src/scripts/subscription.js-71-92 (1)
71-92: Remove duplicate function definitions.Functions
hideElementById(lines 71-74 and 133-138),closeModal(lines 76-80 and 140-143),window.onclick(lines 82-87 and 145-150), andloadModal(lines 89-92 and 256-259) are defined twice. The later definitions override the earlier ones, potentially with different behavior.Analysis
The first set of definitions (lines 71-92) appear to be new additions that are cleaner, while lines 133-259 contain the original implementations. Remove the duplicates and consolidate to a single definition of each function.
src/services/moesifService.js-334-343 (1)
334-343: MissingcreateMoesifSubscriptionfrom exports.The function
createMoesifSubscriptionis defined at lines 252-286 but is not included inmodule.exports. This function is likely used bymonetizationService.jsfor subscription syncing.Proposed fix
module.exports = { getPlansFromMoesif, getCompany, extractSubscriptionsFromCompanyObject, getUsageForSubscription, syncCustomerMapping, createOrUpdateUser, + createMoesifSubscription, createBillingMeter, // (optional) expose app id if needed for ingestion getAppId, };
🟡 Minor comments (6)
package.json-38-38 (1)
38-38: Fix invalid dotenv version in dependencies.The
dotenvpackage version^17.2.3does not exist. The latest published version is9.0.2. Update the dependency to a valid version.src/routes/authRoute.js-23-23 (1)
23-23: Fix the misspelled middleware function name.The middleware function
enforceSecuirtyis misspelled and should beenforceSecurity. Rename the function definition insrc/middlewares/ensureAuthenticated.js(line 34) and update the export (line 449) and corresponding import insrc/routes/authRoute.js(line 23) to use the correct spelling.src/app.js-252-256 (1)
252-256: Potential NaN handling in formatPrice helper.If
priceis a non-numeric string (e.g.,"abc"),parseFloat(price)returnsNaN, andNaN.toString()produces"NaN"which would be displayed in the UI.🔎 Suggested defensive fix
Handlebars.registerHelper('formatPrice', function (price) { if (!price) return '0'; - // Convert to number and remove trailing zeros - return parseFloat(price).toString(); + const parsed = parseFloat(price); + return isNaN(parsed) ? '0' : parsed.toString(); });src/pages/billing/partials/billing-content.hbs-204-206 (1)
204-206: Remove debug console.log statement.The button onclick contains a debug
console.log('🔵 Button clicked!')statement that should be removed before production.🔎 Suggested fix
- <button class="common-btn-outlined" onclick="console.log('🔵 Button clicked!'); addPaymentMethod(); return false;"> + <button class="common-btn-outlined" onclick="addPaymentMethod()"> <i class="fas fa-credit-card me-2"></i>Manage in Stripe </button>src/controllers/usageController.js-37-47 (1)
37-47: Missing date validation in getRange.If
usageFromorusageTocontain invalid date strings (e.g.,"invalid"),new Date("invalid").toISOString()throws aRangeError: Invalid time value. Consider adding validation.🔎 Suggested defensive fix
function getRange(req) { const usageFrom = req.query.usageFrom; const usageTo = req.query.usageTo; - const to = usageTo ? new Date(usageTo).toISOString() : new Date().toISOString(); - const from = usageFrom - ? new Date(usageFrom).toISOString() - : new Date(Date.now() - 30 * 24 * 60 * 60 * 1000).toISOString(); + const now = new Date(); + const defaultFrom = new Date(Date.now() - 30 * 24 * 60 * 60 * 1000); + + let to = now; + let from = defaultFrom; + + if (usageTo) { + const parsedTo = new Date(usageTo); + if (!isNaN(parsedTo.getTime())) { + to = parsedTo; + } + } + + if (usageFrom) { + const parsedFrom = new Date(usageFrom); + if (!isNaN(parsedFrom.getTime())) { + from = parsedFrom; + } + } - return { from, to }; + return { from: from.toISOString(), to: to.toISOString() }; }src/services/stripeService.js-188-198 (1)
188-198: Wrap switch case variable declaration in a block.The
const subStatusdeclaration at line 190 inside the switch case can be erroneously accessed by other cases. Wrap in a block as suggested by static analysis.Proposed fix
case "customer.subscription.updated": + { // Check if subscription is past_due or unpaid const subStatus = event.data?.object?.status; if (subStatus === "past_due" || subStatus === "unpaid") { newStatus = PAYMENT_STATUS.PAST_DUE; } else if (subStatus === "active") { newStatus = PAYMENT_STATUS.ACTIVE; } else if (subStatus === "canceled") { newStatus = PAYMENT_STATUS.CANCELED; } break; + }
🧹 Nitpick comments (31)
src/utils/cryptoUtil.js (1)
14-21: Add error handling for malformed encrypted input.The
decryptfunction will throw cryptic errors if the input doesn't contain a colon separator or contains invalid base64. Consider wrapping in try-catch with a clearer error message.🔎 Proposed fix
function decrypt(text) { + if (!text || !text.includes(':')) { + throw new Error('Invalid encrypted text format'); + } const [ivStr, encrypted] = text.split(':'); + if (!ivStr || !encrypted) { + throw new Error('Invalid encrypted text format'); + } const iv = Buffer.from(ivStr, 'base64'); const decipher = crypto.createDecipheriv('aes-256-cbc', Buffer.from(ENCRYPTION_KEY), iv); let decrypted = decipher.update(encrypted, 'base64', 'utf8'); decrypted += decipher.final('utf8'); return decrypted; }src/models/subscriptionPolicy.js (1)
61-68: Consider specifying precision and scale for monetary DECIMAL fields.For monetary values, it's best practice to explicitly define precision and scale to avoid unexpected rounding. Typical currency precision is (10, 2) for amounts up to 99,999,999.99.
🔎 Proposed enhancement
FLAT_AMOUNT: { - type: DataTypes.DECIMAL, + type: DataTypes.DECIMAL(10, 2), allowNull: true }, UNIT_AMOUNT: { - type: DataTypes.DECIMAL, + type: DataTypes.DECIMAL(10, 2), allowNull: true },src/defaultContent/layout/main.hbs (2)
16-16: Consider lazy-loading Stripe.js only on billing pages.Loading Stripe.js on every page adds ~30KB to the initial bundle and an external request, even for users who never visit billing. Consider loading it dynamically only when needed.
50-56: Extract inline styles to a CSS file for maintainability.The modal contains extensive inline styles that should be moved to a stylesheet for consistency, maintainability, and separation of concerns. Consider adding modal-related styles to a dedicated CSS file (e.g.,
src/defaultContent/styles/modal.cssor within an existing stylesheet likemain.css). Note thatcloseStripeCheckoutModal()is already properly defined insrc/scripts/subscription.jsand exposed to the window object.src/pages/layout/main.hbs (1)
13-24: Duplicate code across layout files creates maintenance burden.The Stripe.js integration and modal are duplicated in both
src/defaultContent/layout/main.hbsandsrc/pages/layout/main.hbs. Consider extracting to a shared partial (e.g.,{{> stripe-checkout-modal }}) to ensure consistency and easier maintenance.The same issues noted for the other layout file apply here:
- Remove debug
console.logstatements before production- Extract inline styles to CSS
- Consider lazy-loading Stripe.js
Also applies to: 31-39
src/defaultContent/pages/billing/page.hbs (1)
38-72: Reduce production logging verbosity.While the script loading mechanism is sound, the extensive console logging (lines 39-40, 46, 54-60, 63-67, 71) is excessive for production. Consider wrapping debug logs in a development-mode check or removing non-essential logs.
🔎 Proposed refactor
<script> - console.log('📄 Billing page template rendered - ' + new Date().toISOString()); - console.log('🌐 Current URL:', window.location.href); - // Clear any cached scripts var cacheBuster = '?v=' + Date.now(); var scriptUrl = window.location.origin + '/technical-scripts/billing.js' + cacheBuster; - console.log('🔍 Loading billing.js from:', scriptUrl); - // Dynamically load the script to avoid path and caching issues var script = document.createElement('script'); script.src = scriptUrl; script.type = 'text/javascript'; script.async = false; script.onload = function() { - console.log('✅ billing.js loaded and executed successfully!'); - console.log('✅ Window functions exposed:', { - loadUsageData: typeof window.loadUsageData, - loadInvoices: typeof window.loadInvoices, - loadPaymentDetails: typeof window.loadPaymentDetails - }); + // Script loaded successfully }; script.onerror = function(error) { console.error('❌ FAILED to load billing.js'); - console.error('❌ Attempted URL:', scriptUrl); - console.error('❌ Error:', error); alert('Failed to load billing.js from: ' + scriptUrl); }; document.body.appendChild(script); - console.log('📌 Script tag appended to body'); </script>src/pages/billing/page.hbs (2)
28-54: Reduce production console logging.Similar to the defaultContent version, this template contains excessive console logging. Consider keeping only error logs and removing debug-level logs for production.
🔎 Proposed refactor
<script> - console.log('📄 Billing page rendered from src/pages/billing/page.hbs'); - console.log('🌐 Current URL:', window.location.href); - console.log('🔍 orgId:', '{{orgId}}'); - // Load billing.js with cache busting var cacheBuster = '?v=' + Date.now(); var scriptUrl = window.location.origin + '/technical-scripts/billing.js' + cacheBuster; - console.log('🔍 Loading billing.js from:', scriptUrl); - var script = document.createElement('script'); script.src = scriptUrl; script.type = 'text/javascript'; script.async = false; script.onload = function() { - console.log('✅ billing.js loaded and executed successfully!'); + // Script loaded successfully }; script.onerror = function(error) { console.error('❌ FAILED to load billing.js'); - console.error('❌ Attempted URL:', scriptUrl); }; document.body.appendChild(script); </script>
1-54: Consider consolidating duplicate billing page templates.Two nearly identical billing page templates exist:
src/defaultContent/pages/billing/page.hbsandsrc/pages/billing/page.hbs. This duplication increases maintenance burden and the risk of inconsistencies.Consider consolidating these into a single template or clearly documenting when each should be used.
src/models/billingEngineKey.js (1)
30-37: Consider enabling Sequelize automatic timestamps.The model has manual
CREATED_ATandUPDATED_ATfields but setstimestamps: false. For consistency and to leverage Sequelize's automatic timestamp management, consider enablingtimestamps: trueand using the standardcreatedAt/updatedAtnaming or configuring custom field names.🔎 Alternative approach
CREATED_AT: { type: DataTypes.DATE, - defaultValue: DataTypes.NOW + field: 'CREATED_AT' }, UPDATED_AT: { type: DataTypes.DATE, - defaultValue: DataTypes.NOW + field: 'UPDATED_AT' } }, { tableName: 'DP_BILLING_ENGINE_KEYS', indexes: [ { fields: ['ORG_ID', 'BILLING_ENGINE'], unique: true } ], - timestamps: false + timestamps: true, + createdAt: 'CREATED_AT', + updatedAt: 'UPDATED_AT' });Also applies to: 43-43
src/controllers/authController.js (2)
252-263: Remove or reduce debug logging.The debug logging block (lines 258-263) exposes potentially sensitive session information. Consider removing this or wrapping it in a development-mode check.
🔎 Proposed refactor
const renderBillingPage = async (req, res) => { try { const orgName = req.params.orgName; const viewName = req.params.viewName || 'default'; - // Debug: Log user session info - logger.info('Rendering billing page', { - hasUser: !!req.user, - hasAccessToken: !!req.user?.accessToken, - userEmail: req.user?.email, - isAuthenticated: req.isAuthenticated ? req.isAuthenticated() : false - }); - // Get orgId from the orgName in the URL
302-312: Avoid exposing error details in production.The error handler exposes the full error object in developer mode (line 304). While useful for debugging, consider using a more controlled approach that doesn't leak implementation details.
🔎 Proposed improvement
const errorContent = { message: 'Failed to load billing page', - error: config.devportalMode === 'developer' ? error : {} + error: (config.devportalMode === 'developer' && config.mode === 'development') + ? { message: error.message, stack: error.stack } + : {} };src/styles/billing.css (2)
204-218: Overlapping media queries with redundant rules.The breakpoints at
max-width: 767px(line 204) andmax-width: 768px(line 214) are nearly identical and both set the samemargin-bottom: 16pxfor.stat-card. Consider consolidating these into a single media query.🔎 Suggested consolidation
@media (max-width: 767px) { .stat-card { margin-bottom: 16px; } .stat-cards-row { gap: 12px; } } - -@media (max-width: 768px) { - .stat-card { - margin-bottom: 16px; - } -}
46-75: Hardcoded colors mixed with CSS variables.The billing tabs use hardcoded colors (e.g.,
#6c757d,#0d6efd) while other parts of the stylesheet reference CSS variables likevar(--primary-main-color). For consistency and easier theming, consider using CSS variables for these colors as well.src/pages/billing/partials/billing-content.hbs (1)
67-113: Consider using responsive grid classes for stat cards.The stat cards use fixed
col-md-3which may cause layout issues on smaller screens. ThedefaultContentversion uses more responsive classes (col-xl-3 col-lg-3 col-md-6 col-sm-6). Consider aligning both templates for consistency.src/models/application.js (1)
147-152: Consider using ENUM for PAYMENT_STATUS to enforce valid values.The
PAYMENT_STATUSfield accepts any string, but based on typical billing workflows, it likely has a finite set of valid states (e.g., 'ACTIVE', 'PENDING', 'CANCELED', 'PAST_DUE'). Using an ENUM, similar toTOKEN_TYPEon line 102-104, would provide data integrity at the model level.🔎 Proposed fix
- PAYMENT_STATUS: { type: DataTypes.STRING, allowNull: false, defaultValue: 'ACTIVE' }, + PAYMENT_STATUS: { + type: DataTypes.ENUM, + values: ['PENDING', 'ACTIVE', 'CANCELED', 'PAST_DUE', 'INCOMPLETE'], + allowNull: false, + defaultValue: 'ACTIVE' + },src/controllers/invoiceController.js (2)
19-19: Misleading variable name:subscriptionServiceformonetizationService.The import names the module
subscriptionServicebut it's actuallymonetizationService. This inconsistency could cause confusion during maintenance.🔎 Proposed fix
-const subscriptionService = require("../services/monetizationService"); +const monetizationService = require("../services/monetizationService");Then update all usages from
subscriptionService.*tomonetizationService.*.
24-35:errorToResponseis duplicated across controllers.This utility function is identical in
usageController.js(line 57) andbillingController.js(line 106). Consider extracting it to a shared utility module to reduce duplication.src/defaultContent/pages/api-landing/partials/api-subscription-plans.hbs (1)
156-156: Clarify the empty string parameter insubscribe()calls.Both free subscription handlers pass an empty string as the second argument:
subscribe('{{@root.orgID}}', '', ...). If this represents an optionalappId, consider using a named constant or documenting the parameter's purpose for maintainability.Also applies to: 170-170
src/routes/devportalRoute.js (1)
192-193: Remove commented-out code.The TODO comment indicates
devportalController.logindoesn't exist. Either implement the route or remove this dead code entirely rather than leaving it commented.src/services/moesifService.js (3)
55-59: Log or include fetched error text in the exception.The error text is fetched at line 56 but never used. Either log it for debugging purposes or include it in the error details.
Proposed fix
if (!res.ok) { const text = await res.text(); - throw new BadRequestError("Failed to fetch plans from Moesif", { status: res.status }); + logger.error({ status: res.status, text }, "Failed to fetch plans from Moesif"); + throw new BadRequestError("Failed to fetch plans from Moesif", { status: res.status, detail: text }); }
121-124: Same pattern: fetched error text is unused.This occurs in multiple places (lines 56, 68, 122, 169, 195, 231, 275, 321). Consider a helper function to consistently handle Moesif API errors.
236-238: Remove redundant catch-rethrow block.The catch block at lines 236-238 only rethrows the error without any additional handling. This is unnecessary.
Proposed fix
- try { const res = await fetch(`https://api.moesif.com/v1/search/~/users/${encodeURIComponent(userId)}`, { method: "POST", headers: { Authorization: `Bearer ${token}`, "Content-Type": "application/json", }, body: JSON.stringify(userPayload), }); if (!res.ok) { const text = await res.text(); throw new BadRequestError("Failed to create/update user in Moesif", { status: res.status }); } return res.json(); - } catch (e) { - throw e; - }src/dao/admin.js (2)
880-903: Movesequelizerequire to top-level imports.Requiring
sequelizeinside the function body at line 880 is unusual. While functional, it's better to import at the module level for consistency and slight performance improvement.Proposed fix
Add at top of file (around line 19):
const sequelize = require('../db/sequelize');Then remove line 880 from the function.
1093-1096: Consider using PAYMENT_STATUS constants instead of hardcoded string.The filter at line 1095 correctly uses
'ACTIVE'(which matches the database default value), but it's inconsistent with the codebase pattern. Constants forPAYMENT_STATUSare defined insrc/services/monetizationService.js(PAYMENT_STATUS.ACTIVE = "ACTIVE") andsrc/services/stripeService.js. For maintainability and consistency, import and use these constants instead of the hardcoded string'ACTIVE'.src/scripts/billing.js (2)
19-61: Remove or conditionally disable debug console.log statements.Extensive debug logging (lines 19, 23-24, 28, 31-34, 36, 43, 48, 60, etc.) throughout the file should be removed or wrapped in a development flag before production deployment.
729-738: Avoid falling back to 'default' orgId.Falling back to
'default'when orgId is not found (line 737) could lead to unexpected behavior or security issues. Consider showing an error message instead.Proposed fix
// Fallback: try to extract from URL path (e.g., /hiranyakavi/views/default/billing) const pathParts = window.location.pathname.split('/'); if (pathParts.length > 1 && pathParts[1]) { console.log('Using orgId from URL path:', pathParts[1]); return pathParts[1]; } - console.warn('No orgId found, using default'); - return 'default'; + console.error('No orgId found in page context or URL'); + return null;src/scripts/subscription.js (2)
767-772: Avoid hardcoded external URL for subscription icon.The subscription icon URL at line 767 points to an external GitHub raw content URL. This creates a dependency on external availability and may break if the repository changes.
Proposed fix
Host the icon locally within the project assets:
- const subscriptionIconHtml = `<img src="https://raw.githubusercontent.com/wso2/docs-bijira/refs/heads/main/en/devportal-theming/success-rounded.svg" + const subscriptionIconHtml = `<img src="/assets/icons/success-rounded.svg" alt="Subscribed" class="subscription-icon" style="display: inline-block;" />`;
394-486: Remove extensive debug logging before production.The
openStripeEmbeddedCheckoutfunction contains ~20 console.log statements for debugging. Consider removing these or using a configurable logger.src/controllers/billingController.js (2)
81-91: Use logger consistently instead of mixing console.log.The controller mixes
console.log(lines 81, 108, 244, 254, 260-261, 266, 273, 284, 288, 295-296, 300) with structuredloggercalls. Use the logger consistently for production code.
336-337: Move require statements to top-level imports.Lines 336-337, 361-362, 387, 408-409 use inline
require()calls. Move these to the top of the file for clarity and slight performance improvement.src/services/monetizationService.js (1)
357-362: Use logger instead of console.error for Moesif integration failures.Lines 357-362 use
console.logandconsole.errorfor Moesif integration logging. Use the structured logger for consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (40)
artifacts/script.sqlpackage.jsonsrc/app.jssrc/controllers/apiContentController.jssrc/controllers/authController.jssrc/controllers/billingController.jssrc/controllers/invoiceController.jssrc/controllers/usageController.jssrc/dao/admin.jssrc/dao/apiMetadata.jssrc/defaultContent/layout/main.hbssrc/defaultContent/pages/api-landing/partials/api-subscription-plans.hbssrc/defaultContent/pages/apis/partials/api-listing.hbssrc/defaultContent/pages/apis/partials/subscription-plans.hbssrc/defaultContent/pages/billing/page.hbssrc/defaultContent/pages/billing/partials/billing-content.hbssrc/defaultContent/partials/header.hbssrc/dto/apiDTO.jssrc/dto/subscriptionPolicy.jssrc/middlewares/billingAuth.jssrc/models/apiMetadata.jssrc/models/application.jssrc/models/billingEngineKey.jssrc/models/subscriptionPolicy.jssrc/pages/billing/page.hbssrc/pages/billing/partials/billing-content.hbssrc/pages/layout/main.hbssrc/routes/authRoute.jssrc/routes/devportalRoute.jssrc/scripts/billing.jssrc/scripts/common.jssrc/scripts/subscription.jssrc/services/adminService.jssrc/services/moesifService.jssrc/services/monetizationService.jssrc/services/orgBillingKeyService.jssrc/services/stripeService.jssrc/styles/billing.csssrc/utils/cryptoUtil.jssrc/utils/util.js
🧰 Additional context used
🧬 Code graph analysis (16)
src/models/billingEngineKey.js (7)
src/dao/admin.js (1)
sequelize(880-880)src/models/apiMetadata.js (1)
sequelize(19-19)src/models/application.js (1)
sequelize(19-19)src/models/subscriptionPolicy.js (1)
sequelize(19-19)src/services/adminService.js (1)
sequelize(29-29)src/services/monetizationService.js (1)
sequelize(20-20)src/services/orgBillingKeyService.js (1)
BillingEngineKey(1-1)
src/routes/authRoute.js (3)
src/routes/devportalRoute.js (4)
billingController(25-25)require(28-28)require(32-32)constants(33-33)src/middlewares/ensureAuthenticated.js (4)
require(24-24)require(25-25)require(27-27)constants(20-20)src/controllers/billingController.js (1)
constants(24-24)
src/controllers/authController.js (6)
src/controllers/billingController.js (3)
orgName(283-283)adminDao(275-275)util(25-25)src/dao/admin.js (1)
logger(27-27)src/utils/util.js (4)
logger(23-23)adminDao(25-25)constants(26-26)config(31-31)src/services/monetizationService.js (1)
adminDao(43-43)src/models/application.js (1)
constants(22-22)src/routes/authRoute.js (1)
constants(24-24)
src/utils/util.js (1)
src/scripts/api-key-generation.js (1)
subscriptionPlan(62-62)
src/middlewares/billingAuth.js (2)
src/app.js (4)
logger(33-33)require(24-24)require(28-28)require(34-34)src/controllers/billingController.js (14)
logger(23-23)require(22-22)require(336-336)require(361-361)require(409-409)req(72-72)req(242-242)req(332-332)req(357-357)req(432-432)req(453-453)req(473-473)req(493-493)req(512-512)
src/controllers/invoiceController.js (2)
src/controllers/usageController.js (1)
errorToResponse(58-58)src/controllers/billingController.js (6)
errorToResponse(107-107)errorToResponse(141-141)errorToResponse(159-159)errorToResponse(176-176)errorToResponse(197-197)errorToResponse(217-217)
src/utils/cryptoUtil.js (1)
src/controllers/billingController.js (4)
require(22-22)require(336-336)require(361-361)require(409-409)
src/controllers/usageController.js (2)
src/controllers/invoiceController.js (8)
subscriptionService(19-19)require(20-20)logger(21-21)errorToResponse(45-45)errorToResponse(59-59)errorToResponse(72-72)errorToResponse(90-90)errorToResponse(104-104)src/services/monetizationService.js (2)
logger(22-22)usage(626-630)
src/scripts/common.js (1)
src/scripts/subscription.js (6)
subscribeButton(271-271)subscribeButton(605-605)subscribeButton(852-852)card(270-270)card(604-604)card(850-850)
src/dto/subscriptionPolicy.js (1)
src/services/apiMetadataService.js (2)
subscriptionPolicy(768-768)subscriptionPolicy(869-869)
src/services/orgBillingKeyService.js (1)
src/models/billingEngineKey.js (1)
BillingEngineKey(4-44)
src/dao/apiMetadata.js (2)
src/models/apiSubscriptionPolicy.js (2)
APISubscriptionPolicy(23-38)SubscriptionPolicy(20-20)src/models/subscriptionPolicy.js (1)
SubscriptionPolicy(22-77)
src/routes/devportalRoute.js (1)
src/routes/authRoute.js (2)
billingController(21-21)router(19-19)
src/dao/admin.js (1)
src/models/application.js (1)
SubscriptionMapping(113-158)
src/services/stripeService.js (2)
src/services/monetizationService.js (6)
stripeSecretKey(426-426)stripeSecretKey(451-451)stripeSecretKey(460-460)stripeSecretKey(476-476)stripeSecretKey(497-497)stripeSecretKey(533-533)src/dao/admin.js (1)
getSubscription(709-724)
src/controllers/billingController.js (2)
src/dao/admin.js (5)
require(18-18)require(19-19)require(20-20)require(21-21)logger(27-27)src/models/billingEngineKey.js (1)
BillingEngineKey(4-44)
🪛 Biome (2.1.2)
src/services/stripeService.js
[error] 201-201: Illegal return statement outside of a function
(parse)
[error] 205-205: Illegal return statement outside of a function
(parse)
[error] 190-190: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (35)
package.json (2)
58-58: Downgrade fromnode-fetchv3 to v2 for CommonJS compatibility.This change from v3.x (ESM-only) to v2.x (CommonJS) is a common pattern for Node.js projects that haven't migrated to ES modules. Ensure this is intentional and that all fetch usages are compatible with v2 API.
74-74: Stripe package version is valid and up-to-date. The version^20.0.0resolves to [email protected] (or potentially 20.1.0), which is an active, non-deprecated release in the npm registry. No deprecation warnings are present. Consider monitoring for updates as the latest version in the 20.x line is currently 20.1.0.src/defaultContent/partials/header.hbs (2)
12-12: Good fix: Properly quoted imagesrcattributes.The change from
src={{profile.imageURL}}tosrc="{{profile.imageURL}}"ensures proper HTML attribute syntax and prevents potential parsing issues with special characters in URLs.Also applies to: 18-18
28-31: LGTM: Billing menu item added to authenticated user dropdown.The new Billing link is properly placed within the authenticated user context (
{{#if profile}}) and follows the same pattern as the existing logout item.src/middlewares/billingAuth.js (1)
25-45: Middleware logic is sound with proper defensive checks.The guard
req.isAuthenticated && req.isAuthenticated()correctly handles cases whereisAuthenticatedmight not be defined on the request object.src/services/adminService.js (2)
1204-1214: LGTM: Billing data extraction for paid subscriptions.The code correctly checks for both billing fields before constructing the billingData object, ensuring incomplete billing info isn't propagated.
1396-1418: LGTM: Optional billing metadata propagation to control plane.The billingMetadata is conditionally added to the subscription request only when billing data is present. Note that the 409 conflict fallback (lines 1421-1423) returns the existing subscription without updating billing metadata - verify this is the intended behavior for re-subscriptions.
src/models/subscriptionPolicy.js (1)
49-72: LGTM: New pricing-related fields for monetization support.The new fields appropriately model flexible pricing configurations with JSONB for extensible metadata. All fields being nullable ensures backward compatibility with existing subscription policies.
src/utils/util.js (1)
809-814: LGTM - Subscription plan details enriched with pricing information.The additional pricing fields (
pricingModel,currency,billingPeriod,flatAmount,unitAmount,pricingMetadata) properly extend subscription plan objects to support the new monetization features. The implementation correctly propagates these fields from the loaded subscription plan.src/models/billingEngineKey.js (1)
4-44: LGTM - Billing key model properly structured.The
BillingEngineKeymodel correctly defines:
- Encrypted key storage with
_ENCsuffix naming- Unique constraint on
(ORG_ID, BILLING_ENGINE)to prevent duplicate entries- Manual timestamp fields
src/routes/authRoute.js (1)
21-21: ThebillingControllerimport is actively used on line 28.The import is correctly utilized for the
/billing/returnroute handler (billingController.handleBillingReturn). The controller is properly implemented with multiple billing endpoints (checkout sessions, subscriptions, billing portals, payment methods, etc.), so the single usage in this route file is intentional and appropriate.src/models/apiMetadata.js (1)
106-110: LGTM - Monetization flag added correctly.The
MONETIZATION_ENABLEDfield is properly defined as a nullable boolean with a default value offalsein both the Sequelize model and the database schema (artifacts/script.sql). This allows incremental adoption of monetization features per API.src/app.js (2)
276-277: LGTM! Stripe webhook raw body parsing is correctly positioned.The raw body parser for
/webhooks/stripeis placed beforeexpress.json(), which is required for Stripe signature verification to work correctly.
155-191: LGTM! Well-implemented compare helper.The helper properly validates argument count, handles all common comparison operators, and throws informative errors for unsupported operators.
src/defaultContent/pages/apis/partials/subscription-plans.hbs (2)
79-93: Verify pricing model fallback logic.The
{{else}}block of{{#compare pricingModel "===" "FLAT"}}at line 85 handles all non-FLAT pricing as PER_UNIT. If additional pricing models are added in the future (e.g., TIERED), they would incorrectly render as PER_UNIT pricing. Consider explicitly checking for PER_UNIT or adding a comment clarifying this is intentional.
112-123: LGTM! Subscribe button data attributes are well-structured.The button correctly exposes
data-policy-id,data-policy-name,data-external-price-id, anddata-is-paidattributes for the client-side subscription flow.src/defaultContent/pages/billing/partials/billing-content.hbs (2)
27-43: LGTM! Well-structured accessible tab navigation.The tab navigation uses proper ARIA attributes (
role="tablist",role="presentation",aria-controls,aria-selected) for accessibility compliance.
66-112: LGTM! Stat cards with appropriate loading states.The stat cards layout is responsive and includes placeholder values (
--) that will be populated dynamically. The loading states in the usage table provide good UX feedback.src/controllers/apiContentController.js (2)
94-102: LGTM! Subscription policy augmentation is well-implemented.The code correctly populates
subscriptionPolicywithpolicyIdandpolicyNamewhen a subscription exists, with a sensible fallback to'Unknown'if the policy is not found in the metadata.
358-366: LGTM! Consistent implementation with loadAPIs.The same subscription policy augmentation pattern is correctly applied in
loadAPIContent, maintaining consistency across both code paths.src/controllers/usageController.js (2)
24-35: LGTM! Error handling follows established pattern.The
errorToResponsehelper correctly handlesCustomErrorinstances and provides a sensible fallback for unexpected errors, matching the pattern used ininvoiceController.js.
57-60: No action required — logger format is consistent.The
logger.error({ err, orgId, subId }, "message")format in usageController.js is consistent with related billing and usage files (invoiceController.js, billingController.js, monetizationService.js). This format is the established convention for the billing/usage domain in the codebase.src/dto/apiDTO.js (2)
29-33: LGTM!The monetization info initialization correctly defaults to
falsewhenMONETIZATION_ENABLEDis undefined or falsy.
93-102: LGTM!The pricing fields are properly mapped from the subscription policy object, maintaining consistency with the SubscriptionPolicy DTO pattern.
src/dto/subscriptionPolicy.js (1)
28-35: LGTM!The pricing field mappings are consistent with the
APISubscriptionPolicyclass and align with the database schema additions.src/defaultContent/pages/apis/partials/api-listing.hbs (1)
103-139: LGTM!The pricing chips UI properly handles FLAT vs PER_UNIT pricing models with appropriate fallbacks to "Free" when amounts are missing.
src/dao/apiMetadata.js (4)
430-442: LGTM!The
toUpperandcomputeRequestCounthelpers properly handle edge cases with null/undefined inputs and correctly normalize request/event count values.
444-496: Good refactoring to centralize subscription policy row construction.The
buildPricingMetadataandbuildSubscriptionPolicyRowhelpers consolidate previously duplicated logic. Minor note: on line 488,toUpper(policy.pricingModel) ?? nullis slightly redundant sincetoUpperalready returnsnullfor falsy inputs, but this doesn't affect correctness.
498-510: LGTM!The refactored
createAPISubscriptionPolicycorrectly supports bothpolicyIdandpolicyIDnaming conventions for API compatibility.
562-583: LGTM!Properly removes
ORG_IDandPOLICY_IDfrom update payload to prevent modifying primary keys. Thereturning: trueoption ensures the updated row is returned.src/controllers/invoiceController.js (1)
96-108: Verifyhosted_invoice_urlorigin before redirecting.The redirect uses the URL returned from Stripe without validation. While Stripe-returned URLs should be trusted, consider validating that the URL hostname matches expected Stripe domains (
*.stripe.com) as a defense-in-depth measure against potential data corruption or injection.src/defaultContent/pages/api-landing/partials/api-subscription-plans.hbs (1)
15-59: LGTM!The pricing display logic correctly handles FLAT vs PER_UNIT models with appropriate fallbacks and clear visual layout for price and rate limit information.
src/routes/devportalRoute.js (1)
161-190: Well-structured billing routes with appropriate middleware.The new billing routes are properly organized under
/organizations/:orgIdpaths withensureBillingAuthmiddleware. The Stripe webhook at line 190 correctly omits authentication since webhook verification is handled via Stripe signature in the controller.src/dao/admin.js (1)
793-828: Consider using Sequelize'supdatewith direct field mapping.The
updateBillingFieldsfunction manually checks each field. This is safe but verbose. Consider a whitelist-based approach for maintainability.src/scripts/billing.js (1)
512-538: I am ready to rewrite a review comment, but I need you to provide the review comment that requires verification and rewriting. Please share the review comment within<review_comment>tags, along with any relevant context about the code being reviewed.
| CREATE INDEX "IDX_DP_SUB_ORG_APP_API" ON "DP_API_SUBSCRIPTION" ("ORG_ID","APP_ID","API_ID"); | ||
|
|
||
| CREATE INDEX "IDX_DP_SUB_BILLING_SUB" ON "DP_API_SUBSCRIPTION" ("BILLING_SUBSCRIPTION_ID"); | ||
|
|
||
| CREATE INDEX "IDX_DP_SUB_BILLING_CUSTOMER" ON "DP_API_SUBSCRIPTION" ("BILLING_CUSTOMER_ID"); |
There was a problem hiding this comment.
Indexes reference columns that don't exist yet in CREATE TABLE.
The indexes IDX_DP_SUB_BILLING_SUB and IDX_DP_SUB_BILLING_CUSTOMER reference BILLING_SUBSCRIPTION_ID and BILLING_CUSTOMER_ID columns, but these columns are only added via ALTER TABLE statements at lines 296-300. The CREATE TABLE for DP_API_SUBSCRIPTION (lines 208-218) doesn't include these columns.
For a fresh database install, this script will fail because the indexes are created before the columns exist.
🔎 Proposed fix
Move the index creation statements after the ALTER TABLE statements, or add the columns directly in the CREATE TABLE statement:
CREATE TABLE "DP_API_SUBSCRIPTION" (
"SUB_ID" VARCHAR(256) NOT NULL,
"APP_ID" VARCHAR(256) NOT NULL,
"ORG_ID" VARCHAR(256) NOT NULL,
"API_ID" VARCHAR(256) NOT NULL,
"POLICY_ID" VARCHAR(256) NOT NULL,
+ "BILLING_CUSTOMER_ID" VARCHAR(256),
+ "BILLING_SUBSCRIPTION_ID" VARCHAR(256),
+ "PAYMENT_PROVIDER" VARCHAR(64),
+ "PAYMENT_STATUS" VARCHAR(64),
+ "CHECKOUT_SESSION_ID" VARCHAR(256),
PRIMARY KEY ("SUB_ID"),
...
);Then remove the corresponding ALTER TABLE statements at lines 296-300.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In artifacts/script.sql around lines 267 to 271, the two index statements
IDX_DP_SUB_BILLING_SUB and IDX_DP_SUB_BILLING_CUSTOMER reference
BILLING_SUBSCRIPTION_ID and BILLING_CUSTOMER_ID which are not present in the
CREATE TABLE for DP_API_SUBSCRIPTION (lines ~208-218) and are only added later
via ALTER TABLE (lines ~296-300); to fix, either move these CREATE INDEX
statements so they appear after the ALTER TABLE block that adds those columns,
or instead add BILLING_SUBSCRIPTION_ID and BILLING_CUSTOMER_ID directly into the
DP_API_SUBSCRIPTION CREATE TABLE and then remove the later ALTER TABLE
statements; ensure index creation happens only after the columns exist.
| -- DP_BILLING_ENGINE_KEYS: Billing engine keys for each org (Stripe, etc.) | ||
| CREATE TABLE IF NOT EXISTS "DP_BILLING_ENGINE_KEYS" ( | ||
| "ID" UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| "ORG_ID" UUID NOT NULL, | ||
| "BILLING_ENGINE" VARCHAR(32) NOT NULL, | ||
| "SECRET_KEY_ENC" TEXT NOT NULL, | ||
| "PUBLISHABLE_KEY_ENC" TEXT NOT NULL, | ||
| "WEBHOOK_SECRET_ENC" TEXT NOT NULL, | ||
| "CREATED_AT" TIMESTAMP DEFAULT NOW(), | ||
| "UPDATED_AT" TIMESTAMP DEFAULT NOW(), | ||
| UNIQUE ("ORG_ID", "BILLING_ENGINE") | ||
| ); | ||
| CREATE INDEX IF NOT EXISTS "IDX_BILLING_ENGINE_KEYS_ORG_ID" ON "DP_BILLING_ENGINE_KEYS"("ORG_ID"); |
There was a problem hiding this comment.
Type mismatch: ORG_ID is UUID here but VARCHAR(256) in DP_ORGANIZATION.
DP_BILLING_ENGINE_KEYS defines ORG_ID as UUID NOT NULL, but DP_ORGANIZATION (line 24) uses VARCHAR(256) for its primary key ORG_ID. This type mismatch will prevent establishing a proper foreign key relationship and may cause join/lookup issues.
🔎 Proposed fix
CREATE TABLE IF NOT EXISTS "DP_BILLING_ENGINE_KEYS" (
"ID" UUID PRIMARY KEY DEFAULT gen_random_uuid(),
- "ORG_ID" UUID NOT NULL,
+ "ORG_ID" VARCHAR(256) NOT NULL,
"BILLING_ENGINE" VARCHAR(32) NOT NULL,
...
+ FOREIGN KEY ("ORG_ID") REFERENCES "DP_ORGANIZATION"("ORG_ID") ON DELETE CASCADE
);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In artifacts/script.sql around lines 273 to 285, DP_BILLING_ENGINE_KEYS defines
ORG_ID as UUID while DP_ORGANIZATION (line 24) defines ORG_ID as VARCHAR(256);
change DP_BILLING_ENGINE_KEYS.ORG_ID to VARCHAR(256) to match the organization
table (or alternatively change DP_ORGANIZATION to UUID if you intend UUIDs
everywhere), then add an explicit FOREIGN KEY constraint referencing
DP_ORGANIZATION(ORG_ID) and ensure the index and any related code use the same
type to avoid join and FK failures.
| } | ||
|
|
||
| const orgName = org.IDENTIFIER || 'default'; | ||
| console.log("💳 Found org name:", orgName); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Organization model fields
rg -n "IDENTIFIER|ORG_HANDLE|ORGANIZATION_IDENTIFIER" src/models/organization.js -C2Repository: wso2/api-developer-portal-core
Length of output: 332
🏁 Script executed:
#!/bin/bash
# Search entire organization.js for IDENTIFIER field
echo "=== Checking entire organization.js for IDENTIFIER ==="
rg -n "IDENTIFIER" src/models/organization.js
echo -e "\n=== Checking billingController.js context around line 283 ==="
sed -n '275,290p' src/controllers/billingController.js
echo -e "\n=== Checking how org object is retrieved in billingController.js ==="
rg -n "org\s*=" src/controllers/billingController.js -B2 -A2 | head -40Repository: wso2/api-developer-portal-core
Length of output: 1375
🏁 Script executed:
#!/bin/bash
# Check the getOrganization implementation in admin.js
echo "=== Checking getOrganization in admin.js ==="
rg -n "getOrganization" src/dao/admin.js -A10
echo -e "\n=== Check other usages of org object in codebase ==="
rg -n "org\.(ORG_HANDLE|ORGANIZATION_IDENTIFIER|IDENTIFIER)" src/ -C1 | head -50Repository: wso2/api-developer-portal-core
Length of output: 1800
Change org.IDENTIFIER to org.ORG_HANDLE.
The Organization model does not have an IDENTIFIER field. The correct field for organization identifiers is ORG_HANDLE (used as the URL-friendly identifier), or alternatively ORGANIZATION_IDENTIFIER or ORG_NAME. Since line 285 constructs a URL path with this value (/${orgName}/apis/...), ORG_HANDLE is the appropriate choice.
🤖 Prompt for AI Agents
In src/controllers/billingController.js around lines 281 to 284, the code reads
org.IDENTIFIER which does not exist on the Organization model; replace
org.IDENTIFIER with org.ORG_HANDLE (the URL-friendly identifier) and keep the
same fallback (e.g., 'default') so the constructed path `/${orgName}/apis/...`
uses the correct field; if ORG_HANDLE may be absent in some records, consider
falling back to ORGANIZATION_IDENTIFIER or ORG_NAME in order.
| module.exports = { | ||
| createCheckoutSessionForSubscription, | ||
| registerStripeCheckoutSession, | ||
| cancelSubscription, | ||
| getSubscriptionBillingStatus, | ||
| createBillingPortal, | ||
| createBillingPortalByOrg, | ||
| handleStripeWebhook, | ||
| handleBillingReturn, | ||
| getUsageData, | ||
| getPaymentMethods, | ||
| removePaymentMethod, | ||
| getBillingInfo, | ||
| getActiveSubscriptions | ||
| , addBillingEngineKeys | ||
| , updateBillingEngineKeys | ||
| , deleteBillingEngineKeys | ||
| , getBillingEngineKeys | ||
| }; |
There was a problem hiding this comment.
Module exports reference functions defined after this block.
module.exports at lines 307-325 references getUsageData, getPaymentMethods, removePaymentMethod, getBillingInfo, getActiveSubscriptions, addBillingEngineKeys, updateBillingEngineKeys, deleteBillingEngineKeys, and getBillingEngineKeys, but these functions are defined after line 325. Move module.exports to the end of the file.
Proposed fix
Move the module.exports block to the end of the file (after line 424).
🤖 Prompt for AI Agents
In src/controllers/billingController.js around lines 307-325, the module.exports
block is placed before several functions it references (getUsageData,
getPaymentMethods, removePaymentMethod, getBillingInfo, getActiveSubscriptions,
addBillingEngineKeys, updateBillingEngineKeys, deleteBillingEngineKeys,
getBillingEngineKeys); cut this entire module.exports block and paste it at the
end of the file (after line 424) so it exports the functions after they are
defined, preserving the same export order and commas and ensuring the file ends
with the single module.exports statement.
| {{#if @root.apiMetadata.monetizationInfo.enabled}} | ||
| {{!-- Monetization enabled - check if plan is paid --}} | ||
| {{#if externalPriceId}} | ||
| {{!-- Paid plan with Stripe price ID --}} | ||
| <a | ||
| id="subscribe-btn-{{policyID}}" | ||
| type="button" | ||
| class="common-btn-primary subscribe-btn {{#if @root.isReadOnlyMode}}disabled{{/if}}" | ||
| data-policy-id="{{policyID}}" | ||
| data-policy-name="{{policyName}}" | ||
| data-external-price-id="{{externalPriceId}}" | ||
| data-is-paid="true" | ||
| onclick="showSubscribeButtonLoading(this); handleSubscribeWithPayment('{{@root.orgID}}','{{../apiMetadata.apiID}}','{{../apiMetadata.apiReferenceID}}','{{policyID}}','{{policyName}}', this)" | ||
| > | ||
| Subscribe & Pay | ||
| </a> | ||
| {{else}} | ||
| {{!-- Free plan --}} | ||
| <a | ||
| id="subscribe-btn-{{policyID}}" | ||
| type="button" | ||
| class="common-btn-primary subscribe-btn {{#if @root.isReadOnlyMode}}disabled{{/if}}" | ||
| data-policy-id="{{policyID}}" | ||
| data-policy-name="{{policyName}}" | ||
| data-is-paid="false" | ||
| onclick="showSubscribeButtonLoading(this); subscribe('{{@root.orgID}}', '', '{{../apiMetadata.apiID}}', '{{../apiMetadata.apiReferenceID}}', '{{policyID}}', '{{policyName}}')" | ||
| > | ||
| Subscribe | ||
| </a> | ||
| {{/if}} | ||
| {{else}} | ||
| {{!-- Monetization disabled - all plans are free --}} | ||
| <a | ||
| id="subscribe-btn-{{policyID}}" | ||
| type="button" | ||
| class="common-btn-primary subscribe-btn {{#if @root.isReadOnlyMode}}disabled{{/if}}" | ||
| data-policy-id="{{policyID}}" | ||
| data-policy-name="{{policyName}}" | ||
| data-is-paid="false" | ||
| onclick="showSubscribeButtonLoading(this); subscribe('{{@root.orgID}}', '', '{{../apiMetadata.apiID}}', '{{../apiMetadata.apiReferenceID}}', '{{policyID}}', '{{policyName}}')" | ||
| > | ||
| Subscribe | ||
| </a> | ||
| {{/if}} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for externalPriceId usage in JS files to understand how it's exposed
rg -n "externalPriceId" --type js -C 3 | head -100Repository: wso2/api-developer-portal-core
Length of output: 2880
🏁 Script executed:
#!/bin/bash
# Find where api-subscription-plans.hbs is rendered and what data is passed
rg -n "api-subscription-plans" --type js -C 3Repository: wso2/api-developer-portal-core
Length of output: 573
🏁 Script executed:
#!/bin/bash
# Find the template file and search for route handlers
find . -name "*.js" -type f | xargs rg -l "api-landing" | head -20Repository: wso2/api-developer-portal-core
Length of output: 153
🏁 Script executed:
#!/bin/bash
# Examine the apiContentController.js to see how the template is rendered
cat -n src/controllers/apiContentController.js | head -150Repository: wso2/api-developer-portal-core
Length of output: 8105
🏁 Script executed:
#!/bin/bash
# Continue reading apiContentController.js to find api-landing page rendering
cat -n src/controllers/apiContentController.js | sed -n '150,350p'Repository: wso2/api-developer-portal-core
Length of output: 12204
🏁 Script executed:
#!/bin/bash
# Continue reading to see how subscription plans are rendered
cat -n src/controllers/apiContentController.js | sed -n '350,450p'Repository: wso2/api-developer-portal-core
Length of output: 5991
🏁 Script executed:
#!/bin/bash
# Now let's find appendSubscriptionPlanDetails function in util.js
rg -n "appendSubscriptionPlanDetails" --type js -A 30 | head -80Repository: wso2/api-developer-portal-core
Length of output: 6470
🏁 Script executed:
#!/bin/bash
# Check the subscriptionPolicyDTO to see what properties it exposes
rg -n "class subscriptionPolicyDTO\|function subscriptionPolicyDTO" --type js -A 40Repository: wso2/api-developer-portal-core
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Find the subscriptionPolicyDTO file
fd "subscriptionPolicy" --type f --extension jsRepository: wso2/api-developer-portal-core
Length of output: 136
🏁 Script executed:
#!/bin/bash
# Read the subscriptionPolicyDTO file
cat -n src/dto/subscriptionPolicy.jsRepository: wso2/api-developer-portal-core
Length of output: 1927
🏁 Script executed:
#!/bin/bash
# Check the buildPricingMetadata function and how pricingMetadata is populated
rg -n "buildPricingMetadata\|PRICING_METADATA" --type js -B 2 -A 20 | head -100Repository: wso2/api-developer-portal-core
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Check apiMetadata.js to see how pricingMetadata is built
cat -n src/dao/apiMetadata.js | sed -n '440,460p'Repository: wso2/api-developer-portal-core
Length of output: 852
🏁 Script executed:
#!/bin/bash
# Find where buildPricingMetadata is called
rg -n "buildPricingMetadata" --type js -B 2 -A 5Repository: wso2/api-developer-portal-core
Length of output: 904
🏁 Script executed:
#!/bin/bash
# Verify if externalPriceId is ever added to the plan objects passed to template
# Check if there's any code that flattens or adds externalPriceId to subscription plans
rg -n "externalPriceId" src/utils/util.js src/controllers/apiContentController.js src/dto/subscriptionPolicy.jsRepository: wso2/api-developer-portal-core
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Check if externalPriceId is accessed anywhere in the template
rg -n "externalPriceId" --type hbsRepository: wso2/api-developer-portal-core
Length of output: 946
🏁 Script executed:
#!/bin/bash
# Check the structure more carefully - look at what buildPricingMetadata returns
cat -n src/dao/apiMetadata.js | sed -n '444,480p'Repository: wso2/api-developer-portal-core
Length of output: 1530
Fix incorrect property access for external price ID in subscription plan check.
Line 133 checks {{#if externalPriceId}} directly on the plan object, but externalPriceId is not exposed at the top level. According to buildPricingMetadata in src/dao/apiMetadata.js (lines 448-452), the external price ID is nested under pricingMetadata.external.priceId. Change the condition to {{#if pricingMetadata.external.priceId}} and update the corresponding data attribute on line 141 to use the same nested path. Also check src/defaultContent/pages/apis/partials/api-listing.hbs and src/defaultContent/pages/apis/partials/subscription-plans.hbs, which incorrectly access pricingMetadata.externalPriceId and should use pricingMetadata.external.priceId instead.
🤖 Prompt for AI Agents
In src/defaultContent/pages/api-landing/partials/api-subscription-plans.hbs
around lines 131 to 174, the template checks and uses a non-existent top-level
externalPriceId; change the condition to use the nested property
pricingMetadata.external.priceId and update the data attribute
(data-external-price-id) to read from pricingMetadata.external.priceId as well;
also search and fix similar incorrect accesses in
src/defaultContent/pages/apis/partials/api-listing.hbs and
src/defaultContent/pages/apis/partials/subscription-plans.hbs where
pricingMetadata.externalPriceId should be replaced with
pricingMetadata.external.priceId.
| // Get all subscriptions for the user in this organization | ||
| let subscriptions = []; | ||
| try { | ||
| subscriptions = await adminDao.getAllSubscriptions(orgId, userContext.userId); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file structure and find the dao/admin.js file
find . -name "admin.js" -o -name "monetizationService.js" | head -20Repository: wso2/api-developer-portal-core
Length of output: 130
🏁 Script executed:
# Search for the specific method calls mentioned in the review
rg "getAllSubscriptions|getSubscriptionPolicy|getAPIMetadata" src/services/ -nRepository: wso2/api-developer-portal-core
Length of output: 2441
🏁 Script executed:
# Check the admin.js file to see what methods are exported
rg -n "module.exports|exports\." src/dao/admin.js | head -100Repository: wso2/api-developer-portal-core
Length of output: 97
🏁 Script executed:
# Read the module.exports section of admin.js to see all exported methods
cat -n src/dao/admin.js | tail -150Repository: wso2/api-developer-portal-core
Length of output: 5401
🏁 Script executed:
# Also search for the method definitions themselves in admin.js
rg -n "getAllSubscriptions|getSubscriptionPolicy|getAPIMetadata" src/dao/admin.jsRepository: wso2/api-developer-portal-core
Length of output: 179
🏁 Script executed:
# Check if adminDao is imported in monetizationService and how
rg -n "adminDao|require.*admin" src/services/monetizationService.js | head -20Repository: wso2/api-developer-portal-core
Length of output: 1383
🏁 Script executed:
# Verify if these methods might exist in other dao files
rg "getAllSubscriptions|getSubscriptionPolicy|getAPIMetadata" src/dao/ -lRepository: wso2/api-developer-portal-core
Length of output: 113
🏁 Script executed:
# Check if these exact methods are defined anywhere in admin.js
cat -n src/dao/admin.js | grep -E "^[[:space:]]*(const|async|function).*(getAllSubscriptions|getSubscriptionPolicy|getAPIMetadata)"Repository: wso2/api-developer-portal-core
Length of output: 56
🏁 Script executed:
# Check if these methods are in apiMetadata.js dao
rg -n "getAllSubscriptions|getSubscriptionPolicy|getAPIMetadata" src/dao/apiMetadata.js | head -20Repository: wso2/api-developer-portal-core
Length of output: 552
🏁 Script executed:
# Check what adminDao is being used for - verify its actual source
grep -n "adminDao" src/services/monetizationService.js | grep -E "(require|import)"Repository: wso2/api-developer-portal-core
Length of output: 118
🏁 Script executed:
# Verify if getAllSubscriptions exists anywhere in the codebase
rg -n "const getAllSubscriptions|function getAllSubscriptions" src/Repository: wso2/api-developer-portal-core
Length of output: 157
🏁 Script executed:
# Check the exact signatures of the methods in apiMetadata.js to compare
rg -n "const getSubscriptionPolicy|const getAPIMetadata" src/dao/apiMetadata.js -A 1Repository: wso2/api-developer-portal-core
Length of output: 384
🏁 Script executed:
# Check what methods are actually exported from apiMetadata.js
rg -n "module.exports" src/dao/apiMetadata.js -A 50 | head -80Repository: wso2/api-developer-portal-core
Length of output: 1419
🏁 Script executed:
# Double-check that adminDao.getAllSubscriptions is not in admin.js by searching for the exact pattern
rg "getAllSubscriptions" src/dao/admin.jsRepository: wso2/api-developer-portal-core
Length of output: 56
Missing adminDao methods will cause runtime errors.
The following methods are called on adminDao but do not exist in admin.js:
- Line 582:
adminDao.getAllSubscriptions(exists only in adminService.js as a service handler, not as a DAO method) - Line 636:
adminDao.getSubscriptionPolicy(exists in apiMetadata.js, not admin.js) - Line 651:
adminDao.getAPIMetadata(exists in apiMetadata.js, not admin.js)
The admin.js DAO exports getSubscriptionPolicyById (not getSubscriptionPolicy), and does not export getAPIMetadata or getAllSubscriptions. These calls will fail at runtime. Either these methods need to be added to admin.js, or the code should use the appropriate DAO (apiMetadata.js for policy/metadata methods) or service layer.
Also applies to: 626-626, 636-636, 651-651
🤖 Prompt for AI Agents
In src/services/monetizationService.js around lines 582, 626, 636 and 651, calls
to adminDao.getAllSubscriptions, adminDao.getSubscriptionPolicy and
adminDao.getAPIMetadata reference DAO methods that don't exist; either add those
functions to admin.js or, preferably, call the correct DAO methods (use the DAO
that actually exports them: e.g. replace adminDao.getSubscriptionPolicy with
apiMetadataDao.getSubscriptionPolicy or adminDao.getSubscriptionPolicyById as
appropriate, replace adminDao.getAPIMetadata with apiMetadataDao.getAPIMetadata,
and replace adminDao.getAllSubscriptions with the DAO that provides subscription
listing or implement a new getAllSubscriptions in admin.js), and update the
require/import statements at the top of the file to import apiMetadataDao (or
the correct DAO) and adjust parameter names/return handling to match the actual
DAO method signatures.
| const paymentMethods = await stripeService.listPaymentMethods(customerId); | ||
|
|
||
| // Get customer default payment method | ||
| const customer = await stripeService.getCustomer(customerId); | ||
| const defaultPaymentMethodId = customer.invoice_settings?.default_payment_method; |
There was a problem hiding this comment.
Missing stripeSecretKey parameter in Stripe service calls.
Lines 716, 719, 741, and 780 call stripeService methods without passing stripeSecretKey. The Stripe service requires per-org keys.
Proposed fix
async function getPaymentMethods(userContext, orgId) {
try {
// ... existing code ...
const customerId = stripeSubscription.BILLING_CUSTOMER_ID;
+ const { secretKey: stripeSecretKey } = await getDecryptedStripeKeysForOrg(orgId);
+
// Get payment methods from Stripe
- const paymentMethods = await stripeService.listPaymentMethods(customerId);
+ const paymentMethods = await stripeService.listPaymentMethods(customerId, stripeSecretKey);
// Get customer default payment method
- const customer = await stripeService.getCustomer(customerId);
+ const customer = await stripeService.getCustomer(customerId, stripeSecretKey);Apply similar fixes to removePaymentMethod (line 741) and getBillingInfo (line 780).
🤖 Prompt for AI Agents
In src/services/monetizationService.js around lines 716-720, 740-742 and
778-781, Stripe service methods are called without the required per-org
stripeSecretKey; update the calls to pass stripeSecretKey as the first (or
expected) argument when invoking stripeService.listPaymentMethods,
stripeService.getCustomer, stripeService.removePaymentMethod and
stripeService.getBillingInfo (use the stripeSecretKey available in scope or
passed into the function) so each call supplies the org-specific secret key.
|
|
||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Critical syntax error: Stray return statement outside function.
Line 161 contains return; outside of any function body. This appears to be a remnant of incomplete code or a merge issue. Combined with static analysis hints showing "Illegal return statement outside of a function", this file will fail to parse.
Analysis of the issue
The applyWebhookToLocalState function starting at line 142 is missing its closing brace. The early return at line 161 and subsequent code (lines 164-216) should be inside that function. Additionally, module.exports at line 218 appears before several function definitions (lines 236-289), causing those functions to be undefined when exported.
The file structure should be:
- Fix the missing
}forapplyWebhookToLocalState - Move
module.exportsto the end of the file after all function definitions
🤖 Prompt for AI Agents
In src/services/stripeService.js around lines 142–163 and onward, there's a
stray "return;" at line 161 because the applyWebhookToLocalState function is
missing its closing brace, leaving that return and subsequent code outside any
function and causing parse errors; close the applyWebhookToLocalState function
by adding the missing "}" just before that return so the return (and the rest of
the webhook-handling code) is inside the function, then move the module.exports
statement to the end of the file after all function definitions (lines ~236–289)
so exported functions are defined before export.
| module.exports = { | ||
| findOrCreateCustomerByEmail, | ||
| createCheckoutSession, | ||
| retrieveCheckoutSession, | ||
| cancelSubscription, | ||
| listInvoicesByCustomer, | ||
| listInvoices, | ||
| getInvoice, | ||
| createCustomerPortalSession, | ||
| createPortalSession, | ||
| verifyAndConstructWebhookEvent, | ||
| applyWebhookToLocalState, | ||
| listPaymentMethods, | ||
| getCustomer, | ||
| detachPaymentMethod, | ||
| getSubscription | ||
| }; |
There was a problem hiding this comment.
Module exports reference functions defined after this block.
The exports at lines 218-234 reference listInvoices, listPaymentMethods, getCustomer, detachPaymentMethod, getSubscription, and createPortalSession, but these functions are defined at lines 236-289. Move module.exports to the end of the file.
Proposed fix
Move the entire module.exports block to line 290, after all function definitions.
🤖 Prompt for AI Agents
In src/services/stripeService.js around lines 218-234 the module.exports block
references functions (listInvoices, listPaymentMethods, getCustomer,
detachPaymentMethod, getSubscription, createPortalSession) that are defined
later (lines ~236-289); move the entire module.exports block to the end of the
file (after the last function definition, ~line 290) so all exported identifiers
are defined before export.
| @@ -0,0 +1,23 @@ | |||
| const crypto = require('crypto'); | |||
|
|
|||
| const ENCRYPTION_KEY = process.env.BILLING_KEY_ENCRYPTION_KEY || 'changemechangemechangemechangeme'; // 32 chars for AES-256 | |||
There was a problem hiding this comment.
Critical: Hardcoded fallback encryption key is a security vulnerability.
Using a hardcoded default key allows encrypted data to be decrypted by anyone with access to the source code. The application should fail to start if BILLING_KEY_ENCRYPTION_KEY is not set, rather than silently using an insecure default.
🔎 Proposed fix
-const ENCRYPTION_KEY = process.env.BILLING_KEY_ENCRYPTION_KEY || 'changemechangemechangemechangeme'; // 32 chars for AES-256
+const ENCRYPTION_KEY = process.env.BILLING_KEY_ENCRYPTION_KEY;
+
+if (!ENCRYPTION_KEY) {
+ throw new Error('BILLING_KEY_ENCRYPTION_KEY environment variable is required');
+}
+
+if (Buffer.byteLength(ENCRYPTION_KEY) !== 32) {
+ throw new Error('BILLING_KEY_ENCRYPTION_KEY must be exactly 32 bytes for AES-256');
+}📝 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 ENCRYPTION_KEY = process.env.BILLING_KEY_ENCRYPTION_KEY || 'changemechangemechangemechangeme'; // 32 chars for AES-256 | |
| const ENCRYPTION_KEY = process.env.BILLING_KEY_ENCRYPTION_KEY; | |
| if (!ENCRYPTION_KEY) { | |
| throw new Error('BILLING_KEY_ENCRYPTION_KEY environment variable is required'); | |
| } | |
| if (Buffer.byteLength(ENCRYPTION_KEY) !== 32) { | |
| throw new Error('BILLING_KEY_ENCRYPTION_KEY must be exactly 32 bytes for AES-256'); | |
| } |
🤖 Prompt for AI Agents
In src/utils/cryptoUtil.js around line 3, the code currently falls back to a
hardcoded 32-char encryption key when process.env.BILLING_KEY_ENCRYPTION_KEY is
not set, which is a security risk; remove the insecure default and instead check
for the presence of BILLING_KEY_ENCRYPTION_KEY at startup and immediately throw
a clear error (or exit the process) if it's undefined so the application fails
fast and does not run with a predictable key; update the code to read the env
var only and perform a runtime guard that raises an exception with a descriptive
message like "BILLING_KEY_ENCRYPTION_KEY must be set" when missing.
1 participant
Purpose
$Subject
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.