Conversation
github-actions
Bot
added
docs
docker
New Migration
unittests
integration_tests
settings_changes
apiv2
ui
labels
Review Summary by QodoMigrate UI from Bootstrap to Tailwind CSS with classic template library and remove group/SSO functionality
WalkthroughsDescription• Complete redesign of the UI from Bootstrap/SB Admin 2 to modern Tailwind CSS with Alpine.js and htmx • Migrated base template (base.html) from Bootstrap to Tailwind with sticky topbar, collapsible sidebar, and native HTML dialogs • Created comprehensive classic UI templates in dojo/templates_classic/ directory including base layout, engagement view, test view, finding view, metrics dashboard, and product pages • Added new templates for error pages, template management, and webhook notifications • Removed jQuery UI, Bootstrap Select, and MetisMenu dependencies; replaced with vanilla JavaScript and Alpine.js • Removed group management functionality and SSO-related code (views, middleware, context processors, SAML/OIDC configurations) • Updated form styling by removing form-horizontal class from profile and user forms • Implemented extensive JavaScript functionality for keyboard shortcuts, JIRA integration, and interactive features across multiple views Diagramflowchart LR
A["Bootstrap/SB Admin 2<br/>jQuery UI"] -->|"Redesign"| B["Tailwind CSS<br/>Alpine.js + htmx"]
C["Single base.html"] -->|"Separate"| D["Modern base.html<br/>+ Classic templates"]
E["Group Management<br/>SSO Features"] -->|"Remove"| F["Simplified Auth"]
B --> G["New UI"]
D --> G
F --> G
File Changes1. dojo/templates/base.html
|
Code Review by Qodo
1. Role.Meta sets app_label
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+24
to
+27
| class Meta: | ||
| app_label = "dojo" | ||
| db_table = "dojo_role" | ||
| managed = False |
There was a problem hiding this comment.
1. role.meta sets app_label 📘 Rule violation ≡ Correctness
Extracted models in dojo/authorization/models.py explicitly set Meta.app_label, which violates the extraction rule and can cause Django model discovery/migration inconsistencies. This should rely on the default dojo app inheritance instead of forcing app_label.
Agent Prompt
## Issue description
Extracted models in `dojo/authorization/models.py` explicitly set `Meta.app_label`, which is disallowed by the extraction compliance rules.
## Issue Context
Django should infer the `dojo` app label automatically for models inside the `dojo` app; forcing `app_label` can introduce discovery/migration issues.
## Fix Focus Areas
- dojo/authorization/models.py[24-27]
- dojo/authorization/models.py[45-48]
- dojo/authorization/models.py[64-67]
- dojo/authorization/models.py[75-78]
- dojo/authorization/models.py[86-89]
- dojo/authorization/models.py[97-100]
- dojo/authorization/models.py[108-111]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+4521
to
+4529
| from dojo.authorization.models import ( # noqa: E402 | ||
| Dojo_Group_Member, | ||
| Global_Role, | ||
| Product_Group, | ||
| Product_Member, | ||
| Product_Type_Group, | ||
| Product_Type_Member, | ||
| Role, | ||
| ) |
There was a problem hiding this comment.
2. dojo.models registers auth models 📘 Rule violation ⚙ Maintainability
Admin registrations for extracted authorization models are still being maintained from
dojo/models.py, which violates the requirement to keep module admin registrations in
dojo/{module}/admin.py. This increases the risk of double-registration and breaks the intended
module boundaries.
Agent Prompt
## Issue description
Authorization model admin registrations are still effectively anchored in `dojo/models.py` via a new import block, rather than being located in `dojo/authorization/admin.py`.
## Issue Context
Per the modular extraction rules, each module must own its Django admin registrations to avoid double-registration and to keep a clean module boundary.
## Fix Focus Areas
- dojo/models.py[4521-4533]
- dojo/authorization/__init__.py[1-11]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1
to
+11
| # Import query_registrations to trigger RBAC filter registration at startup | ||
| from dojo.authorization import query_registrations # noqa: F401 | ||
| from dojo.authorization.authorization import ( # noqa: F401 | ||
| user_has_configuration_permission, | ||
| user_has_global_permission, | ||
| user_has_global_permission_or_403, | ||
| user_has_permission, | ||
| user_has_permission_or_403, | ||
| user_is_superuser_or_global_owner, | ||
| ) | ||
| from dojo.authorization.roles_permissions import Permissions, Roles # noqa: F401 |
There was a problem hiding this comment.
3. authorization.init lacks admin import 📘 Rule violation ☼ Reliability
dojo/authorization/__init__.py does not import dojo.authorization.admin, breaking the required import chain used for model discovery in extracted modules. This can prevent Django from consistently discovering module models/admin registrations.
Agent Prompt
## Issue description
`dojo/authorization/__init__.py` does not import the module’s `admin.py`, which breaks the expected discovery/import chain.
## Issue Context
The canonical pattern (e.g., `dojo/url/__init__.py`) imports `dojo.{module}.admin` to ensure model/admin discovery.
## Fix Focus Areas
- dojo/authorization/__init__.py[1-11]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+137
to
+162
| "view_endpoint": [("object", Endpoint, "view", "eid")], | ||
| "view_endpoint_host": [("object", Endpoint, "view", "eid")], | ||
| "edit_endpoint": [("object", Endpoint, "edit", "eid")], | ||
| "add_endpoint": [("object", Product, "add", "pid")], | ||
| "delete_endpoint": [("object", Endpoint, "delete", "eid")], | ||
| "add_endpoint_meta_data": [("object", Endpoint, "edit", "eid")], | ||
| "edit_endpoint_meta_data": [("object", Endpoint, "edit", "eid")], | ||
| "endpoints_status_bulk": [("object", Finding, "edit", "fid")], | ||
| "import_endpoint_meta": [("object", Product, "edit", "pid")], | ||
| "endpoint_report": [("object", Endpoint, "view", "eid")], | ||
| "endpoint_host_report": [("object", Endpoint, "view", "eid")], | ||
|
|
||
| # ----------------------------------------------------------------------- | ||
| # URL / Location UI (dojo/url/ui/views.py -> dojo/url/ui/urls.py) | ||
| # | ||
| # These URL names overlap with the endpoint module above. Since Django | ||
| # uses the last-registered pattern for reverse() and the middleware reads | ||
| # view_kwargs from the matched pattern, the kwarg names from the actually | ||
| # matched URL are used. The endpoint entries above use "eid"; if the | ||
| # url/ui pattern matched instead, "location_id" will be present and the | ||
| # middleware will fall back (skip checks where the kwarg is missing). | ||
| # | ||
| # Unique URL names from url/ui: | ||
| # ----------------------------------------------------------------------- | ||
| "add_endpoint_to_product": [("object", Product, "add", "product_id")], | ||
| "add_endpoint_to_finding": [("object", Product, "add", "finding_id")], |
There was a problem hiding this comment.
4. Endpoint ui auth bypass 🐞 Bug ⛨ Security
When V3 locations are enabled, endpoint UI URL names (e.g., view_endpoint/delete_endpoint) resolve with kwargs like location_id/product_id, but URL_PERMISSIONS expects eid/pid and AuthorizationMiddleware skips the check when the kwarg is missing, leaving these views effectively without object authorization. This can expose findings via view_endpoint and allow destructive actions like delete_endpoint for any authenticated user.
Agent Prompt
### Issue description
`AuthorizationMiddleware` + `URL_PERMISSIONS` do not correctly authorize endpoint/location UI routes when `V3_FEATURE_LOCATIONS` is enabled. The middleware silently skips object checks when the expected kwarg is absent, and `URL_PERMISSIONS` uses legacy kwarg names (`eid`/`pid`) for URL names that (under V3) resolve with `location_id`/`product_id`, leaving sensitive views (including delete) without object authorization.
### Issue Context
- Under `V3_FEATURE_LOCATIONS`, `dojo/urls.py` includes `dojo/url/ui/urls.py` instead of `dojo/endpoint/urls.py`.
- `dojo/url/ui/urls.py` uses `location_id` and `product_id` for URL names like `view_endpoint`, `delete_endpoint`, `add_endpoint_meta_data`, etc.
- `dojo/url/ui/views.py` does not enforce object authorization itself for many of these operations.
### Fix Focus Areas
- dojo/authorization/middleware.py[19-48]
- dojo/authorization/url_permissions.py[134-163]
- dojo/url/ui/views.py[46-110]
- dojo/url/ui/views.py[325-410]
- dojo/url/ui/urls.py[24-52]
- dojo/urls.py[206-210]
### Expected fix
- Make object checks **fail closed** when a configured check cannot be evaluated (missing kwarg), OR support `arg_name` being a list/tuple and use the first kwarg present.
- Update `URL_PERMISSIONS` endpoint/location entries to correctly authorize V3 routes (e.g., use `Location` + `location_id` for `view_endpoint`/`delete_endpoint`/reports/meta-data, and `Product` + `product_id` for the legacy `add_endpoint` duplicate route).
- Add explicit `user_has_permission_or_403(...)` calls in `dojo/url/ui/views.py` for operations that mutate or disclose data (defense-in-depth), so misconfiguration in `URL_PERMISSIONS` cannot become an authorization bypass.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+161
to
+162
| "add_endpoint_to_product": [("object", Product, "add", "product_id")], | ||
| "add_endpoint_to_finding": [("object", Product, "add", "finding_id")], |
There was a problem hiding this comment.
5. Wrong model permission lookup 🐞 Bug ≡ Correctness
URL_PERMISSIONS defines add_endpoint_to_finding as a Product lookup by finding_id, so AuthorizationMiddleware authorizes against the wrong object (or returns 404) and does not reflect the Finding/Product actually being modified. This can break the route or allow/deny access incorrectly depending on ID collisions.
Agent Prompt
### Issue description
`URL_PERMISSIONS` uses the wrong model for the `add_endpoint_to_finding` URL name: it looks up a `Product` by `finding_id`, but `finding_id` is a `Finding` primary key.
### Issue Context
`AuthorizationMiddleware` performs object checks by doing `get_object_or_404(model, pk=view_kwargs[arg_name])` and then calls `user_has_permission_or_403` on that object. Using the wrong model means the permission check is performed on the wrong object (or becomes a 404).
### Fix Focus Areas
- dojo/authorization/url_permissions.py[159-163]
- dojo/authorization/middleware.py[42-48]
- dojo/url/ui/urls.py[32-36]
- dojo/url/ui/views.py[346-365]
### Expected fix
- Change the `add_endpoint_to_finding` entry to authorize against `Finding` with kwarg `finding_id` (since `user_has_permission` can derive the Product scope from a Finding), or introduce a dedicated check type that can resolve `Finding -> product` and then enforce `add` on the derived Product.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Maffooch
force-pushed
the
tailwind
branch
5 times, most recently
from
90cf026 to
43e7754
Compare
Hand the authorization layer off to dojo-pro. OS keeps a legacy ``is_superuser`` / ``is_staff`` / ``authorized_users`` model and the seven RBAC + Dojo_Group classes survive as ``managed=False`` shells in ``dojo/authorization/models.py`` so historical pro migrations (``pro.0001_plugiun_consolidation`` ``EnhancedDojoGroup.group``, ``pro.0034_pghistory_for_permissions_models`` proxy bases) keep resolving when Django reloads project state. OS code makes no runtime references to Pro. Single ``dojo.0268_release_authorization_to_pro`` migration folds: - Re-introduces ``authorized_users`` M2M on Product / Product_Type and backfills it from the RBAC tables (Member / Group → flat membership; Global_Role(Owner|Writer|Maintainer|API_Importer) → ``is_superuser`` / ``is_staff``). - Drops the redundant post-RBAC ``members`` / ``authorization_groups`` M2M accessors on Product / Product_Type (the through-tables remain). - Flips the eight authorization shells to ``managed=False`` and pins their ``db_table``s. - ``RemoveField``s ``default_group`` / ``default_group_role`` / ``default_group_email_pattern`` from ``dojo_system_settings`` (Pro copies the values onto ``EnhancedSystemSettings`` first via ``run_before``). Plus the Tailwind UI rebuild and the OS surface tidying that this branch was already carrying. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.