Skip to content

Conversation

Baalmart
Copy link
Contributor

@Baalmart Baalmart commented May 18, 2025

Description

Implements access control for private cohorts through enhanced JWT tokens, allowing cohort owners to view measurements from their private cohorts while maintaining microservice separation.

Changes Made

  • Add getCohortAccessClaims() method to User schema in Auth Service
  • Enhance JWT token generation to include cohort access information
  • Create JWT extraction middleware in Device Registry Service
  • Update Event aggregation pipeline to apply visibility filtering based on token claims
  • Add string normalization for consistent group identifier matching
  • Add feature flag for controlled rollout
  • Create unit tests for JWT token generation and extraction
  • Add integration tests for the end-to-end access control flow
  • Update event controller to use cohort access claims from middleware

Testing

  • Tested locally
  • Tested against staging environment
  • Relevant tests passed: [List test names]

Affected Services

  • Which services were modified:
    • Auth Service
    • Device Registry

Endpoints Ready for Testing

  • New endpoints ready for testing:
    • POST /api/auth/login - Enhanced token generation with cohort access claims
    • POST /api/auth/refresh - Enhanced token refresh with cohort access claims
    • GET /api/events - Modified to use JWT claims for visibility filtering
    • GET /api/events/recent - Modified to use JWT claims for visibility filtering
    • GET /api/events/historical - Modified to use JWT claims for visibility filtering

API Documentation Updated?

  • Yes, API documentation was updated
  • No, API documentation does not need updating

Additional Notes

Note: No new endpoints were required; this implementation modifies existing endpoints to interpret JWT tokens differently and apply appropriate access control.

Summary by CodeRabbit

  • New Features
    • Introduced cohort-based access control for private data, enabling users to access private cohorts based on their group roles.
    • JWT tokens now include cohort access claims, allowing seamless authorization across services.
    • Added a new endpoint to fetch events with cohort access enforcement.
  • Bug Fixes
    • Ensured group identifiers are consistently formatted in lowercase for reliable access checks.
  • Documentation
    • Added comprehensive documentation outlining the new cohort access control system and its phased rollout.
  • Chores
    • Implemented middleware for extracting and normalizing cohort access claims from JWT tokens.

@Baalmart Baalmart marked this pull request as draft May 18, 2025 12:37
Copy link
Contributor

coderabbitai bot commented May 18, 2025

📝 Walkthrough

Walkthrough

This update introduces cohort access control features across authentication and device registry services. It adds methods to embed and extract cohort access claims in JWT tokens, normalizes group identifiers, updates event fetching to honor cohort permissions, and documents the phased implementation plan. New middleware and route handlers support these authorization enhancements.

Changes

File(s) Change Summary
src/auth-service/models/User.js Added getCohortAccessClaims method and updated createToken to include cohort access claims in JWT payload.
src/device-registry/controllers/event.controller.js Added getEvents method to handle event fetching with cohort access control.
src/device-registry/docs/COHORT_ACCESS_CONTROL.md Introduced a comprehensive implementation plan for cohort access control across services.
src/device-registry/middleware/extractCohortAccess.js Added middleware to extract and normalize cohort access claims from JWT tokens in incoming requests.
src/device-registry/models/Cohort.js Added setter to normalize groups array elements to lowercase on assignment.
src/device-registry/routes/v2/events.routes.js Applied extractCohortAccess middleware globally and introduced new GET /get-events route.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant AuthService
    participant DeviceRegistry
    participant DB

    Client->>AuthService: Login/Request Token
    AuthService->>DB: Fetch user, group roles, permissions
    AuthService->>AuthService: getCohortAccessClaims()
    AuthService->>Client: Issue JWT with cohortAccess claims

    Client->>DeviceRegistry: GET /get-events (with JWT)
    DeviceRegistry->>DeviceRegistry: extractCohortAccess middleware
    DeviceRegistry->>DB: Query events with cohortAccess claims
    DB-->>DeviceRegistry: Return filtered events
    DeviceRegistry-->>Client: Respond with events
Loading

Poem

In the realm where tokens flow,
Cohort secrets now we know.
With lowercase groups and claims in hand,
Access is granted just as planned.
Middleware stands at every gate,
Ensuring only the right cohorts await.
Secure and tidy, the services sing—
Authorization has a brand new ring!

🗝️🔐✨

Note

⚡️ AI Code Reviews for VS Code, Cursor, Windsurf

CodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback.
Learn more here.


Note

⚡️ Faster reviews with caching

CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.
Enjoy the performance boost—your workflow just got faster.

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)
  • JIRA integration encountered authorization issues. Please disconnect and reconnect the integration in the CodeRabbit UI.
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate Unit Tests
  • Create PR with Unit Tests
  • Commit Unit Tests in branch private-cohort-access
  • Post Copyable Unit Tests in Comment

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai auto-generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

codecov bot commented May 18, 2025

Codecov Report

Attention: Patch coverage is 12.50000% with 56 lines in your changes missing coverage. Please review.

Project coverage is 11.78%. Comparing base (a1b5a59) to head (046a6bf).
Report is 5 commits behind head on staging.

Files with missing lines Patch % Lines
src/auth-service/models/User.js 4.00% 24 Missing ⚠️
.../device-registry/middleware/extractCohortAccess.js 16.66% 20 Missing ⚠️
...rc/device-registry/controllers/event.controller.js 0.00% 8 Missing ⚠️
src/device-registry/models/Cohort.js 0.00% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           staging    #4781   +/-   ##
========================================
  Coverage    11.78%   11.78%           
========================================
  Files          165      166    +1     
  Lines        19609    19664   +55     
  Branches       588      595    +7     
========================================
+ Hits          2310     2318    +8     
- Misses       17295    17342   +47     
  Partials         4        4           
Files with missing lines Coverage Δ
src/device-registry/routes/v2/events.routes.js 100.00% <100.00%> (ø)
src/device-registry/models/Cohort.js 22.85% <0.00%> (-0.91%) ⬇️
...rc/device-registry/controllers/event.controller.js 1.60% <0.00%> (-0.02%) ⬇️
.../device-registry/middleware/extractCohortAccess.js 16.66% <16.66%> (ø)
src/auth-service/models/User.js 10.06% <4.00%> (-0.20%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

Auth-service changes in this PR available for preview here

@Baalmart Baalmart marked this pull request as ready for review May 20, 2025 09:36
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (6)
src/device-registry/controllers/event.controller.js (1)

3159-3159: Error handling inconsistency.

This method directly handles errors and returns responses, while other methods in the controller use the next middleware pattern for error handling.

For consistency with the rest of the controller, consider using the next middleware:

- return res.status(200).json(result);
+ res.status(200).json(result);
- return res.status(500).json({
-   success: false,
-   message: error.message,
- });
+ next(
+   new HttpError(
+     "Internal Server Error",
+     httpStatus.INTERNAL_SERVER_ERROR,
+     { message: error.message }
+   )
+ );
src/device-registry/middleware/extractCohortAccess.js (1)

23-26: Security consideration: Silent failure when no token is present.

The middleware silently continues when no token is provided, which is appropriate for a graceful degradation strategy, but could lead to unexpected access denials without clear feedback to users.

Consider adding a debug log with more specific details about why access is being denied:

if (!authHeader || !authHeader.startsWith("Bearer ")) {
-  logger.debug("No authorization token provided");
+  logger.debug(`No valid authorization token provided for request to ${req.originalUrl}`);
  return next();
}
src/auth-service/models/User.js (3)

1190-1210: Deduplicate managedGroups to keep the JWT lean

A user can hold several qualifying roles in the same group, leading to duplicate titles in managedGroups.
Consider returning Array.from(new Set(managedGroups)) before signing the token; it trims the payload and prevents downstream duplicate checks.

🧰 Tools
🪛 Biome (1.9.4)

[error] 1197-1202: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 1203-1204: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


1196-1200: Minor readability – optional chaining removes triple guards

The triple && guards clutter the condition. Optional chaining keeps intent clear:

-          groupRole.role &&
-          groupRole.role.role_permissions &&
-          groupRole.role.role_permissions.some(
+          groupRole.role?.role_permissions?.some(
               (p) => …
 )

1248-1271: Consider specifying the JWT algorithm & shrinking the payload

  1. jwt.sign defaults to HS256, but an explicit algorithm strengthens clarity and avoids surprises when library defaults change.
-  constants.JWT_SECRET,
-  { expiresIn: "2h" }
+  constants.JWT_SECRET,
+  { expiresIn: "2h", algorithm: "HS256" }
  1. The payload now carries ~20 user fields + cohortAccess. Very large tokens bloat every request header.
    Evaluate trimming rarely-needed fields (profilePicture, phoneNumber, etc.) or nest them under a short key.
src/device-registry/docs/COHORT_ACCESS_CONTROL.md (1)

9-12: Tiny wording nit – insert “the” for smoother reading

Device/Cohort/Event data is in Device Registry Service database
Device/Cohort/Event data is in **the** Device Registry Service database.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~11-~11: You might be missing the article “the” here.
Context: ...tabase - Device/Cohort/Event data is in Device Registry Service database - No direct d...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 09b24ea and fb30e98.

📒 Files selected for processing (6)
  • src/auth-service/models/User.js (1 hunks)
  • src/device-registry/controllers/event.controller.js (1 hunks)
  • src/device-registry/docs/COHORT_ACCESS_CONTROL.md (1 hunks)
  • src/device-registry/middleware/extractCohortAccess.js (1 hunks)
  • src/device-registry/models/Cohort.js (1 hunks)
  • src/device-registry/routes/v2/events.routes.js (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/device-registry/controllers/event.controller.js (2)
src/device-registry/utils/event.util.js (6)
  • req (512-512)
  • req (779-779)
  • req (2306-2334)
  • req (3467-3467)
  • EventModel (1-1)
  • logger (25-25)
src/device-registry/middleware/extractCohortAccess.js (1)
  • logger (4-6)
🪛 Biome (1.9.4)
src/device-registry/middleware/extractCohortAccess.js

[error] 35-35: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

src/auth-service/models/User.js

[error] 1197-1202: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 1203-1204: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

src/device-registry/controllers/event.controller.js

[error] 3160-3161: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🪛 LanguageTool
src/device-registry/docs/COHORT_ACCESS_CONTROL.md

[uncategorized] ~11-~11: You might be missing the article “the” here.
Context: ...tabase - Device/Cohort/Event data is in Device Registry Service database - No direct d...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)


[uncategorized] ~302-~302: You might be missing the article “a” here.
Context: ... ); } next(); }); ``` - [ ] Add pre-save hook to normalize group identifier...

(AI_EN_LECTOR_MISSING_DETERMINER_A)


[style] ~353-~353: ‘emergency situations’ might be wordy. Consider a shorter alternative.
Context: ...in ability to bypass access control for emergency situations ## Communication Plan - [ ] Document ...

(EN_WORDINESS_PREMIUM_EMERGENCY_SITUATIONS)

🔇 Additional comments (6)
src/device-registry/models/Cohort.js (1)

22-30: Well-implemented group normalization.

The setter function for groups properly ensures consistent formatting by converting group identifiers to lowercase. This aligns well with the JWT token normalization in the middleware, ensuring consistent matching between cohort groups and user permissions.

src/device-registry/routes/v2/events.routes.js (3)

7-7: Good integration of the cohort access middleware.

The import of the new middleware is clean and follows the module pattern used throughout the codebase.


10-10: Appropriate middleware placement in request chain.

Adding the middleware after headers validation but before route handlers ensures all downstream handlers have access to normalized cohort permissions.


60-62: Missing validation for new endpoint and purpose of commented routes.

The new endpoint lacks validation middleware unlike other routes. Also, the purpose of commenting out the existing routes rather than updating them should be documented.

Consider adding validation similar to other routes:

-router.get("/get-events", eventController.getEvents);
+router.get("/get-events", eventValidations.listEvents, pagination(), eventController.getEvents);

Also, please clarify if commented routes are part of a phased implementation or if they're being replaced by the new endpoint.

src/device-registry/middleware/extractCohortAccess.js (2)

12-63: Well-structured JWT token extraction middleware.

The middleware properly implements the extraction of cohort access claims from JWT tokens, sets appropriate defaults, normalizes group identifiers, and handles errors gracefully.

Minor improvement suggestion for more robust code:

- if (decoded && decoded.cohortAccess) {
+ if (decoded?.cohortAccess) {
🧰 Tools
🪛 Biome (1.9.4)

[error] 35-35: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


32-32:

✅ Verification successful

Security consideration: JWT verification secrets.

Make sure constants.JWT_SECRET is properly configured in all environments. If this is a shared secret across services, consider implementing a secret rotation strategy.

Run this shell script to verify the secret is consistently configured:


🏁 Script executed:

#!/bin/bash
# Check JWT_SECRET configuration across services
echo "Checking JWT secret configuration..."

# Check if JWT_SECRET is defined in device-registry
grep -r "JWT_SECRET" --include="*.js" src/device-registry/config/

# Check for the same secret in auth-service
grep -r "JWT_SECRET" --include="*.js" src/auth-service/config/

echo "These should match to ensure proper token verification between services"

Length of output: 521


Security: JWT_SECRET configuration verified

I’ve confirmed that both services load the JWT_SECRET from the environment:

  • src/device-registry/config/global/envs.js: JWT_SECRET: process.env.JWT_SECRET
  • src/auth-service/config/global/envs.js: JWT_SECRET: process.env.JWT_SECRET

This ensures consistent token verification across services. No code changes are required here, but for long-term security you may want to:

  • Use a centralized secrets manager (e.g. AWS Secrets Manager, HashiCorp Vault)
  • Implement automated secret rotation (or leverage JWKs with short-lived keys)

getEvents: async (req, res) => {
try {
const filter = buildFilterFromRequest(req);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Undefined buildFilterFromRequest function.

The buildFilterFromRequest function is called but not imported or defined in the file.

Either import this utility function or define it in this file:

const createEventUtil = require("@utils/event.util");
+const { buildFilterFromRequest } = require("@utils/filter.util"); // Adjust path as needed

Please verify the correct import path for this utility function.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/device-registry/controllers/event.controller.js at line 3148, the
function buildFilterFromRequest is used but not imported or defined. To fix
this, locate the correct module where buildFilterFromRequest is exported from,
then add an import statement at the top of this file to import
buildFilterFromRequest from that module. If the function is not available
externally, define it within this file before its usage.

Comment on lines +3145 to +3167
getEvents: async (req, res) => {
try {
const filter = buildFilterFromRequest(req);

// Use cohort access from middleware instead of making API calls
const result = await EventModel(tenant).fetch(filter, req.cohortAccess);

// Check if any private cohorts were accessed for logging/monitoring
if (req.cohortAccess && req.cohortAccess.canAccessPrivateCohorts) {
logger.info(
`User ${req.userId} accessed events with private cohort access`
);
}

return res.status(200).json(result);
} catch (error) {
logger.error(`Error getting events: ${error.message}`);
return res.status(500).json({
success: false,
message: error.message,
});
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Missing tenant initialization in getEvents method.

The method uses the tenant variable that isn't initialized, which will cause runtime errors.

  getEvents: async (req, res) => {
    try {
+     const defaultTenant = constants.DEFAULT_TENANT || "airqo";
+     const tenant = isEmpty(req.query.tenant) ? defaultTenant : req.query.tenant;
      const filter = buildFilterFromRequest(req);

      // Use cohort access from middleware instead of making API calls
      const result = await EventModel(tenant).fetch(filter, req.cohortAccess);

      // Check if any private cohorts were accessed for logging/monitoring
-     if (req.cohortAccess && req.cohortAccess.canAccessPrivateCohorts) {
+     if (req.cohortAccess?.canAccessPrivateCohorts) {
        logger.info(
          `User ${req.userId} accessed events with private cohort access`
        );
      }

      return res.status(200).json(result);
    } catch (error) {
      logger.error(`Error getting events: ${error.message}`);
      return res.status(500).json({
        success: false,
        message: error.message,
      });
    }
  },

This fix addresses both the missing tenant initialization and incorporates the static analysis suggestion to use optional chaining.

📝 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.

Suggested change
getEvents: async (req, res) => {
try {
const filter = buildFilterFromRequest(req);
// Use cohort access from middleware instead of making API calls
const result = await EventModel(tenant).fetch(filter, req.cohortAccess);
// Check if any private cohorts were accessed for logging/monitoring
if (req.cohortAccess && req.cohortAccess.canAccessPrivateCohorts) {
logger.info(
`User ${req.userId} accessed events with private cohort access`
);
}
return res.status(200).json(result);
} catch (error) {
logger.error(`Error getting events: ${error.message}`);
return res.status(500).json({
success: false,
message: error.message,
});
}
},
getEvents: async (req, res) => {
try {
const defaultTenant = constants.DEFAULT_TENANT || "airqo";
const tenant = isEmpty(req.query.tenant) ? defaultTenant : req.query.tenant;
const filter = buildFilterFromRequest(req);
// Use cohort access from middleware instead of making API calls
const result = await EventModel(tenant).fetch(filter, req.cohortAccess);
// Check if any private cohorts were accessed for logging/monitoring
if (req.cohortAccess?.canAccessPrivateCohorts) {
logger.info(
`User ${req.userId} accessed events with private cohort access`
);
}
return res.status(200).json(result);
} catch (error) {
logger.error(`Error getting events: ${error.message}`);
return res.status(500).json({
success: false,
message: error.message,
});
}
},
🧰 Tools
🪛 Biome (1.9.4)

[error] 3160-3161: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🤖 Prompt for AI Agents
In src/device-registry/controllers/event.controller.js around lines 3145 to
3167, the getEvents method uses the tenant variable without initializing it,
causing runtime errors. Initialize the tenant variable at the start of the
method, typically by extracting it from the request object or context. Also,
apply optional chaining where appropriate to safely access nested properties
like req.cohortAccess to prevent potential errors if those properties are
undefined.

Comment on lines +1228 to +1235
// Get base user data
const userWithDerivedAttributes = await UserModel("airqo").list({
filter: { _id: this._id },
});

if (!userWithDerivedAttributes.success) {
logger.error(
`Internal Server Error -- ${JSON.stringify(userWithDerivedAttributes)}`
`Failed to get user attributes for token: ${JSON.stringify(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Missing defensive check when list() returns zero items

userWithDerivedAttributes.data[0] is accessed without confirming that data.length > 0.
If the aggregation for some reason yields no document, a TypeError will crash token generation.

-if (!userWithDerivedAttributes.success) {
+if (
+  !userWithDerivedAttributes.success ||
+  !Array.isArray(userWithDerivedAttributes.data) ||
+  userWithDerivedAttributes.data.length === 0
+) {
   logger.error(
     `Failed to get user attributes for token: ${JSON.stringify(
       userWithDerivedAttributes
     )}`
   );
   return userWithDerivedAttributes;
 }
📝 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.

Suggested change
// Get base user data
const userWithDerivedAttributes = await UserModel("airqo").list({
filter: { _id: this._id },
});
if (!userWithDerivedAttributes.success) {
logger.error(
`Internal Server Error -- ${JSON.stringify(userWithDerivedAttributes)}`
`Failed to get user attributes for token: ${JSON.stringify(
// Get base user data
const userWithDerivedAttributes = await UserModel("airqo").list({
filter: { _id: this._id },
});
if (
!userWithDerivedAttributes.success ||
!Array.isArray(userWithDerivedAttributes.data) ||
userWithDerivedAttributes.data.length === 0
) {
logger.error(
`Failed to get user attributes for token: ${JSON.stringify(
userWithDerivedAttributes
)}`
);
return userWithDerivedAttributes;
}
🤖 Prompt for AI Agents
In src/auth-service/models/User.js around lines 1228 to 1235, add a defensive
check to verify that userWithDerivedAttributes.data has at least one item before
accessing userWithDerivedAttributes.data[0]. If data is empty, handle this case
gracefully by logging an error or returning early to prevent a TypeError during
token generation.

Comment on lines +1188 to +1204

if (user.group_roles && Array.isArray(user.group_roles)) {
for (const groupRole of user.group_roles) {
const isAdmin =
groupRole.userType === "user" ||
(groupRole.role && groupRole.role.role_name === "admin");

const hasPermission =
groupRole.role &&
groupRole.role.role_permissions &&
groupRole.role.role_permissions.some(
(p) => p === "view:private_cohorts" || p === "manage:cohorts"
);

if (isAdmin || hasPermission) {
if (groupRole.group && groupRole.group.grp_title) {
// Using group title as the identifier since it's what cohorts use
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Permission check is very likely broken & may grant unintended access

groupRole.role.role_permissions is an array of ObjectIds.
Comparing those IDs to string literals "view:private_cohorts" / "manage:cohorts" will always return false, unless the permission documents’ IDs coincidentally equal those strings.

This means:

  1. Owners relying on the permission mechanism will never be recognised.
  2. Because groupRole.userType === "user" is treated as admin, every registered user of a group gains private-cohort access – an escalation risk.

Recommended fix:

-const hasPermission =
-  groupRole.role &&
-  groupRole.role.role_permissions &&
-  groupRole.role.role_permissions.some(
-    (p) => p === "view:private_cohorts" || p === "manage:cohorts"
-  );
+// populate role_permissions with the actual permission docs first
+const permissionNames =
+  groupRole.role?.role_permissions?.map((perm) =>
+    perm.permission ? perm.permission : perm // handle populated & raw
+  ) ?? [];
+const hasPermission = permissionNames.some((p) =>
+  ["view:private_cohorts", "manage:cohorts"].includes(p)
+);

At a minimum, drop the userType === "user" shortcut and rely on explicit permission checks.

📝 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.

Suggested change
if (user.group_roles && Array.isArray(user.group_roles)) {
for (const groupRole of user.group_roles) {
const isAdmin =
groupRole.userType === "user" ||
(groupRole.role && groupRole.role.role_name === "admin");
const hasPermission =
groupRole.role &&
groupRole.role.role_permissions &&
groupRole.role.role_permissions.some(
(p) => p === "view:private_cohorts" || p === "manage:cohorts"
);
if (isAdmin || hasPermission) {
if (groupRole.group && groupRole.group.grp_title) {
// Using group title as the identifier since it's what cohorts use
if (user.group_roles && Array.isArray(user.group_roles)) {
for (const groupRole of user.group_roles) {
const isAdmin =
groupRole.userType === "user" ||
(groupRole.role && groupRole.role.role_name === "admin");
// populate role_permissions with the actual permission docs first
const permissionNames =
groupRole.role?.role_permissions?.map((perm) =>
perm.permission ? perm.permission : perm // handle populated & raw
) ?? [];
const hasPermission = permissionNames.some((p) =>
["view:private_cohorts", "manage:cohorts"].includes(p)
);
if (isAdmin || hasPermission) {
if (groupRole.group && groupRole.group.grp_title) {
// Using group title as the identifier since it's what cohorts use
🧰 Tools
🪛 Biome (1.9.4)

[error] 1197-1202: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 1203-1204: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

🤖 Prompt for AI Agents
In src/auth-service/models/User.js around lines 1188 to 1204, the permission
check incorrectly compares ObjectId values in groupRole.role.role_permissions to
string literals, causing the check to always fail and unintentionally granting
access to all users with userType "user". To fix this, remove the isAdmin
condition that treats userType "user" as admin, and instead implement explicit
permission checks by properly resolving or comparing the ObjectId permissions to
the intended permission identifiers, ensuring only users with the correct
permissions gain access.

Comment on lines +1169 to +1178
const user = await UserModel("airqo")
.findById(this._id)
.populate({
path: "group_roles.role",
select: "role_name role_permissions",
})
.populate({
path: "group_roles.group",
select: "grp_title _id",
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid hard-coding the tenant – make the helper reusable

UserModel("airqo") bakes the default tenant into every token-generation path. If the project on-boards a second tenant, users from that tenant will silently receive incorrect (or empty) cohort claims.

-const user = await UserModel("airqo")
+const tenant = this.tenant || constants.DEFAULT_TENANT || "airqo"
+const user = await UserModel(tenant)

You already follow this pattern elsewhere in the file (TenantSettingsModel(tenant)). Re-using it here keeps the logic multi-tenant-safe.

📝 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.

Suggested change
const user = await UserModel("airqo")
.findById(this._id)
.populate({
path: "group_roles.role",
select: "role_name role_permissions",
})
.populate({
path: "group_roles.group",
select: "grp_title _id",
})
const tenant = this.tenant || constants.DEFAULT_TENANT || "airqo"
const user = await UserModel(tenant)
.findById(this._id)
.populate({
path: "group_roles.role",
select: "role_name role_permissions",
})
.populate({
path: "group_roles.group",
select: "grp_title _id",
})
🤖 Prompt for AI Agents
In src/auth-service/models/User.js around lines 1169 to 1178, the code
hard-codes the tenant string "airqo" in the UserModel call, which breaks
multi-tenant support. To fix this, replace the hard-coded "airqo" with a dynamic
tenant variable passed into the function or accessible in the current context,
similar to how TenantSettingsModel(tenant) is used elsewhere. This change will
make the helper reusable and ensure correct tenant-specific data is fetched.

Copy link
Contributor

Auth-service changes in this PR available for preview here

Copy link
Contributor

Device registry changes in this PR available for preview here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant