Implement API Subscription Access Control with Role-Based Permissions and Rate Limiting #4549
Conversation
📝 WalkthroughWalkthroughThis pull request introduces bulk creation functionality for scopes by adding a new asynchronous Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as Router ("/bulk")
participant M as Middleware
participant Ctrl as Controller (createBulk)
participant Util as ScopeUtil (createBulkScopes)
participant DB as Database
C->>R: POST /bulk (scopes payload)
R->>M: Validate & authenticate request
M->>Ctrl: Forward validated request
Ctrl->>Util: Invoke createBulkScopes
Util->>DB: Perform bulk insert
DB-->>Util: Return inserted scopes
Util-->>Ctrl: Return operation result
Ctrl-->>C: Respond with JSON (HTTP 200)
sequenceDiagram
participant R as Request
participant TU as TokenUtil (checkResourceAccess)
participant SM as ScopeModel
participant U as User Document
R->>TU: Invoke checkResourceAccess(request, accessToken, user)
TU->>SM: Retrieve scopes for subscription tier
TU->>U: Validate resource IDs and permissions
TU-->>R: Return true (access granted) / false (denied)
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.31.1)src/auth-service/utils/user.util.jsWarning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
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. 🪧 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## staging #4549 +/- ##
===========================================
- Coverage 11.39% 11.14% -0.25%
===========================================
Files 156 292 +136
Lines 19161 35685 +16524
Branches 584 3998 +3414
===========================================
+ Hits 2183 3977 +1794
- Misses 16974 31659 +14685
- Partials 4 49 +45
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
src/auth-service/utils/token.util.js (1)
771-807: 💡 Verification agent🧩 Analysis chain
Verify user subscription tier existence.
Both the
regenerateAccessTokenandcreateAccessTokenflows rely onuser.subscriptionTierto determine scopes. Ensure your user entries always have this field set, or handle cases where it is undefined.Below script checks if any user documents lack
subscriptionTier, which could cause edge cases:Also applies to: 994-1014
🏁 Script executed:
#!/bin/bash # Attempt to find any user documents missing the subscriptionTier field ast-grep --pattern $'class $_ {\n $$$\n}' # If user model is a class definition, adapt accordingly. (Example placeholder) # or fallback to simple grep if for a simple Mongoose schema rg "subscriptionTier"Length of output: 42796
Action Required: Add Defensive Checks for
subscriptionTierWhile the User model enforces that a
subscriptionTieris provided (as evidenced insrc/auth-service/models/User.js), the token flows insrc/auth-service/utils/token.util.js(lines 771–807) assume this field is always present. Defensive coding suggests explicitly verifying thatuser.subscriptionTierexists immediately after retrieving the user document. This will help guard against any edge cases where user entries might bypass model validation or become inconsistent. Please consider the following actions:
- In
token.util.js(lines 771–807):
After fetching the user document, add a condition such as:if (!user.subscriptionTier) { next( new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, { message: "Invalid request, user subscription tier is missing", }) ); return; }- Review Similar Flows (e.g., lines 994–1014):
Ensure that any flow that relies onuser.subscriptionTierhas a corresponding safeguard.Implementing these checks will enhance the robustness of your access token creation logic by gracefully handling unexpected cases.
🧰 Tools
🪛 Biome (1.9.4)
[error] 771-820: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
src/auth-service/models/AccessToken.js (1)
37-66:⚠️ Potential issueResolve conflicting definitions of the
scopesfield.Lines 36-37 reference an ObjectId-based
scopes, while lines 66-67 redefinescopesas an array of strings. This mismatch will cause schema conflicts and confusion.To fix, remove or modify the old
scopes: [{ type: ObjectId, ref: "scope" }]reference:- scopes: [{ type: ObjectId, ref: "scope" }], name: { type: String, required: [true, "name is required!"], default: "api access", }, ... tier: { type: String, enum: ["Free", "Standard", "Premium"] }, scopes: [{ type: String }],
🧹 Nitpick comments (5)
src/auth-service/controllers/scope.controller.js (2)
18-52: Minor inconsistency in error handling structure.The implementation of the
createBulkmethod follows the controller's established patterns with proper error extraction, tenant handling, and utility method invocation. However, there's a small inconsistency in how errors are structured.In the
createBulkmethod, the error structure at lines 23-25 uses:new HttpError("bad request errors", httpStatus.BAD_REQUEST, { message: errors, })While in other methods like
createat line 58, it uses:new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors)Consider standardizing the error handling approach across all methods for better consistency.
37-41: Response handling differs from other methods.The
createBulkmethod handles the response differently than other methods in the controller. While other methods (likecreate,list, etc.) use structured responses withsuccess,message, and data fields, thecreateBulkmethod directly returns whateverscopeUtil.createBulkScopesreturns.Consider standardizing the response format to maintain API consistency:
- return res.status(httpStatus.OK).json(response); + return res.status(httpStatus.OK).json({ + success: true, + message: response.message || "Scopes created successfully", + created_scopes: response.data || response + });src/auth-service/utils/scope.util.js (1)
7-7: Avoid redeclaringHttpError.
HttpErroris already imported at line 3. The redundant import at line 7 should be removed to prevent confusion.- const { logObject, logText, HttpError } = require("@utils/shared"); + const { logObject, logText } = require("@utils/shared");🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Shouldn't redeclare 'HttpError'. Consider to delete it or rename it.
'HttpError' is defined here:
(lint/suspicious/noRedeclare)
src/auth-service/models/User.js (2)
1032-1041: Enhance error handling in modify method.The validation for
subscriptionTierhas been correctly added to the modify method, but there's a potential issue with the flow:
- After calling
next()with an error, the function should immediately return to prevent further execution.- The current implementation correctly returns but consider adding more context to help with debugging.
if (!subscriptionTier) { next( new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, { - message: "Invalid request, subscriptionTier is required", + message: "Invalid request, subscriptionTier is required for user modification", + field: "subscriptionTier", + operation: "modify" }) ); return; }
1032-1041: Consider backward compatibility for existing users.The new requirement for a mandatory
subscriptionTiermight cause issues when updating existing users who don't have this field set. Consider implementing a migration strategy or adding fallback logic.Options to consider:
- Add a migration script to set default subscription tiers for existing users
- Modify the validation to only apply for new users or specific update operations
- Add conditional logic that sets a default tier when missing rather than throwing an error
This ensures a smoother transition to the new subscription-based access control system.
Also applies to: 631-639
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/auth-service/controllers/scope.controller.js(1 hunks)src/auth-service/models/AccessToken.js(2 hunks)src/auth-service/models/Scope.js(2 hunks)src/auth-service/models/User.js(3 hunks)src/auth-service/routes/v2/scopes.routes.js(1 hunks)src/auth-service/utils/scope.util.js(3 hunks)src/auth-service/utils/token.util.js(8 hunks)src/auth-service/validators/scopes.validators.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/auth-service/utils/scope.util.js
[error] 7-7: Shouldn't redeclare 'HttpError'. Consider to delete it or rename it.
'HttpError' is defined here:
(lint/suspicious/noRedeclare)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
src/auth-service/routes/v2/scopes.routes.js (1)
44-50: Implementation of bulk scope creation route looks good.The new route for bulk scope creation is well-structured with appropriate middleware for validation and authentication before the controller method, following the same pattern as existing routes.
src/auth-service/models/Scope.js (1)
26-26:❓ Verification inconclusive
Changed network_id requirement to default value.
The
network_idfield has been changed from being required to having a default value. This is a significant change that affects how scopes are created. Make sure this change is intentional and that all related code that expectsnetwork_idto be required has been updated accordingly.
🏁 Script executed:
#!/bin/bash # Check if any other parts of the code expect network_id to be required rg -A 2 -B 2 "network_id.*required" --glob "!src/auth-service/models/Scope.js"Length of output: 749
Network_id Field Change – Verify Consistency Across Schemas
The change in
src/auth-service/models/Scope.js(line 26) updates thenetwork_idfield from being strictly required to using a default value. However, our search revealed that similar fields in other parts of the codebase still enforcenetwork_id(or related identifier fields) as required:
- In
src/auth-service/models/Department.js, the field is marked as required.- In
src/auth-service/models/Candidate.js, the field is similarly enforced as required.Please double-check if the defaulting behavior in
Scope.jsis intentional and whether the consuming parts of your system (especially departments and candidate handling) have been updated to accommodate this change. Ensure that this change won’t inadvertently break validations or business logic in other modules.src/auth-service/validators/scopes.validators.js (2)
124-172: Comprehensive validation for bulk scope creation.The validation for bulk scope creation is well-implemented with appropriate checks for all required fields. The validators ensure:
- The
scopesfield exists, is an array, and is not empty- Each scope has the required fields: scope, description, and tier
- The tier field has the correct enum values: "Free", "Standard", "Premium"
- The network_id is optional, aligning with the model change
One minor observation: The network_id is optional in this validation but required in the single create validation (lines 43-53). This difference should be intentional and align with the model change where network_id now has a default value.
166-170: Subscription tier validation matches schema requirements.The tier validation correctly enforces the same enum values as defined in the Scope schema: "Free", "Standard", "Premium". This consistency ensures data integrity across the application.
src/auth-service/utils/scope.util.js (1)
77-84: Looks good on the subscription tier check.Requiring the
tierfield for scope creation is sensible, preventing incomplete scope data.src/auth-service/models/User.js (1)
86-89: Good addition of entity association fields.These fields enable the subscription-based access control to associate users with various entities (cohorts, grids, devices, and sites). This implementation follows the standard Mongoose schema pattern for references and supports the PR's objective of implementing entity associations for user access control.
| createBulkScopes: async (request, next) => { | ||
| try { | ||
| const { query, body } = request; | ||
| const { tenant } = query; | ||
| const { scopes } = body; | ||
|
|
||
| if (!Array.isArray(scopes) || scopes.length === 0) { | ||
| return next( | ||
| new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, { | ||
| message: "scopes must be a non-empty array of scope objects", | ||
| }) | ||
| ); | ||
| } | ||
|
|
||
| // Validate that each scope has the required tier | ||
| for (const scope of scopes) { | ||
| if (!scope.tier) { | ||
| return next( | ||
| new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, { | ||
| message: "the subscription tier is required for all scopes", | ||
| }) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Use the bulk operation for better performance | ||
| const ScopeModelInstance = ScopeModel(tenant.toLowerCase()); | ||
| const bulkOps = scopes.map((scopeData) => ({ | ||
| insertOne: { document: scopeData }, | ||
| })); | ||
|
|
||
| const result = await ScopeModelInstance.bulkWrite(bulkOps, { | ||
| ordered: false, | ||
| }).catch((error) => { | ||
| // Handle duplicate key errors separately | ||
| if (error.code === 11000) { | ||
| const duplicateScopes = error.writeErrors | ||
| .filter((err) => err.code === 11000) | ||
| .map((err) => err.errmsg || err.keyValue); | ||
|
|
||
| return { | ||
| success: false, | ||
| message: "Some scopes already exist", | ||
| existingScopes: duplicateScopes, | ||
| insertedCount: error.result?.nInserted || 0, | ||
| }; | ||
| } | ||
| throw error; // Re-throw other errors | ||
| }); | ||
|
|
||
| // Fetch the created documents | ||
| const createdScopeIds = result.insertedIds | ||
| ? Object.values(result.insertedIds).map((id) => id) | ||
| : []; | ||
|
|
||
| let createdScopes = []; | ||
| if (createdScopeIds.length > 0) { | ||
| createdScopes = await ScopeModelInstance.find({ | ||
| _id: { $in: createdScopeIds }, | ||
| }); | ||
| } | ||
|
|
||
| return { | ||
| success: true, | ||
| message: `Successfully created ${ | ||
| result.insertedCount || createdScopes.length | ||
| } scopes`, | ||
| failedCount: result.writeErrors?.length || 0, | ||
| data: createdScopes, | ||
| status: httpStatus.OK, | ||
| }; | ||
| } catch (error) { | ||
| logObject("the error", error); | ||
| logger.error(`🐛🐛 Internal Server Error ${error.message}`); | ||
| next( | ||
| new HttpError( | ||
| "Internal Server Error", | ||
| httpStatus.INTERNAL_SERVER_ERROR, | ||
| { message: error.message } | ||
| ) | ||
| ); | ||
| } | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure no other mandatory fields for each scope are omitted.
While there's a check for tier in createBulkScopes, additional validations (e.g., scope name) might be needed if your scope model expects more fields.
To confirm, run this script to inspect references to the scope model and identify other required fields:
🏁 Script executed:
#!/bin/bash
# Search for 'ScopeModel(' usage and see what fields are required
rg -A 10 "ScopeModel\(" Length of output: 6511
Action Required: Add Validation for Mandatory Scope Fields
The createBulkScopes function currently only validates that each scope has a tier. However, evidence from our tests and utility references indicates that a valid scope object should also include a non-empty scope field (the scope name) and may require other properties (e.g., a description). To avoid potential runtime issues, please add validations to ensure that every scope object includes all mandatory fields as defined by the ScopeModel.
For example, consider adding a check similar to:
if (!scope.scope) {
return next(
new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, {
message: "the scope name is required for all scopes",
})
);
}Please verify any additional required fields and update validations accordingly.
Baalmart
changed the title
… and Rate Limiting
|
Auth-service changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 14
🔭 Outside diff range comments (2)
src/auth-service/utils/scripts/migrate-existing-tokens.js (1)
1-44: 🛠️ Refactor suggestionMigration script for tokens needs error handling and performance improvements
This migration script updates existing user tokens with subscription tier information, which is necessary for the new tier-based access control system. While the overall approach is sound, there are several improvements that could make this script more robust:
- Add error handling within the loops to prevent a single failure from stopping the entire migration
- Implement progress reporting for monitoring during execution
- Consider processing users in batches to avoid memory issues with large datasets
Consider implementing these improvements:
- async function migrateExistingTokens() { + async function migrateExistingTokens() { + let processed = 0; + let success = 0; + let errors = 0; + + const batchSize = 100; + let skip = 0; + let hasMore = true; + + while (hasMore) { + logger.info(`Processing batch starting at user ${skip}`); + const users = await UserModel("airqo").find({}).limit(batchSize).skip(skip); + + if (users.length === 0) { + hasMore = false; + break; + } + + skip += users.length; - const users = await UserModel("airqo").find({}); for (const user of users) { + try { + processed++; // Set default tier if not set if (!user.subscriptionTier) { user.subscriptionTier = "Free"; await user.save(); } // Update user's tokens const tokens = await AccessTokenModel("airqo").find({ user_id: user._id, }); for (const token of tokens) { + try { // Get scopes for the user's tier const scopes = await ScopeModel("airqo") .find({ tier: user.subscriptionTier }) .select("scope"); const scopeNames = scopes.map((scope) => scope.scope); // Update token token.scopes = scopeNames; token.tier = user.subscriptionTier; await token.save(); + success++; + } catch (tokenError) { + errors++; + logger.error(`Error updating token ${token._id} for user ${user._id}: ${tokenError.message}`); + } } + } catch (userError) { + errors++; + logger.error(`Error processing user ${user._id}: ${userError.message}`); + } + + // Log progress every 100 users + if (processed % 100 === 0) { + logger.info(`Processed ${processed} users, ${success} successful token updates, ${errors} errors`); + } } + } - console.log("Migration completed"); + logger.info(`Migration completed: Processed ${processed} users, ${success} successful token updates, ${errors} errors`); }src/auth-service/models/Scope.js (1)
80-88:⚠️ Potential issueUndeclared variable
dataleaks to global scopeWithin
register,datais assigned withoutlet/const, creating an implicit global and potentially clobbering other variables.- data = await this.create({ + const data = await this.create({
♻️ Duplicate comments (1)
src/auth-service/utils/scope.util.js (1)
113-122:⚠️ Potential issueStill missing validation for mandatory “scope” field
Previous reviews flagged that every scope needs a non‑empty
scope(name) property – yet this loop only checkstier. Please ensure all mandatory fields defined in theScopeschema (at minimumscope, maybedescription, etc.) are validated before the bulk insert.
🧹 Nitpick comments (10)
src/auth-service/bin/jobs/initialize-scopes.js (1)
10-25: Consider making the job more reusableThe job uses an IIFE (Immediately Invoked Function Expression) that runs on import. While this works for a standalone script, it limits reusability when imported as a module.
Consider refactoring to support both use cases:
-// @bin/jobs/initialize-scopes.js +// @bin/jobs/initialize-scopes.js const scopeUtil = require("@utils/scope.util"); const constants = require("@config/constants"); const log4js = require("log4js"); const logger = log4js.getLogger( `${constants.ENVIRONMENT} -- initialize-scopes-job` ); +async function initializeScopes() { + try { + logger.info("Starting scope initialization job..."); + + const tenant = constants.DEFAULT_TENANT || "airqo"; + const result = await scopeUtil.initializeDefaultScopes(tenant); + + if (result.success) { + logger.info(`Scope initialization job completed: ${result.message}`); + return { success: true, message: result.message }; + } else { + logger.error(`Scope initialization job failed: ${result.message}`); + return { success: false, message: result.message }; + } + } catch (error) { + logger.error(`Error in scope initialization job: ${error.message}`); + return { success: false, message: error.message }; + } +} -(async () => { - try { - logger.info("Starting scope initialization job..."); - - const tenant = constants.DEFAULT_TENANT || "airqo"; - const result = await scopeUtil.initializeDefaultScopes(tenant); - - if (result.success) { - logger.info(`Scope initialization job completed: ${result.message}`); - } else { - logger.error(`Scope initialization job failed: ${result.message}`); - } - } catch (error) { - logger.error(`Error in scope initialization job: ${error.message}`); - } -})(); +// Run immediately if this file is executed directly (not imported) +if (require.main === module) { + initializeScopes(); +} -module.exports = {}; +module.exports = { initializeScopes };🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 18-18: src/auth-service/bin/jobs/initialize-scopes.js#L18
Added line #L18 was not covered by tests
[warning] 20-20: src/auth-service/bin/jobs/initialize-scopes.js#L20
Added line #L20 was not covered by tests
[warning] 23-23: src/auth-service/bin/jobs/initialize-scopes.js#L23
Added line #L23 was not covered by testssrc/auth-service/utils/scope.util.js (1)
286-289: Minor: replace verbose null‑checks with optional chainingif (existingScopes?.data?.length)keeps the intent while trimming three logical operators.
🧰 Tools
🪛 Biome (1.9.4)
[error] 288-289: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/auth-service/middleware/rate-limit.js (2)
56-59: Token discovery ignores standardAuthorizationheaderMany clients send the bearer token via
Authorization: Bearer <token>. Consider parsing that header as well to improve compatibility.- const token = req.query.token || req.headers.token || req.params.token; + const token = + req.query.token || + req.headers.token || + req.params.token || + (req.headers.authorization?.startsWith("Bearer ") + ? req.headers.authorization.split(" ")[1] + : null);
137-148: Expose monthly limits for full transparencyHourly and daily headers are set, but monthly limits are omitted. Adding
res.setHeader("X-RateLimit-Limit-Monthly", limits.monthly); res.setHeader( "X-RateLimit-Remaining-Monthly", Math.max(0, limits.monthly - monthlyCount) );helps consumers monitor quota usage without extra support calls.
src/auth-service/bin/server.js (1)
313-315: Logged port may not match actual port
logTextprintsconstants.PORT, but the server binds to the environment‑derivedportvariable (line 278). Ifconstants.PORTdiffers, logs will mislead operators.-logText(`server is running on port: ${constants.PORT}`); +logText(`server is running on port: ${port}`);src/auth-service/models/Scope.js (2)
208-214: Unusedtenantparameter suggests dead code
initializeDefaultScopes(tenant, next)never referencestenant; the active tenant is already baked intothis.
Either drop the parameter or use it to fetch a model for a different tenant to avoid confusion.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 208-209: src/auth-service/models/Scope.js#L208-L209
Added lines #L208 - L209 were not covered by tests
[warning] 211-211: src/auth-service/models/Scope.js#L211
Added line #L211 was not covered by tests
291-306: Error handling inside initialization loop hides failuresThe inner
try/catchlogs but swallows individual upsert errors. A partial initialization silently succeeds, leading to missing scopes.Consider accumulating failed scopes and bubbling one aggregated error:
- for (const scope of allScopes) { - try { - await this.findOneAndUpdate(...); - } catch (err) { - logger.error(...); - } - } + const failures = []; + for (const scope of allScopes) { + try { + await this.findOneAndUpdate(...); + } catch (err) { + logger.error(...); + failures.push(scope.scope); + } + } + if (failures.length) { + return { success: false, message: "Some scopes failed", failures }; + }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 291-291: src/auth-service/models/Scope.js#L291
Added line #L291 was not covered by tests
[warning] 294-296: src/auth-service/models/Scope.js#L294-L296
Added lines #L294 - L296 were not covered by tests
[warning] 301-301: src/auth-service/models/Scope.js#L301
Added line #L301 was not covered by testssrc/auth-service/utils/token.util.js (1)
300-311: Fire‑and‑forget save silently swallows errors
Promise.resolve().then(() => { … update token usage … })detaches the update from the request lifecycle.
If Mongo rejects the write (network hiccup, validation error) the client will still receive 200 OK.Consider awaiting the update (or at least logging a metric) when the API call is critical for billing / rate‑limiting accuracy.
src/auth-service/models/User.js (2)
1215-1269: Safe‑navigation & micro‑optimisation inhasResourceAccessThe method accesses
this.cohorts.some(…), etc. If the array isundefinedit’s already guarded, but the code can be shorter and clearer with optional chaining:- case "cohorts": - return ( - this.cohorts && - this.cohorts.some((id) => id.toString() === resourceId.toString()) - ); + case "cohorts": + return this.cohorts?.some((id) => id.toString() === resourceId.toString()) ?? false;(Not a functional change, just readability.)
🧰 Tools
🪛 Biome (1.9.4)
[error] 1223-1226: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1229-1232: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1235-1238: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1241-1244: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
246-276: Consider indexing high‑cardinality rate‑limit fields
apiUsageandapiRateLimitswill be queried frequently by the rate‑limiting middleware.
MongoDB docs recommend a compound index for{ _id, "apiUsage.date": 1 }(or similar) to avoid collection scans when fetching today’s usage.This is optional for now but will pay dividends once traffic scales.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/auth-service/bin/jobs/initialize-scopes.js(1 hunks)src/auth-service/bin/server.js(1 hunks)src/auth-service/middleware/rate-limit.js(1 hunks)src/auth-service/models/AccessToken.js(2 hunks)src/auth-service/models/Scope.js(5 hunks)src/auth-service/models/User.js(6 hunks)src/auth-service/routes/v2/index.js(1 hunks)src/auth-service/utils/common/generate-filter.js(2 hunks)src/auth-service/utils/scope.util.js(3 hunks)src/auth-service/utils/scripts/migrate-existing-tokens.js(1 hunks)src/auth-service/utils/token.util.js(8 hunks)src/auth-service/validators/scopes.validators.js(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/auth-service/utils/common/generate-filter.js (3)
src/auth-service/models/User.js (2)
tier(1252-1252)filter(1325-1325)src/auth-service/utils/scope.util.js (4)
filter(14-14)filter(35-35)filter(55-55)filter(280-280)src/auth-service/middleware/rate-limit.js (2)
tier(82-82)tier(271-271)
src/auth-service/models/Scope.js (6)
src/auth-service/bin/jobs/initialize-scopes.js (3)
constants(4-4)logger(6-8)tenant(14-14)src/auth-service/models/AccessToken.js (4)
constants(6-6)require(9-15)require(16-16)logger(8-8)src/auth-service/models/User.js (8)
constants(6-6)require(12-12)require(17-17)require(19-19)logger(13-15)tier(1252-1252)tenant(343-343)tenant(479-479)src/auth-service/utils/scope.util.js (5)
constants(4-4)require(3-3)require(6-6)logger(7-7)scope(9-335)src/auth-service/controllers/scope.controller.js (3)
constants(2-2)require(9-15)logger(6-8)src/auth-service/routes/v2/scopes.routes.js (1)
require(6-6)
src/auth-service/models/AccessToken.js (5)
src/auth-service/models/Scope.js (1)
ObjectId(4-4)src/auth-service/models/User.js (1)
ObjectId(7-7)src/auth-service/validators/scopes.validators.js (1)
ObjectId(6-6)src/auth-service/utils/common/generate-filter.js (1)
ObjectId(2-2)src/auth-service/utils/token.util.js (1)
ObjectId(26-26)
🪛 Biome (1.9.4)
src/auth-service/utils/scope.util.js
[error] 288-289: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/auth-service/models/User.js
[error] 1223-1226: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1229-1232: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1235-1238: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1241-1244: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/auth-service/utils/token.util.js
[error] 166-167: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 168-169: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 171-172: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 173-174: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/auth-service/models/AccessToken.js
[error] 54-58: This property value named tier is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named tier
(lint/suspicious/noDuplicateObjectKeys)
[error] 59-63: This property value named scopes is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named scopes
(lint/suspicious/noDuplicateObjectKeys)
🪛 GitHub Check: codecov/patch
src/auth-service/bin/server.js
[warning] 157-158: src/auth-service/bin/server.js#L157-L158
Added lines #L157 - L158 were not covered by tests
[warning] 162-163: src/auth-service/bin/server.js#L162-L163
Added lines #L162 - L163 were not covered by tests
src/auth-service/models/Scope.js
[warning] 176-178: src/auth-service/models/Scope.js#L176-L178
Added lines #L176 - L178 were not covered by tests
[warning] 181-181: src/auth-service/models/Scope.js#L181
Added line #L181 was not covered by tests
[warning] 188-188: src/auth-service/models/Scope.js#L188
Added line #L188 was not covered by tests
[warning] 196-197: src/auth-service/models/Scope.js#L196-L197
Added lines #L196 - L197 were not covered by tests
[warning] 208-209: src/auth-service/models/Scope.js#L208-L209
Added lines #L208 - L209 were not covered by tests
[warning] 211-211: src/auth-service/models/Scope.js#L211
Added line #L211 was not covered by tests
[warning] 256-256: src/auth-service/models/Scope.js#L256
Added line #L256 was not covered by tests
[warning] 269-269: src/auth-service/models/Scope.js#L269
Added line #L269 was not covered by tests
[warning] 291-291: src/auth-service/models/Scope.js#L291
Added line #L291 was not covered by tests
[warning] 294-296: src/auth-service/models/Scope.js#L294-L296
Added lines #L294 - L296 were not covered by tests
[warning] 301-301: src/auth-service/models/Scope.js#L301
Added line #L301 was not covered by tests
[warning] 307-307: src/auth-service/models/Scope.js#L307
Added line #L307 was not covered by tests
[warning] 313-314: src/auth-service/models/Scope.js#L313-L314
Added lines #L313 - L314 were not covered by tests
src/auth-service/models/User.js
[warning] 668-668: src/auth-service/models/User.js#L668
Added line #L668 was not covered by tests
[warning] 670-670: src/auth-service/models/User.js#L670
Added line #L670 was not covered by tests
[warning] 675-675: src/auth-service/models/User.js#L675
Added line #L675 was not covered by tests
[warning] 1069-1069: src/auth-service/models/User.js#L1069
Added line #L1069 was not covered by tests
src/auth-service/bin/jobs/initialize-scopes.js
[warning] 18-18: src/auth-service/bin/jobs/initialize-scopes.js#L18
Added line #L18 was not covered by tests
[warning] 20-20: src/auth-service/bin/jobs/initialize-scopes.js#L20
Added line #L20 was not covered by tests
[warning] 23-23: src/auth-service/bin/jobs/initialize-scopes.js#L23
Added line #L23 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
src/auth-service/utils/common/generate-filter.js (1)
891-912: Added tier support to scopes filter functionThe addition of the
tierparameter to the filter generation for scopes is a logical extension that supports the new subscription-based API access control system. This change ensures that scopes can be filtered by their associated tier ("Free", "Standard", or "Premium"), which is essential for the tier-based permissions system being implemented.The implementation follows the existing pattern used throughout the file, maintaining consistency with how other filters are constructed.
src/auth-service/routes/v2/index.js (3)
3-16: Excellent error resilience with safe module loadingThe addition of the
safeRequirefunction significantly improves application resilience by preventing a single failed route module from crashing the entire API. This is a high-quality implementation that:
- Properly isolates each module load in a try-catch block
- Provides detailed error logging to aid troubleshooting
- Returns null for failed modules so they can be conditionally registered
This defensive programming approach aligns well with the PR objective of enhancing server resilience.
19-75: Safe route loading implementation is clean and comprehensiveThe replacement of direct require calls with
safeRequirefor all routes is thorough and consistent. Each route is loaded with appropriate error handling, and the code is well-organized for maintainability.
77-104: Conditional route registration enhances API stabilityThe conditional registration of routes (only if successfully loaded) completes the error resilience pattern. This ensures that even if certain route modules fail to load, the API continues to function with the available routes.
This approach is especially valuable in microservice architectures where partial functionality is preferable to complete service failure.
src/auth-service/models/AccessToken.js (1)
215-218: Aggregation$lookupis incompatible with string‑based scopesIf you switch
scopestoString[], the$lookupfrom"scopes"collection will never match. Either:
- Keep
ObjectIdreferences and maintain the lookup, or- Remove the lookup and populate scope metadata differently (e.g.,
$matchwith$in: "$scopes").Failing to align these will make
list()silently return emptyscopesarrays.
| // @bin/jobs/initialize-scopes.js | ||
|
|
||
| const scopeUtil = require("@utils/scope.util"); | ||
| const constants = require("@config/constants"); | ||
| const log4js = require("log4js"); | ||
| const logger = log4js.getLogger( | ||
| `${constants.ENVIRONMENT} -- initialize-scopes-job` | ||
| ); | ||
|
|
||
| (async () => { | ||
| try { | ||
| logger.info("Starting scope initialization job..."); | ||
|
|
||
| const tenant = constants.DEFAULT_TENANT || "airqo"; | ||
| const result = await scopeUtil.initializeDefaultScopes(tenant); | ||
|
|
||
| if (result.success) { | ||
| logger.info(`Scope initialization job completed: ${result.message}`); | ||
| } else { | ||
| logger.error(`Scope initialization job failed: ${result.message}`); | ||
| } | ||
| } catch (error) { | ||
| logger.error(`Error in scope initialization job: ${error.message}`); | ||
| } | ||
| })(); | ||
|
|
||
| module.exports = {}; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Well-structured scope initialization job with missing test coverage
This job script effectively initializes default scopes for the system, providing the foundation for the tier-based permission system. The implementation is clean with good error handling and logging.
However, the static analysis indicates missing test coverage for success, failure, and error handling paths.
Consider adding tests to cover all execution paths in this job:
🏁 Script executed:
#!/bin/bash
# Look for existing tests that might cover this job
echo "Searching for tests covering the initialize-scopes job..."
rg -l "initializeDefaultScopes" --type=test
# Check if this job is referenced or executed during application startup
echo "Checking if the job is executed during startup..."
rg -l "initialize-scopes" src/auth-service/bin/server.jsLength of output: 394
Add missing tests for the initialize‑scopes job
We didn’t find any existing tests that exercise the initialize-scopes.js script or its call to scopeUtil.initializeDefaultScopes, even though it’s hooked into src/auth-service/bin/server.js. Please add unit (and, if desired, integration) tests to cover all paths:
• File to test
src/auth-service/bin/jobs/initialize‑scopes.js
– Success path: stub initializeDefaultScopes to return { success: true, message: … } and assert the appropriate logger.info call.
– Failure path: stub it to return { success: false, message: … } and assert logger.error.
– Error path: stub it to throw an error and assert logger.error with the thrown message.
• Startup integration (optional)
src/auth-service/bin/server.js
– Verify that the job is invoked on application startup (e.g. by spying on scopeUtil.initializeDefaultScopes).
Fix by creating a test file (e.g. tests/bin/jobs/initialize‑scopes.test.js) and covering the above scenarios.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 18-18: src/auth-service/bin/jobs/initialize-scopes.js#L18
Added line #L18 was not covered by tests
[warning] 20-20: src/auth-service/bin/jobs/initialize-scopes.js#L20
Added line #L20 was not covered by tests
[warning] 23-23: src/auth-service/bin/jobs/initialize-scopes.js#L23
Added line #L23 was not covered by tests
| const { tier } = body; | ||
| if (!tier) { | ||
| next( | ||
| new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, { | ||
| message: "the subscription tier is required", | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Return after emitting the error to prevent double‑response issues
next(...) is invoked correctly when tier is missing, but execution continues, which can lead to “Cannot set headers after they are sent” or multiple JSON responses. Add an early return (or restructure with an if/else) so the function short‑circuits once the error has been handed off.
- if (!tier) {
- next(
+ if (!tier) {
+ return next(
new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, {
message: "the subscription tier is required",
})
);
}📝 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 { tier } = body; | |
| if (!tier) { | |
| next( | |
| new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, { | |
| message: "the subscription tier is required", | |
| }) | |
| ); | |
| } | |
| const { tier } = body; | |
| if (!tier) { | |
| return next( | |
| new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, { | |
| message: "the subscription tier is required", | |
| }) | |
| ); | |
| } |
| // Use the bulk operation for better performance | ||
| const ScopeModelInstance = ScopeModel(tenant.toLowerCase()); | ||
| const bulkOps = scopes.map((scopeData) => ({ | ||
| insertOne: { document: scopeData }, | ||
| })); | ||
|
|
||
| const result = await ScopeModelInstance.bulkWrite(bulkOps, { | ||
| ordered: false, | ||
| }).catch((error) => { | ||
| // Handle duplicate key errors separately | ||
| if (error.code === 11000) { | ||
| const duplicateScopes = error.writeErrors | ||
| .filter((err) => err.code === 11000) | ||
| .map((err) => err.errmsg || err.keyValue); | ||
|
|
||
| return { | ||
| success: false, | ||
| message: "Some scopes already exist", | ||
| existingScopes: duplicateScopes, | ||
| insertedCount: error.result?.nInserted || 0, | ||
| }; | ||
| } | ||
| throw error; // Re-throw other errors | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
bulkWrite result handling can crash on duplicate‑key path
When duplicates occur you return a custom object inside the .catch, so later references to result.insertedIds / result.insertedCount (151‑166) will be undefined, raising TypeError: Cannot convert undefined or null to object.
Consider normalising the response structure or, simpler, exit early on duplicate error:
- const result = await ScopeModelInstance.bulkWrite(bulkOps, { ordered: false })
- .catch(/* custom object */);
- // later: uses result.insertedIds ...
+ let bulkResult;
+ try {
+ bulkResult = await ScopeModelInstance.bulkWrite(bulkOps, { ordered: false });
+ } catch (error) {
+ if (error.code === 11000) {
+ return next(
+ new HttpError("Conflict", httpStatus.CONFLICT, {
+ message: "Some scopes already exist",
+ duplicates: error.writeErrors.map(e => e.keyValue),
+ })
+ );
+ }
+ throw error;
+ }Committable suggestion skipped: line range outside the PR's diff.
| tier: { | ||
| type: String, | ||
| enum: ["Free", "Standard", "Premium"], | ||
| default: "Free", | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate tier property—schema will keep only the last one
The tier field appears at 54‑58 and again at 75‑77. Mongoose will ignore the first definition and keep the last, which may not include your intended defaults. Remove the duplicate and keep a single definition with default: "Free" to avoid silent mis‑configurations.
Also applies to: 75-77
🧰 Tools
🪛 Biome (1.9.4)
[error] 54-58: This property value named tier is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named tier
(lint/suspicious/noDuplicateObjectKeys)
| permissions: [{ type: ObjectId, ref: "permission" }], | ||
| scopes: [{ type: ObjectId, ref: "scope" }], | ||
| name: { type: String, required: [true, "name is required!"] }, | ||
| name: { |
There was a problem hiding this comment.
scopes defined twice with different types – pick one
Lines 35‑37 declare scopes as an array of ObjectId, while lines 59‑63 redeclare it as an array of String. The latter silently overwrites the former, causing Mongoose/TS tooling confusion and, more importantly, breaking the aggregation $lookup on lines 215‑218 that expects ObjectIds.
Choose one representation and update downstream code accordingly (lookup → $match if strings, or keep ObjectIds).
- scopes: [{ type: ObjectId, ref: "scope" }],
+ // Prefer storing the scope name as string to avoid join‑costs
+ scopes: [{ type: String }],📝 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.
| permissions: [{ type: ObjectId, ref: "permission" }], | |
| scopes: [{ type: ObjectId, ref: "scope" }], | |
| name: { type: String, required: [true, "name is required!"] }, | |
| name: { | |
| permissions: [{ type: ObjectId, ref: "permission" }], | |
| // Prefer storing the scope name as string to avoid join‑costs | |
| scopes: [{ type: String }], | |
| name: { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: This property value named scopes is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named scopes
(lint/suspicious/noDuplicateObjectKeys)
| /************************ New Functions for Resource Access ****************************/ | ||
|
|
||
| /** | ||
| * Checks if an access token has the required permissions to access a specific resource. | ||
| * @param {object} request - The HTTP request object. | ||
| * @param {object} accessToken - The AccessToken document. | ||
| * @param {object} user - The User document. | ||
| * @param {function} next - The next middleware function. | ||
| * @returns {boolean} - True if access is allowed, false otherwise. | ||
| */ | ||
| const checkResourceAccess = async (request, accessToken, user, next) => { | ||
| try { | ||
| const { path, query, method } = request; | ||
| const { cohort_id, grid_id, device_id, site_id } = query; | ||
|
|
||
| // 1. Check subscription status | ||
| if (!user.subscriptionStatus || user.subscriptionStatus !== "active") { | ||
| logger.error( | ||
| `🚨 User ${user._id} has an inactive subscription: ${user.subscriptionStatus}` | ||
| ); | ||
| return { | ||
| allowed: false, | ||
| reason: "inactive_subscription", | ||
| message: | ||
| "Your subscription is not active. Please renew your subscription to access this resource.", | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Please consolidate the two checkResourceAccess implementations
There are now two full definitions: one declared at the top‑level (lines 36‑221) and another nested inside the exported token object (lines 2423‑2535).
Maintaining duplicate logic is high‑risk – sooner or later the two versions will diverge and subtle security gaps will creep in.
Action plan (one possible diff):
-// ************************ New Functions for Resource Access ****************************/
-const checkResourceAccess = async (request, accessToken, user, next) => { … }
-
-… lots of code …
-
- checkResourceAccess: async (request, accessToken, user, next) => { … duplicate … },
+// ************************ New Functions for Resource Access ****************************/
+const checkResourceAccess = async (request, accessToken, user, next) => { … } // keep ONE
+
+…
+
+// remove the duplicate entry from the token objectAfter removal, make sure module.exports still exposes the chosen function.
This clean‑up will reduce cognitive load and prevent regression bugs.
Also applies to: 2423-2535
| // Enhanced verifyToken function to use the new checkResourceAccess | ||
| const verifyToken = async (request, next) => { | ||
| try { | ||
| logText("Entering enhanced verifyToken function"); | ||
| const ip = | ||
| request.headers["x-client-ip"] || request.headers["x-client-original-ip"]; | ||
| const endpoint = request.headers["x-original-uri"]; | ||
| const { token } = { | ||
| ...request.params, | ||
| }; | ||
|
|
||
| // 1. Basic token validation | ||
| const accessToken = await AccessTokenModel("airqo") | ||
| .findOne({ token }) | ||
| .select("client_id token scopes tier"); | ||
|
|
||
| if (isEmpty(accessToken)) { | ||
| return createUnauthorizedResponse(); | ||
| } else if (isEmpty(ip)) { | ||
| logText(`🚨 Token is being accessed without an IP address`); | ||
| logger.error(`🚨 Token is being accessed without an IP address`); | ||
| return createUnauthorizedResponse(); | ||
| } | ||
|
|
||
| // 2. Check client status | ||
| const client = await ClientModel("airqo") | ||
| .findById(accessToken.client_id) | ||
| .select("isActive user_id"); | ||
|
|
||
| if (isEmpty(client) || (client && !client.isActive)) { | ||
| logger.error( | ||
| `🚨 Client ${accessToken.client_id} associated with Token ${accessToken.token} is INACTIVE or does not exist` | ||
| ); | ||
| return createUnauthorizedResponse(); | ||
| } | ||
|
|
||
| // 3. Check IP blacklisting | ||
| const isBlacklisted = await isIPBlacklisted({ | ||
| request, | ||
| next, | ||
| }); | ||
|
|
||
| if (isBlacklisted) { | ||
| return createUnauthorizedResponse(); | ||
| } | ||
|
|
||
| // 4. Load the user data | ||
| const user = await UserModel("airqo").findById(client.user_id).lean(); | ||
| if (!user) { | ||
| logger.error(`🚨 User not found for Access Token ${accessToken.token}`); | ||
| return createUnauthorizedResponse(); | ||
| } | ||
|
|
||
| // 5. Enhanced access control check | ||
| const accessCheck = await checkResourceAccess( | ||
| request, | ||
| accessToken, | ||
| user, | ||
| next | ||
| ); | ||
|
|
||
| if (!accessCheck.allowed) { | ||
| logger.info( | ||
| `🔒 Access denied: ${accessCheck.reason} - ${accessCheck.message}` | ||
| ); | ||
| return { | ||
| success: false, | ||
| message: accessCheck.message, | ||
| status: httpStatus.FORBIDDEN, | ||
| errors: { | ||
| reason: accessCheck.reason, | ||
| message: accessCheck.message, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| // 6. Update token usage metadata asynchronously | ||
| Promise.resolve().then(() => { | ||
| // Update last_used_at and last_ip_address | ||
| AccessTokenModel("airqo") | ||
| .findOneAndUpdate( | ||
| { token }, | ||
| { | ||
| last_used_at: new Date(), | ||
| last_ip_address: ip, | ||
| }, | ||
| { new: true } | ||
| ) | ||
| .catch((err) => { | ||
| logger.error(`Failed to update token usage metadata: ${err.message}`); | ||
| }); | ||
|
|
||
| // Log token usage for analytics | ||
| postProcessing({ | ||
| ip, | ||
| token: accessToken.token, | ||
| name: accessToken.name || "unknown", | ||
| client_id: accessToken.client_id, | ||
| endpoint, | ||
| day: getDay(), | ||
| }); | ||
| }); | ||
|
|
||
| // 7. Log successful token verification | ||
| winstonLogger.info("verify token", { | ||
| token: token, | ||
| service: "verify-token", | ||
| clientIp: ip, | ||
| clientOriginalIp: ip, | ||
| endpoint: endpoint ? endpoint : "unknown", | ||
| }); | ||
|
|
||
| return createValidTokenResponse(); | ||
| } catch (error) { | ||
| logger.error(`🐛 Internal Server Error in verifyToken: ${error.message}`); | ||
| next( | ||
| new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, { | ||
| message: error.message, | ||
| }) | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate verifyToken paths create an API mine‑field
Similar to the previous point, two distinct verifyToken variants exist:
- Enhanced version (lines 223‑344)
- Legacy version inside
token.verifyToken(lines 1192‑1241)
Because module.exports later places the enhanced version after the spread operator, the legacy implementation is silently shadowed – yet it is still executed by other internal helpers (isIPBlacklistedHelper), and future refactors could accidentally flip the order.
Recommend:
- Delete the older implementation or migrate any remaining call‑sites.
- Add a unit‑test that fails when more than one
verifyTokendefinition is present.
Also applies to: 1192-1241
| // Check if subscription check is outdated (older than 24 hours) | ||
| const lastCheck = user.lastSubscriptionCheck; | ||
| const needsCheck = | ||
| !lastCheck || moment().diff(moment(lastCheck), "hours") > 24; | ||
|
|
||
| if (needsCheck) { | ||
| // Schedule an async task to update subscription status | ||
| // This won't block the current request but ensures we regularly check | ||
| Promise.resolve().then(async () => { | ||
| try { | ||
| // Update subscription status from payment provider (this would be implemented separately) | ||
| // For now, just update the lastSubscriptionCheck timestamp | ||
| await UserModel("airqo").findByIdAndUpdate( | ||
| user._id, | ||
| { lastSubscriptionCheck: new Date() }, | ||
| { new: true } | ||
| ); | ||
| } catch (error) { | ||
| logger.error( | ||
| `Failed to update subscription check for user ${user._id}: ${error.message}` | ||
| ); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Hard‑coded tenant in subscription check breaks multi‑tenancy
Inside the async “needsCheck” block the code calls
UserModel("airqo").findByIdAndUpdate(…), hard‑wiring the airqo tenant.
If this utility is reused by other tenants the subscription status will never be refreshed for them.
Proposed fix:
- await UserModel("airqo").findByIdAndUpdate(
+ const tenant = request.query.tenant || request.body.tenant || "airqo";
+ await UserModel(tenant).findByIdAndUpdate(Alternatively, pass the tenant down from the calling context instead of re‑deriving it.
Replace hard‑coded tenant in async subscription check
The current implementation always uses the "airqo" tenant when updating lastSubscriptionCheck, which will break subscription refresh for any other tenant.
Please update src/auth-service/utils/token.util.js (lines 64–87) to accept or derive the tenant dynamically:
• File: src/auth-service/utils/token.util.js (around line 75)
- Replace:
await UserModel("airqo").findByIdAndUpdate( user._id, { lastSubscriptionCheck: new Date() }, { new: true } );
- With something like:
// Derive tenant from user object or pass it into this helper const tenant = user.tenant || /* fall‑back logic */; await UserModel(tenant).findByIdAndUpdate( user._id, { lastSubscriptionCheck: new Date() }, { new: true } );
• Ensure that whichever caller invokes this util provides the correct tenant (e.g. from the HTTP request context or user.tenant) rather than re‑deriving it here.
• Update the function signature or calling code to pass the tenant down into this utility.
This change will restore proper multi‑tenant support for subscription checks.
| const { subscriptionTier } = update; | ||
| if (!subscriptionTier) { | ||
| next( | ||
| new HttpError("Bad Request Error", httpStatus.BAD_REQUEST, { | ||
| message: "Invalid request, subscriptionTier is required", | ||
| }) | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
modify now requires subscriptionTier for every patch
modify() bails out unless the incoming update payload contains subscriptionTier.
That means a simple “change lastName” call will be rejected unless callers redundantly include the current tier.
Either:
- Make the check conditional – only enforce when the caller is actively changing the tier, or
- Populate
update.subscriptionTierwith the existing value fetched from DB before validating.
This will keep partial updates ergonomic.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1069-1069: src/auth-service/models/User.js#L1069
Added line #L1069 was not covered by tests
| trackApiUsage: async function (endpoint) { | ||
| const today = new Date(); | ||
| today.setHours(0, 0, 0, 0); | ||
|
|
||
| let usage = this.apiUsage || []; | ||
| let todayUsage = usage.find((u) => { | ||
| const usageDate = new Date(u.date); | ||
| usageDate.setHours(0, 0, 0, 0); | ||
| return usageDate.getTime() === today.getTime(); | ||
| }); | ||
|
|
||
| if (!todayUsage) { | ||
| todayUsage = { | ||
| date: today, | ||
| count: 0, | ||
| endpoints: [], | ||
| }; | ||
| usage.push(todayUsage); | ||
| } | ||
|
|
||
| // Increment total count | ||
| todayUsage.count += 1; | ||
|
|
||
| // Track endpoint usage | ||
| let endpointUsage = todayUsage.endpoints.find((e) => e.path === endpoint); | ||
| if (!endpointUsage) { | ||
| endpointUsage = { | ||
| path: endpoint, | ||
| count: 0, | ||
| }; | ||
| todayUsage.endpoints.push(endpointUsage); | ||
| } | ||
| endpointUsage.count += 1; | ||
|
|
||
| // Update user | ||
| this.apiUsage = usage; | ||
| this.lastRateLimitCheck = new Date(); | ||
|
|
||
| // Check if user has exceeded rate limits | ||
| const dailyLimit = this.apiRateLimits?.dailyLimit || 1000; | ||
| if (todayUsage.count > dailyLimit) { | ||
| this.rateLimitNotified = true; | ||
| return false; // Rate limit exceeded | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
trackApiUsage never persists – rate‑limit stats evaporate
The method mutates this.apiUsage but does not await this.save() (or updateOne) afterwards.
As a result, callers will believe usage was recorded while MongoDB remains unchanged, defeating daily limit enforcement.
Quick remedy:
this.apiUsage = usage;
this.lastRateLimitCheck = new Date();
+ await this.save(); // <- persist documentAlso remember the method is async; propagate the rejection so upstream can handle DB errors.
📝 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.
| trackApiUsage: async function (endpoint) { | |
| const today = new Date(); | |
| today.setHours(0, 0, 0, 0); | |
| let usage = this.apiUsage || []; | |
| let todayUsage = usage.find((u) => { | |
| const usageDate = new Date(u.date); | |
| usageDate.setHours(0, 0, 0, 0); | |
| return usageDate.getTime() === today.getTime(); | |
| }); | |
| if (!todayUsage) { | |
| todayUsage = { | |
| date: today, | |
| count: 0, | |
| endpoints: [], | |
| }; | |
| usage.push(todayUsage); | |
| } | |
| // Increment total count | |
| todayUsage.count += 1; | |
| // Track endpoint usage | |
| let endpointUsage = todayUsage.endpoints.find((e) => e.path === endpoint); | |
| if (!endpointUsage) { | |
| endpointUsage = { | |
| path: endpoint, | |
| count: 0, | |
| }; | |
| todayUsage.endpoints.push(endpointUsage); | |
| } | |
| endpointUsage.count += 1; | |
| // Update user | |
| this.apiUsage = usage; | |
| this.lastRateLimitCheck = new Date(); | |
| // Check if user has exceeded rate limits | |
| const dailyLimit = this.apiRateLimits?.dailyLimit || 1000; | |
| if (todayUsage.count > dailyLimit) { | |
| this.rateLimitNotified = true; | |
| return false; // Rate limit exceeded | |
| } | |
| // Update user | |
| this.apiUsage = usage; | |
| this.lastRateLimitCheck = new Date(); | |
| + await this.save(); // <- persist document | |
| // Check if user has exceeded rate limits | |
| const dailyLimit = this.apiRateLimits?.dailyLimit || 1000; | |
| if (todayUsage.count > dailyLimit) { | |
| this.rateLimitNotified = true; | |
| return false; // Rate limit exceeded | |
| } |
|
Auth-service changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/auth-service/API_SUBSCRIPTION_ACCESS_CONTROL.md (3)
61-61: Convert bare URL to markdown link.Using a bare URL can lead to stability issues with markdown linters (MD034). Please convert
https://analytics.airqo.net/account/logininto a proper markdown link.Example:
-To use the AirQo API, first create an account at https://analytics.airqo.net/account/login +To use the AirQo API, first create an account at [AirQo Account Login](https://analytics.airqo.net/account/login)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
61-61: Bare URL used
null(MD034, no-bare-urls)
86-88: Specify language for the HTTP code block.The fenced code block for the request example lacks a language specifier and triggers MD040. Consider changing:
-``` +```http GET https://api.airqo.net/api/v2/measurements/recent?token=YOUR_TOKEN -``` +```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
86-86: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
183-203: Convert bare emails and URLs to markdown links.Lines with raw
[email protected]andhttps://support.airqo.nettrigger MD034. Please format them as links:-Contact [email protected] to request access to additional resources. +Contact [[email protected]](mailto:[email protected]) to request access to additional resources. - Email: [email protected] - Support Portal: https://support.airqo.net + Email: [[email protected]](mailto:[email protected]) + Support Portal: [support.airqo.net](https://support.airqo.net)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
183-183: Bare URL used
null(MD034, no-bare-urls)
201-201: Bare URL used
null(MD034, no-bare-urls)
202-202: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/auth-service/API_SUBSCRIPTION_ACCESS_CONTROL.md(1 hunks)src/auth-service/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/auth-service/README.md
🧰 Additional context used
🪛 LanguageTool
src/auth-service/API_SUBSCRIPTION_ACCESS_CONTROL.md
[uncategorized] ~48-~48: Loose punctuation mark.
Context: ...g headers: - X-RateLimit-Limit-Hourly: Maximum hourly requests - `X-RateLimit-...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
src/auth-service/API_SUBSCRIPTION_ACCESS_CONTROL.md
61-61: Bare URL used
null
(MD034, no-bare-urls)
86-86: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
183-183: Bare URL used
null
(MD034, no-bare-urls)
201-201: Bare URL used
null
(MD034, no-bare-urls)
202-202: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
src/auth-service/API_SUBSCRIPTION_ACCESS_CONTROL.md (3)
83-89: Verify token usage matches implementation.The docs instruct to pass the token as a query parameter (
?token=YOUR_TOKEN), but the middleware may expect anAuthorization: Bearerheader. Please confirm the actual authentication mechanism and update this section accordingly to avoid confusion.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
86-86: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
46-52: Confirm rate limit header names.The documentation lists these headers:
X-RateLimit-Limit-HourlyX-RateLimit-Remaining-HourlyX-RateLimit-Limit-DailyX-RateLimit-Remaining-DailyEnsure these match the exact header names emitted by the rate-limiting middleware (
src/auth-service/middleware/rate-limit.js). Inconsistencies here could break client integrations.🧰 Tools
🪛 LanguageTool
[uncategorized] ~48-~48: Loose punctuation mark.
Context: ...g headers: -X-RateLimit-Limit-Hourly: Maximum hourly requests - `X-RateLimit-...(UNLIKELY_OPENING_PUNCTUATION)
96-100: Validate resource endpoint paths.The Free Tier endpoints are listed as:
/api/v2/devices/sites /api/v2/devices/cohorts /api/v2/devices/gridsPlease verify that these paths match the actual route definitions in the codebase (
src/auth-service/routes/...). If the correct routes are/api/v2/sites,/api/v2/cohorts, or similar, adjust the documentation.
|
Auth-service changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
src/auth-service/utils/token.util.js (2)
1207-1267:⚠️ Potential issueRedundant verifyToken implementation
The file contains two implementations of
verifyToken:
- Enhanced version as a standalone function (lines 223-358)
- Older version inside the token object (lines 1207-1267)
This creates confusion and potential security risks if different parts of the codebase use different implementations.
Recommend consolidating to the enhanced standalone version and removing the object method:
- verifyToken: async (request, next) => { - // Entire older implementation... - }, + verifyToken, // Use the enhanced standalone functionEnsure all existing code is updated to use the correct implementation.
1350-1414:⚠️ Potential issueDuplicate token creation implementation
The file contains two implementations of token creation functionality:
- Enhanced standalone
createAccessTokenfunction (lines 360-460)- Object-based implementation in
token.createAccessToken(lines 1340-1430)While they share similar logic, maintaining both creates a risk of inconsistent behavior.
Recommend consolidating to use the standalone implementation and updating this method to use it:
- createAccessToken: async (request, next) => { - // Duplicate implementation... - }, + createAccessToken: (request, next) => createAccessToken(request, next),
♻️ Duplicate comments (4)
src/auth-service/API_SUBSCRIPTION_ACCESS_CONTROL.md (1)
1-271: 🛠️ Refactor suggestionComprehensive documentation for the subscription access control system.
The documentation provides a clear explanation of the subscription tiers, access permissions, rate limits, and resource access control mechanisms. This will be valuable for API users to understand how the system works.
However, there are a few areas that could be improved:
- Missing documentation for the new endpoints related to scope management and token verification that were introduced in this PR.
- Several URLs are written as bare URLs, which should be properly formatted as Markdown links.
- Some code blocks are missing language specifiers.
Here are suggestions for improvement:
Add documentation for the new scope management endpoints (POST/GET/PUT/DELETE /api/v2/scopes) and token verification routes, including expected request/response formats.
Format bare URLs as proper Markdown links:
-https://analytics.airqo.net/account/login +[AirQo Analytics Platform](https://analytics.airqo.net/account/login)
- Add language specifiers to code blocks:
-``` +```http GET https://api.airqo.net/api/v2/measurements/recent?token=YOUR_TOKEN🧰 Tools
🪛 LanguageTool
[uncategorized] ~48-~48: Loose punctuation mark.
Context: ...g headers: -X-RateLimit-Limit-Hourly: Maximum hourly requests - `X-RateLimit-...(UNLIKELY_OPENING_PUNCTUATION)
[style] ~180-~180: Consider using “email”.
Context: ...cific resources: 1. Email Request: Send an email to [email protected] with the following in...(SEND_AN_EMAIL)
🪛 markdownlint-cli2 (0.17.2)
61-61: Bare URL used
null(MD034, no-bare-urls)
86-86: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
180-180: Bare URL used
null(MD034, no-bare-urls)
187-187: Bare URL used
null(MD034, no-bare-urls)
251-251: Bare URL used
null(MD034, no-bare-urls)
269-269: Bare URL used
null(MD034, no-bare-urls)
270-270: Bare URL used
null(MD034, no-bare-urls)
src/auth-service/utils/token.util.js (3)
75-87: Fix the hard-coded tenant in subscription checkThe subscription check update uses a hardcoded "airqo" tenant, which breaks multi-tenancy support as highlighted in previous reviews.
This should be dynamically derived from the user object or request context.
36-221:⚠️ Potential issueStrong implementation of resource access control with tiered permissions
The
checkResourceAccessfunction provides a comprehensive approach to subscription-based API access control. The multi-layered checks (subscription status, resource type, tier permissions, resource ownership) create a robust permission system.However, there's an issue with the subscription check implementation:
The code at line 76 hard-codes the "airqo" tenant for subscription status updates, which will break multi-tenancy support:
- await UserModel("airqo").findByIdAndUpdate( + const tenant = user.tenant || request.query.tenant || request.body.tenant || "airqo"; + await UserModel(tenant).findByIdAndUpdate(Additionally, consider implementing the commented rate limiting functionality (lines 191-209) or removing the placeholders to avoid confusion.
🧰 Tools
🪛 Biome (1.9.4)
[error] 166-167: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 168-169: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 171-172: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 173-174: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
2438-2550:⚠️ Potential issueDuplicate implementation of checkResourceAccess
This is a duplicate implementation of the
checkResourceAccessfunction that was already defined at lines 36-221. Having two versions of the same logic creates maintenance challenges and security risks as the implementations could diverge over time.Recommend consolidating to a single implementation:
- checkResourceAccess: async (request, accessToken, user, next) => { - // Entire duplicate implementation... - }, + checkResourceAccess, // Use the standalone function defined earlierThe differences between implementations (like return values and error handling) need to be carefully reconciled to ensure consistent behavior.
🧹 Nitpick comments (8)
src/auth-service/API_SUBSCRIPTION_ACCESS_CONTROL.md (2)
228-228: Fix typo in HTTP method example.There's a typo in the HTTP method for accessing cohort data.
-httpGET /api/v2/cohorts/5f7b85a1c9a4c80012345678/recent?token=YOUR_TOKEN +GET /api/v2/cohorts/5f7b85a1c9a4c80012345678/recent?token=YOUR_TOKEN
46-52: Consider adding examples of rate limit headers.The documentation mentions rate limit headers but doesn't provide examples of what they look like in actual responses.
Add an example showing what the rate limit headers look like in an actual response:
### Example Rate Limit Headers ```http HTTP/1.1 200 OK Content-Type: application/json X-RateLimit-Limit-Hourly: 100 X-RateLimit-Remaining-Hourly: 95 X-RateLimit-Limit-Daily: 1000 X-RateLimit-Remaining-Daily: 950🧰 Tools
🪛 LanguageTool
[uncategorized] ~48-~48: Loose punctuation mark.
Context: ...g headers: -X-RateLimit-Limit-Hourly: Maximum hourly requests - `X-RateLimit-...(UNLIKELY_OPENING_PUNCTUATION)
src/auth-service/utils/user.util.js (4)
372-404: Possible loss of scopes & N+1 DB hit inupdateTokensForUser
- If
ScopeModel.find({ tier })returns an empty array (e.g., admin forgot to seed scopes for the tier), all affected access tokens will end up with an emptyscopeslist, effectively blocking every request for that user. Add a fallback or abort the update whenscopeNames.length === 0.- The code first fetches clients, then extracts their IDs – good – but afterwards updates tokens in bulk. 👍
- Minor: You issue two queries (
find,updateMany) but could combine them with aggregation; not blocking, but worth noting for high‑traffic tenants.
540-620: Duplicate$addToSetlogic – shrink ~40 lines to 4The four almost‑identical blocks can be reduced to a single loop for maintainability:
- if (resourcesByType.device) { ... } - if (resourcesByType.site) { ... } - ... + Object.entries(resourcesByType).forEach(([type, ids]) => { + const field = `${type}s`; // device -> devices + update.$addToSet ??= {}; + update.$addToSet[field] = { $each: ids }; + });Cleaner, easier to extend when a new resource type is introduced.
624-697: Symmetric issue inremoveResourcesApply the same consolidation technique as suggested for
assignResourcesto avoid code drift between the two helpers.
3754-3764: Export list deviates from the public API elsewhereExposing the new helpers is great, but be careful: adding many top‑level exports can create a breaking‑change ripple for consumers doing
const { generateNumericToken } = require(...). Consider grouping new subscription helpers under asubscriptionnamespace to keep the surface tidy.Not blocking, just a design consideration.
src/auth-service/utils/token.util.js (2)
799-875: Successfully integrated access control into IP blacklist checkThe enhanced
isIPBlacklistedHelperfunction now properly loads the user and checks resource access permissions as part of the blacklist verification process.Consider combining the permission check logic in this function with the main
checkResourceAccessimplementation to avoid potential divergence in logic:- const isAllowed = await checkResourceAccess( - request, - accessToken, - user, - next - ); - if (!isAllowed) { + const accessCheck = await checkResourceAccess(request, accessToken, user, next); + if (!accessCheck.allowed) { + logger.info(`🔒 Access denied: ${accessCheck.reason} - ${accessCheck.message}`);
165-175: Improve optional chaining for resource access checksThe resource access checking code can be improved with optional chaining to make it more concise and robust:
- if (resourceType === "cohorts" && cohort_id) { - resourceAllowed = - user.cohorts && user.cohorts.map(String).includes(cohort_id); - } else if (resourceType === "grids" && grid_id) { - resourceAllowed = user.grids && user.grids.map(String).includes(grid_id); - } else if (resourceType === "devices" && device_id) { - resourceAllowed = - user.devices && user.devices.map(String).includes(device_id); - } else if (resourceType === "sites" && site_id) { - resourceAllowed = user.sites && user.sites.map(String).includes(site_id); - } + if (resourceType === "cohorts" && cohort_id) { + resourceAllowed = user.cohorts?.map(String).includes(cohort_id) ?? false; + } else if (resourceType === "grids" && grid_id) { + resourceAllowed = user.grids?.map(String).includes(grid_id) ?? false; + } else if (resourceType === "devices" && device_id) { + resourceAllowed = user.devices?.map(String).includes(device_id) ?? false; + } else if (resourceType === "sites" && site_id) { + resourceAllowed = user.sites?.map(String).includes(site_id) ?? false; + }This addresses the static analysis hints while making the code more robust against undefined properties.
🧰 Tools
🪛 Biome (1.9.4)
[error] 166-167: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 168-169: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 171-172: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 173-174: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/auth-service/API_SUBSCRIPTION_ACCESS_CONTROL.md(1 hunks)src/auth-service/controllers/token.controller.js(1 hunks)src/auth-service/controllers/user.controller.js(1 hunks)src/auth-service/routes/v2/tokens.routes.js(1 hunks)src/auth-service/routes/v2/users.routes.js(1 hunks)src/auth-service/utils/token.util.js(9 hunks)src/auth-service/utils/user.util.js(2 hunks)src/auth-service/validators/users.validators.js(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/auth-service/routes/v2/tokens.routes.js (2)
src/auth-service/routes/v2/users.routes.js (1)
router(3-3)src/auth-service/validators/users.validators.js (1)
validateTenant(6-16)
src/auth-service/validators/users.validators.js (1)
src/auth-service/utils/user.util.js (18)
body(313-313)body(544-544)body(628-628)body(1108-1112)body(1199-1199)body(1254-1254)body(1364-1364)body(1365-1365)body(1478-1479)body(1638-1638)body(1918-1918)body(2039-2039)body(2771-2771)body(2938-2938)body(3526-3526)updateSubscription(308-367)assignResources(540-619)removeResources(624-697)
src/auth-service/utils/user.util.js (6)
src/auth-service/utils/token.util.js (59)
request(48-48)request(230-232)request(363-363)request(924-927)request(1060-1060)request(1124-1124)request(1186-1186)request(1214-1216)request(1270-1270)request(1302-1302)request(1322-1322)request(1350-1350)request(1441-1441)request(1547-1547)request(1668-1672)request(1693-1696)query(49-49)query(1061-1061)query(1125-1125)query(1187-1187)query(1271-1271)query(1303-1303)query(1323-1323)query(1443-1443)query(1548-1548)query(2448-2448)user(270-270)user(376-376)user(801-801)user(1148-1148)user(1361-1361)user(1597-1597)UserModel(11-11)httpStatus(12-12)logger(28-28)body(1442-1442)update(967-969)update(1074-1074)update(1164-1164)update(1578-1582)subscriptionTier(419-419)subscriptionTier(1158-1158)subscriptionTier(1371-1371)tier(122-122)tier(2475-2475)ClientModel(7-7)client(248-250)client(387-389)client(1228-1230)client(1373-1375)scopes(123-123)scopes(422-424)scopes(1159-1161)scopes(1408-1410)scopes(2476-2476)ScopeModel(8-8)AccessTokenModel(9-9)ObjectId(26-26)user_id(1503-1503)src/auth-service/controllers/user.controller.js (8)
user(1190-1190)user(1229-1229)UserModel(14-14)tenant(61-63)tenant(1715-1715)tenant(1754-1756)httpStatus(1-1)logger(15-15)src/auth-service/middleware/rate-limit.js (1)
getRateLimits(20-42)src/auth-service/validators/users.validators.js (4)
updateSubscription(926-937)assignResources(939-963)ObjectId(4-4)removeResources(965-989)src/auth-service/utils/scope.util.js (1)
scope(9-335)src/auth-service/models/User.js (1)
usage(1278-1278)
🪛 Biome (1.9.4)
src/auth-service/controllers/token.controller.js
[error] 328-330: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/auth-service/utils/token.util.js
[error] 166-167: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 168-169: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 171-172: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 173-174: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: codecov/patch
src/auth-service/controllers/token.controller.js
[warning] 277-277: src/auth-service/controllers/token.controller.js#L277
Added line #L277 was not covered by tests
[warning] 280-280: src/auth-service/controllers/token.controller.js#L280
Added line #L280 was not covered by tests
[warning] 288-288: src/auth-service/controllers/token.controller.js#L288
Added line #L288 was not covered by tests
[warning] 294-294: src/auth-service/controllers/token.controller.js#L294
Added line #L294 was not covered by tests
[warning] 297-298: src/auth-service/controllers/token.controller.js#L297-L298
Added lines #L297 - L298 were not covered by tests
[warning] 305-305: src/auth-service/controllers/token.controller.js#L305
Added line #L305 was not covered by tests
[warning] 308-310: src/auth-service/controllers/token.controller.js#L308-L310
Added lines #L308 - L310 were not covered by tests
[warning] 312-312: src/auth-service/controllers/token.controller.js#L312
Added line #L312 was not covered by tests
[warning] 315-315: src/auth-service/controllers/token.controller.js#L315
Added line #L315 was not covered by tests
[warning] 317-317: src/auth-service/controllers/token.controller.js#L317
Added line #L317 was not covered by tests
[warning] 323-324: src/auth-service/controllers/token.controller.js#L323-L324
Added lines #L323 - L324 were not covered by tests
[warning] 332-332: src/auth-service/controllers/token.controller.js#L332
Added line #L332 was not covered by tests
[warning] 338-338: src/auth-service/controllers/token.controller.js#L338
Added line #L338 was not covered by tests
[warning] 341-341: src/auth-service/controllers/token.controller.js#L341
Added line #L341 was not covered by tests
src/auth-service/controllers/user.controller.js
[warning] 2184-2186: src/auth-service/controllers/user.controller.js#L2184-L2186
Added lines #L2184 - L2186 were not covered by tests
[warning] 2188-2188: src/auth-service/controllers/user.controller.js#L2188
Added line #L2188 was not covered by tests
[warning] 2191-2191: src/auth-service/controllers/user.controller.js#L2191
Added line #L2191 was not covered by tests
🪛 LanguageTool
src/auth-service/API_SUBSCRIPTION_ACCESS_CONTROL.md
[uncategorized] ~48-~48: Loose punctuation mark.
Context: ...g headers: - X-RateLimit-Limit-Hourly: Maximum hourly requests - `X-RateLimit-...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~180-~180: Consider using “email”.
Context: ...cific resources: 1. Email Request: Send an email to [email protected] with the following in...
(SEND_AN_EMAIL)
🪛 markdownlint-cli2 (0.17.2)
src/auth-service/API_SUBSCRIPTION_ACCESS_CONTROL.md
61-61: Bare URL used
null
(MD034, no-bare-urls)
86-86: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
180-180: Bare URL used
null
(MD034, no-bare-urls)
187-187: Bare URL used
null
(MD034, no-bare-urls)
251-251: Bare URL used
null
(MD034, no-bare-urls)
269-269: Bare URL used
null
(MD034, no-bare-urls)
270-270: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-push-deploy-auth-service
- GitHub Check: Analyze (javascript)
🔇 Additional comments (22)
src/auth-service/routes/v2/tokens.routes.js (2)
122-122: Effective rate limiting implementation for token verification.Adding
rateLimitMiddlewareto the verification endpoint is an appropriate security improvement that will help protect the API from abuse and ensure fair resource distribution among users based on their subscription tiers.
126-133: Well-designed new auto-detect verification endpoint.The new
/:token/auto-verifyroute is a thoughtful addition that enhances API usability through content negotiation. This endpoint maintains consistent middleware with the standard verification route while adding the flexibility to respond in the appropriate format based on client requirements.The route structure follows established patterns in the codebase, maintaining consistency with other routes through proper middleware sequencing: tenant validation → token parameter validation → rate limiting → controller method.
src/auth-service/routes/v2/users.routes.js (2)
317-341: Subscription management routes effectively implement tiered access control.These new routes provide a comprehensive subscription management interface:
- GET for retrieving subscription details
- PUT for updating subscription tier
- GET /usage for monitoring consumption
The routes follow established authentication and validation patterns, ensuring secure access to these critical subscription management features.
342-365: Resource assignment routes enable granular access control.These routes create a robust foundation for the resource-specific permissions system:
- POST for assigning resources to users
- DELETE for revoking resource access
- GET for listing assigned resources
The consistent application of middleware (tenant validation, JWT auth) maintains security while enabling fine-grained control over resources such as devices, sites, cohorts, and grids.
src/auth-service/validators/users.validators.js (6)
451-456: Appropriate subscription tier validation in user creation.The optional
subscriptionTierfield with validation for values "Free", "Standard", or "Premium" aligns perfectly with the subscription tiering system described in the PR objectives. Making this field optional ensures backward compatibility with existing user creation flows.
561-566: Consistent subscription tier validation in user updates.Adding the same
subscriptionTiervalidation to theupdateUserschema maintains consistency throughout the API. The validation ensures that subscription tiers can only be set to valid values, protecting data integrity.
926-937: Well-structured dedicated subscription update validator.The new
updateSubscriptionvalidator appropriately requires thesubscriptionTierfield (unlike in user creation/update where it's optional), ensuring that subscription updates always include a valid tier. This specialized validator provides a dedicated path for subscription management, separating it from general user updates.
939-964: Comprehensive resource assignment validation schema.The
assignResourcesvalidator implements thorough validation for the resource assignment feature:
- Validates user_id as a MongoDB ObjectId
- Ensures resources is provided as an array
- Validates each resource has a proper type (device, site, cohort, grid)
- Confirms each resource ID is a valid ObjectId
This schema provides strong validation for the resource assignment system, helping prevent data integrity issues.
965-989: Mirrored validation for resource removal operations.The
removeResourcesvalidator mirrors the structure of theassignResourcesvalidator, ensuring consistency between assignment and removal operations. This parallel structure makes the API more intuitive and maintainable.
996-997: Updated exports include new validators.The module exports have been correctly updated to include the new validators, making them available for use in routes. This ensures the validation middleware can be properly applied to the new endpoints.
Also applies to: 1001-1001
src/auth-service/controllers/user.controller.js (6)
2184-2228: Well-structured subscription retrieval method with good error handling.This
getSubscriptionmethod follows the established pattern in the controller file, with proper error extraction, utility function invocation, and response formatting. The response structure is consistent with other endpoints, returning the subscription data on success.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 2184-2186: src/auth-service/controllers/user.controller.js#L2184-L2186
Added lines #L2184 - L2186 were not covered by tests
[warning] 2188-2188: src/auth-service/controllers/user.controller.js#L2188
Added line #L2188 was not covered by tests
[warning] 2191-2191: src/auth-service/controllers/user.controller.js#L2191
Added line #L2191 was not covered by tests
2229-2273: Appropriate implementation of subscription update functionality.The
updateSubscriptionmethod correctly validates request errors before delegating to the utility function. The response handling maintains consistency with other controller methods, ensuring a uniform API experience.
2274-2320: Usage tracking implementation follows best practices.The
getSubscriptionUsagemethod is well-implemented, providing a clean interface to retrieve usage metrics. This will be important for users to monitor their API consumption against their subscription tier limits.
2321-2365: Resource assignment functionality properly implemented.The
assignResourcesmethod integrates well with the subscription-based access control system. This enables administrators to explicitly grant users access to specific resources (devices, sites, cohorts, grids) as described in the PR objectives.
2366-2410: Resource removal functionality properly implemented.The
removeResourcesmethod complements the assignment functionality, allowing for the removal of resource access when needed. The consistent error handling and response formatting maintain API uniformity.
2411-2455: Resource listing functionality follows established patterns.The
listResourcesmethod enables users to see which resources they have access to, which is critical for a permission-based system. The implementation follows the same pattern as other controller methods in this file.src/auth-service/utils/user.util.js (2)
409-535:getSubscriptionUsageignores per‑client limits & may mis‑count
clientIdsare fetched but never used. If usage is supposed to be aggregated across all of a user’s clients that’s fine, but please either:
- Remove the dead code, or
- Use
client_idin the Redis keys (e.g.,rate:${userId}:${clientId}:daily) to provide granular stats.Clarifying this now prevents confused analytics later.
308-367:❓ Verification inconclusive
Sanity‑check & secure the tier update flow
- The function trusts
body.subscriptionTierafter validator middleware, but still consider re‑validating against the enum (["Free","Standard","Premium"]) here to protect against routes that bypass the validator (e.g., internal calls).nextBillingDate.setMonth(nextBillingDate.getMonth() + 1)may yield surprising dates on 29th–31st (e.g., 31‑Jan → 03‑Mar). Consider usingmoment().add(1, "month").startOf("day")for safer rollover.- If
updateTokensForUserfails, the user record will already be updated, leaving tokens & scopes out‑of‑sync. Wrap the user+token update in a transaction (Mongo session) or catch the token update error and roll back the user document.
Sanity‑check & secure the tier update flow
- Re‑validate
body.subscriptionTieragainst your canonical enum ("Free","Standard","Premium") inside this function—don’t assume the validator middleware can’t be bypassed (e.g., internal calls).- Native
nextBillingDate.setMonth(getMonth() + 1)can roll past month ends (31 Jan → 3 Mar, etc.). Use a date library (e.g.moment().add(1, "month").startOf("day")or date‑fns) for predictable month‑increment behavior.- If
updateTokensForUserthrows, the user document is already updated—tokens/scopes will be out of sync. Either wrap the user update + token update in a Mongo transaction (session) or catch token errors and roll back the user change.— Please verify that no server‑side enum check currently exists and adjust accordingly.
src/auth-service/utils/token.util.js (4)
360-460: Well-structured token creation with dynamic scope assignmentThe
createAccessTokenfunction properly fetches scopes based on the user's subscription tier and assigns them to the token along with the tier information. This ensures tokens have the correct permissions for the user's subscription level.The token expiration setting (line 441) uses a 7-month period, which provides a good balance between security and user convenience.
462-505: Clean implementation of tier-based permission helpersThe helper functions
isPathAllowedForTierandgenerateScopesForTierprovide a clean abstraction for tier-based permission logic. The explicit mappings make the permission structure clear and maintainable.The permission tiers follow good security practices with progressive access levels:
- Free: Basic read access to recent measurements and resources
- Standard: Adds historical measurements
- Premium: Adds forecasts and insights
1134-1167: Comprehensive token regeneration with subscription tier awarenessThe updated
regenerateAccessTokenfunction now properly fetches the user's subscription tier and associated scopes when regenerating tokens, maintaining consistency with the access control system.This implementation ensures that when tokens are regenerated, they maintain the proper permission scopes based on the user's current subscription level.
2553-2560: Updated module exports with explicit function exposureThe module exports structure now explicitly exposes both the object and the standalone functions, providing flexibility in how the utilities are consumed.
This pattern allows for gradual migration from the object-based approach to direct function imports.
| // Get API version from request headers (if present) | ||
| const apiVersion = req.headers["x-api-version"] || "v1"; | ||
|
|
||
| const result = await tokenUtil.verifyToken(request, next); | ||
| const status = result.status; | ||
|
|
||
| if (!res.headersSent) { | ||
| if (result.success) { | ||
| if (apiVersion === "v1") { | ||
| // Original v1 format - just send message | ||
| res.status(status).send(result.message); | ||
| } else { | ||
| // v2 and above - enhanced JSON response with token data | ||
| res.status(status).json({ | ||
| message: result.message, | ||
| success: result.success, | ||
| ...(result.data && { data: result.data }), | ||
| }); | ||
| } | ||
| } else { | ||
| // Error response - consistent format for both versions | ||
| res.status(status).json({ | ||
| message: result.message, | ||
| success: result.success, | ||
| ...(result.errors && { errors: result.errors }), | ||
| }); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enhanced token verification with versioned API responses.
The revised verify method intelligently handles API versioning through the x-api-version header, supporting backward compatibility while enabling richer responses in newer versions. The response format distinction between v1 (simple text) and v2+ (structured JSON) is well-implemented.
I appreciate the additional null check with !res.headersSent that prevents potential "headers already sent" errors.
Consider adding automated tests to ensure both API version response formats work correctly:
🏁 Script executed:
#!/bin/bash
# Verify the token verification endpoint handles different API versions correctly
# Test with v1 header
echo "Testing with x-api-version: v1"
curl -s -X GET "http://localhost:3000/api/v2/tokens/sample-token/verify" \
-H "x-api-version: v1" | grep -q "Token is valid" && echo "✅ v1 response format OK" || echo "❌ v1 response format failed"
# Test with v2 header
echo "Testing with x-api-version: v2"
curl -s -X GET "http://localhost:3000/api/v2/tokens/sample-token/verify" \
-H "x-api-version: v2" | jq -e '.success' > /dev/null && echo "✅ v2 response format OK" || echo "❌ v2 response format failed"Length of output: 499
Tests uncovered an issue with the v1 response format
The automated curl test for x‑api‑version: v1 did not return the expected plain‑text message, whereas the v2 JSON payload is working as intended. Before we merge, please:
- Verify what
result.messageactually contains for a valid token (e.g. check for trailing punctuation or wording changes). - Ensure the v1 branch sends that exact string as plain text (
res.send(result.message)). - Update the test’s grep pattern to match the real message (or adjust the code to emit “Token is valid” if that’s the intended phrase).
- Add automated tests for error scenarios and v2+ payloads (including the optional
datafield).
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 277-277: src/auth-service/controllers/token.controller.js#L277
Added line #L277 was not covered by tests
[warning] 280-280: src/auth-service/controllers/token.controller.js#L280
Added line #L280 was not covered by tests
[warning] 288-288: src/auth-service/controllers/token.controller.js#L288
Added line #L288 was not covered by tests
[warning] 294-294: src/auth-service/controllers/token.controller.js#L294
Added line #L294 was not covered by tests
| autoVerify: async (req, res, next) => { | ||
| try { | ||
| const errors = extractErrorsFromRequest(req); | ||
| if (errors) { | ||
| next( | ||
| new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors) | ||
| ); | ||
| return; | ||
| } | ||
| const request = req; | ||
| const defaultTenant = constants.DEFAULT_TENANT || "airqo"; | ||
| request.query.tenant = isEmpty(req.query.tenant) | ||
| ? defaultTenant | ||
| : req.query.tenant; | ||
|
|
||
| const result = await tokenUtil.verifyToken(request, next); | ||
| const status = result.status; | ||
|
|
||
| if (!res.headersSent) { | ||
| res.status(status).send(result.message); | ||
| const acceptsJson = | ||
| req.headers.accept && req.headers.accept.includes("application/json"); | ||
|
|
||
| if (result.success) { | ||
| if (acceptsJson) { | ||
| res.status(status).json({ | ||
| message: result.message, | ||
| success: result.success, | ||
| ...(result.data && { data: result.data }), | ||
| }); | ||
| } else { | ||
| res.status(status).send(result.message); | ||
| } | ||
| } else { | ||
| res.status(status).json({ | ||
| message: result.message, | ||
| success: result.success, | ||
| ...(result.errors && { errors: result.errors }), | ||
| }); | ||
| } | ||
| return; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Well-implemented auto-verify method with content negotiation.
The new autoVerify method elegantly determines the appropriate response format based on the client's Accept header, providing JSON responses for clients requesting application/json and simpler text responses otherwise. This facilitates integration with a wider range of clients while maintaining a consistent verification process.
Note that the CodeCov report indicates this new method lacks test coverage. Consider adding tests for this new functionality.
🏁 Script executed:
#!/bin/bash
# Verify that the auto-verify endpoint correctly handles different Accept headers
# Test with application/json Accept header
echo "Testing with Accept: application/json"
curl -s -X GET "http://localhost:3000/api/v2/tokens/sample-token/auto-verify" \
-H "Accept: application/json" | jq -e '.success' > /dev/null && echo "✅ JSON response OK" || echo "❌ JSON response failed"
# Test with text/plain Accept header
echo "Testing with Accept: text/plain"
curl -s -X GET "http://localhost:3000/api/v2/tokens/sample-token/auto-verify" \
-H "Accept: text/plain" | grep -q "Token is valid" && echo "✅ Text response OK" || echo "❌ Text response failed"Length of output: 513
🏁 Script executed:
#!/bin/bash
# Inspect the actual text/plain response body for the auto-verify endpoint
echo "Text/plain response body:"
curl -s -X GET "http://localhost:3000/api/v2/tokens/sample-token/auto-verify" \
-H "Accept: text/plain" | cat -vetLength of output: 173
To diagnose why the text response is empty, let’s first inspect the JSON payload to confirm what result.message contains and then review how verifyToken sets that message.
🏁 Script executed:
#!/bin/bash
# 1. Show the full JSON response so we can see the "message" field
echo "JSON response body:"
curl -s -X GET "http://localhost:3000/api/v2/tokens/sample-token/auto-verify" \
-H "Accept: application/json" | jq '.'
# 2. Locate the verifyToken implementation to see how `message` is assigned
echo -e "\nverifyToken implementation context:"
rg -n "verifyToken" -C5 --glob '*.js' srcLength of output: 12248
Fix missing text response message
The text/plain branch sends an empty body because result.message is undefined for successful verifications. Please ensure a default message is provided or that verifyToken always populates message.
Locations to update:
- src/auth-service/controllers/token.controller.js (around the
elseat the end ofautoVerify) - src/auth-service/utils/token.util.js – add a
messageincreateValidTokenResponse
Suggested controller diff:
} else {
- res.status(status).send(result.message);
+ res
+ .status(status)
+ .send(
+ result.message ||
+ (result.success ? "Token is valid" : "Token is invalid")
+ );
}And don’t forget to add unit tests for the text/plain path in
src/auth-service/controllers/test/ut_token.controller.js to cover both success and failure messages.
📝 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.
| autoVerify: async (req, res, next) => { | |
| try { | |
| const errors = extractErrorsFromRequest(req); | |
| if (errors) { | |
| next( | |
| new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors) | |
| ); | |
| return; | |
| } | |
| const request = req; | |
| const defaultTenant = constants.DEFAULT_TENANT || "airqo"; | |
| request.query.tenant = isEmpty(req.query.tenant) | |
| ? defaultTenant | |
| : req.query.tenant; | |
| const result = await tokenUtil.verifyToken(request, next); | |
| const status = result.status; | |
| if (!res.headersSent) { | |
| res.status(status).send(result.message); | |
| const acceptsJson = | |
| req.headers.accept && req.headers.accept.includes("application/json"); | |
| if (result.success) { | |
| if (acceptsJson) { | |
| res.status(status).json({ | |
| message: result.message, | |
| success: result.success, | |
| ...(result.data && { data: result.data }), | |
| }); | |
| } else { | |
| res.status(status).send(result.message); | |
| } | |
| } else { | |
| res.status(status).json({ | |
| message: result.message, | |
| success: result.success, | |
| ...(result.errors && { errors: result.errors }), | |
| }); | |
| } | |
| return; | |
| autoVerify: async (req, res, next) => { | |
| try { | |
| const errors = extractErrorsFromRequest(req); | |
| if (errors) { | |
| next( | |
| new HttpError("bad request errors", httpStatus.BAD_REQUEST, errors) | |
| ); | |
| return; | |
| } | |
| const request = req; | |
| const defaultTenant = constants.DEFAULT_TENANT || "airqo"; | |
| request.query.tenant = isEmpty(req.query.tenant) | |
| ? defaultTenant | |
| : req.query.tenant; | |
| const result = await tokenUtil.verifyToken(request, next); | |
| const status = result.status; | |
| if (!res.headersSent) { | |
| const acceptsJson = | |
| req.headers.accept && req.headers.accept.includes("application/json"); | |
| if (result.success) { | |
| if (acceptsJson) { | |
| res.status(status).json({ | |
| message: result.message, | |
| success: result.success, | |
| ...(result.data && { data: result.data }), | |
| }); | |
| } else { | |
| // Provide a default text response when result.message is undefined | |
| res | |
| .status(status) | |
| .send( | |
| result.message || | |
| (result.success ? "Token is valid" : "Token is invalid") | |
| ); | |
| } | |
| } else { | |
| res.status(status).json({ | |
| message: result.message, | |
| success: result.success, | |
| ...(result.errors && { errors: result.errors }), | |
| }); | |
| } | |
| return; | |
| } | |
| // …rest of the method… |
🧰 Tools
🪛 Biome (1.9.4)
[error] 328-330: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: codecov/patch
[warning] 308-310: src/auth-service/controllers/token.controller.js#L308-L310
Added lines #L308 - L310 were not covered by tests
[warning] 312-312: src/auth-service/controllers/token.controller.js#L312
Added line #L312 was not covered by tests
[warning] 315-315: src/auth-service/controllers/token.controller.js#L315
Added line #L315 was not covered by tests
[warning] 317-317: src/auth-service/controllers/token.controller.js#L317
Added line #L317 was not covered by tests
[warning] 323-324: src/auth-service/controllers/token.controller.js#L323-L324
Added lines #L323 - L324 were not covered by tests
[warning] 332-332: src/auth-service/controllers/token.controller.js#L332
Added line #L332 was not covered by tests
[warning] 338-338: src/auth-service/controllers/token.controller.js#L338
Added line #L338 was not covered by tests
[warning] 341-341: src/auth-service/controllers/token.controller.js#L341
Added line #L341 was not covered by tests
| ## API Endpoints | ||
|
|
||
| The available endpoints depend on your subscription tier: | ||
|
|
||
| ### Free Tier Endpoints | ||
|
|
||
| - `/api/v2/devices/measurements/recent` - Get recent measurements (last 24 hours) | ||
| - `/api/v2/devices` - List available devices | ||
| - `/api/v2/devices/sites` - List available sites | ||
| - `/api/v2/devices/cohorts` - List available cohorts | ||
| - `/api/v2/devices/grids` - List available grids | ||
|
|
||
| ### Standard Tier Endpoints | ||
|
|
||
| All Free Tier endpoints, plus: | ||
|
|
||
| - `/api/v2/devices/measurements` - Access historical measurements with date filtering | ||
|
|
||
| ### Premium Tier Endpoints | ||
|
|
||
| All Standard Tier endpoints, plus: | ||
|
|
||
| - `/api/v2/devices/forecasts` - Access air quality forecasts | ||
| - `/api/v2/devices/insights` - Access advanced analytics and insights | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document user subscription management endpoints.
The documentation currently lists API endpoints available to each tier but doesn't include the new subscription management and resource assignment endpoints introduced in this PR.
Add documentation for the subscription management endpoints:
### Subscription Management Endpoints
The following endpoints are available for managing your subscription and resources:
- `/api/v2/users/subscription` - GET: Retrieve your current subscription details
- `/api/v2/users/subscription` - PUT: Update your subscription
- `/api/v2/users/subscription/usage` - GET: Get your API usage statistics
- `/api/v2/users/resources` - GET: List resources assigned to your account
- `/api/v2/users/resources` - POST: Assign resources to your account
- `/api/v2/users/resources` - DELETE: Remove resources from your account| const getSubscription = async (request, next) => { | ||
| try { | ||
| const { query } = request; | ||
| const { tenant } = query; | ||
| const filter = generateFilter.users(request, next); | ||
|
|
||
| const user = await UserModel(tenant) | ||
| .findOne(filter) | ||
| .select( | ||
| "subscriptionTier subscriptionStatus lastSubscriptionCheck apiRateLimits nextBillingDate" | ||
| ) | ||
| .lean(); | ||
|
|
||
| if (!user) { | ||
| return { | ||
| success: false, | ||
| message: "User not found", | ||
| status: httpStatus.NOT_FOUND, | ||
| errors: { message: "User not found" }, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| success: true, | ||
| message: "Successfully retrieved subscription", | ||
| data: { | ||
| tier: user.subscriptionTier || "Free", | ||
| status: user.subscriptionStatus || "inactive", | ||
| lastChecked: user.lastSubscriptionCheck, | ||
| rateLimits: user.apiRateLimits || { | ||
| hourly: getRateLimits(user.subscriptionTier).hourly, | ||
| daily: getRateLimits(user.subscriptionTier).daily, | ||
| monthly: getRateLimits(user.subscriptionTier).monthly, | ||
| }, | ||
| nextBillingDate: user.nextBillingDate, | ||
| }, | ||
| status: httpStatus.OK, | ||
| }; | ||
| } catch (error) { | ||
| logger.error(`Error getting subscription: ${error.message}`); | ||
| next( | ||
| new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, { | ||
| message: error.message, | ||
| }) | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Undefined identifiers getRateLimits & database models – will crash at runtime
getSubscription (and the other subscription‑related helpers) call getRateLimits, ClientModel, ScopeModel, and AccessTokenModel, but those symbols are never imported in this module. As soon as any of these code paths execute the service will throw a ReferenceError, breaking the whole auth flow.
@@
const { db } = require("@config/firebase-admin");
+const { getRateLimits } = require("@auth-service/middleware/rate-limit");
+const ClientModel = require("@models/Client");
+const ScopeModel = require("@models/Scope");
+const AccessTokenModel = require("@models/AccessToken");Please adjust the paths/namespaces to match your actual folder structure.
📝 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 getSubscription = async (request, next) => { | |
| try { | |
| const { query } = request; | |
| const { tenant } = query; | |
| const filter = generateFilter.users(request, next); | |
| const user = await UserModel(tenant) | |
| .findOne(filter) | |
| .select( | |
| "subscriptionTier subscriptionStatus lastSubscriptionCheck apiRateLimits nextBillingDate" | |
| ) | |
| .lean(); | |
| if (!user) { | |
| return { | |
| success: false, | |
| message: "User not found", | |
| status: httpStatus.NOT_FOUND, | |
| errors: { message: "User not found" }, | |
| }; | |
| } | |
| return { | |
| success: true, | |
| message: "Successfully retrieved subscription", | |
| data: { | |
| tier: user.subscriptionTier || "Free", | |
| status: user.subscriptionStatus || "inactive", | |
| lastChecked: user.lastSubscriptionCheck, | |
| rateLimits: user.apiRateLimits || { | |
| hourly: getRateLimits(user.subscriptionTier).hourly, | |
| daily: getRateLimits(user.subscriptionTier).daily, | |
| monthly: getRateLimits(user.subscriptionTier).monthly, | |
| }, | |
| nextBillingDate: user.nextBillingDate, | |
| }, | |
| status: httpStatus.OK, | |
| }; | |
| } catch (error) { | |
| logger.error(`Error getting subscription: ${error.message}`); | |
| next( | |
| new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, { | |
| message: error.message, | |
| }) | |
| ); | |
| } | |
| }; | |
| // at the top of src/auth-service/utils/user.util.js | |
| const { db } = require("@config/firebase-admin"); | |
| const { getRateLimits } = require("@auth-service/middleware/rate-limit"); | |
| const ClientModel = require("@models/Client"); | |
| const ScopeModel = require("@models/Scope"); | |
| const AccessTokenModel = require("@models/AccessToken"); |
| // Enhanced verifyToken function to use the new checkResourceAccess | ||
| const verifyToken = async (request, next) => { | ||
| try { | ||
| logText("Entering enhanced verifyToken function"); | ||
| const ip = | ||
| request.headers["x-client-ip"] || request.headers["x-client-original-ip"]; | ||
| const endpoint = request.headers["x-original-uri"]; | ||
| const { token } = { | ||
| ...request.params, | ||
| }; | ||
|
|
||
| // 1. Basic token validation | ||
| const accessToken = await AccessTokenModel("airqo") | ||
| .findOne({ token }) | ||
| .select("client_id token scopes tier"); | ||
|
|
||
| if (isEmpty(accessToken)) { | ||
| return createUnauthorizedResponse(); | ||
| } else if (isEmpty(ip)) { | ||
| logText(`🚨 Token is being accessed without an IP address`); | ||
| logger.error(`🚨 Token is being accessed without an IP address`); | ||
| return createUnauthorizedResponse(); | ||
| } | ||
|
|
||
| // 2. Check client status | ||
| const client = await ClientModel("airqo") | ||
| .findById(accessToken.client_id) | ||
| .select("isActive user_id"); | ||
|
|
||
| if (isEmpty(client) || (client && !client.isActive)) { | ||
| logger.error( | ||
| `🚨 Client ${accessToken.client_id} associated with Token ${accessToken.token} is INACTIVE or does not exist` | ||
| ); | ||
| return createUnauthorizedResponse(); | ||
| } | ||
|
|
||
| // 3. Check IP blacklisting | ||
| const isBlacklisted = await isIPBlacklisted({ | ||
| request, | ||
| next, | ||
| }); | ||
|
|
||
| if (isBlacklisted) { | ||
| return createUnauthorizedResponse(); | ||
| } | ||
|
|
||
| // 4. Load the user data | ||
| const user = await UserModel("airqo").findById(client.user_id).lean(); | ||
| if (!user) { | ||
| logger.error(`🚨 User not found for Access Token ${accessToken.token}`); | ||
| return createUnauthorizedResponse(); | ||
| } | ||
|
|
||
| const tokenData = { | ||
| userId: user._id, | ||
| clientId: client._id, | ||
| permissions: user.permissions || [], | ||
| resources: { | ||
| devices: user.devices || [], | ||
| sites: user.sites || [], | ||
| cohorts: user.cohorts || [], | ||
| grids: user.grids || [], | ||
| }, | ||
| scopes: accessToken.scopes || [], | ||
| tier: accessToken.tier || user.subscriptionTier || "Free", | ||
| }; | ||
|
|
||
| // 5. Enhanced access control check | ||
| const accessCheck = await checkResourceAccess( | ||
| request, | ||
| accessToken, | ||
| user, | ||
| next | ||
| ); | ||
|
|
||
| if (!accessCheck.allowed) { | ||
| logger.info( | ||
| `🔒 Access denied: ${accessCheck.reason} - ${accessCheck.message}` | ||
| ); | ||
| return { | ||
| success: false, | ||
| message: accessCheck.message, | ||
| status: httpStatus.FORBIDDEN, | ||
| errors: { | ||
| reason: accessCheck.reason, | ||
| message: accessCheck.message, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| // 6. Update token usage metadata asynchronously | ||
| Promise.resolve().then(() => { | ||
| // Update last_used_at and last_ip_address | ||
| AccessTokenModel("airqo") | ||
| .findOneAndUpdate( | ||
| { token }, | ||
| { | ||
| last_used_at: new Date(), | ||
| last_ip_address: ip, | ||
| }, | ||
| { new: true } | ||
| ) | ||
| .catch((err) => { | ||
| logger.error(`Failed to update token usage metadata: ${err.message}`); | ||
| }); | ||
|
|
||
| // Log token usage for analytics | ||
| postProcessing({ | ||
| ip, | ||
| token: accessToken.token, | ||
| name: accessToken.name || "unknown", | ||
| client_id: accessToken.client_id, | ||
| endpoint, | ||
| day: getDay(), | ||
| }); | ||
| }); | ||
|
|
||
| // 7. Log successful token verification | ||
| winstonLogger.info("verify token", { | ||
| token: token, | ||
| service: "verify-token", | ||
| clientIp: ip, | ||
| clientOriginalIp: ip, | ||
| endpoint: endpoint ? endpoint : "unknown", | ||
| }); | ||
|
|
||
| return createValidTokenResponse(tokenData); | ||
| } catch (error) { | ||
| logger.error(`🐛 Internal Server Error in verifyToken: ${error.message}`); | ||
| next( | ||
| new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, { | ||
| message: error.message, | ||
| }) | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhanced token verification with integrated permission checks
The enhanced verifyToken function properly integrates the new access control system with existing token verification. The structured approach (token validation → client check → IP check → user loading → access control) creates a clear authentication flow.
The function uses a hard-coded "airqo" tenant in multiple places:
- const accessToken = await AccessTokenModel("airqo")
+ const tenant = request.query.tenant || request.body.tenant || "airqo";
+ const accessToken = await AccessTokenModel(tenant)- const client = await ClientModel("airqo")
+ const client = await ClientModel(tenant)- const user = await UserModel("airqo").findById(client.user_id).lean();
+ const user = await UserModel(tenant).findById(client.user_id).lean();This should be replaced with dynamic tenant resolution for proper multi-tenancy support.
📝 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.
| // Enhanced verifyToken function to use the new checkResourceAccess | |
| const verifyToken = async (request, next) => { | |
| try { | |
| logText("Entering enhanced verifyToken function"); | |
| const ip = | |
| request.headers["x-client-ip"] || request.headers["x-client-original-ip"]; | |
| const endpoint = request.headers["x-original-uri"]; | |
| const { token } = { | |
| ...request.params, | |
| }; | |
| // 1. Basic token validation | |
| const accessToken = await AccessTokenModel("airqo") | |
| .findOne({ token }) | |
| .select("client_id token scopes tier"); | |
| if (isEmpty(accessToken)) { | |
| return createUnauthorizedResponse(); | |
| } else if (isEmpty(ip)) { | |
| logText(`🚨 Token is being accessed without an IP address`); | |
| logger.error(`🚨 Token is being accessed without an IP address`); | |
| return createUnauthorizedResponse(); | |
| } | |
| // 2. Check client status | |
| const client = await ClientModel("airqo") | |
| .findById(accessToken.client_id) | |
| .select("isActive user_id"); | |
| if (isEmpty(client) || (client && !client.isActive)) { | |
| logger.error( | |
| `🚨 Client ${accessToken.client_id} associated with Token ${accessToken.token} is INACTIVE or does not exist` | |
| ); | |
| return createUnauthorizedResponse(); | |
| } | |
| // 3. Check IP blacklisting | |
| const isBlacklisted = await isIPBlacklisted({ | |
| request, | |
| next, | |
| }); | |
| if (isBlacklisted) { | |
| return createUnauthorizedResponse(); | |
| } | |
| // 4. Load the user data | |
| const user = await UserModel("airqo").findById(client.user_id).lean(); | |
| if (!user) { | |
| logger.error(`🚨 User not found for Access Token ${accessToken.token}`); | |
| return createUnauthorizedResponse(); | |
| } | |
| const tokenData = { | |
| userId: user._id, | |
| clientId: client._id, | |
| permissions: user.permissions || [], | |
| resources: { | |
| devices: user.devices || [], | |
| sites: user.sites || [], | |
| cohorts: user.cohorts || [], | |
| grids: user.grids || [], | |
| }, | |
| scopes: accessToken.scopes || [], | |
| tier: accessToken.tier || user.subscriptionTier || "Free", | |
| }; | |
| // 5. Enhanced access control check | |
| const accessCheck = await checkResourceAccess( | |
| request, | |
| accessToken, | |
| user, | |
| next | |
| ); | |
| if (!accessCheck.allowed) { | |
| logger.info( | |
| `🔒 Access denied: ${accessCheck.reason} - ${accessCheck.message}` | |
| ); | |
| return { | |
| success: false, | |
| message: accessCheck.message, | |
| status: httpStatus.FORBIDDEN, | |
| errors: { | |
| reason: accessCheck.reason, | |
| message: accessCheck.message, | |
| }, | |
| }; | |
| } | |
| // 6. Update token usage metadata asynchronously | |
| Promise.resolve().then(() => { | |
| // Update last_used_at and last_ip_address | |
| AccessTokenModel("airqo") | |
| .findOneAndUpdate( | |
| { token }, | |
| { | |
| last_used_at: new Date(), | |
| last_ip_address: ip, | |
| }, | |
| { new: true } | |
| ) | |
| .catch((err) => { | |
| logger.error(`Failed to update token usage metadata: ${err.message}`); | |
| }); | |
| // Log token usage for analytics | |
| postProcessing({ | |
| ip, | |
| token: accessToken.token, | |
| name: accessToken.name || "unknown", | |
| client_id: accessToken.client_id, | |
| endpoint, | |
| day: getDay(), | |
| }); | |
| }); | |
| // 7. Log successful token verification | |
| winstonLogger.info("verify token", { | |
| token: token, | |
| service: "verify-token", | |
| clientIp: ip, | |
| clientOriginalIp: ip, | |
| endpoint: endpoint ? endpoint : "unknown", | |
| }); | |
| return createValidTokenResponse(tokenData); | |
| } catch (error) { | |
| logger.error(`🐛 Internal Server Error in verifyToken: ${error.message}`); | |
| next( | |
| new HttpError("Internal Server Error", httpStatus.INTERNAL_SERVER_ERROR, { | |
| message: error.message, | |
| }) | |
| ); | |
| } | |
| }; | |
| const verifyToken = async (request, next) => { | |
| try { | |
| logText("Entering enhanced verifyToken function"); | |
| const ip = | |
| request.headers["x-client-ip"] || request.headers["x-client-original-ip"]; | |
| const endpoint = request.headers["x-original-uri"]; | |
| const { token } = { | |
| ...request.params, | |
| }; | |
| // Resolve tenant dynamically for multi‑tenancy | |
| const tenant = request.query.tenant || request.body.tenant || "airqo"; | |
| // 1. Basic token validation | |
| const accessToken = await AccessTokenModel(tenant) | |
| .findOne({ token }) | |
| .select("client_id token scopes tier"); | |
| if (isEmpty(accessToken)) { | |
| return createUnauthorizedResponse(); | |
| } else if (isEmpty(ip)) { | |
| logText(`🚨 Token is being accessed without an IP address`); | |
| logger.error(`🚨 Token is being accessed without an IP address`); | |
| return createUnauthorizedResponse(); | |
| } | |
| // 2. Check client status | |
| const client = await ClientModel(tenant) | |
| .findById(accessToken.client_id) | |
| .select("isActive user_id"); | |
| if (isEmpty(client) || (client && !client.isActive)) { | |
| logger.error( | |
| `🚨 Client ${accessToken.client_id} associated with Token ${accessToken.token} is INACTIVE or does not exist` | |
| ); | |
| return createUnauthorizedResponse(); | |
| } | |
| // 3. Check IP blacklisting | |
| const isBlacklisted = await isIPBlacklisted({ request, next }); | |
| if (isBlacklisted) { | |
| return createUnauthorizedResponse(); | |
| } | |
| // 4. Load the user data | |
| const user = await UserModel(tenant).findById(client.user_id).lean(); | |
| if (!user) { | |
| logger.error(`🚨 User not found for Access Token ${accessToken.token}`); | |
| return createUnauthorizedResponse(); | |
| } | |
| const tokenData = { | |
| userId: user._id, | |
| clientId: client._id, | |
| permissions: user.permissions || [], | |
| resources: { | |
| devices: user.devices || [], | |
| sites: user.sites || [], | |
| cohorts: user.cohorts || [], | |
| grids: user.grids || [], | |
| }, | |
| scopes: accessToken.scopes || [], | |
| tier: accessToken.tier || user.subscriptionTier || "Free", | |
| }; | |
| // 5. Enhanced access control check | |
| const accessCheck = await checkResourceAccess( | |
| request, | |
| accessToken, | |
| user, | |
| next | |
| ); | |
| if (!accessCheck.allowed) { | |
| logger.info( | |
| `🔒 Access denied: ${accessCheck.reason} - ${accessCheck.message}` | |
| ); | |
| return { | |
| success: false, | |
| message: accessCheck.message, | |
| status: httpStatus.FORBIDDEN, | |
| errors: { | |
| reason: accessCheck.reason, | |
| message: accessCheck.message, | |
| }, | |
| }; | |
| } | |
| // 6. Update token usage metadata asynchronously | |
| Promise.resolve().then(() => { | |
| AccessTokenModel(tenant) | |
| .findOneAndUpdate( | |
| { token }, | |
| { last_used_at: new Date(), last_ip_address: ip }, | |
| { new: true } | |
| ) | |
| .catch((err) => | |
| logger.error(`Failed to update token usage metadata: ${err.message}`) | |
| ); | |
| postProcessing({ | |
| ip, | |
| token: accessToken.token, | |
| name: accessToken.name || "unknown", | |
| client_id: accessToken.client_id, | |
| endpoint, | |
| day: getDay(), | |
| }); | |
| }); | |
| // 7. Log successful token verification | |
| winstonLogger.info("verify token", { | |
| token, | |
| service: "verify-token", | |
| clientIp: ip, | |
| clientOriginalIp: ip, | |
| endpoint: endpoint || "unknown", | |
| }); | |
| return createValidTokenResponse(tokenData); | |
| } catch (error) { | |
| logger.error(`🐛 Internal Server Error in verifyToken: ${error.message}`); | |
| next( | |
| new HttpError( | |
| "Internal Server Error", | |
| httpStatus.INTERNAL_SERVER_ERROR, | |
| { message: error.message } | |
| ) | |
| ); | |
| } | |
| }; |
1 participant
Description
This PR adds comprehensive API subscription tiering, allowing differentiated access to AirQo API endpoints with granular API access control capabilities based on user subscription tiers (Free, Standard, Premium) and resource access permissions, rate limiting capabilities along with new endpoints for subscription management and resource assignment.
Changes Made
Testing
Affected Services
Endpoints Ready for Testing
API Documentation Updated?
Summary by CodeRabbit