feat-2187: add additional default roles while bootstrap#2188
Conversation
Review FeedbackHi @KKNithin, thank you for this feature addition. I've done a deep review and found some issues that need to be addressed before merging. Critical Bug: Missing Early ExitThe code logs a warning when the file doesn't exist but still tries to open it: # bootstrap_db.py lines 276-280
if not additonal_default_roles_path.exists():
logger.warning(f"Catalog file not found: {additonal_default_roles_path}")
# BUG: No return/continue here!
with open(additonal_default_roles_path, "r", encoding="utf-8") as f: # Will fail!Compare to the correct pattern already in the codebase at if not catalog_path.exists():
logger.warning(f"Catalog file not found: {catalog_path}")
return {"catalog_servers": [], "categories": [], "auth_types": []} # Returns early!Fix: Add early exit after the warning (e.g., don't try to open the file if it doesn't exist). Invalid JSON Example in .env.exampleThe example JSON uses Python syntax, not valid JSON: # Current (INVALID JSON):
# "is_system_role": True, # Python True, not JSON true
# }, # Trailing comma not allowed in JSONFix: Use valid JSON syntax: [{
"name": "example_role_1",
"description": "Read-only access to resources",
"scope": "team",
"permissions": ["teams.join", "tools.read", "resources.read"],
"is_system_role": true
}]If a user copies the current example, their JSON file will fail to parse with a confusing error. Other Issues
What Works Well
Please address these issues and I'll re-review. Let me know if you have any questions! |
|
@crivetimihai I have resolved your code review comments can you please review it for approval |
There was a problem hiding this comment.
@crivetimihai The functionality of adding roles worked when I tested by updating the .env variables and crated the role JSON.
However, the new roles can't be used without more changes to the RBAC system which is hardcoded to look for roles in EmailTeamMember rather than UserRoles. Team member management also does not link them to the Roles in the code.
Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
Fixes identified by code review: 1. Path resolution: Fixed parent.parent.parent -> parent.parent to correctly resolve project root from mcpgateway/bootstrap_db.py 2. JSON validation: Added validation that loaded JSON is a list of dicts with required keys (name, scope, permissions). Invalid entries are skipped with warnings instead of crashing bootstrap. 3. Improved logging: Log all attempted paths when file not found Added tests: - test_bootstrap_roles_with_dict_instead_of_list: Validates error when JSON is a dict instead of array - test_bootstrap_roles_with_missing_required_keys: Validates warning when roles are missing required fields Added documentation: - docs/docs/manage/rbac.md: New "Bootstrap Custom Roles" section with configuration examples for Docker Compose and Kubernetes - docs/docs/architecture/adr/036-bootstrap-custom-roles.md: ADR documenting the feature design, error handling, and security considerations Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
ChatGPT review identified that description and is_system_role were accessed
unconditionally via role_def["key"], causing KeyError for minimal roles.
Fix:
- Use role_def.get("description", "") with empty string default
- Use role_def.get("is_system_role", False) with False default
Added test:
- test_bootstrap_roles_with_minimal_valid_role: Verifies a role with only
required fields (name, scope, permissions) is created successfully with
correct defaults for optional fields
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
3fa3765 to
1bb7207
Compare
PR Review & Fixes SummaryThis PR has been rebased onto main and includes several fixes identified during code review. Original FeatureLoad custom RBAC roles from a JSON configuration file during database bootstrap, allowing organizations to pre-configure roles before deployment. Fixes Applied1. Path Resolution Bug (Medium)
2. JSON Structure Validation (Medium)
3. Optional Fields Bug (Medium)
Tests Added
Documentation Added
CommitsTest ResultsAll 35 bootstrap_db tests pass. Docker Compose TestingVerified end-to-end:
|
* feat-2187: add additional default roles while bootstrap
Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
* feat-2187: fix lint issues
Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
* feat-2187: fixing review comments
Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
* feat-2187: fixing review comments
Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
* feat-2187: test fix
Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
* fix: Improve bootstrap roles validation and documentation
Fixes identified by code review:
1. Path resolution: Fixed parent.parent.parent -> parent.parent to correctly
resolve project root from mcpgateway/bootstrap_db.py
2. JSON validation: Added validation that loaded JSON is a list of dicts with
required keys (name, scope, permissions). Invalid entries are skipped with
warnings instead of crashing bootstrap.
3. Improved logging: Log all attempted paths when file not found
Added tests:
- test_bootstrap_roles_with_dict_instead_of_list: Validates error when JSON is
a dict instead of array
- test_bootstrap_roles_with_missing_required_keys: Validates warning when roles
are missing required fields
Added documentation:
- docs/docs/manage/rbac.md: New "Bootstrap Custom Roles" section with
configuration examples for Docker Compose and Kubernetes
- docs/docs/architecture/adr/036-bootstrap-custom-roles.md: ADR documenting
the feature design, error handling, and security considerations
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
* fix: Make description and is_system_role optional for bootstrap roles
ChatGPT review identified that description and is_system_role were accessed
unconditionally via role_def["key"], causing KeyError for minimal roles.
Fix:
- Use role_def.get("description", "") with empty string default
- Use role_def.get("is_system_role", False) with False default
Added test:
- test_bootstrap_roles_with_minimal_valid_role: Verifies a role with only
required fields (name, scope, permissions) is created successfully with
correct defaults for optional fields
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
---------
Signed-off-by: Nithin Katta <Nithin.Katta@ibm.com>
Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Co-authored-by: Nithin Katta <Nithin.Katta@ibm.com>
Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
resolves new feature request #2187
Closes #2187