initial support for organizations #36
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds graph-backed organization management (backend services, email service), integrates org creation into user onboarding, exposes REST endpoints and UI (modal, JS, CSS, templates), replaces the auth blueprint with a main blueprint, and updates Python package dependencies. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant FlaskApp
participant OrgService
participant GraphDB
User->>Browser: Click "Organization" button
Browser->>FlaskApp: GET /api/organization/status (AJAX w/ token)
FlaskApp->>OrgService: validate token & request status
OrgService->>GraphDB: query org and user nodes by email/domain
GraphDB-->>OrgService: org/user data
OrgService-->>FlaskApp: JSON status
FlaskApp-->>Browser: JSON response
Browser->>User: Render modal (shows admin UI if role == admin)
alt Admin action (add/approve/update)
User->>Browser: Submit add/approve/update
Browser->>FlaskApp: POST /api/organization/<action>
FlaskApp->>OrgService: authorize & perform change
OrgService->>GraphDB: create/update relationships/nodes
GraphDB-->>OrgService: ack/result
OrgService->>FlaskApp: result JSON
FlaskApp-->>Browser: success/error
Browser->>User: Update UI
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Dependency ReviewThe following issues were found:
License IssuesPipfile
Pipfile.lock
OpenSSF ScorecardScorecard details
Scanned Files
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| except Exception as e: | ||
| logging.error("Error adding user %s to organization %s by admin %s: %s", | ||
| target_email, organization_domain, admin_email, e) |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the log injection vulnerability, all user-controlled input that is logged should be sanitized to remove newline (\n) and carriage return (\r) characters before being passed to the logger. The best way to do this is to sanitize the target_email, organization_domain, and admin_email variables before they are used in the logging statement on line 363. This can be done by replacing any \r and \n characters with an empty string. The fix should be applied directly in the except block before logging, or by creating a small helper function for sanitization if this pattern is used in multiple places.
Only the relevant lines in api/auth/organization_management.py need to be changed. A helper function can be added at the top of the file (after imports) for clarity and reusability.
| @@ -3,6 +3,12 @@ | ||
| import logging | ||
| from typing import Tuple, Optional, Dict, Any | ||
|
|
||
|
|
||
| def _sanitize_log_input(value: str) -> str: | ||
| """Sanitize input for logging to prevent log injection.""" | ||
| if not isinstance(value, str): | ||
| return value | ||
| return value.replace('\r', '').replace('\n', '') | ||
| from api.extensions import db | ||
|
|
||
|
|
||
| @@ -360,7 +366,7 @@ | ||
|
|
||
| except Exception as e: | ||
| logging.error("Error adding user %s to organization %s by admin %s: %s", | ||
| target_email, organization_domain, admin_email, e) | ||
| _sanitize_log_input(target_email), _sanitize_log_input(organization_domain), _sanitize_log_input(admin_email), e) | ||
| return False, f"Error adding user: {str(e)}" | ||
|
|
||
|
|
|
|
||
| except Exception as e: | ||
| logging.error("Error approving user %s in organization %s by admin %s: %s", | ||
| target_email, organization_domain, admin_email, e) |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the log injection vulnerability, we should sanitize the user-provided target_email before logging it. Specifically, we should remove any newline (\n) and carriage return (\r) characters from the email string before it is passed to the logger. This can be done by using the replace() method on the string. The best place to do this is immediately before logging, so that all log entries involving user input are sanitized. We should apply this fix to the logging statement on line 448 in api/auth/organization_management.py. No new methods or imports are required, as string replacement is a built-in operation.
| @@ -444,8 +444,10 @@ | ||
| return False, "User not found or not pending approval" | ||
|
|
||
| except Exception as e: | ||
| # Sanitize target_email to prevent log injection | ||
| safe_target_email = target_email.replace('\r', '').replace('\n', '') | ||
| logging.error("Error approving user %s in organization %s by admin %s: %s", | ||
| target_email, organization_domain, admin_email, e) | ||
| safe_target_email, organization_domain, admin_email, e) | ||
| return False, f"Error approving user: {str(e)}" | ||
|
|
||
|
|
|
|
||
| if result.result_set: | ||
| logging.info("Admin %s updated role of user %s to %s in organization %s", | ||
| admin_email, target_email, new_role, organization_domain) |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the log injection vulnerability, we should sanitize the target_email value before logging it. Specifically, we should remove any newline (\n) and carriage return (\r) characters from the email string before it is included in the log entry. This can be done by replacing these characters with an empty string using the replace() method. The best place to do this is immediately before logging, so that the log entry is guaranteed to be safe regardless of upstream validation. Only the log entry needs to be sanitized; the actual value used in business logic should remain unchanged to avoid breaking functionality.
Required changes:
- In
api/auth/organization_management.py, line 547, sanitizetarget_emailbefore logging by removing\nand\rcharacters. - No new imports are needed, as string replacement is built-in.
| @@ -543,8 +543,10 @@ | ||
| }) | ||
|
|
||
| if result.result_set: | ||
| # Sanitize target_email before logging to prevent log injection | ||
| safe_target_email = target_email.replace('\r', '').replace('\n', '') | ||
| logging.info("Admin %s updated role of user %s to %s in organization %s", | ||
| admin_email, target_email, new_role, organization_domain) | ||
| admin_email, safe_target_email, new_role, organization_domain) | ||
| return True, f"User {target_email} role updated to {new_role}" | ||
| else: | ||
| return False, "User not found or not a member of the organization" |
| if success: | ||
| return jsonify({"message": message}), 200 | ||
| else: | ||
| return jsonify({"error": message}), 400 |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, we need to ensure that exception details from the backend (add_user_to_organization_by_email) are not exposed to the user via the API response. Instead, only generic error messages should be returned. The best way to do this is to update the route handler in api/routes/organization.py so that, when success is False, it checks if the error message is a generic, expected error (e.g., "Unauthorized: ...", "Email domain ... does not match ...", "Failed to add user to organization"). If the error message starts with "Error adding user:" (which is the prefix used in the backend for exception-based errors), the route should return a generic error message like "Internal server error" instead of the actual message. This way, only non-sensitive, expected error messages are returned to the user, and unexpected backend errors are hidden.
The change should be made in the add_user_to_organization function in api/routes/organization.py, specifically in the block:
if success:
return jsonify({"message": message}), 200
else:
return jsonify({"error": message}), 400We should add a check for the error message prefix and replace it with a generic message if needed.
No new imports or dependencies are required.
| @@ -195,3 +195,8 @@ | ||
| else: | ||
| return jsonify({"error": message}), 400 | ||
| # Hide backend exception details from user | ||
| if message.startswith("Error adding user:"): | ||
| logging.error("Backend error in add_user_to_organization_by_email: %s", message) | ||
| return jsonify({"error": "Internal server error"}), 500 | ||
| else: | ||
| return jsonify({"error": message}), 400 | ||
|
|
| ) | ||
|
|
||
| if success: | ||
| return jsonify({"message": message}), 200 |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, we need to ensure that exception details from approve_pending_user (and similar functions) are not returned to the client. Instead, only generic error messages should be sent, while the detailed exception information is logged server-side. The fix should be applied in the API endpoint in api/routes/organization.py, specifically in the approve_user_in_organization function. When success is False, we should check if the message starts with "Error approving user:" (or similar), and replace it with a generic error message before returning it to the client. The same pattern should be applied to other endpoints that return error messages from backend functions that may include exception details.
Steps:
- In
approve_user_in_organization, before returning the error response, check if the error message is an internal error (e.g., starts with"Error approving user:"). - If so, log the detailed message and return a generic error message to the client.
- Optionally, apply the same logic to similar endpoints (e.g.,
update_user_role_endpoint) if they have the same pattern.
No new imports are needed, as logging is already imported.
| @@ -239,2 +239,6 @@ | ||
| else: | ||
| # Sanitize internal error messages before returning to client | ||
| if message.startswith("Error approving user:"): | ||
| logging.error("Internal error in approve_user_in_organization: %s", message) | ||
| return jsonify({"error": "Internal server error"}), 500 | ||
| return jsonify({"error": message}), 400 |
| if success: | ||
| return jsonify({"message": message}), 200 | ||
| else: | ||
| return jsonify({"error": message}), 400 |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, we should ensure that any error messages returned to the client do not include exception details or stack traces. Instead, return a generic error message such as "Internal server error" or "Failed to approve user." The detailed error (including the exception message) should be logged on the server for debugging purposes. Specifically, in api/routes/organization.py, when approve_pending_user returns success == False, we should return a generic error message to the client, and log the detailed message. This change should be made in the approve_user_in_organization function, replacing the line that returns {"error": message} with a generic message and a server-side log of the actual error.
| @@ -239,3 +239,4 @@ | ||
| else: | ||
| return jsonify({"error": message}), 400 | ||
| logging.error("Failed to approve user: %s", message) | ||
| return jsonify({"error": "Failed to approve user"}), 400 | ||
|
|
| ) | ||
|
|
||
| if success: | ||
| return jsonify({"message": message}), 200 |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, we should ensure that any error message returned to the client does not include details from exception objects. In the update_user_role_endpoint function in api/routes/organization.py, when success is False, the "error" field should contain a generic message such as "Failed to update user role" rather than the potentially sensitive message returned from update_user_role. The detailed error (including the exception message) should be logged server-side for debugging purposes.
Specifically:
- In
update_user_role_endpoint, after callingupdate_user_role, ifsuccessisFalse, check if the error message starts with"Error updating user role:"(which indicates it came from an exception). If so, return a generic error message to the client and log the detailed message. - For other error messages (such as validation errors), it is safe to return them to the client, as they do not contain sensitive information.
No changes are needed in api/auth/organization_management.py since logging is already performed there.
| @@ -284,7 +284,12 @@ | ||
| if success: | ||
| return jsonify({"message": message}), 200 | ||
| else: | ||
| return jsonify({"error": message}), 400 | ||
| # If the error message is from an exception, log it and return a generic error | ||
| if message and message.startswith("Error updating user role:"): | ||
| logging.error("Detailed error updating user role: %s", message) | ||
| return jsonify({"error": "Failed to update user role"}), 400 | ||
| else: | ||
| return jsonify({"error": message}), 400 | ||
|
|
||
| except Exception as e: | ||
| logging.error("Error updating user role: %s", e) |
| if success: | ||
| return jsonify({"message": message}), 200 | ||
| else: | ||
| return jsonify({"error": message}), 400 |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the problem, we should ensure that exception messages (or any internal error details) are never sent directly to the client. Instead, we should return a generic error message to the user, while logging the detailed error on the server for debugging purposes. Specifically, in api/routes/organization.py, in the update_user_role_endpoint function, we should replace the error response in the else block (line 287) with a generic message such as "Internal server error", and ensure the actual error message is logged. This change should be made only for cases where the error message may originate from an exception (i.e., when success is False and the message may be tainted). For controlled error messages (e.g., validation errors), it's safe to return them, but since the function can return both types, the safest fix is to always return a generic error for failures.
No new imports are needed, as logging is already imported.
| @@ -284,7 +284,8 @@ | ||
| if success: | ||
| return jsonify({"message": message}), 200 | ||
| else: | ||
| return jsonify({"error": message}), 400 | ||
| logging.error("User role update failed: %s", message) | ||
| return jsonify({"error": "Internal server error"}), 500 | ||
|
|
||
| except Exception as e: | ||
| logging.error("Error updating user role: %s", e) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Nitpick comments (2)
api/auth/organization_management.py (2)
321-321: Make default role configurable.The function hard-codes the default role as "user" when approving pending users. This should be configurable or passed as a parameter.
Consider making the default role configurable:
-# Also ensure the user has the correct default role -update_user_role_direct(target_email, "user") +# Also ensure the user has the correct default role +default_role = os.environ.get('DEFAULT_USER_ROLE', 'user') +update_user_role_direct(target_email, default_role)
405-407: Make allowed roles configurable.The allowed roles are hard-coded. Consider making them configurable to support different organization needs.
Extract role configuration:
# At module level DEFAULT_ALLOWED_ROLES = ['user', 'admin', 'analyst', 'viewer', 'manager'] def get_allowed_roles(): """Get allowed roles from configuration or use defaults.""" # Could be extended to read from config/database return DEFAULT_ALLOWED_ROLESThen use in validation:
-allowed_roles = ['user', 'admin', 'analyst', 'viewer', 'manager'] +allowed_roles = get_allowed_roles()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Pipfile(1 hunks)api/app_factory.py(3 hunks)api/auth/organization_management.py(1 hunks)api/auth/user_management.py(4 hunks)api/routes/__init__.py(1 hunks)api/routes/auth.py(0 hunks)api/routes/main.py(1 hunks)api/routes/organization.py(1 hunks)api/static/css/modals.css(1 hunks)api/templates/chat.j2(1 hunks)api/templates/components/organization_modal.j2(1 hunks)api/templates/components/user_profile.j2(1 hunks)
💤 Files with no reviewable changes (1)
- api/routes/auth.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/auth/user_management.py (1)
api/auth/organization_management.py (4)
check_or_create_organization(16-73)link_user_to_organization(76-120)extract_email_domain(9-13)update_user_role_direct(471-514)
🪛 GitHub Actions: Pylint
api/routes/__init__.py
[warning] 1-1: Pylint C0114: Missing module docstring.
api/auth/user_management.py
[warning] 46-47: Pylint C0303: Trailing whitespace.
[warning] 118-125: Pylint C0303: Trailing whitespace.
[warning] 15-15: Pylint R0914: Too many local variables (24/15).
[warning] 45-45: Pylint C0415: Import outside toplevel.
[warning] 116-116: Pylint C0415: Import outside toplevel.
[warning] 129-129: Pylint R1705: Unnecessary "elif" after "return", remove the leading "el" from "elif".
[warning] 15-15: Pylint R0911: Too many return statements (8/6).
[warning] 15-15: Pylint R0912: Too many branches (13/12).
[warning] 183-183: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 284-284: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 194-194: Pylint R1702: Too many nested blocks (7/5).
[warning] 188-188: Pylint R0911: Too many return statements (8/6).
[warning] 188-188: Pylint R0912: Too many branches (15/12).
[warning] 307-307: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
api/routes/organization.py
[warning] 181-181: Pylint C0301: Line too long (113/100).
[warning] 57-300: Pylint W0718: Catching too general exception Exception (broad-exception-caught) at multiple lines.
[warning] 33-280: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it at multiple lines.
[warning] 34-249: Pylint C0415: Import outside toplevel and W0404: Reimport detected.
[warning] 150-201: Pylint R0911: Too many return statements (7/6 and 8/6).
[warning] 248-249: Pylint W0621: Redefining name from outer scope.
api/auth/organization_management.py
[warning] 76-76: Pylint C0301: Line too long (131/100).
[warning] 111-111: Pylint C0303: Trailing whitespace.
[warning] 115-115: Pylint C0301: Line too long (103/100).
[warning] 119-119: Pylint C0301: Line too long (105/100).
[warning] 145-145: Pylint C0303: Trailing whitespace.
[warning] 164-164: Pylint C0303: Trailing whitespace.
[warning] 194-194: Pylint C0303: Trailing whitespace.
[warning] 219-219: Pylint C0301: Line too long (121/100).
[warning] 244-244: Pylint C0301: Line too long (114/100).
[warning] 268-268: Pylint C0303: Trailing whitespace.
[warning] 270-270: Pylint C0301: Line too long (117/100).
[warning] 275-275: Pylint C0303: Trailing whitespace.
[warning] 280-280: Pylint C0301: Line too long (108/100).
[warning] 322-322: Pylint C0303: Trailing whitespace.
[warning] 329-329: Pylint C0303: Trailing whitespace.
[warning] 358-358: Pylint C0303: Trailing whitespace.
[warning] 382-382: Pylint C0301: Line too long (119/100).
[warning] 429-429: Pylint C0303: Trailing whitespace.
[warning] 436-436: Pylint C0303: Trailing whitespace.
[warning] 71-71: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 37-37: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 63-63: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 118-118: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 110-110: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 165-165: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 140-140: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 214-214: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 274-274: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 267-267: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 328-328: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 319-319: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 377-377: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 435-435: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 428-428: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 466-466: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 461-461: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 512-512: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 505-505: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
🪛 GitHub Check: CodeQL
api/routes/organization.py
[warning] 190-190: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 192-192: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 234-234: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 236-236: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 281-281: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 283-283: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
api/auth/organization_management.py
[failure] 269-269: Log Injection
This log entry depends on a user-provided value.
[failure] 276-276: Log Injection
This log entry depends on a user-provided value.
[failure] 323-323: Log Injection
This log entry depends on a user-provided value.
[failure] 330-330: Log Injection
This log entry depends on a user-provided value.
[failure] 430-430: Log Injection
This log entry depends on a user-provided value.
[failure] 430-430: Log Injection
This log entry depends on a user-provided value.
[failure] 437-437: Log Injection
This log entry depends on a user-provided value.
[failure] 467-467: Log Injection
This log entry depends on a user-provided value.
[failure] 487-487: Log Injection
This log entry depends on a user-provided value.
[failure] 506-506: Log Injection
This log entry depends on a user-provided value.
[failure] 509-509: Log Injection
This log entry depends on a user-provided value.
[failure] 513-513: Log Injection
This log entry depends on a user-provided value.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (9)
Pipfile (1)
15-15: Pipfile: disposable-email-domains v0.0.129 is up-to-date and secureThe added
disposable-email-domains = "~=0.0.129"matches the latest release (July 28, 2025) and has no reported vulnerabilities. No further changes needed.• File: Pipfile, Line 15
disposable-email-domains = "~=0.0.129"api/templates/components/user_profile.j2 (1)
12-12: LGTM! Well-integrated organization access point.The organization button is properly placed in the user profile dropdown with consistent styling and appropriate onclick handler. This provides a logical entry point for organization management features.
api/templates/chat.j2 (1)
35-37: LGTM! Proper authentication-based conditional rendering.The organization modal is correctly included only for authenticated users, which is a good security practice. The conditional rendering follows proper Jinja2 syntax and integrates well with the existing modals block.
api/app_factory.py (3)
63-63: LGTM! Blueprint registration follows Flask best practices.The blueprint registration changes are correctly implemented, replacing
auth_bpwithmain_bpand addingorganization_bp. The registration pattern follows Flask conventions.Also applies to: 66-66
75-75: LGTM! Error handler updated consistently.The error handler redirect is properly updated to use
main.homeinstead ofauth.home, maintaining consistency with the blueprint refactoring.
16-16: Blueprint modules verified
- api/routes/main.py defines and exports
main_bp- api/routes/organization.py defines and exports
organization_bpNo further action required.
api/routes/__init__.py (1)
3-3: LGTM! Blueprint exports updated consistently.The import and export changes from
auth_bptomain_bpare consistent with the blueprint refactoring across the codebase. The organization blueprint is intentionally not exported here, maintaining separation of concerns.Also applies to: 7-7
api/static/css/modals.css (1)
179-193: LGTM! Clean and consistent modal styling.The refactoring from
.user-profile-actionsto.user-profile-actionwith comprehensive styling properties improves the UI consistency. Good use of CSS variables for theming.api/auth/organization_management.py (1)
248-259: Consider privacy implications of creating user records without consent.The function creates user records for email addresses that haven't logged into the system yet. This could have privacy implications.
Consider adding a configuration option to control this behavior or implementing an invitation system instead of directly creating user records.
| """Organization management functions for text2sql API.""" | ||
|
|
||
| import logging | ||
| from typing import Tuple, Optional, Dict, Any | ||
|
|
||
| from api.extensions import db | ||
|
|
||
|
|
||
| def extract_email_domain(email: str) -> str: | ||
| """Extract domain from email address.""" | ||
| if not email or "@" not in email: | ||
| return "" | ||
| return email.split("@")[-1].lower() | ||
|
|
||
|
|
||
| def check_or_create_organization(user_email: str) -> Tuple[bool, Optional[Dict[str, Any]]]: | ||
| """ | ||
| Check if organization exists for email domain, create if not. | ||
| Returns (is_new_organization, organization_info) | ||
| """ | ||
| domain = extract_email_domain(user_email) | ||
| if not domain: | ||
| logging.error("Invalid email domain for user: %s", user_email) | ||
| return False, None | ||
|
|
||
| try: | ||
| organizations_graph = db.select_graph("Organizations") | ||
|
|
||
| # Check if organization exists for this domain | ||
| check_query = """ | ||
| MATCH (org:Organization {domain: $domain}) | ||
| RETURN org | ||
| """ | ||
|
|
||
| result = organizations_graph.query(check_query, {"domain": domain}) | ||
|
|
||
| if result.result_set: | ||
| # Organization exists | ||
| organization = result.result_set[0][0] | ||
| logging.info("Found existing organization for domain: %s", domain) | ||
| return False, organization | ||
| else: | ||
| # Create new organization with first user as admin | ||
| create_query = """ | ||
| CREATE (org:Organization { | ||
| domain: $domain, | ||
| name: $organization_name, | ||
| created_at: timestamp(), | ||
| admin_email: $admin_email | ||
| }) | ||
| RETURN org | ||
| """ | ||
|
|
||
| # Generate organization name from domain (e.g., example.com -> Example) | ||
| organization_name = domain.split('.')[0].capitalize() | ||
|
|
||
| result = organizations_graph.query(create_query, { | ||
| "domain": domain, | ||
| "organization_name": organization_name, | ||
| "admin_email": user_email | ||
| }) | ||
|
|
||
| if result.result_set: | ||
| organization = result.result_set[0][0] | ||
| logging.info("Created new organization for domain: %s", domain) | ||
| return True, organization | ||
| else: | ||
| logging.error("Failed to create organization for domain: %s", domain) | ||
| return False, None | ||
|
|
||
| except Exception as e: | ||
| logging.error("Error managing organization for domain %s: %s", domain, e) | ||
| return False, None | ||
|
|
||
|
|
||
| def link_user_to_organization(user_email: str, organization_domain: str, is_admin: bool = False, is_pending: bool = False) -> bool: | ||
| """ | ||
| Link a user to an organization. | ||
| Args: | ||
| user_email: The user's email | ||
| organization_domain: The organization's domain | ||
| is_admin: Whether the user should be an admin | ||
| is_pending: Whether the user needs admin approval | ||
| Returns: | ||
| bool: Success status | ||
| """ | ||
| try: | ||
| organizations_graph = db.select_graph("Organizations") | ||
|
|
||
| # Create the relationship between user and organization | ||
| link_query = """ | ||
| MATCH (user:User {email: $user_email}) | ||
| MATCH (org:Organization {domain: $domain}) | ||
| MERGE (user)-[r:BELONGS_TO]->(org) | ||
| SET r.is_admin = $is_admin, | ||
| r.is_pending = $is_pending, | ||
| r.joined_at = timestamp() | ||
| RETURN user, org, r | ||
| """ | ||
|
|
||
| result = organizations_graph.query(link_query, { | ||
| "user_email": user_email, | ||
| "domain": organization_domain, | ||
| "is_admin": is_admin, | ||
| "is_pending": is_pending | ||
| }) | ||
|
|
||
| if result.result_set: | ||
| logging.info("Linked user %s to organization %s (admin: %s, pending: %s)", | ||
| user_email, organization_domain, is_admin, is_pending) | ||
| return True | ||
| else: | ||
| logging.error("Failed to link user %s to organization %s", user_email, organization_domain) | ||
| return False | ||
|
|
||
| except Exception as e: | ||
| logging.error("Error linking user %s to organization %s: %s", user_email, organization_domain, e) | ||
| return False | ||
|
|
||
|
|
||
| def get_user_organization_status(user_email: str) -> Optional[Dict[str, Any]]: | ||
| """ | ||
| Get user's organization status and details. | ||
| Returns: | ||
| Dict with organization info, user role, and pending status, or None if no organization | ||
| """ | ||
| try: | ||
| organizations_graph = db.select_graph("Organizations") | ||
|
|
||
| query = """ | ||
| MATCH (user:User {email: $user_email})-[r:BELONGS_TO]->(org:Organization) | ||
| RETURN org, r.is_admin as is_admin, r.is_pending as is_pending, r.joined_at as joined_at | ||
| """ | ||
|
|
||
| result = organizations_graph.query(query, {"user_email": user_email}) | ||
|
|
||
| if result.result_set: | ||
| org_data = result.result_set[0][0] | ||
| is_admin = result.result_set[0][1] | ||
| is_pending = result.result_set[0][2] | ||
| joined_at = result.result_set[0][3] | ||
|
|
||
| # Convert Node object to dictionary | ||
| org_dict = {} | ||
| if hasattr(org_data, 'properties'): | ||
| org_dict = org_data.properties | ||
| elif hasattr(org_data, '__dict__'): | ||
| org_dict = {k: v for k, v in org_data.__dict__.items() if not k.startswith('_')} | ||
| else: | ||
| # Fallback for different Node implementations | ||
| org_dict = dict(org_data) if org_data else {} | ||
|
|
||
| return { | ||
| "organization": org_dict, | ||
| "is_admin": is_admin, | ||
| "is_pending": is_pending, | ||
| "joined_at": joined_at | ||
| } | ||
| else: | ||
| return None | ||
|
|
||
| except Exception as e: | ||
| logging.error("Error getting organization status for user %s: %s", user_email, e) | ||
| return None | ||
|
|
||
|
|
||
| def get_organization_users(organization_domain: str) -> list: | ||
| """ | ||
| Get all users in an organization. | ||
| Returns: | ||
| List of user dictionaries with their roles and status | ||
| """ | ||
| try: | ||
| organizations_graph = db.select_graph("Organizations") | ||
|
|
||
| query = """ | ||
| MATCH (user:User)-[r:BELONGS_TO]->(org:Organization {domain: $domain}) | ||
| RETURN user, r.is_admin as is_admin, r.is_pending as is_pending, r.joined_at as joined_at | ||
| ORDER BY r.is_admin DESC, user.email ASC | ||
| """ | ||
|
|
||
| result = organizations_graph.query(query, {"domain": organization_domain}) | ||
|
|
||
| users = [] | ||
| for row in result.result_set: | ||
| user_data = row[0] | ||
| is_admin = row[1] | ||
| is_pending = row[2] | ||
| joined_at = row[3] | ||
|
|
||
| # Convert Node object to dictionary | ||
| user_dict = {} | ||
| if hasattr(user_data, 'properties'): | ||
| user_dict = user_data.properties | ||
| elif hasattr(user_data, '__dict__'): | ||
| user_dict = {k: v for k, v in user_data.__dict__.items() if not k.startswith('_')} | ||
| else: | ||
| # Fallback for different Node implementations | ||
| user_dict = dict(user_data) if user_data else {} | ||
|
|
||
| users.append({ | ||
| "user": user_dict, | ||
| "is_admin": is_admin, | ||
| "is_pending": is_pending, | ||
| "joined_at": joined_at | ||
| }) | ||
|
|
||
| return users | ||
|
|
||
| except Exception as e: | ||
| logging.error("Error getting users for organization %s: %s", organization_domain, e) | ||
| return [] | ||
|
|
||
|
|
||
| def add_user_to_organization_by_email(admin_email: str, target_email: str, organization_domain: str) -> Tuple[bool, str]: | ||
| """ | ||
| Add a user to organization by email (admin function). | ||
| Creates a pending user entry that will be activated when they log in. | ||
| Args: | ||
| admin_email: The admin user's email | ||
| target_email: The email of user to add | ||
| organization_domain: The organization domain | ||
| Returns: | ||
| Tuple[bool, str]: (success, message) | ||
| """ | ||
| try: | ||
| # Validate admin permissions | ||
| admin_status = get_user_organization_status(admin_email) | ||
| if not admin_status or not admin_status.get("is_admin"): | ||
| return False, "Unauthorized: Only organization admins can add users" | ||
|
|
||
| if admin_status["organization"]["domain"] != organization_domain: | ||
| return False, "Unauthorized: Admin can only add users to their own organization" | ||
|
|
||
| # Validate target email domain matches organization | ||
| target_domain = extract_email_domain(target_email) | ||
| if target_domain != organization_domain: | ||
| return False, f"Email domain {target_domain} does not match organization domain {organization_domain}" | ||
|
|
||
| organizations_graph = db.select_graph("Organizations") | ||
|
|
||
| # Create or update user with pending organization relationship | ||
| query = """ | ||
| MATCH (org:Organization {domain: $domain}) | ||
| MERGE (user:User {email: $target_email}) | ||
| ON CREATE SET user.created_at = timestamp(), user.role = 'user' | ||
| MERGE (user)-[r:BELONGS_TO]->(org) | ||
| SET r.is_admin = false, | ||
| r.is_pending = true, | ||
| r.invited_by = $admin_email, | ||
| r.invited_at = timestamp() | ||
| RETURN user, r | ||
| """ | ||
|
|
||
| result = organizations_graph.query(query, { | ||
| "domain": organization_domain, | ||
| "target_email": target_email, | ||
| "admin_email": admin_email | ||
| }) | ||
|
|
||
| if result.result_set: | ||
| logging.info("Admin %s added user %s to organization %s (pending)", | ||
| admin_email, target_email, organization_domain) | ||
| return True, f"User {target_email} has been added to organization and will be activated when they log in" | ||
| else: | ||
| return False, "Failed to add user to organization" | ||
|
|
||
| except Exception as e: | ||
| logging.error("Error adding user %s to organization %s by admin %s: %s", | ||
| target_email, organization_domain, admin_email, e) | ||
| return False, f"Error adding user: {str(e)}" | ||
|
|
||
|
|
||
| def approve_pending_user(admin_email: str, target_email: str, organization_domain: str) -> Tuple[bool, str]: | ||
| """ | ||
| Approve a pending user in the organization. | ||
| Args: | ||
| admin_email: The admin user's email | ||
| target_email: The email of user to approve | ||
| organization_domain: The organization domain | ||
| Returns: | ||
| Tuple[bool, str]: (success, message) | ||
| """ | ||
| try: | ||
| # Validate admin permissions | ||
| admin_status = get_user_organization_status(admin_email) | ||
| if not admin_status or not admin_status.get("is_admin"): | ||
| return False, "Unauthorized: Only organization admins can approve users" | ||
|
|
||
| if admin_status["organization"]["domain"] != organization_domain: | ||
| return False, "Unauthorized: Admin can only approve users in their own organization" | ||
|
|
||
| organizations_graph = db.select_graph("Organizations") | ||
|
|
||
| # Update user's pending status | ||
| query = """ | ||
| MATCH (user:User {email: $target_email})-[r:BELONGS_TO]->(org:Organization {domain: $domain}) | ||
| WHERE r.is_pending = true | ||
| SET r.is_pending = false, | ||
| r.approved_by = $admin_email, | ||
| r.approved_at = timestamp() | ||
| RETURN user, r | ||
| """ | ||
|
|
||
| result = organizations_graph.query(query, { | ||
| "target_email": target_email, | ||
| "domain": organization_domain, | ||
| "admin_email": admin_email | ||
| }) | ||
|
|
||
| if result.result_set: | ||
| # Also ensure the user has the correct default role | ||
| update_user_role_direct(target_email, "user") | ||
| logging.info("Admin %s approved user %s in organization %s", | ||
| admin_email, target_email, organization_domain) | ||
| return True, f"User {target_email} has been approved" | ||
| else: | ||
| return False, "User not found or not pending approval" | ||
|
|
||
| except Exception as e: | ||
| logging.error("Error approving user %s in organization %s by admin %s: %s", | ||
| target_email, organization_domain, admin_email, e) | ||
| return False, f"Error approving user: {str(e)}" | ||
|
|
||
|
|
||
| def get_pending_users(organization_domain: str) -> list: | ||
| """ | ||
| Get all pending users for an organization. | ||
| Returns: | ||
| List of pending user dictionaries | ||
| """ | ||
| try: | ||
| organizations_graph = db.select_graph("Organizations") | ||
|
|
||
| query = """ | ||
| MATCH (user:User)-[r:BELONGS_TO]->(org:Organization {domain: $domain}) | ||
| WHERE r.is_pending = true | ||
| RETURN user, r.invited_by as invited_by, r.invited_at as invited_at | ||
| ORDER BY r.invited_at DESC | ||
| """ | ||
|
|
||
| result = organizations_graph.query(query, {"domain": organization_domain}) | ||
|
|
||
| pending_users = [] | ||
| for row in result.result_set: | ||
| user_data = row[0] | ||
| invited_by = row[1] | ||
| invited_at = row[2] | ||
|
|
||
| # Convert Node object to dictionary | ||
| user_dict = {} | ||
| if hasattr(user_data, 'properties'): | ||
| user_dict = user_data.properties | ||
| elif hasattr(user_data, '__dict__'): | ||
| user_dict = {k: v for k, v in user_data.__dict__.items() if not k.startswith('_')} | ||
| else: | ||
| # Fallback for different Node implementations | ||
| user_dict = dict(user_data) if user_data else {} | ||
|
|
||
| pending_users.append({ | ||
| "user": user_dict, | ||
| "invited_by": invited_by, | ||
| "invited_at": invited_at | ||
| }) | ||
|
|
||
| return pending_users | ||
|
|
||
| except Exception as e: | ||
| logging.error("Error getting pending users for organization %s: %s", organization_domain, e) | ||
| return [] | ||
|
|
||
|
|
||
| def update_user_role(admin_email: str, target_email: str, new_role: str, organization_domain: str) -> Tuple[bool, str]: | ||
| """ | ||
| Update a user's role (admin function). | ||
| Args: | ||
| admin_email: The admin user's email | ||
| target_email: The email of user whose role to update | ||
| new_role: The new role to assign (e.g., 'user', 'admin', 'analyst', 'viewer') | ||
| organization_domain: The organization domain | ||
| Returns: | ||
| Tuple[bool, str]: (success, message) | ||
| """ | ||
| try: | ||
| # Validate admin permissions | ||
| admin_status = get_user_organization_status(admin_email) | ||
| if not admin_status or not admin_status.get("is_admin"): | ||
| return False, "Unauthorized: Only organization admins can update user roles" | ||
|
|
||
| if admin_status["organization"]["domain"] != organization_domain: | ||
| return False, "Unauthorized: Admin can only update roles in their own organization" | ||
|
|
||
| # Validate role value | ||
| allowed_roles = ['user', 'admin', 'analyst', 'viewer', 'manager'] | ||
| if new_role not in allowed_roles: | ||
| return False, f"Invalid role '{new_role}'. Allowed roles: {', '.join(allowed_roles)}" | ||
|
|
||
| organizations_graph = db.select_graph("Organizations") | ||
|
|
||
| # Update user's role | ||
| query = """ | ||
| MATCH (user:User {email: $target_email})-[r:BELONGS_TO]->(org:Organization {domain: $domain}) | ||
| WHERE r.is_pending = false | ||
| SET user.role = $new_role, | ||
| user.role_updated_by = $admin_email, | ||
| user.role_updated_at = timestamp() | ||
| RETURN user | ||
| """ | ||
|
|
||
| result = organizations_graph.query(query, { | ||
| "target_email": target_email, | ||
| "domain": organization_domain, | ||
| "new_role": new_role, | ||
| "admin_email": admin_email | ||
| }) | ||
|
|
||
| if result.result_set: | ||
| logging.info("Admin %s updated role of user %s to %s in organization %s", | ||
| admin_email, target_email, new_role, organization_domain) | ||
| return True, f"User {target_email} role updated to {new_role}" | ||
| else: | ||
| return False, "User not found or not a member of the organization" | ||
|
|
||
| except Exception as e: | ||
| logging.error("Error updating role for user %s in organization %s by admin %s: %s", | ||
| target_email, organization_domain, admin_email, e) | ||
| return False, f"Error updating user role: {str(e)}" | ||
|
|
||
|
|
||
| def get_user_role(user_email: str) -> str: | ||
| """ | ||
| Get a user's role. | ||
| Args: | ||
| user_email: The user's email | ||
| Returns: | ||
| str: The user's role, or 'user' as default | ||
| """ | ||
| try: | ||
| organizations_graph = db.select_graph("Organizations") | ||
|
|
||
| query = """ | ||
| MATCH (user:User {email: $email}) | ||
| RETURN user.role as role | ||
| """ | ||
|
|
||
| result = organizations_graph.query(query, {"email": user_email}) | ||
|
|
||
| if result.result_set and result.result_set[0][0]: | ||
| return result.result_set[0][0] | ||
| else: | ||
| return 'user' # Default role | ||
|
|
||
| except Exception as e: | ||
| logging.error("Error getting role for user %s: %s", user_email, e) | ||
| return 'user' # Default role on error | ||
|
|
||
|
|
||
| def update_user_role_direct(user_email: str, new_role: str) -> bool: | ||
| """ | ||
| Update a user's role directly (internal function, no auth checks). | ||
| Used for system operations like making first user admin. | ||
| Args: | ||
| user_email: The user's email | ||
| new_role: The new role to assign | ||
| Returns: | ||
| bool: Success status | ||
| """ | ||
| try: | ||
| # Validate role value | ||
| allowed_roles = ['user', 'admin', 'analyst', 'viewer', 'manager'] | ||
| if new_role not in allowed_roles: | ||
| logging.error("Invalid role '%s' for user %s", new_role, user_email) | ||
| return False | ||
|
|
||
| organizations_graph = db.select_graph("Organizations") | ||
|
|
||
| # Update user's role directly | ||
| query = """ | ||
| MATCH (user:User {email: $email}) | ||
| SET user.role = $new_role, | ||
| user.role_updated_at = timestamp() | ||
| RETURN user | ||
| """ | ||
|
|
||
| result = organizations_graph.query(query, { | ||
| "email": user_email, | ||
| "new_role": new_role | ||
| }) | ||
|
|
||
| if result.result_set: | ||
| logging.info("Updated role of user %s to %s", user_email, new_role) | ||
| return True | ||
| else: | ||
| logging.warning("User %s not found when updating role to %s", user_email, new_role) | ||
| return False | ||
|
|
||
| except Exception as e: | ||
| logging.error("Error updating role for user %s to %s: %s", user_email, new_role, e) | ||
| return False |
There was a problem hiding this comment.
Implement centralized log sanitization.
Multiple log injection vulnerabilities exist throughout this file where user-provided values are logged directly. Instead of fixing each instance individually, implement a centralized solution.
Create a logging utility module:
# api/utils/secure_logging.py
import re
import logging
def sanitize_log_value(value: str) -> str:
"""Remove newlines and carriage returns from log values."""
if not isinstance(value, str):
return str(value)
return re.sub(r'[\r\n]', '', value)
def get_secure_logger(name: str) -> logging.Logger:
"""Get a logger with secure formatting."""
logger = logging.getLogger(name)
# Could add custom formatter here
return logger
# Usage example:
# from api.utils.secure_logging import sanitize_log_value, get_secure_logger
# logger = get_secure_logger(__name__)
# logger.info("User %s added", sanitize_log_value(email))This approach ensures consistent security practices across the codebase.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 269-269: Log Injection
This log entry depends on a user-provided value.
[failure] 276-276: Log Injection
This log entry depends on a user-provided value.
[failure] 323-323: Log Injection
This log entry depends on a user-provided value.
[failure] 330-330: Log Injection
This log entry depends on a user-provided value.
[failure] 430-430: Log Injection
This log entry depends on a user-provided value.
[failure] 430-430: Log Injection
This log entry depends on a user-provided value.
[failure] 437-437: Log Injection
This log entry depends on a user-provided value.
[failure] 467-467: Log Injection
This log entry depends on a user-provided value.
[failure] 487-487: Log Injection
This log entry depends on a user-provided value.
[failure] 506-506: Log Injection
This log entry depends on a user-provided value.
[failure] 509-509: Log Injection
This log entry depends on a user-provided value.
[failure] 513-513: Log Injection
This log entry depends on a user-provided value.
🪛 GitHub Actions: Pylint
[warning] 76-76: Pylint C0301: Line too long (131/100).
[warning] 111-111: Pylint C0303: Trailing whitespace.
[warning] 115-115: Pylint C0301: Line too long (103/100).
[warning] 119-119: Pylint C0301: Line too long (105/100).
[warning] 145-145: Pylint C0303: Trailing whitespace.
[warning] 164-164: Pylint C0303: Trailing whitespace.
[warning] 194-194: Pylint C0303: Trailing whitespace.
[warning] 219-219: Pylint C0301: Line too long (121/100).
[warning] 244-244: Pylint C0301: Line too long (114/100).
[warning] 268-268: Pylint C0303: Trailing whitespace.
[warning] 270-270: Pylint C0301: Line too long (117/100).
[warning] 275-275: Pylint C0303: Trailing whitespace.
[warning] 280-280: Pylint C0301: Line too long (108/100).
[warning] 322-322: Pylint C0303: Trailing whitespace.
[warning] 329-329: Pylint C0303: Trailing whitespace.
[warning] 358-358: Pylint C0303: Trailing whitespace.
[warning] 382-382: Pylint C0301: Line too long (119/100).
[warning] 429-429: Pylint C0303: Trailing whitespace.
[warning] 436-436: Pylint C0303: Trailing whitespace.
[warning] 71-71: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 37-37: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 63-63: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 118-118: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 110-110: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 165-165: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 140-140: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 214-214: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 274-274: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 267-267: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 328-328: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 319-319: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 377-377: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 435-435: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 428-428: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 466-466: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 461-461: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
[warning] 512-512: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
[warning] 505-505: Pylint R1705: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it.
🤖 Prompt for AI Agents
In api/auth/organization_management.py lines 1 to 514, user-provided values are
logged directly, causing log injection risks. To fix this, create a new module
api/utils/secure_logging.py with a sanitize_log_value function that removes
newlines and carriage returns from strings, and a get_secure_logger function to
obtain loggers. Then, replace all logging calls in organization_management.py to
use a logger obtained from get_secure_logger and wrap all user-controlled
variables passed to logging methods with sanitize_log_value to ensure sanitized,
safe log output consistently across the file.
| # Generate organization name from domain (e.g., example.com -> Example) | ||
| organization_name = domain.split('.')[0].capitalize() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve organization name generation logic.
The current logic domain.split('.')[0].capitalize() is too simplistic and could fail for domains without dots or produce poor names for subdomains.
Apply this diff to improve organization name generation:
-# Generate organization name from domain (e.g., example.com -> Example)
-organization_name = domain.split('.')[0].capitalize()
+# Generate organization name from domain
+domain_parts = domain.split('.')
+if len(domain_parts) >= 2:
+ # Use the second-to-last part for standard domains (e.g., example.com -> Example)
+ organization_name = domain_parts[-2].capitalize()
+else:
+ # Fallback for single-part domains
+ organization_name = domain_parts[0].capitalize()📝 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.
| # Generate organization name from domain (e.g., example.com -> Example) | |
| organization_name = domain.split('.')[0].capitalize() | |
| # Generate organization name from domain | |
| domain_parts = domain.split('.') | |
| if len(domain_parts) >= 2: | |
| # Use the second-to-last part for standard domains (e.g., example.com -> Example) | |
| organization_name = domain_parts[-2].capitalize() | |
| else: | |
| # Fallback for single-part domains | |
| organization_name = domain_parts[0].capitalize() |
🤖 Prompt for AI Agents
In api/auth/organization_management.py around lines 54 to 55, the current
organization name generation uses domain.split('.')[0].capitalize(), which is
too simplistic and may fail for domains without dots or produce poor names for
subdomains. Improve this by adding checks to handle domains without dots
gracefully and extract the main domain part more reliably, then capitalize it
properly to generate a better organization name.
| except Exception as e: | ||
| logging.error("Error managing organization for domain %s: %s", domain, e) | ||
| return False, None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace generic exception handling with specific exceptions.
Catching Exception is too broad and could hide unexpected errors.
Consider catching specific exceptions from the graph database operations:
-except Exception as e:
- logging.error("Error managing organization for domain %s: %s", domain, e)
+except (AttributeError, KeyError, ValueError) as e:
+ logging.error("Error managing organization for domain %s: %s", domain, e)
+ return False, None
+except Exception as e:
+ logging.error("Unexpected error managing organization for domain %s: %s", domain, e)
return False, None📝 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.
| except Exception as e: | |
| logging.error("Error managing organization for domain %s: %s", domain, e) | |
| return False, None | |
| except (AttributeError, KeyError, ValueError) as e: | |
| logging.error("Error managing organization for domain %s: %s", domain, e) | |
| return False, None | |
| except Exception as e: | |
| logging.error("Unexpected error managing organization for domain %s: %s", domain, e) | |
| return False, None |
🧰 Tools
🪛 GitHub Actions: Pylint
[warning] 71-71: Pylint W0718: Catching too general exception Exception (broad-exception-caught).
🤖 Prompt for AI Agents
In api/auth/organization_management.py around lines 71 to 73, replace the
generic `except Exception` block with specific exception handlers for the graph
database operations. Identify the common exceptions that can be raised by the
graph database client (e.g., connection errors, query errors) and catch them
individually to provide more precise error handling and avoid masking unexpected
issues.
| # Create the relationship between user and organization | ||
| link_query = """ | ||
| MATCH (user:User {email: $user_email}) | ||
| MATCH (org:Organization {domain: $domain}) | ||
| MERGE (user)-[r:BELONGS_TO]->(org) | ||
| SET r.is_admin = $is_admin, | ||
| r.is_pending = $is_pending, | ||
| r.joined_at = timestamp() | ||
| RETURN user, org, r | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for user and organization existence.
The function attempts to create a relationship without verifying that both the user and organization exist, which could fail silently in the MATCH clauses.
Add existence validation before creating the relationship:
# Create the relationship between user and organization
+# First verify both entities exist
+verify_query = """
+MATCH (user:User {email: $user_email})
+MATCH (org:Organization {domain: $domain})
+RETURN count(user) as user_exists, count(org) as org_exists
+"""
+verify_result = organizations_graph.query(verify_query, {
+ "user_email": user_email,
+ "domain": organization_domain
+})
+
+if verify_result.result_set:
+ user_exists = verify_result.result_set[0][0]
+ org_exists = verify_result.result_set[0][1]
+ if not user_exists or not org_exists:
+ logging.error("User or organization not found: user=%s, org=%s", user_email, organization_domain)
+ return False
+
link_query = """
MATCH (user:User {email: $user_email})
MATCH (org:Organization {domain: $domain})📝 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.
| # Create the relationship between user and organization | |
| link_query = """ | |
| MATCH (user:User {email: $user_email}) | |
| MATCH (org:Organization {domain: $domain}) | |
| MERGE (user)-[r:BELONGS_TO]->(org) | |
| SET r.is_admin = $is_admin, | |
| r.is_pending = $is_pending, | |
| r.joined_at = timestamp() | |
| RETURN user, org, r | |
| """ | |
| # Create the relationship between user and organization | |
| # First verify both entities exist | |
| verify_query = """ | |
| MATCH (user:User {email: $user_email}) | |
| MATCH (org:Organization {domain: $domain}) | |
| RETURN count(user) as user_exists, count(org) as org_exists | |
| """ | |
| verify_result = organizations_graph.query(verify_query, { | |
| "user_email": user_email, | |
| "domain": organization_domain | |
| }) | |
| if verify_result.result_set: | |
| user_exists = verify_result.result_set[0][0] | |
| org_exists = verify_result.result_set[0][1] | |
| if not user_exists or not org_exists: | |
| logging.error("User or organization not found: user=%s, org=%s", user_email, organization_domain) | |
| return False | |
| link_query = """ | |
| MATCH (user:User {email: $user_email}) | |
| MATCH (org:Organization {domain: $domain}) | |
| MERGE (user)-[r:BELONGS_TO]->(org) | |
| SET r.is_admin = $is_admin, | |
| r.is_pending = $is_pending, | |
| r.joined_at = timestamp() | |
| RETURN user, org, r | |
| """ |
🤖 Prompt for AI Agents
In api/auth/organization_management.py around lines 92 to 101, the code creates
a relationship between user and organization without checking if both exist,
which can cause silent failures. Add explicit checks before the MERGE statement
to verify that the user and organization nodes exist by running separate queries
or using conditional logic. If either does not exist, handle the error
appropriately before proceeding to create the relationship.
| org_dict = {} | ||
| if hasattr(org_data, 'properties'): | ||
| org_dict = org_data.properties | ||
| elif hasattr(org_data, '__dict__'): | ||
| org_dict = {k: v for k, v in org_data.__dict__.items() if not k.startswith('_')} | ||
| else: | ||
| # Fallback for different Node implementations | ||
| org_dict = dict(org_data) if org_data else {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract Node to dictionary conversion logic.
This Node object conversion logic is duplicated in multiple functions (get_user_organization_status, get_organization_users, get_pending_users). Extract it to a reusable helper function.
Add this helper function at the module level:
def _node_to_dict(node) -> dict:
"""Convert a graph database Node object to a dictionary."""
if hasattr(node, 'properties'):
return node.properties
elif hasattr(node, '__dict__'):
return {k: v for k, v in node.__dict__.items() if not k.startswith('_')}
else:
# Fallback for different Node implementations
return dict(node) if node else {}Then simplify this code:
-# Convert Node object to dictionary
-org_dict = {}
-if hasattr(org_data, 'properties'):
- org_dict = org_data.properties
-elif hasattr(org_data, '__dict__'):
- org_dict = {k: v for k, v in org_data.__dict__.items() if not k.startswith('_')}
-else:
- # Fallback for different Node implementations
- org_dict = dict(org_data) if org_data else {}
+org_dict = _node_to_dict(org_data)🤖 Prompt for AI Agents
In api/auth/organization_management.py around lines 147 to 154, the Node object
to dictionary conversion logic is duplicated across multiple functions. Extract
this logic into a module-level helper function named _node_to_dict that takes a
Node object and returns its dictionary representation by checking for
'properties', '__dict__', or falling back to dict conversion. Replace the
existing conversion code in these functions with calls to this new helper
function to improve code reuse and maintainability.
| html += ` | ||
| <div class="user-item"> | ||
| <div class="user-info"> | ||
| <strong>${user.first_name} ${user.last_name}</strong> | ||
| <br> | ||
| <span class="user-email">${user.email}</span> | ||
| <br> | ||
| <span class="user-role">${roleDisplay}</span> | ||
| ${user.is_pending ? ' (Pending)' : ''} | ||
| </div> |
There was a problem hiding this comment.
Escape user data to prevent XSS attacks.
User-provided data (names, emails) is inserted directly into HTML without escaping, which could lead to XSS vulnerabilities if the data contains malicious scripts.
Add an escape function and use it for all user data:
+function escapeHtml(unsafe) {
+ return unsafe
+ .replace(/&/g, "&")
+ .replace(/</g, "<")
+ .replace(/>/g, ">")
+ .replace(/"/g, """)
+ .replace(/'/g, "'");
+}
+
html += `
<div class="user-item">
<div class="user-info">
- <strong>${user.first_name} ${user.last_name}</strong>
+ <strong>${escapeHtml(user.first_name)} ${escapeHtml(user.last_name)}</strong>
<br>
- <span class="user-email">${user.email}</span>
+ <span class="user-email">${escapeHtml(user.email)}</span>Apply similar escaping to all places where user data is displayed (lines 63-65, 133-136, 174-178).
📝 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.
| html += ` | |
| <div class="user-item"> | |
| <div class="user-info"> | |
| <strong>${user.first_name} ${user.last_name}</strong> | |
| <br> | |
| <span class="user-email">${user.email}</span> | |
| <br> | |
| <span class="user-role">${roleDisplay}</span> | |
| ${user.is_pending ? ' (Pending)' : ''} | |
| </div> | |
| function escapeHtml(unsafe) { | |
| return unsafe | |
| .replace(/&/g, "&") | |
| .replace(/</g, "<") | |
| .replace(/>/g, ">") | |
| .replace(/"/g, """) | |
| .replace(/'/g, "'"); | |
| } | |
| html += ` | |
| <div class="user-item"> | |
| <div class="user-info"> | |
| <strong>${escapeHtml(user.first_name)} ${escapeHtml(user.last_name)}</strong> | |
| <br> | |
| <span class="user-email">${escapeHtml(user.email)}</span> | |
| <br> | |
| <span class="user-role">${roleDisplay}</span> | |
| ${user.is_pending ? ' (Pending)' : ''} | |
| </div> |
🤖 Prompt for AI Agents
In api/templates/components/organization_modal.j2 around lines 130 to 139, user
data such as first_name, last_name, and email are directly inserted into HTML
without escaping, risking XSS attacks. Define an escape function that converts
special characters to HTML entities and apply it to all user data insertions in
this block and also at lines 63-65 and 174-178 to sanitize the output and
prevent script injection.
| async function addUserToOrganization() { | ||
| const email = document.getElementById('new-user-email').value.trim(); | ||
| if (!email) { | ||
| showError('Please enter a valid email address'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper email validation on client side.
The function only checks if the email is empty but doesn't validate the email format before sending to the server.
Apply this diff to add email validation:
async function addUserToOrganization() {
const email = document.getElementById('new-user-email').value.trim();
if (!email) {
showError('Please enter a valid email address');
return;
}
+
+ // Validate email format
+ const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
+ if (!emailRegex.test(email)) {
+ showError('Please enter a valid email address');
+ return;
+ }📝 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.
| async function addUserToOrganization() { | |
| const email = document.getElementById('new-user-email').value.trim(); | |
| if (!email) { | |
| showError('Please enter a valid email address'); | |
| return; | |
| } | |
| async function addUserToOrganization() { | |
| const email = document.getElementById('new-user-email').value.trim(); | |
| if (!email) { | |
| showError('Please enter a valid email address'); | |
| return; | |
| } | |
| // Validate email format | |
| const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | |
| if (!emailRegex.test(email)) { | |
| showError('Please enter a valid email address'); | |
| return; | |
| } | |
| // …rest of the function… | |
| } |
🤖 Prompt for AI Agents
In api/templates/components/organization_modal.j2 around lines 208 to 214, the
addUserToOrganization function only checks if the email input is empty but lacks
proper format validation. Add a client-side email format validation using a
regular expression to verify the email structure before proceeding. If the email
format is invalid, call showError with an appropriate message and return early
to prevent sending invalid data to the server.
| const response = await fetch('/api/organization/add-user', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify({ email: email }) | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify CSRF protection for API endpoints.
The POST requests don't include CSRF tokens, which could make the application vulnerable to CSRF attacks.
Please verify that your Flask application has CSRF protection enabled globally or add CSRF tokens to these requests.
🌐 Web query:
Does Flask-WTF or Flask-SeaSurf provide automatic CSRF protection for all POST requests, or do CSRF tokens need to be explicitly included in AJAX requests?
💡 Result:
Flask-WTF and Flask-SeaSurf both provide CSRF protection for POST requests, but when using AJAX requests, you must explicitly include the CSRF token, usually by setting it in the request headers.
Flask-WTF:
- For regular HTML forms rendered with FlaskForm, Flask-WTF automatically includes a CSRF token in the form as a hidden field, and the server validates this token during POST requests[1][5][6][7].
- For AJAX requests, you must manually include the CSRF token, typically by adding it to the
X-CSRFTokenheader. Flask-WTF's documentation explicitly states that for AJAX calls (from jQuery, Axios, etc.), you should add the CSRF token to the request header, e.g.:This requires generating the CSRF token in your page template and making sure the JavaScript uses it for every non-GET AJAX request[2][7].var csrf_token = "{{ csrf_token() }}"; $.ajaxSetup({ beforeSend: function(xhr, settings) { if (!/^(GET|HEAD|OPTIONS|TRACE)$/i.test(settings.type)) { xhr.setRequestHeader("X-CSRFToken", csrf_token); } } });
Flask-SeaSurf:
- Flask-SeaSurf works similarly: it protects all POST, PUT, PATCH, and DELETE requests, but for AJAX clients, you must explicitly set the CSRF token in the
X-CSRFTokenheader. SeaSurf uses the same header name and approach. The token is usually available to your rendered templates ascsrf_token()[2][7].
Summary:
Neither Flask-WTF nor Flask-SeaSurf will automatically protect plain AJAX POST requests unless you explicitly send the CSRF token with the request, usually in the X-CSRFToken header. HTML forms generated via FlaskForm or similar wrappers are handled automatically, but pure AJAX APIs are not[2][5][7].
Citations:
- 1: https://www.geeksforgeeks.org/python/csrf-protection-in-flask/
- 2: https://behainguyen.wordpress.com/2022/03/19/python-flask-wtfs-jquery-ajax-requests-and-date-validation/
- 3: https://www.youtube.com/watch?v=lc1sOvRaFpg
- 4: https://www.tadarox.ir/static/files/product/product_d03672cacfbd4d2dbe5a02675be142e5/product_file_MS-1025.pdf
- 5: https://testdriven.io/blog/csrf-flask/
- 6: https://www.peterspython.com/en/blog/flask-wtforms-and-ajax-csrf-protection-before-request-and-multilanguage
- 7: https://github.com/wtforms/flask-wtf/blob/main/docs/csrf.rst
- 8: https://github.com/wtforms/flask-wtf/blob/main/src/flask_wtf/csrf.py
- 9: https://flask-wtf.readthedocs.io/en/1.0.x/csrf/
- 10: https://wtforms.readthedocs.io/en/2.3.x/csrf/
Add CSRF tokens to your AJAX POST requests
Your fetch call in api/templates/components/organization_modal.j2 (lines 217–223) doesn’t include a CSRF token. Neither Flask-WTF nor Flask-SeaSurf will automatically protect plain AJAX requests—you must explicitly send the token (typically in the X-CSRFToken header).
Please update your template to expose the token and include it in the headers, for example:
{# somewhere in your template head or modal component #}
<script>
const CSRF_TOKEN = "{{ csrf_token() }}";
</script>--- a/api/templates/components/organization_modal.j2
+++ b/api/templates/components/organization_modal.j2
@@ lines 217-223
- const response = await fetch('/api/organization/add-user', {
- method: 'POST',
- headers: {
- 'Content-Type': 'application/json',
- },
- body: JSON.stringify({ email: email })
- });
+ const response = await fetch('/api/organization/add-user', {
+ method: 'POST',
+ headers: {
+ 'Content-Type': 'application/json',
+ 'X-CSRFToken': CSRF_TOKEN
+ },
+ body: JSON.stringify({ email: email })
+ });• Ensure your base template renders {{ csrf_token() }} so it’s available to your JavaScript.
• Verify that your Flask app is configured with Flask-WTF or Flask-SeaSurf and expects the X-CSRFToken header on AJAX calls.
• Repeat for any other non-GET fetch/axios/XHR requests.
🤖 Prompt for AI Agents
In api/templates/components/organization_modal.j2 around lines 217 to 223, the
fetch POST request lacks a CSRF token in its headers, which is required for
security. Fix this by exposing the CSRF token in the template using {{
csrf_token() }} in a script tag, then include this token in the fetch request
headers as 'X-CSRFToken'. Also ensure your Flask app is configured to expect
this header for AJAX POST requests.
| function showError(message) { | ||
| // You can customize this based on your existing error handling | ||
| alert('Error: ' + message); | ||
| } | ||
| function showSuccess(message) { | ||
| // You can customize this based on your existing success handling | ||
| alert('Success: ' + message); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace alert() with proper UI notifications.
Using alert() for error and success messages provides poor user experience. Consider implementing proper toast notifications or inline message displays.
Would you like me to generate a proper notification system implementation that integrates with your existing UI framework?
🤖 Prompt for AI Agents
In api/templates/components/organization_modal.j2 around lines 292 to 300,
replace the use of alert() in showError and showSuccess functions with proper UI
notifications such as toast messages or inline alerts. Identify the existing UI
framework's notification system or integrate a lightweight toast library, then
update these functions to trigger those notifications instead of blocking alert
dialogs for better user experience.
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
api/auth/organization_management.py (1)
559-594: Remove duplicate function definition.The function
complete_admin_setup_backendis defined twice (lines 76-111 and 559-594) with identical implementations.Remove the duplicate definition at lines 559-594:
-def complete_admin_setup_backend(user_email: str) -> bool: - """ - Complete admin setup for a user who is marked as admin but still pending. - This handles cases where the organization creator ended up in pending status. - - Args: - user_email: The admin user's email - - Returns: - bool: Success status - """ - # ... entire function body ...
♻️ Duplicate comments (8)
api/routes/organization.py (4)
34-35: Fix duplicate import and move to module level.The
get_user_rolefunction is imported locally when organization management functions are already imported at the top.Add
get_user_roleto the existing import at the top (line 7-14) and remove line 34:from api.auth.organization_management import ( get_user_organization_status, get_organization_users, add_user_to_organization_by_email, approve_pending_user, get_pending_users, - extract_email_domain + extract_email_domain, + get_user_role )
252-253: Remove duplicate imports.These imports are already defined at the module level and cause namespace pollution.
Remove lines 252-253 entirely as they duplicate the module-level imports.
57-59: Replace broad exception catching with specific exceptions.Catching generic
Exceptioncan hide programming errors and make debugging difficult.Replace with specific exception handling:
- except Exception as e: - logging.error("Error getting organization status: %s", e) + except (ValueError, KeyError, AttributeError) as e: + logging.error("Error getting organization status: %s", e, exc_info=True) return jsonify({"error": "Internal server error"}), 500 + except Exception as e: + logging.critical("Unexpected error in get_organization_status: %s", e, exc_info=True) + return jsonify({"error": "Internal server error"}), 500Also applies to: 101-103, 143-145, 199-200, 243-244, 290-291, 304-306
189-196: Sanitize error messages to prevent information disclosure.Error messages from internal functions may contain sensitive information or stack traces.
Log detailed errors internally and return generic messages to users:
if success: return jsonify({"message": message}), 200 else: - return jsonify({"error": message}), 400 + logging.warning("Failed to add user: %s", message) + return jsonify({"error": "Failed to add user to organization"}), 400Also applies to: 233-240, 280-287
api/auth/organization_management.py (4)
54-55: Improve organization name generation logic.The current logic
domain.split('.')[0].capitalize()is too simplistic and could produce poor names for subdomains or single-part domains.Improve the name generation:
-# Generate organization name from domain (e.g., example.com -> Example) -organization_name = domain.split('.')[0].capitalize() +# Generate organization name from domain +domain_parts = domain.split('.') +if len(domain_parts) >= 2: + # Use the second-to-last part for standard domains (e.g., example.com -> Example) + organization_name = domain_parts[-2].capitalize() +else: + # Fallback for single-part domains + organization_name = domain_parts[0].capitalize() if domain_parts else "Organization"
311-311: Implement centralized log sanitization to prevent log injection.User-provided values are logged directly throughout the file, creating log injection vulnerabilities.
Create a utility module for secure logging:
# api/utils/secure_logging.py import re import logging def sanitize_log_value(value: str) -> str: """Remove newlines and carriage returns from log values.""" if not isinstance(value, str): return str(value) return re.sub(r'[\r\n]', '', value) def get_secure_logger(name: str) -> logging.Logger: """Get a logger with secure formatting.""" return logging.getLogger(name)Then update all logging calls:
-logging.info("Admin %s added user %s to organization %s (pending)", - admin_email, target_email, organization_domain) +from api.utils.secure_logging import sanitize_log_value, get_secure_logger +logger = get_secure_logger(__name__) +logger.info("Admin %s added user %s to organization %s (pending)", + sanitize_log_value(admin_email), + sanitize_log_value(target_email), + sanitize_log_value(organization_domain))Also applies to: 318-318, 365-365, 372-372, 472-472, 479-479, 509-509, 529-529, 548-548, 551-551, 555-555
184-191: Extract Node to dictionary conversion logic.The Node object conversion logic is duplicated in multiple functions.
Add a helper function at module level:
def _node_to_dict(node) -> dict: """Convert a graph database Node object to a dictionary.""" if hasattr(node, 'properties'): return node.properties elif hasattr(node, '__dict__'): return {k: v for k, v in node.__dict__.items() if not k.startswith('_')} else: # Fallback for different Node implementations return dict(node) if node else {}Then simplify the code:
-# Convert Node object to dictionary -org_dict = {} -if hasattr(org_data, 'properties'): - org_dict = org_data.properties -elif hasattr(org_data, '__dict__'): - org_dict = {k: v for k, v in org_data.__dict__.items() if not k.startswith('_')} -else: - # Fallback for different Node implementations - org_dict = dict(org_data) if org_data else {} +org_dict = _node_to_dict(org_data)Also applies to: 233-240, 402-409
447-449: Extract shared role validation logic.Role validation logic is duplicated between
update_user_roleandupdate_user_role_direct.Create a shared validation function:
def validate_role(role: str) -> bool: """Validate if a role is allowed.""" allowed_roles = ['user', 'admin', 'analyst', 'viewer', 'manager'] return role in allowed_roles def get_allowed_roles() -> list: """Get list of allowed roles.""" return ['user', 'admin', 'analyst', 'viewer', 'manager']Then use in both functions:
-# Validate role value -allowed_roles = ['user', 'admin', 'analyst', 'viewer', 'manager'] -if new_role not in allowed_roles: - return False, f"Invalid role '{new_role}'. Allowed roles: {', '.join(allowed_roles)}" +# Validate role value +if not validate_role(new_role): + allowed_roles = get_allowed_roles() + return False, f"Invalid role '{new_role}'. Allowed roles: {', '.join(allowed_roles)}"Also applies to: 527-529
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/auth/organization_management.py(1 hunks)api/routes/organization.py(1 hunks)api/static/css/modals.css(2 hunks)api/static/js/app.js(2 hunks)api/static/js/modules/organization.js(1 hunks)api/templates/components/organization_modal.j2(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/templates/components/organization_modal.j2
🧰 Additional context used
🧬 Code Graph Analysis (2)
api/static/js/app.js (1)
api/static/js/modules/organization.js (2)
openOrganizationModal(7-10)closeOrganizationModal(12-14)
api/routes/organization.py (2)
api/auth/user_management.py (2)
validate_and_cache_user(188-287)token_required(290-312)api/auth/organization_management.py (8)
get_user_organization_status(160-204)get_organization_users(207-253)add_user_to_organization_by_email(256-319)approve_pending_user(322-373)get_pending_users(376-421)extract_email_domain(9-13)get_user_role(483-510)update_user_role(424-480)
🪛 Ruff (0.12.2)
api/auth/organization_management.py
72-72: Undefined name new_role
(F821)
559-559: Redefinition of unused complete_admin_setup_backend from line 76
(F811)
🪛 GitHub Check: CodeQL
api/auth/organization_management.py
[failure] 311-311: Log Injection
This log entry depends on a user-provided value.
[failure] 318-318: Log Injection
This log entry depends on a user-provided value.
[failure] 365-365: Log Injection
This log entry depends on a user-provided value.
[failure] 372-372: Log Injection
This log entry depends on a user-provided value.
[failure] 472-472: Log Injection
This log entry depends on a user-provided value.
[failure] 472-472: Log Injection
This log entry depends on a user-provided value.
[failure] 479-479: Log Injection
This log entry depends on a user-provided value.
[failure] 509-509: Log Injection
This log entry depends on a user-provided value.
[failure] 529-529: Log Injection
This log entry depends on a user-provided value.
[failure] 548-548: Log Injection
This log entry depends on a user-provided value.
[failure] 551-551: Log Injection
This log entry depends on a user-provided value.
[failure] 555-555: Log Injection
This log entry depends on a user-provided value.
api/routes/organization.py
[warning] 194-194: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 196-196: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 238-238: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 240-240: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 285-285: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 287-287: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 GitHub Actions: Pylint
api/auth/organization_management.py
[warning] 89-590: Multiple Pylint warnings including trailing whitespace, line too long, broad-exception-caught, no-else-return, undefined variable 'new_role', function redefined, and others.
api/routes/organization.py
[warning] 33-304: Pylint warnings including line too long, broad-exception-caught, no-else-return, import outside toplevel, too many return statements, redefined outer name, reimported, and import outside toplevel.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (2)
api/static/js/app.js (1)
19-19: LGTM!The integration of organization modal functions follows the existing module pattern and is properly exposed globally within the appropriate setup function.
Also applies to: 82-85
api/static/css/modals.css (1)
179-193: LGTM!The refactored
.user-profile-actionstyling improves consistency with full width buttons and proper hover effects.
api/auth/organization_management.py
Outdated
| except Exception as e: | ||
| logging.error("Error updating user %s role to %s: %s", user_email, new_role, e) | ||
| return False |
There was a problem hiding this comment.
Fix error message and undefined variable.
The error message references user_email and new_role which are not in scope for this function. This appears to be a copy-paste error from another function.
Fix the error message to match the function context:
except Exception as e:
- logging.error("Error updating user %s role to %s: %s", user_email, new_role, e)
+ logging.error("Error managing organization for domain %s: %s", domain, e)
return False, NoneCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.12.2)
72-72: Undefined name new_role
(F821)
🤖 Prompt for AI Agents
In api/auth/organization_management.py around lines 71 to 73, the error log
references undefined variables user_email and new_role, likely due to
copy-paste. Update the logging.error call to use variables that are defined and
relevant in this function's scope to accurately reflect the error context.
| .users-list { | ||
| margin-top: 10px; | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 0; | ||
| border: 1px solid var(--border-color); | ||
| border-radius: 8px; | ||
| background: var(--falkor-quaternary); | ||
| overflow: hidden; | ||
| } | ||
| .user-item { | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: flex-start; | ||
| gap: 32px; | ||
| padding: 14px 22px; | ||
| border-bottom: 1px solid var(--border-color); | ||
| background: rgba(255,255,255,0.04); | ||
| font-size: 1.08em; | ||
| } | ||
| .user-item:last-child { | ||
| border-bottom: none; | ||
| } | ||
| .user-info { | ||
| display: flex; | ||
| flex-direction: row; | ||
| align-items: center; | ||
| gap: 24px; | ||
| flex: 1 1 0; | ||
| } | ||
| .user-name { | ||
| color: var(--text-primary); | ||
| font-weight: 600; | ||
| margin-right: 12px; | ||
| font-size: 1.08em; | ||
| } | ||
| .user-email { | ||
| color: var(--text-secondary); | ||
| font-size: 1em; | ||
| margin-right: 12px; | ||
| } | ||
| .user-role { | ||
| color: var(--falkor-tertiary); | ||
| font-size: 1em; | ||
| font-weight: 500; | ||
| margin-right: 12px; | ||
| text-transform: uppercase; | ||
| } | ||
| .user-pending { | ||
| color: #e0a800; | ||
| font-size: 0.98em; | ||
| font-style: italic; | ||
| margin-left: 8px; | ||
| } | ||
| .user-actions { | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 8px; | ||
| } | ||
| .role-select { |
There was a problem hiding this comment.
Remove duplicate CSS rules to prevent specificity conflicts.
The CSS classes .users-list, .user-item, .user-info, .user-email, .user-role, .user-actions, and .add-user-form are defined twice with conflicting styles. This will cause specificity issues and unpredictable styling behavior.
Consolidate the duplicate rules by merging them or removing one set. For example:
-/* User list and add user form improvements */
-
-/* --- Organization Modal User List Redesign --- */
-.users-list {
- margin-top: 10px;
- display: flex;
- flex-direction: column;
- gap: 0;
- border: 1px solid var(--border-color);
- border-radius: 8px;
- background: var(--falkor-quaternary);
- overflow: hidden;
-}
# Keep only one definition of .users-list (lines 412-421)
-
-# Remove duplicate definitions (lines 528-595)
-.users-list {
- border: 1px solid var(--border-color);
- border-radius: 6px;
- margin: 15px 0;
- background-color: var(--falkor-quaternary);
-}
-# ... remove all duplicate rulesAlso applies to: 528-595
🤖 Prompt for AI Agents
In api/static/css/modals.css around lines 412 to 471, there are duplicate CSS
class definitions for .users-list, .user-item, .user-info, .user-email,
.user-role, .user-actions, and .add-user-form causing specificity conflicts and
unpredictable styling. To fix this, locate the duplicate sets of these class
rules (also present around lines 528 to 595), then consolidate them by merging
their styles into a single definition for each class or remove one of the
duplicates entirely to ensure each class is defined only once with consistent
styles.
| .role-select { | ||
| padding: 4px 8px; | ||
| border: 1px solid var(--border-color); | ||
| border-radius: 4px; | ||
| font-size: 12px; | ||
| background-color: var(--falkor-quaternary); | ||
| color: var(--text-primary); | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| .role-select:hover { | ||
| border-color: var(--falkor-primary); | ||
| } | ||
|
|
||
| .role-select:focus { | ||
| outline: none; | ||
| border-color: var(--falkor-primary); | ||
| box-shadow: 0 0 0 2px rgba(91, 107, 192, 0.25); | ||
| } |
There was a problem hiding this comment.
Remove duplicate .role-select CSS rules.
The .role-select class is defined twice (lines 471-483 and 648-666) with different styles, causing specificity conflicts.
Keep only the first, more detailed definition and remove lines 648-666:
-.role-select {
- padding: 4px 8px;
- border: 1px solid var(--border-color);
- border-radius: 4px;
- font-size: 12px;
- background-color: var(--falkor-quaternary);
- color: var(--text-primary);
- cursor: pointer;
-}
-
-.role-select:hover {
- border-color: var(--falkor-primary);
-}
-
-.role-select:focus {
- outline: none;
- border-color: var(--falkor-primary);
- box-shadow: 0 0 0 2px rgba(91, 107, 192, 0.25);
-}📝 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.
| .role-select { | |
| padding: 4px 8px; | |
| border: 1px solid var(--border-color); | |
| border-radius: 4px; | |
| font-size: 12px; | |
| background-color: var(--falkor-quaternary); | |
| color: var(--text-primary); | |
| cursor: pointer; | |
| } | |
| .role-select:hover { | |
| border-color: var(--falkor-primary); | |
| } | |
| .role-select:focus { | |
| outline: none; | |
| border-color: var(--falkor-primary); | |
| box-shadow: 0 0 0 2px rgba(91, 107, 192, 0.25); | |
| } |
🤖 Prompt for AI Agents
In api/static/css/modals.css between lines 648 and 666, there is a duplicate
definition of the .role-select class that conflicts with the earlier one at
lines 471-483. Remove the entire block from lines 648 to 666 to keep only the
first, more detailed .role-select CSS rules and avoid specificity conflicts.
| window.loadOrganizationUsers = async function() { | ||
| try { | ||
| const response = await fetch('/api/organization/users'); | ||
| const data = await response.json(); | ||
|
|
||
| if (response.ok) { | ||
| renderUsersList(data.users, 'Organization Members'); | ||
| } else { | ||
| showError(data.error || 'Failed to load organization users'); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error loading organization users:', error); | ||
| showError('Failed to load organization users'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor global function assignments for consistency.
Functions are inconsistently exposed - some via exports (openOrganizationModal, closeOrganizationModal) and others via direct window assignment. This makes the module interface unclear.
Consider exporting all public functions and having app.js handle global exposure:
-window.loadOrganizationUsers = async function() {
+export async function loadOrganizationUsers() {
try {
const response = await fetch('/api/organization/users');
// ... rest of the function
}
}
-window.loadPendingUsers = async function() {
+export async function loadPendingUsers() {
// ... function body
}
// Similar changes for other window functionsThen in app.js:
import {
openOrganizationModal,
closeOrganizationModal,
loadOrganizationUsers,
loadPendingUsers,
// ... other functions
} from './modules/organization.js';
// Expose globally in setupUIComponents
window.loadOrganizationUsers = loadOrganizationUsers;
window.loadPendingUsers = loadPendingUsers;
// ... etcAlso applies to: 91-105, 181-189, 224-258, 260-282, 284-311
🤖 Prompt for AI Agents
In api/static/js/modules/organization.js around lines 75 to 89, the function
loadOrganizationUsers is assigned directly to the window object, causing
inconsistent exposure compared to other functions exported normally. Refactor by
removing the window assignment and instead export loadOrganizationUsers as a
named export. Then, in app.js, import loadOrganizationUsers along with other
functions and assign them to window there for global exposure, ensuring a
consistent module interface. Apply the same pattern to the other indicated line
ranges.
| const email = document.getElementById('new-user-email').value.trim(); | ||
| const first_name = document.getElementById('new-user-first-name').value.trim(); | ||
| const last_name = document.getElementById('new-user-last-name').value.trim(); | ||
| if (!email) { | ||
| showError('Please enter a valid email address'); | ||
| return; | ||
| } | ||
| if (!first_name) { | ||
| showError('Please enter a first name'); | ||
| return; | ||
| } | ||
| if (!last_name) { | ||
| showError('Please enter a last name'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Add HTML escaping to prevent XSS vulnerabilities.
User inputs (email, first_name, last_name) are collected and sent to the server, but when rendered back in the UI (e.g., in renderUsersList), they need proper escaping.
Add an escaping function and use it when rendering user-provided content:
function escapeHtml(unsafe) {
return unsafe
.replace(/&/g, "&")
.replace(/</g, "<")
.replace(/>/g, ">")
.replace(/"/g, """)
.replace(/'/g, "'");
}Then in renderUsersList (lines 144-145):
-<td style="padding: 0.5em 2em 0.5em 1em; font-weight: bold;">${user.first_name ? user.first_name : ''}</td>
-<td style="padding: 0.5em 2em 0.5em 1em; font-weight: bold;">${user.last_name ? user.last_name : ''}</td>
+<td style="padding: 0.5em 2em 0.5em 1em; font-weight: bold;">${user.first_name ? escapeHtml(user.first_name) : ''}</td>
+<td style="padding: 0.5em 2em 0.5em 1em; font-weight: bold;">${user.last_name ? escapeHtml(user.last_name) : ''}</td>🤖 Prompt for AI Agents
In api/static/js/modules/organization.js around lines 225 to 239, user inputs
email, first_name, and last_name are collected without escaping, which can lead
to XSS vulnerabilities when these values are rendered back in the UI. To fix
this, define an escapeHtml function that replaces special HTML characters with
their safe equivalents, and then use this function to escape user-provided
content in the renderUsersList function (around lines 144-145) before inserting
it into the DOM.
| function showError(message) { | ||
| // You can customize this based on your existing error handling | ||
| alert('Error: ' + message); | ||
| } | ||
|
|
||
| function showSuccess(message) { | ||
| // You can customize this based on your existing success handling | ||
| alert('Success: ' + message); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace alert() with a better notification system.
Using alert() for error and success messages provides poor UX as it blocks the UI and looks unprofessional.
Implement a toast notification system or modal-based messages:
function showError(message) {
- // You can customize this based on your existing error handling
- alert('Error: ' + message);
+ // Create a dismissible error notification
+ const notification = document.createElement('div');
+ notification.className = 'notification notification-error';
+ notification.textContent = message;
+ notification.onclick = () => notification.remove();
+ document.body.appendChild(notification);
+ setTimeout(() => notification.remove(), 5000);
}
function showSuccess(message) {
- // You can customize this based on your existing success handling
- alert('Success: ' + message);
+ // Create a dismissible success notification
+ const notification = document.createElement('div');
+ notification.className = 'notification notification-success';
+ notification.textContent = message;
+ notification.onclick = () => notification.remove();
+ document.body.appendChild(notification);
+ setTimeout(() => notification.remove(), 5000);
}Add corresponding CSS for the notification styles.
📝 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.
| function showError(message) { | |
| // You can customize this based on your existing error handling | |
| alert('Error: ' + message); | |
| } | |
| function showSuccess(message) { | |
| // You can customize this based on your existing success handling | |
| alert('Success: ' + message); | |
| } | |
| function showError(message) { | |
| // Create a dismissible error notification | |
| const notification = document.createElement('div'); | |
| notification.className = 'notification notification-error'; | |
| notification.textContent = message; | |
| notification.onclick = () => notification.remove(); | |
| document.body.appendChild(notification); | |
| setTimeout(() => notification.remove(), 5000); | |
| } | |
| function showSuccess(message) { | |
| // Create a dismissible success notification | |
| const notification = document.createElement('div'); | |
| notification.className = 'notification notification-success'; | |
| notification.textContent = message; | |
| notification.onclick = () => notification.remove(); | |
| document.body.appendChild(notification); | |
| setTimeout(() => notification.remove(), 5000); | |
| } |
🤖 Prompt for AI Agents
In api/static/js/modules/organization.js around lines 313 to 321, replace the
alert() calls in showError and showSuccess functions with a non-blocking toast
notification system or modal-based messages to improve user experience.
Implement the toast notification logic in JavaScript and add the necessary CSS
styles for the notifications to appear styled and unobtrusive. Ensure the
notifications can display messages dynamically and disappear after a short
duration without blocking the UI.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
api/auth/organization_management.py (1)
558-593: Duplicate function definition: complete_admin_setup_backend redefined.Function is defined earlier (Lines 74-109). This later redefinition will shadow the first and triggers Ruff F811.
Apply to remove the duplicate:
- def complete_admin_setup_backend(user_email: str) -> bool: - """ - Complete admin setup for a user who is marked as admin but still pending. - This handles cases where the organization creator ended up in pending status. - - Args: - user_email: The admin user's email - - Returns: - bool: Success status - """ - try: - organizations_graph = db.select_graph("Organizations") - - # Update the user's relationship to set is_pending = False - query = """ - MATCH (user:User {email: $user_email})-[r:BELONGS_TO]->(org:Organization) - WHERE r.is_admin = true AND r.is_pending = true - SET r.is_pending = false, - r.setup_completed_at = timestamp() - RETURN user, org, r - """ - - result = organizations_graph.query(query, {"user_email": user_email}) - - if result.result_set: - logging.info("Completed admin setup for user %s", user_email) - return True - else: - logging.warning("No pending admin relationship found for user %s", user_email) - return False - - except Exception as e: - logging.error("Error completing admin setup for user %s: %s", user_email, e) - return False
♻️ Duplicate comments (7)
api/auth/user_management.py (1)
116-120: Check role update result to avoid inconsistent state.The result of update_user_role_direct is ignored; if it fails, state becomes inconsistent.
Apply:
- from api.auth.organization_management import update_user_role_direct - update_user_role_direct(email, "admin") - logging.info("User %s created organization %s and assigned as admin", - email, domain) + from api.auth.organization_management import update_user_role_direct + role_updated = update_user_role_direct(email, "admin") + if role_updated: + logging.info("User %s created organization %s and assigned as admin", + email, domain) + else: + logging.error("Failed to set admin role for %s after org creation %s", + email, domain)api/auth/organization_management.py (6)
53-55: Improve organization name derivation from domain.Current split on first dot is too naive for subdomains and single-label domains.
Apply:
- # Generate organization name from domain (e.g., example.com -> Example) - organization_name = domain.split('.')[0].capitalize() + # Generate organization name from domain (e.g., foo.example.com -> Example) + parts = domain.split('.') + if len(parts) >= 2: + organization_name = parts[-2].capitalize() + else: + organization_name = parts[0].capitalize()
182-191: DRY: factor Node-to-dict conversion into a helper.Same conversion logic is repeated. Extract a helper and reuse.
Apply within these blocks:
- # Convert Node object to dictionary - org_dict = {} - if hasattr(org_data, 'properties'): - org_dict = org_data.properties - elif hasattr(org_data, '__dict__'): - org_dict = {k: v for k, v in org_data.__dict__.items() if not k.startswith('_')} - else: - # Fallback for different Node implementations - org_dict = dict(org_data) if org_data else {} + org_dict = _node_to_dict(org_data)- # Convert Node object to dictionary - user_dict = {} - if hasattr(user_data, 'properties'): - user_dict = user_data.properties - elif hasattr(user_data, '__dict__'): - user_dict = {k: v for k, v in user_data.__dict__.items() if not k.startswith('_')} - else: - # Fallback for different Node implementations - user_dict = dict(user_data) if user_data else {} + user_dict = _node_to_dict(user_data)- # Convert Node object to dictionary - user_dict = {} - if hasattr(user_data, 'properties'): - user_dict = user_data.properties - elif hasattr(user_data, '__dict__'): - user_dict = {k: v for k, v in user_data.__dict__.items() if not k.startswith('_')} - else: - # Fallback for different Node implementations - user_dict = dict(user_data) if user_data else {} + user_dict = _node_to_dict(user_data)Add this outside the shown ranges (at module top, after imports):
def _node_to_dict(node) -> dict: if not node: return {} if hasattr(node, 'properties'): return node.properties if hasattr(node, '__dict__'): return {k: v for k, v in node.__dict__.items() if not k.startswith('_')} try: return dict(node) except Exception: return {"value": str(node)}Also applies to: 231-240, 400-409
445-449: Deduplicate role validation logic via a helper.allowed_roles is repeated here and in update_user_role_direct.
Apply:
- # Validate role value - allowed_roles = ['user', 'admin', 'analyst', 'viewer', 'manager'] - if new_role not in allowed_roles: - return False, f"Invalid role '{new_role}'. Allowed roles: {', '.join(allowed_roles)}" + # Validate role value + if not validate_role(new_role): + return False, f"Invalid role '{new_role}'. Allowed roles: {', '.join(get_allowed_roles())}"Add these helpers near the top:
def get_allowed_roles() -> list[str]: return ['user', 'admin', 'analyst', 'viewer', 'manager'] def validate_role(role: str) -> bool: return role in get_allowed_roles()
524-530: Use shared role validator here as well.Keep role validation centralized and consistent.
Apply:
- # Validate role value - allowed_roles = ['user', 'admin', 'analyst', 'viewer', 'manager'] - if new_role not in allowed_roles: - logging.error("Invalid role '%s' for user %s", new_role, user_email) - return False + # Validate role value + if not validate_role(new_role): + logging.error("Invalid role '%s' for user %s", new_role, user_email) + return False
111-157: Validate existence of user and org before MERGE relationship to avoid silent failures.MATCH clauses can yield empty sets, causing MERGE to do nothing; add explicit existence checks and clearer errors.
Apply:
- # Create the relationship between user and organization + # Verify both user and org exist, then create the relationship + verify = organizations_graph.query(""" + MATCH (u:User {email: $user_email}) + MATCH (o:Organization {domain: $domain}) + RETURN count(u) AS uc, count(o) AS oc + """, {"user_email": user_email, "domain": organization_domain}) + if not verify.result_set or verify.result_set[0][0] == 0 or verify.result_set[0][1] == 0: + logging.error("User or organization not found: user=%s, org=%s", user_email, organization_domain) + return False + + # Create the relationship between user and organization link_query = """ MATCH (user:User {email: $user_email}) MATCH (org:Organization {domain: $domain}) MERGE (user)-[r:BELONGS_TO]->(org) ON CREATE SET r.is_admin = $is_admin, r.is_pending = $is_pending, r.joined_at = timestamp() RETURN user, org, r """
308-317: Sanitize user-controlled values in logs to address log injection findings.CodeQL flagged these. Sanitize email/domain before logging.
Apply:
- logging.info("Admin %s added user %s to organization %s (pending)", - admin_email, target_email, organization_domain) + from api.utils.secure_logging import sanitize_log_value as _sv + logging.info( + "Admin %s added user %s to organization %s (pending)", + _sv(admin_email), _sv(target_email), _sv(organization_domain) + )Also add a shared utility (new file) for consistent sanitization:
# api/utils/secure_logging.py import re def sanitize_log_value(value): s = str(value) return re.sub(r'[\r\n]', '', s)
🧹 Nitpick comments (4)
api/auth/user_management.py (2)
30-35: Fix typo in comment and improve error logging for invalid emails.Current comment has a stray “pipenv install” and the except block drops the specific validation error details.
Apply:
- # Validate and normalizepipenv install email format using email_validator + # Validate and normalize email format using email_validator try: - email = validate_email(email).normalized - except EmailNotValidError: - logging.error("Invalid email format: %s", email) + email = validate_email(email).normalized + except EmailNotValidError as e: + logging.error("Invalid email: %s", e) return False, None
16-151: Function is getting large; consider splitting to reduce locals and complexity.Pylint flagged too-many-locals. Split: (a) provider/email validation, (b) user/identity merge, (c) org linking/role assignment.
Want me to draft the refactor?
api/auth/organization_management.py (2)
69-71: Correct error message context.Message says “Error updating user” while this function checks/creates orgs.
Apply:
- except Exception as e: - logging.error("Error updating user %s: %s", user_email, e) + except Exception as e: + logging.error("Error checking/creating organization for user %s: %s", user_email, e) return False, None
1-6: Linter/CodeQL hygiene follow-up.Pylint flagged trailing whitespace/line length; CodeQL flagged multiple log injection issues beyond the example above. After adopting sanitize_log_value, sweep the file to sanitize all user-provided values in logs and run the linters.
I can open a follow-up PR to apply secure logging across this module and fix long lines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Pipfile(1 hunks)api/auth/organization_management.py(1 hunks)api/auth/user_management.py(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
api/auth/user_management.py (1)
api/auth/organization_management.py (3)
check_or_create_organization(16-71)link_user_to_organization(111-156)update_user_role_direct(512-555)
api/auth/organization_management.py (2)
api/static/js/modules/organization.js (3)
first_name(226-226)last_name(227-227)tests/e2e/pages/home_page.py (1)
select_graph(57-59)
🪛 GitHub Actions: Pylint
api/auth/user_management.py
[error] 1-1: Pylint: Trailing whitespace and too-many-locals issues detected (trailing-whitespace, too-many-locals)
api/auth/organization_management.py
[error] 1-1: Pylint: Trailing whitespace and line-too-long issues detected (trailing-whitespace, line-too-long)
🪛 Ruff (0.12.2)
api/auth/organization_management.py
558-558: Redefinition of unused complete_admin_setup_backend from line 74
(F811)
🪛 GitHub Check: CodeQL
api/auth/organization_management.py
[failure] 310-310: Log Injection
This log entry depends on a user-provided value.
[failure] 317-317: Log Injection
This log entry depends on a user-provided value.
[failure] 364-364: Log Injection
This log entry depends on a user-provided value.
[failure] 371-371: Log Injection
This log entry depends on a user-provided value.
[failure] 471-471: Log Injection
This log entry depends on a user-provided value.
[failure] 471-471: Log Injection
This log entry depends on a user-provided value.
[failure] 478-478: Log Injection
This log entry depends on a user-provided value.
[failure] 508-508: Log Injection
This log entry depends on a user-provided value.
[failure] 528-528: Log Injection
This log entry depends on a user-provided value.
[failure] 547-547: Log Injection
This log entry depends on a user-provided value.
[failure] 550-550: Log Injection
This log entry depends on a user-provided value.
[failure] 554-554: Log Injection
This log entry depends on a user-provided value.
🔇 Additional comments (5)
Pipfile (2)
16-17: LGTM: email validation dependencies align with new flows.These additions match the new domain/email workflows in auth and org modules.
7-8: Review Pipfile dependencies: Confirm OpenAI retention and Bedrock extras impactWe don’t see any direct
import openaiin the repo, but every LLM call useslitellm.completionorlitellm.embedding. Bedrock is only mentioned in commented‐out code. Before removing or consolidating:• No direct OpenAI imports—only
litellmused across modules:
- api/utils.py, api/helpers/crm_data_generator.py, api/graph.py, api/config.py, and all agents.
• Bedrock references exist only in commented blocks inapi/loaders/csv_loader.py(lines ~274–277).Action items:
- Verify whether
litellm’s core package now includes the OpenAI client (so you can drop the explicitopenaipin) or if you must keep/installopenai(or switch tolitellm[openai]).- Confirm removing the Bedrock extras has no runtime impact (no live Bedrock calls) and clean up those commented lines if you won’t support Bedrock.
- If you do need future Bedrock support, re-add the appropriate
litellm[bedrock]extras.api/auth/user_management.py (2)
11-11: Good call: use email-validator for normalization.Import and usage are appropriate and reduce hand-rolled parsing.
47-51: Imports inside function: keep only if necessary for cycle avoidance.If this is to avoid an import cycle, keep it. Otherwise, move imports to the top-level for clarity and linting.
Do you still hit an import cycle if you move these to the top? If not, consider hoisting them.
api/auth/organization_management.py (1)
452-460: Clarify admin privilege source of truth (user.role vs r.is_admin).update_user_role changes user.role but not r.is_admin. All admin-only operations currently check r.is_admin, so setting role='admin' may not grant admin privileges.
If intended, document that admin is governed solely by the relationship flag and add separate APIs for promoting/demoting admins. Otherwise, update r.is_admin in this flow when new_role == 'admin'.
| def check_or_create_organization(user_email: str) -> Tuple[bool, str]: | ||
| """ | ||
| Check if organization exists for email domain, create if not. | ||
| Returns (is_new_organization, organization_info) | ||
| """ | ||
| domain = extract_email_domain(user_email) | ||
| if not domain: | ||
| logging.error("Invalid email domain for user: %s", user_email) | ||
| return False, None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix return type hint: can return None for domain.
The function returns (False, None) on failures, but the signature declares Tuple[bool, str].
Apply:
-def check_or_create_organization(user_email: str) -> Tuple[bool, str]:
+from typing import Optional
+def check_or_create_organization(user_email: str) -> Tuple[bool, Optional[str]]:📝 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.
| def check_or_create_organization(user_email: str) -> Tuple[bool, str]: | |
| """ | |
| Check if organization exists for email domain, create if not. | |
| Returns (is_new_organization, organization_info) | |
| """ | |
| domain = extract_email_domain(user_email) | |
| if not domain: | |
| logging.error("Invalid email domain for user: %s", user_email) | |
| return False, None | |
| from typing import Optional | |
| def check_or_create_organization(user_email: str) -> Tuple[bool, Optional[str]]: | |
| """ | |
| Check if organization exists for email domain, create if not. | |
| Returns (is_new_organization, organization_info) | |
| """ | |
| domain = extract_email_domain(user_email) | |
| if not domain: | |
| logging.error("Invalid email domain for user: %s", user_email) | |
| return False, None |
🤖 Prompt for AI Agents
In api/auth/organization_management.py around lines 16 to 24, the function
check_or_create_organization is annotated to return Tuple[bool, str] but it can
return None for the organization on failure; update the type hint to Tuple[bool,
Optional[str]] and add Optional to the typing imports, and adjust the
docstring/return description accordingly so callers know the second element may
be None.
| # Validate target email domain matches organization | ||
| target_domain = extract_email_domain(target_email) | ||
| if target_domain != organization_domain: | ||
| return False, f"Email domain {target_domain} does not match organization domain {organization_domain}" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate and normalize target_email using email-validator before domain check.
Avoids false positives and garbage domain extraction.
Apply:
- # Validate target email domain matches organization
- target_domain = extract_email_domain(target_email)
+ # Validate and normalize target email, then check domain
+ from email_validator import validate_email, EmailNotValidError
+ try:
+ target_email = validate_email(target_email).normalized
+ except EmailNotValidError as e:
+ return False, f"Invalid target email: {e}"
+ target_domain = extract_email_domain(target_email)
if target_domain != organization_domain:
return False, f"Email domain {target_domain} does not match organization domain {organization_domain}"📝 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.
| # Validate target email domain matches organization | |
| target_domain = extract_email_domain(target_email) | |
| if target_domain != organization_domain: | |
| return False, f"Email domain {target_domain} does not match organization domain {organization_domain}" | |
| # Validate and normalize target email, then check domain | |
| from email_validator import validate_email, EmailNotValidError | |
| try: | |
| target_email = validate_email(target_email).normalized | |
| except EmailNotValidError as e: | |
| return False, f"Invalid target email: {e}" | |
| target_domain = extract_email_domain(target_email) | |
| if target_domain != organization_domain: | |
| return False, f"Email domain {target_domain} does not match organization domain {organization_domain}" |
🤖 Prompt for AI Agents
In api/auth/organization_management.py around lines 277-281, the code extracts
the domain from target_email without validating it; validate and normalize
target_email using the email-validator library before domain extraction to avoid
garbage domains. Import validate_email and EmailNotValidError, call
validate_email(target_email) (catching EmailNotValidError) to get the normalized
email, extract the domain from that normalized address (lowercased), and compare
it to a normalized organization_domain; if validation fails return False with a
clear invalid-email message, otherwise proceed with the domain equality check.
| is_new_org, domain = check_or_create_organization(email) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against None domain to avoid no-op linking.
check_or_create_organization can return (False, None). The membership block unconditionally tries to link with a None domain, which silently fails and logs a generic error. Guard and exit early.
Apply:
- # Handle organization membership and role assignment
- if is_new_org:
- # User is first in their domain, make them admin
- success = link_user_to_organization(email, domain, is_admin=True, is_pending=False)
- if success:
- # Update user role to admin in the User node
- from api.auth.organization_management import update_user_role_direct
- update_user_role_direct(email, "admin")
- logging.info("User %s created organization %s and assigned as admin",
- email, domain)
- else:
- # Organization exists, check if user needs to be linked
- # For existing organizations, new users are pending until approved
- success = link_user_to_organization(email, domain, is_admin=False, is_pending=True)
- if success:
- logging.info("User %s added to existing organization %s as pending",
- email, domain)
+ # Handle organization membership and role assignment
+ if not domain:
+ logging.error("Skipping organization linking for %s due to invalid domain", email)
+ elif is_new_org:
+ # User is first in their domain, make them admin
+ success = link_user_to_organization(email, domain, is_admin=True, is_pending=False)
+ if success:
+ # Update user role to admin in the User node
+ from api.auth.organization_management import update_user_role_direct
+ update_user_role_direct(email, "admin")
+ logging.info("User %s created organization %s and assigned as admin",
+ email, domain)
+ else:
+ # Organization exists; new users are pending until approved
+ success = link_user_to_organization(email, domain, is_admin=False, is_pending=True)
+ if success:
+ logging.info("User %s added to existing organization %s as pending",
+ email, domain)Also applies to: 111-128
🤖 Prompt for AI Agents
In api/auth/user_management.py around lines 53-54 (and similarly lines 111-128),
check_or_create_organization may return (False, None) so the membership/linking
code must guard against a None domain; add an early check like "if not domain:
log a clear message and return/skip the membership linking flow" (or raise an
appropriate error) before calling any link functions so you don't call them with
None and avoid the silent failure and generic error log.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
api/static/css/modals.css (2)
528-595: Remove duplicate user list and form rules to prevent specificity conflictsThis block duplicates earlier definitions (Lines 412-471), causing unpredictable styling. Keep the earlier, richer set and remove this duplicate.
Apply:
-.users-list { - border: 1px solid var(--border-color); - border-radius: 6px; - margin: 15px 0; - background-color: var(--falkor-quaternary); -} - -.user-item { - display: flex; - justify-content: space-between; - align-items: center; - padding: 15px; - border-bottom: 1px solid var(--border-color); -} - -.user-item:last-child { - border-bottom: none; -} - -.user-info { - flex-grow: 1; -} - -.user-info strong { - color: var(--text-primary); -} - -.user-email { - color: var(--text-secondary); - font-size: 14px; -} - -.user-role { - color: var(--falkor-accent); - font-size: 12px; - text-transform: uppercase; - font-weight: bold; -} - -.user-actions { - margin-left: 15px; - display: flex; - gap: 10px; - align-items: center; -} - -.add-user-form { - display: flex; - gap: 10px; - margin: 15px 0; - flex-wrap: wrap; -} - -.add-user-form input { - flex: 1; - min-width: 250px; - padding: 8px 12px; - border: 1px solid var(--border-color); - border-radius: 4px; - background-color: var(--falkor-quaternary); - color: var(--text-primary); -} - -.help-text { - color: var(--text-secondary); - font-size: 14px; - font-style: italic; -}
471-483: Consolidate duplicate .role-select styles
.role-selectis defined twice. Keep the earlier, focused rules (Lines 471-483) and drop the later block (Lines 648-666).Apply:
-.role-select { - padding: 4px 8px; - border: 1px solid var(--border-color); - border-radius: 4px; - font-size: 12px; - background-color: var(--falkor-quaternary); - color: var(--text-primary); - cursor: pointer; -} - -.role-select:hover { - border-color: var(--falkor-primary); -} - -.role-select:focus { - outline: none; - border-color: var(--falkor-primary); - box-shadow: 0 0 0 2px rgba(91, 107, 192, 0.25); -}Also applies to: 648-666
🧹 Nitpick comments (1)
api/static/css/modals.css (1)
597-607: Scope button utility classes to the organization modal to avoid global bleedThese
.btn*utilities are global and may override styles app-wide. Scope them under#organization-modalto localize impact.Apply:
-.organization-info .btn, -.admin-actions .btn, -.user-actions .btn, -.add-user-form .btn { +#organization-modal .organization-info .btn, +#organization-modal .admin-actions .btn, +#organization-modal .user-actions .btn, +#organization-modal .add-user-form .btn { padding: 8px 16px; border: none; border-radius: 4px; cursor: pointer; font-size: 14px; transition: background-color 0.2s; } -.btn-primary { +#organization-modal .btn-primary { background-color: var(--falkor-primary); color: white; } -.btn-primary:hover { +#organization-modal .btn-primary:hover { background-color: var(--falkor-border-primary); opacity: 0.8; } -.btn-secondary { +#organization-modal .btn-secondary { background-color: var(--text-secondary); color: white; } -.btn-secondary:hover { +#organization-modal .btn-secondary:hover { background-color: var(--text-primary); } -.btn-success { +#organization-modal .btn-success { background-color: #28a745; color: white; } -.btn-success:hover { +#organization-modal .btn-success:hover { background-color: #1e7e34; } -.btn-sm { +#organization-modal .btn-sm { padding: 4px 8px; font-size: 12px; } -.loading { +#organization-modal .loading { text-align: center; padding: 20px; color: var(--text-secondary); }Also applies to: 609-647
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Pipfile(1 hunks)api/app_factory.py(3 hunks)api/static/css/modals.css(2 hunks)api/static/js/app.js(2 hunks)api/templates/chat.j2(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Pipfile
- api/static/js/app.js
- api/app_factory.py
🧰 Additional context used
📓 Path-based instructions (2)
api/templates/**
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Store Jinja2 templates under api/templates/
Files:
api/templates/chat.j2
api/static/**
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Store frontend static assets under api/static/
Files:
api/static/css/modals.css
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (1)
api/templates/chat.j2 (1)
34-36: Confirm modal triggers are only rendered for authenticated usersWe’ve verified that:
openOrganizationModal/closeOrganizationModalare imported and attached towindowinapi/static/js/app.js(lines 19, 84–85), and their unguarded calls todocument.getElementById('organization-modal')would only error if ever invoked without the modal in the DOM.- The only in-template trigger for these functions is the “Organization” button in
api/templates/components/user_profile.j2(line 12).- The modal itself (
components/organization_modal.j2) is only included inchat.j2whenis_authenticatedis true (lines 34–36).Please confirm that
components/user_profile.j2—and thus the Organization button—are only ever included under the sameis_authenticatedguard (e.g. in your base layout or wherever user_profile.j2 is pulled in). Once that’s ensured, the JS bindings remain safe on unauthenticated views.
| .user-profile-action { | ||
| width: 100%; | ||
| padding: 10px; | ||
| background: var(--falkor-quaternary); | ||
| color: var(--text-primary); | ||
| border: none; | ||
| border-radius: 6px; | ||
| cursor: pointer; | ||
| font-weight: bold; | ||
| transition: background 0.2s; | ||
| } | ||
|
|
||
| .user-profile-action:hover { | ||
| background: var(--falkor-accent); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Class rename to .user-profile-action: verify template usage is updated
The new singular class naming is fine. Please verify templates no longer reference the old .user-profile-actions.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find old vs new profile action classes in templates"
rg -n -C2 -g 'api/templates/**' '\.user-profile-action(s)?\b'Length of output: 196
I didn’t find any references because we were searching for “.user-profile-action” (with a leading dot), but HTML templates use class names without the dot. Let’s search for both singular and plural usages across all templates:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for references to user-profile-action and user-profile-actions in templates"
rg -n -C2 -g 'api/templates/**' 'user-profile-action'Length of output: 847
Update template to use singular .user-profile-action
The plural container class is still referenced in your template. Please update it to match the CSS:
• File: api/templates/components/user_profile.j2
Line 11: change
<div class="user-profile-actions">to
<div class="user-profile-action">Then verify there are no remaining .user-profile-actions usages in other templates.
🤖 Prompt for AI Agents
In api/templates/components/user_profile.j2 around line 11, the template uses
the plural class "user-profile-actions" but the CSS defines the singular
".user-profile-action"; update the div class from user-profile-actions to
user-profile-action, then search the templates directory for any remaining
usages of "user-profile-actions" and replace them with "user-profile-action" to
ensure consistency.
| #organization-modal { | ||
| display: none; | ||
| position: fixed; | ||
| z-index: 4000; | ||
| left: 0; | ||
| top: 0; | ||
| width: 100vw; | ||
| height: 100vh; | ||
| background-color: rgba(0,0,0,0.7); | ||
| overflow-y: auto; | ||
| } | ||
|
|
||
| #organization-modal .modal-content { | ||
| background: linear-gradient(135deg, var(--falkor-secondary) 80%, var(--falkor-tertiary) 100%); | ||
| margin: 0 auto; | ||
| padding: 0; | ||
| border: none; | ||
| border-radius: 0; | ||
| width: 100vw; | ||
| min-height: 100vh; | ||
| max-width: 100vw; | ||
| max-height: none; | ||
| box-shadow: none; | ||
| display: flex; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid 100vw-induced horizontal overflow; prefer percentages
Using 100vw on fixed/fullscreen containers often causes horizontal scrollbars on OSes with visible scrollbars. Switch 100vw to 100% where possible.
Apply:
#organization-modal {
- width: 100vw;
- height: 100vh;
+ width: 100%;
+ height: 100%;
background-color: rgba(0,0,0,0.7);
}
#organization-modal .modal-content {
- width: 100vw;
+ width: 100%;
min-height: 100vh;
- max-width: 100vw;
+ max-width: 100%;
max-height: none;
}
#organization-modal .modal-body {
- width: 100vw;
- max-width: 100vw;
+ width: 100%;
+ max-width: 100%;
}Also applies to: 333-340
🤖 Prompt for AI Agents
In api/static/css/modals.css around lines 268 to 291 (and also apply the same
change to lines 333–340), the modal and modal-content use width: 100vw and
min-width/max-width/height values that can trigger horizontal overflow on
platforms with visible scrollbars; replace width: 100vw (and max-width: 100vw
where present) with width: 100% (or remove redundant max-width) and ensure
min-height stays 100vh only where needed, so the fixed fullscreen container uses
percentage widths to avoid creating horizontal scrollbars.
|
|
||
| if email_success: | ||
| logging.info("Admin %s added user %s to organization %s (pending) and sent invitation email", | ||
| admin_email, target_email, organization_domain) |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the log injection vulnerability, we need to sanitize the target_email value before it is logged. The recommended approach is to remove any newline (\n) and carriage return (\r) characters from the email address before including it in the log entry. This can be done by replacing these characters with an empty string using the replace() method. The best place to do this is immediately before logging, so that the log entry is safe regardless of how the value was processed earlier. Only the log statement needs to be changed; the rest of the code can remain as is to preserve existing functionality.
Specifically, in api/auth/organization_management.py, on line 347, sanitize target_email by replacing \n and \r with an empty string before passing it to the logger. The same should be done for any other user-controlled values being logged, but in this case, only target_email is tainted.
No new imports or method definitions are required, as the replace() method is built-in for strings.
| @@ -343,8 +343,10 @@ | ||
| ) | ||
|
|
||
| if email_success: | ||
| # Sanitize target_email to prevent log injection | ||
| safe_target_email = target_email.replace('\n', '').replace('\r', '') | ||
| logging.info("Admin %s added user %s to organization %s (pending) and sent invitation email", | ||
| admin_email, target_email, organization_domain) | ||
| admin_email, safe_target_email, organization_domain) | ||
| return True, f"User {target_email} has been added to organization and an invitation email has been sent" | ||
| else: | ||
| logging.warning("Admin %s added user %s to organization %s (pending) but failed to send email: %s", |
| return True, f"User {target_email} has been added to organization and an invitation email has been sent" | ||
| else: | ||
| logging.warning("Admin %s added user %s to organization %s (pending) but failed to send email: %s", | ||
| admin_email, target_email, organization_domain, email_message) |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the log injection vulnerability, we should sanitize the target_email value before logging it. Specifically, we should remove any newline (\n), carriage return (\r), and other control characters that could be used to manipulate the log file. The best way to do this is to define a helper function (e.g., sanitize_for_log) that strips or replaces these characters, and use it on target_email before passing it to any logging statement. This should be done directly in the add_user_to_organization_by_email function, just before the logging statements that use target_email. The helper function can be defined at the top of the file, and used wherever user input is logged.
| @@ -6,6 +6,13 @@ | ||
| from api.extensions import db | ||
|
|
||
|
|
||
| def sanitize_for_log(value: str) -> str: | ||
| """Sanitize user input for safe logging (remove newlines and control chars).""" | ||
| if not isinstance(value, str): | ||
| return str(value) | ||
| # Remove newlines, carriage returns, and other control characters | ||
| return value.replace('\r', '').replace('\n', '').replace('\x00', '').replace('\x1a', '') | ||
|
|
||
| def extract_email_domain(email: str) -> str: | ||
| """Extract domain from email address.""" | ||
| if not email or "@" not in email: | ||
| @@ -344,23 +351,23 @@ | ||
|
|
||
| if email_success: | ||
| logging.info("Admin %s added user %s to organization %s (pending) and sent invitation email", | ||
| admin_email, target_email, organization_domain) | ||
| admin_email, sanitize_for_log(target_email), organization_domain) | ||
| return True, f"User {target_email} has been added to organization and an invitation email has been sent" | ||
| else: | ||
| logging.warning("Admin %s added user %s to organization %s (pending) but failed to send email: %s", | ||
| admin_email, target_email, organization_domain, email_message) | ||
| admin_email, sanitize_for_log(target_email), organization_domain, email_message) | ||
| return True, f"User {target_email} has been added to organization but invitation email could not be sent: {email_message}" | ||
|
|
||
| except ImportError: | ||
| logging.info("Admin %s added user %s to organization %s (pending) - email service not available", | ||
| admin_email, target_email, organization_domain) | ||
| admin_email, sanitize_for_log(target_email), organization_domain) | ||
| return True, f"User {target_email} has been added to organization (email service not configured)" | ||
| else: | ||
| return False, "Failed to add user to organization" | ||
|
|
||
| except Exception as e: | ||
| logging.error("Error adding user %s to organization %s by admin %s: %s", | ||
| target_email, organization_domain, admin_email, e) | ||
| sanitize_for_log(target_email), organization_domain, admin_email, e) | ||
| return False, f"Error adding user: {str(e)}" | ||
|
|
||
|
|
|
|
||
| except ImportError: | ||
| logging.info("Admin %s added user %s to organization %s (pending) - email service not available", | ||
| admin_email, target_email, organization_domain) |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the log injection vulnerability, we should sanitize the target_email value before logging it. The best practice is to remove any newline (\n) and carriage return (\r) characters from user input before including it in log messages. This can be done by replacing these characters with an empty string. The fix should be applied directly in the logging statement on line 356 of api/auth/organization_management.py, ensuring that only sanitized values are logged. No changes to the logic or functionality are required, and the fix should be as minimal and targeted as possible.
| @@ -352,8 +352,10 @@ | ||
| return True, f"User {target_email} has been added to organization but invitation email could not be sent: {email_message}" | ||
|
|
||
| except ImportError: | ||
| # Sanitize target_email to prevent log injection | ||
| sanitized_target_email = target_email.replace('\r', '').replace('\n', '') | ||
| logging.info("Admin %s added user %s to organization %s (pending) - email service not available", | ||
| admin_email, target_email, organization_domain) | ||
| admin_email, sanitized_target_email, organization_domain) | ||
| return True, f"User {target_email} has been added to organization (email service not configured)" | ||
| else: | ||
| return False, "Failed to add user to organization" |
|
|
||
| if email_success: | ||
| logging.info("Admin %s approved user %s in organization %s and sent notification email", | ||
| admin_email, target_email, organization_domain) |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the log injection vulnerability, we should sanitize the target_email value before logging it. Specifically, we should remove any newline (\n, \r) and other control characters from the email string before it is passed to the logger. The best way to do this is to define a helper function (e.g., sanitize_for_log) that strips out problematic characters, and use it on target_email in all logging calls where user input is logged. This should be done in api/auth/organization_management.py in the approve_pending_user function, specifically before the logging calls on lines 431, 435, 440, and 447. The helper function can be defined at the top of the file.
| @@ -6,6 +6,12 @@ | ||
| from api.extensions import db | ||
|
|
||
|
|
||
| def sanitize_for_log(value: str) -> str: | ||
| """Sanitize user input for safe logging (remove newlines and control chars).""" | ||
| if not isinstance(value, str): | ||
| return str(value) | ||
| return value.replace('\r', '').replace('\n', '').replace('\x00', '') | ||
|
|
||
| def extract_email_domain(email: str) -> str: | ||
| """Extract domain from email address.""" | ||
| if not email or "@" not in email: | ||
| @@ -427,25 +433,29 @@ | ||
| organization_domain=organization_domain | ||
| ) | ||
|
|
||
| # Sanitize target_email for logging | ||
| safe_target_email = sanitize_for_log(target_email) | ||
| if email_success: | ||
| logging.info("Admin %s approved user %s in organization %s and sent notification email", | ||
| admin_email, target_email, organization_domain) | ||
| admin_email, safe_target_email, organization_domain) | ||
| return True, f"User {target_email} has been approved and notified via email" | ||
| else: | ||
| logging.warning("Admin %s approved user %s in organization %s but failed to send notification: %s", | ||
| admin_email, target_email, organization_domain, email_message) | ||
| admin_email, safe_target_email, organization_domain, email_message) | ||
| return True, f"User {target_email} has been approved but notification email could not be sent: {email_message}" | ||
|
|
||
| except ImportError: | ||
| safe_target_email = sanitize_for_log(target_email) | ||
| logging.info("Admin %s approved user %s in organization %s - email service not available", | ||
| admin_email, target_email, organization_domain) | ||
| admin_email, safe_target_email, organization_domain) | ||
| return True, f"User {target_email} has been approved (email service not configured)" | ||
| else: | ||
| return False, "User not found or not pending approval" | ||
|
|
||
| except Exception as e: | ||
| safe_target_email = sanitize_for_log(target_email) | ||
| logging.error("Error approving user %s in organization %s by admin %s: %s", | ||
| target_email, organization_domain, admin_email, e) | ||
| safe_target_email, organization_domain, admin_email, e) | ||
| return False, f"Error approving user: {str(e)}" | ||
|
|
||
|
|
| return True, f"User {target_email} has been approved and notified via email" | ||
| else: | ||
| logging.warning("Admin %s approved user %s in organization %s but failed to send notification: %s", | ||
| admin_email, target_email, organization_domain, email_message) |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the log injection vulnerability, we should sanitize the user-provided value (target_email) before logging it. Specifically, we should remove any newline characters (\n, \r\n) and carriage returns (\r) from the email address before it is used in any log entry. This can be done by replacing these characters with an empty string using the replace() method. The best place to do this is immediately before logging, so that the log entry is guaranteed to be safe regardless of upstream validation. Only the log statement(s) that use target_email need to be changed; the rest of the code can continue to use the original value.
Edit the log statement on line 436 in api/auth/organization_management.py to sanitize target_email before logging. If there are other log statements in the same function that use target_email, they should also be sanitized in the same way.
No new imports are required.
| @@ -432,8 +432,10 @@ | ||
| admin_email, target_email, organization_domain) | ||
| return True, f"User {target_email} has been approved and notified via email" | ||
| else: | ||
| # Sanitize target_email to prevent log injection | ||
| safe_target_email = target_email.replace('\r\n', '').replace('\n', '').replace('\r', '') | ||
| logging.warning("Admin %s approved user %s in organization %s but failed to send notification: %s", | ||
| admin_email, target_email, organization_domain, email_message) | ||
| admin_email, safe_target_email, organization_domain, email_message) | ||
| return True, f"User {target_email} has been approved but notification email could not be sent: {email_message}" | ||
|
|
||
| except ImportError: |
|
|
||
| except ImportError: | ||
| logging.info("Admin %s approved user %s in organization %s - email service not available", | ||
| admin_email, target_email, organization_domain) |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix this log injection vulnerability, all user-provided input that is logged should be sanitized to remove newline (\n), carriage return (\r), and other control characters that could be used to manipulate log entries. The best way to do this is to define a small helper function (e.g., sanitize_log_input) that strips or replaces these characters, and then apply it to target_email (and any other user-controlled values) before they are passed to the logger. This change should be made directly in api/auth/organization_management.py within the approve_pending_user function, specifically before the logging calls that use target_email. The helper function can be defined at the top of the file or near the other utility functions.
Required changes:
- Add a
sanitize_log_inputfunction to remove\rand\nfrom strings. - Sanitize
target_email(and optionallyadmin_emailandorganization_domainfor completeness) before logging in the relevant logging statements inapprove_pending_user. - No new dependencies are required.
| @@ -6,6 +6,12 @@ | ||
| from api.extensions import db | ||
|
|
||
|
|
||
| def sanitize_log_input(value: str) -> str: | ||
| """Sanitize input for logging by removing newlines and carriage returns.""" | ||
| if not isinstance(value, str): | ||
| return value | ||
| return value.replace('\r', '').replace('\n', '') | ||
|
|
||
| def extract_email_domain(email: str) -> str: | ||
| """Extract domain from email address.""" | ||
| if not email or "@" not in email: | ||
| @@ -437,8 +443,12 @@ | ||
| return True, f"User {target_email} has been approved but notification email could not be sent: {email_message}" | ||
|
|
||
| except ImportError: | ||
| logging.info("Admin %s approved user %s in organization %s - email service not available", | ||
| admin_email, target_email, organization_domain) | ||
| logging.info( | ||
| "Admin %s approved user %s in organization %s - email service not available", | ||
| sanitize_log_input(admin_email), | ||
| sanitize_log_input(target_email), | ||
| sanitize_log_input(organization_domain) | ||
| ) | ||
| return True, f"User {target_email} has been approved (email service not configured)" | ||
| else: | ||
| return False, "User not found or not pending approval" |
|
|
||
| logging.info( | ||
| "Invitation email sent successfully to %s for organization %s by admin %s", | ||
| recipient_email, organization_name, admin_email |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the log injection vulnerability, sanitize the recipient_email before logging it. Specifically, remove any newline (\n, \r) and other control characters that could be used to forge log entries. The best way is to define a helper function (e.g., sanitize_for_log) that strips these characters from any string before logging. Apply this function to recipient_email (and optionally to other user-provided values) in the logging statement in send_organization_invitation in api/helpers/email_service.py. The fix should be limited to the region where logging occurs, and the helper function should be defined in the same file for clarity and minimal impact.
| @@ -18,6 +18,14 @@ | ||
| mail: Optional[Any] = Mail() if FLASK_MAIL_AVAILABLE else None | ||
|
|
||
|
|
||
| def sanitize_for_log(value: str) -> str: | ||
| """Sanitize user input for safe logging (remove newlines and control chars).""" | ||
| if not isinstance(value, str): | ||
| return str(value) | ||
| # Remove CR, LF, and other control characters | ||
| return ''.join(c for c in value if c.isprintable() and c not in '\r\n') | ||
|
|
||
|
|
||
| def init_mail(app): | ||
| """Initialize Flask-Mail with the Flask app.""" | ||
| if not FLASK_MAIL_AVAILABLE or mail is None: | ||
| @@ -193,7 +201,7 @@ | ||
|
|
||
| logging.info( | ||
| "Invitation email sent successfully to %s for organization %s by admin %s", | ||
| recipient_email, organization_name, admin_email | ||
| sanitize_for_log(recipient_email), organization_name, admin_email | ||
| ) | ||
|
|
||
| return True, "Invitation email sent successfully" |
| except Exception as e: # pylint: disable=broad-except | ||
| logging.error( | ||
| "Failed to send invitation email to %s for organization %s: %s", | ||
| recipient_email, organization_name, str(e) |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the log injection vulnerability, sanitize the recipient_email value before logging it. Specifically, remove any newline (\n) and carriage return (\r) characters from the email address before passing it to the logger. This can be done by creating a sanitized version of recipient_email using the replace() method, and using this sanitized value in the log entry. The change should be made in api/helpers/email_service.py, in the exception handler on line 208-211, where the log entry is written. No new imports are required, as string replacement is a built-in Python operation.
| @@ -205,9 +205,10 @@ | ||
| ) | ||
| return False, "Email service not available" | ||
| except Exception as e: # pylint: disable=broad-except | ||
| sanitized_recipient_email = recipient_email.replace('\r', '').replace('\n', '') | ||
| logging.error( | ||
| "Failed to send invitation email to %s for organization %s: %s", | ||
| recipient_email, organization_name, str(e) | ||
| sanitized_recipient_email, organization_name, str(e) | ||
| ) | ||
| return False, f"Failed to send invitation email: {str(e)}" | ||
|
|
|
|
||
| logging.info( | ||
| "Approval notification email sent successfully to %s for organization %s", | ||
| recipient_email, organization_name |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the log injection vulnerability, we should sanitize the user-provided email (recipient_email) before logging it. Specifically, we should remove any newline (\n) and carriage return (\r) characters from the email string before it is passed to the logger. This can be done by replacing these characters with an empty string using the replace() method. The sanitization should be performed immediately before the logging statement in send_organization_approval_notification in api/helpers/email_service.py, on line 341. No new methods are needed; a simple inline sanitization is sufficient. No new imports are required.
| @@ -336,9 +336,11 @@ | ||
|
|
||
| mail.send(msg) | ||
|
|
||
| # Sanitize recipient_email to prevent log injection | ||
| sanitized_email = recipient_email.replace('\r', '').replace('\n', '') | ||
| logging.info( | ||
| "Approval notification email sent successfully to %s for organization %s", | ||
| recipient_email, organization_name | ||
| sanitized_email, organization_name | ||
| ) | ||
|
|
||
| return True, "Approval notification email sent successfully" |
| except Exception as e: # pylint: disable=broad-except | ||
| logging.error( | ||
| "Failed to send approval notification email to %s for organization %s: %s", | ||
| recipient_email, organization_name, str(e) |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the log injection vulnerability, we should sanitize the recipient_email value before logging it. The best practice is to remove any newline (\n) and carriage return (\r) characters from user input before including it in log messages. This can be done using the replace() method on the string. The fix should be applied directly in the send_organization_approval_notification function in api/helpers/email_service.py, just before logging the recipient_email value. No new imports are required, as string replacement is a built-in operation.
| @@ -350,8 +350,10 @@ | ||
| ) | ||
| return False, "Email service not available" | ||
| except Exception as e: # pylint: disable=broad-except | ||
| # Sanitize recipient_email to prevent log injection | ||
| sanitized_recipient_email = recipient_email.replace('\r', '').replace('\n', '') | ||
| logging.error( | ||
| "Failed to send approval notification email to %s for organization %s: %s", | ||
| recipient_email, organization_name, str(e) | ||
| sanitized_recipient_email, organization_name, str(e) | ||
| ) | ||
| return False, f"Failed to send approval notification email: {str(e)}" |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (11)
api/helpers/email_service.py (2)
194-196: Sanitize user-provided values before logging to prevent log injection.User-provided email addresses and organization names are being logged directly, which could allow log injection attacks if they contain newline characters.
Apply this diff to sanitize the values:
logging.info( - "Invitation email sent successfully to %s for organization %s by admin %s", - recipient_email, organization_name, admin_email + "Invitation email sent successfully to %s for organization %s by admin %s", + recipient_email.replace('\n', '').replace('\r', ''), + organization_name.replace('\n', '').replace('\r', ''), + admin_email.replace('\n', '').replace('\r', '') )logging.error( - "Failed to send invitation email to %s for organization %s: %s", - recipient_email, organization_name, str(e) + "Failed to send invitation email to %s for organization %s: %s", + recipient_email.replace('\n', '').replace('\r', ''), + organization_name.replace('\n', '').replace('\r', ''), + str(e) )Also applies to: 209-210
339-341: Sanitize user-provided values in approval notification logs.Similar log injection vulnerability exists in the approval notification function.
Apply this diff to sanitize the values:
logging.info( - "Approval notification email sent successfully to %s for organization %s", - recipient_email, organization_name + "Approval notification email sent successfully to %s for organization %s", + recipient_email.replace('\n', '').replace('\r', ''), + organization_name.replace('\n', '').replace('\r', '') )logging.error( - "Failed to send approval notification email to %s for organization %s: %s", - recipient_email, organization_name, str(e) + "Failed to send approval notification email to %s for organization %s: %s", + recipient_email.replace('\n', '').replace('\r', ''), + organization_name.replace('\n', '').replace('\r', ''), + str(e) )Also applies to: 354-355
api/auth/organization_management.py (9)
523-525: Extract shared role validation logic.Role validation logic is duplicated between
update_user_roleandupdate_user_role_direct.Create a shared validation function:
def _validate_role(role: str) -> bool: """Validate if a role is allowed.""" allowed_roles = ['user', 'admin', 'analyst', 'viewer', 'manager'] return role in allowed_rolesThen use it in both functions:
- # Validate role value - allowed_roles = ['user', 'admin', 'analyst', 'viewer', 'manager'] - if new_role not in allowed_roles: + # Validate role value + if not _validate_role(new_role): + allowed_roles = ['user', 'admin', 'analyst', 'viewer', 'manager'] return False, f"Invalid role '{new_role}'. Allowed roles: {', '.join(allowed_roles)}"Also applies to: 602-603
16-24: Fix return type hint to reflect that domain can be None.The function can return
(False, None)on failure, but the type hint declaresTuple[bool, str].Apply this diff to fix the type hint:
+from typing import Tuple, Optional, Dict, Any -from typing import Tuple, Optional, Dict, Any -def check_or_create_organization(user_email: str) -> Tuple[bool, str]: +def check_or_create_organization(user_email: str) -> Tuple[bool, Optional[str]]:
53-54: Improve organization name generation logic.The current logic
domain.split('.')[0].capitalize()could fail for domains without dots or produce poor names for subdomains.Apply this diff to improve organization name generation:
-# Generate organization name from domain (e.g., example.com -> Example) -organization_name = domain.split('.')[0].capitalize() +# Generate organization name from domain +domain_parts = domain.split('.') +if len(domain_parts) >= 2: + # Use the second-to-last part for standard domains (e.g., example.com -> Example) + organization_name = domain_parts[-2].capitalize() +else: + # Fallback for single-part domains + organization_name = domain_parts[0].capitalize() if domain_parts else domain
69-71: Fix error message to use correct variables.The error message references undefined variables
user_emailandnew_roleinstead of the actual parameters.Apply this diff to fix the error message:
except Exception as e: - logging.error("Error updating user %s: %s", user_email, e) + logging.error("Error managing organization for domain %s: %s", domain, e) return False, None
183-190: Extract Node to dictionary conversion logic to avoid duplication.This Node object conversion logic is duplicated in multiple functions.
Add this helper function at the module level and use it in all three locations:
def _node_to_dict(node) -> dict: """Convert a graph database Node object to a dictionary.""" if hasattr(node, 'properties'): return node.properties elif hasattr(node, '__dict__'): return {k: v for k, v in node.__dict__.items() if not k.startswith('_')} else: # Fallback for different Node implementations return dict(node) if node else {}Then replace the duplicated code with:
org_dict = _node_to_dict(org_data)Also applies to: 232-239, 478-485
280-284: Validate and normalize target_email before domain extraction.The code extracts the domain from target_email without proper validation, which could lead to issues with malformed emails.
Apply this diff to validate the email first:
- # Validate target email domain matches organization - target_domain = extract_email_domain(target_email) + # Validate and normalize target email, then check domain + from email_validator import validate_email, EmailNotValidError + try: + validated = validate_email(target_email) + target_email = validated.normalized + except EmailNotValidError as e: + return False, f"Invalid target email: {e}" + target_domain = extract_email_domain(target_email) if target_domain != organization_domain: return False, f"Email domain {target_domain} does not match organization domain {organization_domain}"
346-348: Sanitize user-provided values before logging.User-provided email addresses are being logged directly, creating log injection vulnerabilities.
Apply sanitization to all logging statements with user input:
logging.info("Admin %s added user %s to organization %s (pending) and sent invitation email", - admin_email, target_email, organization_domain) + admin_email.replace('\n', '').replace('\r', ''), + target_email.replace('\n', '').replace('\r', ''), + organization_domain.replace('\n', '').replace('\r', ''))Apply similar sanitization to lines 351, 356, and 363.
Also applies to: 350-352, 355-357, 362-364
431-432: Sanitize user-provided values in approval logs.Similar log injection vulnerabilities exist in the approval function.
Apply sanitization to all logging statements:
logging.info("Admin %s approved user %s in organization %s and sent notification email", - admin_email, target_email, organization_domain) + admin_email.replace('\n', '').replace('\r', ''), + target_email.replace('\n', '').replace('\r', ''), + organization_domain.replace('\n', '').replace('\r', ''))Apply similar sanitization to lines 436, 441, and 448.
Also applies to: 435-437, 440-441, 447-448
546-547: Sanitize remaining log injection vulnerabilities.Additional user-provided values need sanitization in logging statements.
Apply sanitization to all remaining vulnerable logging statements:
logging.info("Admin %s updated role of user %s to %s in organization %s", - admin_email, target_email, new_role, organization_domain) + admin_email.replace('\n', '').replace('\r', ''), + target_email.replace('\n', '').replace('\r', ''), + new_role.replace('\n', '').replace('\r', ''), + organization_domain.replace('\n', '').replace('\r', ''))Apply similar sanitization to lines 554, 584, 604, 623, 626, and 630.
Also applies to: 553-554, 583-584, 604-604, 623-623, 626-626, 630-630
🧹 Nitpick comments (2)
README.md (1)
31-40: Fix grammatical issues in configuration section.The formatting could be improved for better readability.
Apply this diff to improve the formatting:
- - **Required Configuration:** - - `FLASK_SECRET_KEY`: A secure secret key for Flask sessions - - `GOOGLE_CLIENT_ID` & `GOOGLE_CLIENT_SECRET`: For Google OAuth authentication - - `GITHUB_CLIENT_ID` & `GITHUB_CLIENT_SECRET`: For GitHub OAuth authentication - - `FALKORDB_HOST` & `FALKORDB_PORT`: FalkorDB connection settings - - **Optional Configuration:** - - Email settings (for organization invitations): `MAIL_*` variables - - AI/LLM settings: `AZURE_API_KEY`, `OPENAI_API_KEY`, etc. - - Analytics: `GOOGLE_TAG_MANAGER_ID` + + ### Required Configuration + - `FLASK_SECRET_KEY`: A secure secret key for Flask sessions + - `GOOGLE_CLIENT_ID` & `GOOGLE_CLIENT_SECRET`: For Google OAuth authentication + - `GITHUB_CLIENT_ID` & `GITHUB_CLIENT_SECRET`: For GitHub OAuth authentication + - `FALKORDB_HOST` & `FALKORDB_PORT`: FalkorDB connection settings + + ### Optional Configuration + - Email settings (for organization invitations): `MAIL_*` variables + - AI/LLM settings: `AZURE_API_KEY`, `OPENAI_API_KEY`, etc. + - Analytics: `GOOGLE_TAG_MANAGER_ID`api/auth/organization_management.py (1)
86-87: Remove trailing whitespace.These lines have trailing whitespace that should be removed according to the linter.
Apply this diff to remove trailing whitespace:
try: organizations_graph = db.select_graph("Organizations") - + # Update the user's relationship to set is_pending = False-def link_user_to_organization(user_email: str, organization_domain: str, is_admin: bool = False, is_pending: bool = False) -> bool: +def link_user_to_organization(user_email: str, organization_domain: str, + is_admin: bool = False, is_pending: bool = False) -> bool:if result.result_set: - logging.info("Linked user %s to organization %s (admin: %s, pending: %s)", + logging.info("Linked user %s to organization %s (admin: %s, pending: %s)", user_email, organization_domain, is_admin, is_pending)Also applies to: 111-111, 147-147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Pipfile.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.env.example(1 hunks)Pipfile(1 hunks)README.md(3 hunks)api/app_factory.py(3 hunks)api/auth/organization_management.py(1 hunks)api/helpers/email_service.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env.example
🚧 Files skipped from review as they are similar to previous changes (1)
- Pipfile
🧰 Additional context used
📓 Path-based instructions (3)
api/helpers/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Place shared utility functions under api/helpers/
Files:
api/helpers/email_service.py
api/auth/**/*.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Place authentication modules under api/auth/
Files:
api/auth/organization_management.py
api/app_factory.py
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Implement the application factory (including OAuth setup) in api/app_factory.py
Files:
api/app_factory.py
🧠 Learnings (1)
📚 Learning: 2025-08-19T08:18:37.470Z
Learnt from: CR
PR: FalkorDB/QueryWeaver#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-19T08:18:37.470Z
Learning: Applies to api/app_factory.py : Implement the application factory (including OAuth setup) in api/app_factory.py
Applied to files:
api/app_factory.py
🧬 Code Graph Analysis (2)
api/auth/organization_management.py (2)
api/static/js/modules/organization.js (3)
first_name(226-226)last_name(227-227)api/helpers/email_service.py (2)
send_organization_invitation(47-212)send_organization_approval_notification(215-357)
api/app_factory.py (1)
api/helpers/email_service.py (1)
init_mail(21-36)
🪛 GitHub Check: CodeQL
api/helpers/email_service.py
[failure] 196-196: Log Injection
This log entry depends on a user-provided value.
[failure] 210-210: Log Injection
This log entry depends on a user-provided value.
[failure] 341-341: Log Injection
This log entry depends on a user-provided value.
[failure] 355-355: Log Injection
This log entry depends on a user-provided value.
api/auth/organization_management.py
[failure] 347-347: Log Injection
This log entry depends on a user-provided value.
[failure] 351-351: Log Injection
This log entry depends on a user-provided value.
[failure] 356-356: Log Injection
This log entry depends on a user-provided value.
[failure] 363-363: Log Injection
This log entry depends on a user-provided value.
[failure] 432-432: Log Injection
This log entry depends on a user-provided value.
[failure] 436-436: Log Injection
This log entry depends on a user-provided value.
[failure] 441-441: Log Injection
This log entry depends on a user-provided value.
[failure] 448-448: Log Injection
This log entry depends on a user-provided value.
[failure] 547-547: Log Injection
This log entry depends on a user-provided value.
[failure] 547-547: Log Injection
This log entry depends on a user-provided value.
[failure] 554-554: Log Injection
This log entry depends on a user-provided value.
[failure] 584-584: Log Injection
This log entry depends on a user-provided value.
[failure] 604-604: Log Injection
This log entry depends on a user-provided value.
[failure] 623-623: Log Injection
This log entry depends on a user-provided value.
[failure] 626-626: Log Injection
This log entry depends on a user-provided value.
[failure] 630-630: Log Injection
This log entry depends on a user-provided value.
🪛 GitHub Actions: Pylint
api/auth/organization_management.py
[warning] 87-87: Trailing whitespace (trailing-whitespace)
[warning] 111-111: Line too long (131/100) (line-too-long)
[warning] 147-147: Trailing whitespace (trailing-whitespace)
🪛 LanguageTool
README.md
[grammar] ~31-~31: There might be a mistake here.
Context: ... ``` Required Configuration: - FLASK_SECRET_KEY: A secure secret key for Flask sessions...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ...his feature is optional but recommended for better user experience. #### Email Ser...
(QB_NEW_EN)
[grammar] ~79-~79: There might be a mistake here.
Context: ...n your Gmail account 2. Generate an App Password: - Go to Google Account settings → Security...
(QB_NEW_EN)
[grammar] ~199-~199: There might be a mistake here.
Context: ...mins add new users to their organization - Approval notifications when pending us...
(QB_NEW_EN)
[grammar] ~200-~200: There might be a mistake here.
Context: ...ations** when pending users are approved - Professional HTML templates with organ...
(QB_NEW_EN)
🪛 GitHub Actions: Spellcheck
README.md
[warning] 1-1: Spellcheck detected misspelled tokens in README.md: html>body>p, html>body>h5, html>body>ol>li.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e-tests
- GitHub Check: e2e-tests
🔇 Additional comments (7)
api/app_factory.py (3)
16-17: LGTM! Clean integration of organization management components.The imports and initialization of the email service follow the coding guidelines and integrate well with the new organization management features.
Also applies to: 64-64
67-70: LGTM! Blueprint registrations updated correctly.The replacement of
auth_bpwithmain_bpand addition oforganization_bpalign with the PR's restructuring goals. The blueprint registration order is logical.
79-79: LGTM! OAuth redirect updated consistently.The error handler correctly redirects to the new
main.homeendpoint instead of the removedauth.home.README.md (2)
66-131: LGTM! Comprehensive email configuration documentation.The email configuration section is well-structured with clear examples for multiple SMTP providers and important security notes. The documentation effectively explains how email integrates with the organization management features.
183-211: LGTM! Clear organization management documentation.The organization management section clearly explains the features and workflow. The cross-references to email configuration help users understand the integration between features.
api/helpers/email_service.py (2)
8-18: LGTM! Graceful handling of Flask-Mail availability.The module correctly handles the optional Flask-Mail dependency with proper fallback behavior.
21-36: LGTM! Well-implemented email initialization.The function properly configures Flask-Mail from environment variables with appropriate defaults and logging.
Summary by CodeRabbit
New Features
Improvements
Chores
Docs