feat: mapping libraries autoload and various improvements and fixes #2831
Conversation
Detail library management for mappings
WalkthroughAdds an autoload flag to StoredLibrary (migration + data migration), sets autoload during content storage when mapping sets exist, introduces an autoload management command invoked at startup, refactors mapping-set APIs/serializers/views to operate on StoredLibrary content, exposes a mapping-libraries endpoint, updates frontend to show a Mappings tab and new table, and exports many table filter configs. Changes
Sequence Diagram(s)sequenceDiagram
participant Migrator
participant Startup
participant DB as StoredLibrary
participant Loader
Migrator->>DB: Add field autoload (0112)
Migrator->>DB: Set autoload=True where content has mapping sets (0113)
Startup->>Startup: run storelibraries
Startup->>Startup: run autoloadlibraries
autoloadlibraries->>DB: Query autoload=True, is_loaded=False
loop per library
autoloadlibraries->>Loader: lib.load()
Loader->>DB: set is_loaded=True (and keep/reflect autoload)
end
sequenceDiagram
actor Browser
Browser->>Libraries Page: Request libraries page
Libraries Page->>+API: GET /mapping-libraries?filters...
API->>DB: StoredLibrary.objects.filter(content has mapping sets, RBAC)
DB-->>API: mapping libraries JSON
API-->>-Browser: JSON
Browser->>ModelTable: render mappingLibrariesTable (uses PROVIDER_FILTER, LANGUAGE_FILTER)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🔇 Additional comments (3)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Comment |
nas-tabchiche
added
the
db-migration
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
nas-tabchiche
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/library/management/commands/storelibraries.py (3)
12-12: Avoid process-wide SIGINT override at import timeSetting SIGINT handler at module import affects all management commands and may run outside the main thread. Move it into handle() and restore previous handler afterward.
Apply:
-import signal +import signal +import threading @@ -signal.signal(signal.SIGINT, signal.SIG_DFL) @@ def handle(self, *args, **options): + prev_sigint = signal.getsignal(signal.SIGINT) + try: + if threading.current_thread() is threading.main_thread(): + signal.signal(signal.SIGINT, signal.SIG_DFL) StoredLibrary.__init_class__() path = Path(options.get("path") or LIBRARIES_PATH) if path.is_dir(): @@ - for fname in library_files: + for fname in library_files: # logger.info("Begin library file storage", filename=fname) try: library = StoredLibrary.store_library_file(fname, True) @@ - except: - logger.error("Invalid library file", filename=fname) + except Exception: + logger.exception("Invalid library file", filename=fname) + finally: + try: + signal.signal(signal.SIGINT, prev_sigint) + except Exception: + pass
26-26: Call Path.is_file()
is_fileis a method; without parentheses it always evaluates truthy and defeats the check.Apply:
- f for f in path.iterdir() if f.is_file and f.suffix == ".yaml" + f for f in path.iterdir() if f.is_file() and f.suffix == ".yaml"
40-41: Don’t swallow exception detailsUse logger.exception to include stacktrace, and avoid bare except.
Apply:
- except: - logger.error("Invalid library file", filename=fname) + except Exception: + logger.exception("Invalid library file", filename=fname)backend/library/utils.py (1)
130-156: Potential UnboundLocalError and invalid rows in mapping creationWhen a requirement node is missing,
target_requirementorsource_requirementis never assigned, but still used inRequirementMapping.objects.create(...).Apply:
def load( self, mapping_set: RequirementMappingSet, ): - try: - target_requirement = RequirementNode.objects.get( + target_requirement = None + source_requirement = None + try: + target_requirement = RequirementNode.objects.get( urn=self.data["target_requirement_urn"].lower(), default_locale=True ) except RequirementNode.DoesNotExist: logger.error( "Target requirement does not exist", error=self.data["target_requirement_urn"], ) - try: - source_requirement = RequirementNode.objects.get( + try: + source_requirement = RequirementNode.objects.get( urn=self.data["source_requirement_urn"].lower(), default_locale=True ) except RequirementNode.DoesNotExist: logger.error( "Source requirement does not exist", error=self.data["source_requirement_urn"], ) + if not target_requirement or not source_requirement: + logger.warn("Skipping mapping creation due to missing requirement(s)", + target=self.data.get("target_requirement_urn"), + source=self.data.get("source_requirement_urn")) + return None return RequirementMapping.objects.create( mapping_set=mapping_set, target_requirement=target_requirement, source_requirement=source_requirement, relationship=self.data["relationship"], annotation=self.data.get("annotation"), strength_of_relationship=self.data.get("strength_of_relationship"), rationale=self.data.get("rationale"), )
🧹 Nitpick comments (8)
frontend/messages/fr.json (1)
2536-2538: FR translation is fine; optional wording“Bibliothèques de mappings” is understandable. Optionally: “Bibliothèques de mapping” or “Bibliothèques de correspondances” for style consistency.
backend/library/utils.py (1)
686-693: Dependency skip logic may hide missing prerequisites for mappingsEarly-returning when requirement_mapping_set(s) exist means you won’t import dependencies even if declared, potentially leaving source/target frameworks absent.
Apply:
- if ( - not self._library.dependencies - or self._library.content.get( - "requirement_mapping_set", - self._library.content.get("requirement_mapping_sets"), - ) - is not None - ): + if not self._library.dependencies: return NoneIf the intent is to skip dependency import only for standalone mapping libraries that bundle required frameworks, guard explicitly with a feature flag or content marker.
backend/core/migrations/0113_autoload_mappingsets.py (1)
10-19: Improve error handling specificity.The broad
except Exceptionclause will catch all exceptions (including programming errors) and silently continue. Consider:
- Being more specific about which exceptions to catch (e.g., database-related errors)
- Re-raising the exception after logging, or at minimum logging it as an error with context
Apply this diff to improve error handling:
def autoload_mapping_libraries(apps, schema_editor): StoredLibrary = apps.get_model("core", "StoredLibrary") try: StoredLibrary.objects.filter( Q(content__requirement_mapping_set__isnull=False) | Q(content__requirement_mapping_sets__isnull=False) ).update(autoload=True) - except Exception: + except Exception as e: - logger.error("Error autoloading mapping sets", exc_info=True) + logger.error("Error autoloading mapping sets", error=str(e), exc_info=True) + raisefrontend/src/routes/(app)/(internal)/mapping-libraries/+server.ts (1)
6-22: Simplify endpoint construction.The conditional on line 7-8 is redundant. When
url.searchParamsis empty,toString()returns an empty string, making the ternary unnecessary.Apply this diff to simplify:
export const GET: RequestHandler = async ({ fetch, url }) => { - const endpoint = `${BASE_API_URL}/mapping-libraries/${ - url.searchParams ? '?' + url.searchParams.toString() : '' - }`; + const params = url.searchParams.toString(); + const endpoint = `${BASE_API_URL}/mapping-libraries/${params ? '?' + params : ''}`; const res = await fetch(endpoint); if (!res.ok) { error(res.status as NumericRange<400, 599>, await res.json()); } const data = await res.json(); return new Response(JSON.stringify(data), { headers: { 'Content-Type': 'application/json' } }); };backend/library/management/commands/autoloadlibraries.py (1)
10-10: Consider removing or relocating the module-level signal handler.Setting
SIGINTto default behavior at module import time can interfere with Django's signal handling and testing frameworks. If this is needed, consider moving it inside thehandlemethod.If the signal handler is necessary, apply this diff:
-signal.signal(signal.SIGINT, signal.SIG_DFL) - - class Command(BaseCommand): help = "Load autoloaded libraries into the database" def handle(self, *args, **options): + signal.signal(signal.SIGINT, signal.SIG_DFL) StoredLibrary.__init_class__()backend/core/serializers.py (2)
1930-1930: Consider removing commented-out code.The commented-out
libraryfield should be removed rather than left in the codebase, as it can lead to confusion about the current state of the API.Apply this diff to remove the commented code:
- # library = FieldsRelatedField(["name", "id"]) folder = FieldsRelatedField()
1955-1985: Refactor to eliminate code duplication.The
get_source_frameworkandget_target_frameworkmethods contain nearly identical logic. Extract the common pattern into a helper method to improve maintainability.Add this helper method and refactor both methods:
def _get_framework_from_mapping(self, obj, framework_urn_key): """Helper to extract framework data from mapping set content.""" mapping_set = obj.content.get( "requirement_mapping_sets", [obj.content.get("requirement_mapping_set", {})] ) if not mapping_set: return {"str": "Unknown", "urn": None} mapping_set = mapping_set[0] framework_urn = mapping_set.get(framework_urn_key) if not framework_urn: return {"str": "Unknown", "urn": None} framework_lib = StoredLibrary.objects.filter( content__framework__urn=framework_urn, content__framework__isnull=False, content__requirement_mapping_set__isnull=True, content__requirement_mapping_sets__isnull=True, ).first() if not framework_lib: return {"str": framework_urn, "urn": framework_urn} framework = framework_lib.content.get("framework", {}) return { "str": framework.get("name", framework.get("urn", "Unknown")), "urn": framework.get("urn"), } def get_source_framework(self, obj): return self._get_framework_from_mapping(obj, "source_framework_urn") def get_target_framework(self, obj): return self._get_framework_from_mapping(obj, "target_framework_urn")backend/core/views.py (1)
7243-7249: Optional: safer labels for categories and node values.Fallback to URN when name/description missing to avoid empty labels in UI; already covered in previous diff but calling it out if you split changes.
Also applies to: 7251-7261, 7263-7272
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
backend/core/mappings/engine.py(2 hunks)backend/core/migrations/0112_storedlibrary_autoload.py(1 hunks)backend/core/migrations/0113_autoload_mappingsets.py(1 hunks)backend/core/models.py(4 hunks)backend/core/serializers.py(1 hunks)backend/core/startup.py(1 hunks)backend/core/tests/test_models.py(3 hunks)backend/core/urls.py(2 hunks)backend/core/views.py(3 hunks)backend/library/management/commands/autoloadlibraries.py(1 hunks)backend/library/management/commands/storelibraries.py(1 hunks)backend/library/tests/test_store_library_content.py(1 hunks)backend/library/utils.py(2 hunks)backend/library/views.py(6 hunks)documentation/architecture/data-model.md(1 hunks)frontend/messages/en.json(1 hunks)frontend/messages/fr.json(1 hunks)frontend/src/lib/components/ModelTable/ModelTable.svelte(4 hunks)frontend/src/lib/utils/table.ts(88 hunks)frontend/src/routes/(app)/(internal)/libraries/+page.server.ts(4 hunks)frontend/src/routes/(app)/(internal)/libraries/+page.svelte(3 hunks)frontend/src/routes/(app)/(internal)/mapping-libraries/+server.ts(1 hunks)frontend/src/routes/(app)/(internal)/stored-libraries/[id=uuid]/+page.svelte(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
frontend/src/routes/(app)/(internal)/mapping-libraries/+server.ts (1)
frontend/src/lib/utils/constants.ts (1)
BASE_API_URL(4-8)
backend/core/migrations/0113_autoload_mappingsets.py (1)
backend/core/models.py (1)
StoredLibrary(242-375)
backend/core/tests/test_models.py (1)
backend/core/models.py (2)
LoadedLibrary(866-982)delete(969-982)
backend/core/mappings/engine.py (1)
backend/core/models.py (1)
StoredLibrary(242-375)
backend/core/serializers.py (2)
backend/core/serializer_fields.py (1)
FieldsRelatedField(26-84)backend/core/models.py (1)
StoredLibrary(242-375)
backend/library/management/commands/autoloadlibraries.py (2)
backend/core/models.py (1)
StoredLibrary(242-375)backend/library/management/commands/storelibraries.py (2)
Command(15-41)handle(21-41)
frontend/src/routes/(app)/(internal)/libraries/+page.server.ts (1)
frontend/src/lib/utils/constants.ts (1)
BASE_API_URL(4-8)
backend/core/views.py (4)
backend/library/serializers.py (1)
StoredLibrarySerializer(8-29)backend/core/models.py (2)
StoredLibrary(242-375)get_queryset(4136-4137)backend/library/views.py (6)
get_queryset(176-181)get_queryset(558-570)filter(46-50)provider(318-324)content(202-208)content(481-487)backend/core/serializers.py (1)
RequirementMappingSetReadSerializer(1929-1991)
backend/core/urls.py (1)
backend/library/views.py (1)
MappingLibrariesList(541-570)
backend/library/views.py (4)
backend/core/models.py (2)
LoadedLibrary(866-982)StoredLibrary(242-375)backend/library/utils.py (1)
LibraryImporter(440-797)backend/library/serializers.py (1)
StoredLibrarySerializer(8-29)backend/iam/models.py (4)
RoleAssignment(807-1094)get_accessible_object_ids(917-1015)Folder(68-316)get_root_folder(75-77)
backend/library/utils.py (1)
backend/library/views.py (2)
content(202-208)content(481-487)
backend/library/tests/test_store_library_content.py (1)
backend/core/models.py (2)
StoredLibrary(242-375)store_library_content(263-353)
⏰ 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). (8)
- GitHub Check: build_enterprise_frontend
- GitHub Check: build_community_frontend
- GitHub Check: build (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
🔇 Additional comments (22)
frontend/messages/en.json (1)
2577-2579: LGTM — key added and JSON fixedNew i18n key mappingLibraries is fine; en value reads well.
Confirm other locales include this key (fr.json already does).
backend/core/startup.py (1)
1170-1170: LGTM! Autoload libraries command integration looks correct.The placement of
autoloadlibrariesafterstorelibrariesis appropriate, as libraries must be stored before they can be autoloaded. This ensures that mapping libraries are automatically loaded during startup.backend/core/mappings/engine.py (1)
67-71: LGTM! Filter logic correctly targets mapping libraries.The filter appropriately narrows the scope to only loaded libraries containing requirement mapping sets, using both singular and plural field names to handle different library formats.
backend/core/urls.py (2)
8-12: LGTM! Import addition is correct.The import of
MappingLibrariesListfromlibrary.viewsis properly structured and aligns with the new mapping libraries endpoint.
189-192: LGTM! URL pattern correctly exposes the mapping libraries endpoint.The new route follows Django REST conventions and properly maps to the
MappingLibrariesListview.documentation/architecture/data-model.md (1)
1151-1153: LGTM! Documentation accurately describes autoload behavior.The documentation clearly explains the autoload field behavior, including its default value, the exception for requirement mapping sets, and the effect of unloading libraries.
frontend/src/routes/(app)/(internal)/stored-libraries/[id=uuid]/+page.svelte (2)
100-106: LGTM! Display logic correctly handles autoload libraries.The updated logic appropriately hides the import button for libraries that are already loaded or contain requirement mapping sets (which are autoloaded). This aligns with the backend autoload behavior.
133-137: LGTM! Accessibility improvement.The addition of
aria-label="load library"improves accessibility for screen readers.backend/core/migrations/0112_storedlibrary_autoload.py (1)
12-19: LGTM! Schema migration is correctly structured.The AddField operation properly adds the
autoloadBooleanField with an appropriate default value and clear help text.backend/library/tests/test_store_library_content.py (1)
140-155: LGTM! Comprehensive test coverage for autoload behavior.The tests effectively verify that the
autoloadfield is set correctly based on the presence of requirement mappings in the library content. The YAML fixtures are well-structured and the test assertions are clear.backend/core/tests/test_models.py (2)
1094-1094: Good refactor: Direct foreign key access is cleaner.Using
risk_matrix.librarydirectly is more straightforward than querying the LoadedLibrary model.
1248-1254: Filtering logic correctly excludes mapping libraries.The updated assertions properly filter out libraries containing requirement mappings, which aligns with the PR's separation of mapping libraries from regular libraries. The consistent pattern across all three test assertions is good.
Also applies to: 1284-1302
frontend/src/routes/(app)/(internal)/libraries/+page.svelte (2)
52-55: LGTM! Mappings tab follows established pattern.The new tab control is consistent with the existing stored and loaded library tabs.
93-104: Good implementation of the mappings table with appropriate filters.The combination of
URLModel="stored-libraries"withbaseEndpoint="mapping-libraries"effectively reuses the stored libraries schema while fetching from the dedicated mapping libraries endpoint. The addition of locale and provider filters is appropriate for mapping libraries.frontend/src/lib/components/ModelTable/ModelTable.svelte (1)
21-21: Well-structured addition of tableFilters prop.The new
tableFiltersprop is properly typed, has sensible defaults, and integrates cleanly with the existing filter logic using a fallback pattern. This allows tables to specify custom filters when not derived from the source.Also applies to: 82-82, 131-135, 275-275
frontend/src/routes/(app)/(internal)/libraries/+page.server.ts (1)
17-17: Consistent implementation of mapping libraries data loading.The new
mappingLibrariesTablefollows the established pattern for loading and preparing library data. Reusing thestored-librariesschema (viamakeHeadData) is appropriate since mapping libraries are a subset of stored libraries with the same structure.Also applies to: 24-24, 30-30, 37-37, 53-53, 71-75, 88-88
backend/core/views.py (2)
61-61: LGTM: import is fine.No issues with importing StoredLibrarySerializer here.
102-102: LGTM: NotFound import added appropriately.Used below in graph_data; good.
frontend/src/lib/utils/table.ts (4)
21-25: LGTM! Good refactoring.Exporting the
ListViewFilterConfiginterface enables type safety and reusability across the codebase.
68-1043: LGTM! Excellent refactoring.Converting filter configurations to exported constants promotes reusability and maintains consistency across the application. The pattern is applied uniformly throughout.
517-533: Endpoint changes verified in backend routing.Both updated endpoints have been confirmed:
threatsendpoint is registered inbackend/core/urls.py:34withThreatViewSetloaded-librariesendpoint is registered inbackend/core/urls.py:83withLoadedLibraryViewSetThe filter configurations are aligned with the existing backend API structure.
1452-1461: The review comment is incorrect. The backend already supports theproviderquery parameter.The
providerfield is a direct CharField on the model (defined in ReferentialObjectMixin at backend/core/models.py:101-103), and the backend's FilterSet explicitly configures it:provider = MultiStringFilter(field_name="provider")at backend/library/views.py:58. The RequirementMappingSetViewSet also includesproviderin its filterset_fields at backend/core/views.py:7175. The change fromlibrary__providertoprovideris correct becauseprovideris accessed directly on the StoredLibrary model, not through a related field traversal.Likely an incorrect or invalid review comment.
| + LoadedLibrary.objects.filter( | ||
| objects_meta__requirement_mapping_sets__isnull=True, | ||
| objects_meta__requirement_mapping_set__isnull=True, | ||
| dependencies=self, | ||
| ) | ||
| .distinct() | ||
| .count() | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Don't drop dependencies coming from mapping libraries.
The new objects_meta__requirement_mapping_set(s)__isnull=True guard means any loaded mapping library (which always has those keys populated) is no longer counted as a dependent. As soon as a mapping library depends on another library, that dependency is now invisible here, so we can delete the underlying library even though the mapping still needs it. That breaks the safety guarantee reference_count was providing. Please revert to counting all LoadedLibrary.objects.filter(dependencies=self) (or otherwise keep mapping libraries in the set).
- + LoadedLibrary.objects.filter(
- objects_meta__requirement_mapping_sets__isnull=True,
- objects_meta__requirement_mapping_set__isnull=True,
- dependencies=self,
- )
+ + LoadedLibrary.objects.filter(dependencies=self)📝 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.
| + LoadedLibrary.objects.filter( | |
| objects_meta__requirement_mapping_sets__isnull=True, | |
| objects_meta__requirement_mapping_set__isnull=True, | |
| dependencies=self, | |
| ) | |
| .distinct() | |
| .count() | |
| ) | |
| LoadedLibrary.objects.filter(dependencies=self) | |
| .distinct() | |
| .count() | |
| ) |
🤖 Prompt for AI Agents
In backend/core/models.py around lines 936 to 943, the new filters
objects_meta__requirement_mapping_sets__isnull=True and
objects_meta__requirement_mapping_set__isnull=True exclude mapping libraries
from the dependency count so mapping-library dependencies become invisible;
remove those isnull filters (or otherwise include mapping libraries) and revert
to counting all LoadedLibrary.objects.filter(dependencies=self). Ensure you
still call .distinct().count() so the original reference_count safety guarantee
is preserved.
| def get_source_framework(self, obj): | ||
| mapping_set = obj.content.get( | ||
| "requirement_mapping_sets", [obj.content.get("requirement_mapping_set", {})] | ||
| )[0] | ||
| framework_lib = StoredLibrary.objects.filter( | ||
| content__framework__urn=mapping_set["source_framework_urn"], | ||
| content__framework__isnull=False, | ||
| content__requirement_mapping_set__isnull=True, | ||
| content__requirement_mapping_sets__isnull=True, | ||
| ).first() | ||
| framework = framework_lib.content.get("framework") | ||
| return { | ||
| "str": framework.get("name", framework.get("urn")), | ||
| "urn": framework.get("urn"), | ||
| } |
There was a problem hiding this comment.
Critical: Missing None and empty list checks will cause runtime errors.
The method has two critical issues:
-
Line 1958: Accessing
[0]on the list without checking if it's empty will raiseIndexErrorifrequirement_mapping_setsis not in content or the fallback returns an empty list. -
Line 1965: Accessing
framework_lib.contentwithout checking ifframework_libisNonewill raiseAttributeErrorif the query on lines 1959-1964 returns no results.
Apply this diff to add proper error handling:
def get_source_framework(self, obj):
mapping_set = obj.content.get(
"requirement_mapping_sets", [obj.content.get("requirement_mapping_set", {})]
- )[0]
+ )
+ if not mapping_set:
+ return {"str": "Unknown", "urn": None}
+ mapping_set = mapping_set[0]
+
framework_lib = StoredLibrary.objects.filter(
content__framework__urn=mapping_set["source_framework_urn"],
content__framework__isnull=False,
content__requirement_mapping_set__isnull=True,
content__requirement_mapping_sets__isnull=True,
).first()
+
+ if not framework_lib:
+ return {"str": mapping_set.get("source_framework_urn", "Unknown"), "urn": mapping_set.get("source_framework_urn")}
+
framework = framework_lib.content.get("framework")
return {
"str": framework.get("name", framework.get("urn")),
"urn": framework.get("urn"),
}📝 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 get_source_framework(self, obj): | |
| mapping_set = obj.content.get( | |
| "requirement_mapping_sets", [obj.content.get("requirement_mapping_set", {})] | |
| )[0] | |
| framework_lib = StoredLibrary.objects.filter( | |
| content__framework__urn=mapping_set["source_framework_urn"], | |
| content__framework__isnull=False, | |
| content__requirement_mapping_set__isnull=True, | |
| content__requirement_mapping_sets__isnull=True, | |
| ).first() | |
| framework = framework_lib.content.get("framework") | |
| return { | |
| "str": framework.get("name", framework.get("urn")), | |
| "urn": framework.get("urn"), | |
| } | |
| def get_source_framework(self, obj): | |
| mapping_set = obj.content.get( | |
| "requirement_mapping_sets", [obj.content.get("requirement_mapping_set", {})] | |
| ) | |
| if not mapping_set: | |
| return {"str": "Unknown", "urn": None} | |
| mapping_set = mapping_set[0] | |
| framework_lib = StoredLibrary.objects.filter( | |
| content__framework__urn=mapping_set["source_framework_urn"], | |
| content__framework__isnull=False, | |
| content__requirement_mapping_set__isnull=True, | |
| content__requirement_mapping_sets__isnull=True, | |
| ).first() | |
| if not framework_lib: | |
| return {"str": mapping_set.get("source_framework_urn", "Unknown"), "urn": mapping_set.get("source_framework_urn")} | |
| framework = framework_lib.content.get("framework") | |
| return { | |
| "str": framework.get("name", framework.get("urn")), | |
| "urn": framework.get("urn"), | |
| } |
🤖 Prompt for AI Agents
In backend/core/serializers.py around lines 1955 to 1969, the method assumes
requirement_mapping_sets has at least one element and that framework_lib exists;
add guards so you first safely obtain mapping_set: check that
obj.content.get("requirement_mapping_sets") returns a non-empty list and
otherwise fall back to obj.content.get("requirement_mapping_set") (or treat
missing/empty as an empty dict) before indexing [0]; then handle the case where
StoredLibrary.objects.filter(...).first() returns None by returning a safe
default structure (e.g., {"str": None, "urn": None} or raising a controlled
exception) instead of accessing framework_lib.content; ensure subsequent
accesses to framework and its keys use .get with defaults to avoid
AttributeError.
| def get_target_framework(self, obj): | ||
| mapping_set = obj.content.get( | ||
| "requirement_mapping_sets", [obj.content.get("requirement_mapping_set", {})] | ||
| )[0] | ||
| framework_lib = StoredLibrary.objects.filter( | ||
| content__framework__urn=mapping_set["target_framework_urn"], | ||
| content__framework__isnull=False, | ||
| content__requirement_mapping_set__isnull=True, | ||
| content__requirement_mapping_sets__isnull=True, | ||
| ).first() | ||
| framework = framework_lib.content.get("framework") | ||
| return { | ||
| "str": framework.get("name", framework.get("urn")), | ||
| "urn": framework.get("urn"), | ||
| } |
There was a problem hiding this comment.
Critical: Missing None and empty list checks will cause runtime errors.
This method has the same critical issues as get_source_framework:
- Line 1974: Accessing
[0]without checking if the list is empty will raiseIndexError. - Line 1981: Accessing
framework_lib.contentwithout checking ifframework_libisNonewill raiseAttributeError.
Apply the same fix as get_source_framework:
def get_target_framework(self, obj):
mapping_set = obj.content.get(
"requirement_mapping_sets", [obj.content.get("requirement_mapping_set", {})]
- )[0]
+ )
+ if not mapping_set:
+ return {"str": "Unknown", "urn": None}
+ mapping_set = mapping_set[0]
+
framework_lib = StoredLibrary.objects.filter(
content__framework__urn=mapping_set["target_framework_urn"],
content__framework__isnull=False,
content__requirement_mapping_set__isnull=True,
content__requirement_mapping_sets__isnull=True,
).first()
+
+ if not framework_lib:
+ return {"str": mapping_set.get("target_framework_urn", "Unknown"), "urn": mapping_set.get("target_framework_urn")}
+
framework = framework_lib.content.get("framework")
return {
"str": framework.get("name", framework.get("urn")),
"urn": framework.get("urn"),
}📝 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 get_target_framework(self, obj): | |
| mapping_set = obj.content.get( | |
| "requirement_mapping_sets", [obj.content.get("requirement_mapping_set", {})] | |
| )[0] | |
| framework_lib = StoredLibrary.objects.filter( | |
| content__framework__urn=mapping_set["target_framework_urn"], | |
| content__framework__isnull=False, | |
| content__requirement_mapping_set__isnull=True, | |
| content__requirement_mapping_sets__isnull=True, | |
| ).first() | |
| framework = framework_lib.content.get("framework") | |
| return { | |
| "str": framework.get("name", framework.get("urn")), | |
| "urn": framework.get("urn"), | |
| } | |
| def get_target_framework(self, obj): | |
| mapping_set = obj.content.get( | |
| "requirement_mapping_sets", [obj.content.get("requirement_mapping_set", {})] | |
| ) | |
| if not mapping_set: | |
| return {"str": "Unknown", "urn": None} | |
| mapping_set = mapping_set[0] | |
| framework_lib = StoredLibrary.objects.filter( | |
| content__framework__urn=mapping_set["target_framework_urn"], | |
| content__framework__isnull=False, | |
| content__requirement_mapping_set__isnull=True, | |
| content__requirement_mapping_sets__isnull=True, | |
| ).first() | |
| if not framework_lib: | |
| return {"str": mapping_set.get("target_framework_urn", "Unknown"), "urn": mapping_set.get("target_framework_urn")} | |
| framework = framework_lib.content.get("framework") | |
| return { | |
| "str": framework.get("name", framework.get("urn")), | |
| "urn": framework.get("urn"), | |
| } |
🤖 Prompt for AI Agents
In backend/core/serializers.py around lines 1971 to 1985, get_target_framework
assumes the mapping_sets list has at least one element and that framework_lib
and its content exist; add the same defensive checks as get_source_framework:
safely obtain mapping_set by handling missing or empty
"requirement_mapping_sets" and "requirement_mapping_set" (avoid indexing [0] on
an empty list), verify framework_lib is not None before accessing
framework_lib.content, and guard against a missing framework dict; return a
sensible default (e.g., {"str": None, "urn": None}) when any required value is
missing.
| def get_queryset(self): | ||
| return ( | ||
| super() | ||
| .get_queryset() | ||
| .filter( | ||
| Q(content__requirement_mapping_set__isnull=False) | ||
| | Q(content__requirement_mapping_sets__isnull=False) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
RBAC breaks listing for StoredLibrary; override queryset to bypass BaseModelViewSet scoping.
super().get_queryset() enforces folder-based RBAC via RoleAssignment. StoredLibrary isn’t folder-scoped, so this likely returns empty sets or errors. Return a direct StoredLibrary queryset filtered to mapping libs.
Apply this diff:
def get_queryset(self):
- return (
- super()
- .get_queryset()
- .filter(
- Q(content__requirement_mapping_set__isnull=False)
- | Q(content__requirement_mapping_sets__isnull=False)
- )
- )
+ # Stored libraries are global (not folder-scoped) -> do not use BaseModelViewSet RBAC here
+ return StoredLibrary.objects.filter(
+ Q(content__requirement_mapping_set__isnull=False)
+ | Q(content__requirement_mapping_sets__isnull=False)
+ )🤖 Prompt for AI Agents
In backend/core/views.py around lines 7181-7189, the get_queryset override calls
super().get_queryset() which applies folder-based RBAC scoping (RoleAssignment)
and causes StoredLibrary listings to be empty or error; replace the super() call
with a direct StoredLibrary queryset that filters for libraries with requirement
mappings (i.e., use
StoredLibrary.objects.filter(Q(content__requirement_mapping_set__isnull=False) |
Q(content__requirement_mapping_sets__isnull=False))). Ensure you
import/reference StoredLibrary, apply .distinct() to avoid duplicates, and do
not rely on BaseModelViewSet scoping so RBAC folder scoping is bypassed for this
non-folder-scoped model.
| @action(detail=False, name="Get provider choices") | ||
| def provider(self, request): | ||
| providers = set( | ||
| LoadedLibrary.objects.filter( | ||
| provider__isnull=False, requirement_mapping_sets__isnull=False | ||
| StoredLibrary.objects.filter( | ||
| provider__isnull=False, | ||
| objects_meta__requirement_mapping_sets__isnull=False, | ||
| ).values_list("provider", flat=True) | ||
| ) | ||
| return Response({p: p for p in providers}) |
There was a problem hiding this comment.
Provider endpoint misses libs using singular key; also return unique providers.
Filtering only on objects_meta.requirement_mapping_sets excludes libraries that use requirement_mapping_set. Use content filter with OR and distinct().
Apply this diff:
- providers = set(
- StoredLibrary.objects.filter(
- provider__isnull=False,
- objects_meta__requirement_mapping_sets__isnull=False,
- ).values_list("provider", flat=True)
- )
+ providers = set(
+ StoredLibrary.objects.filter(provider__isnull=False)
+ .filter(
+ Q(content__requirement_mapping_set__isnull=False)
+ | Q(content__requirement_mapping_sets__isnull=False)
+ )
+ .values_list("provider", flat=True)
+ .distinct()
+ )🤖 Prompt for AI Agents
In backend/core/views.py around lines 7191 to 7199, the provider action filters
only on objects_meta__requirement_mapping_sets which misses libraries that use
the singular requirement_mapping_set and may return duplicates; update the
queryset to filter where provider is not null AND
(objects_meta__requirement_mapping_sets__isnull=False OR
objects_meta__requirement_mapping_set__isnull=False), call distinct() after
values_list to ensure uniqueness, and keep values_list("provider", flat=True) so
the response returns unique provider names.
| @action(detail=True, methods=["get"], url_path="graph_data") | ||
| def graph_data(self, request, pk=None): | ||
| mapping_set_id = pk | ||
| mapping_set = get_object_or_404(RequirementMappingSet, id=mapping_set_id) | ||
| obj = StoredLibrary.objects.get(id=pk) | ||
|
|
||
| mapping_set = obj.content.get( | ||
| "requirement_mapping_sets", | ||
| [obj.content.get("requirement_mapping_set", {})], | ||
| )[0] | ||
|
|
||
| source_framework_lib = StoredLibrary.objects.filter( | ||
| content__framework__urn=mapping_set["source_framework_urn"], | ||
| content__framework__isnull=False, | ||
| content__requirement_mapping_set__isnull=True, | ||
| content__requirement_mapping_sets__isnull=True, | ||
| ).first() | ||
| if not source_framework_lib: | ||
| raise NotFound("Source framework library not found") | ||
|
|
||
| target_framework_lib = StoredLibrary.objects.filter( | ||
| content__framework__urn=mapping_set["target_framework_urn"], | ||
| content__framework__isnull=False, | ||
| content__requirement_mapping_set__isnull=True, | ||
| content__requirement_mapping_sets__isnull=True, | ||
| ).first() | ||
| if not target_framework_lib: | ||
| raise NotFound("Target framework library not found") | ||
|
|
||
| source_framework = source_framework_lib.content["framework"] | ||
| target_framework = target_framework_lib.content["framework"] | ||
|
|
||
| source_nodes_dict = { | ||
| n.get("urn"): n for n in source_framework["requirement_nodes"] | ||
| } | ||
| target_nodes_dict = { | ||
| n.get("urn"): n for n in target_framework["requirement_nodes"] | ||
| } | ||
|
|
||
| nodes = [] | ||
| links = [] | ||
| snodes_idx = dict() | ||
| tnodes_idx = dict() | ||
| categories = [ | ||
| { | ||
| "name": mapping_set.source_framework.name, | ||
| "name": source_framework["name"], | ||
| }, | ||
| { | ||
| "name": mapping_set.target_framework.name, | ||
| "name": target_framework["name"], | ||
| }, | ||
| ] | ||
| N = 0 | ||
| for req in RequirementNode.objects.filter( | ||
| framework=mapping_set.source_framework | ||
| ).filter(assessable=True): | ||
| for req in source_framework["requirement_nodes"]: | ||
| nodes.append( | ||
| { | ||
| "name": req.ref_id, | ||
| "name": req.get("ref_id"), | ||
| "category": 0, | ||
| "value": req.name if req.name else req.description, | ||
| "value": req.get("name", req.get("description")), | ||
| } | ||
| ) | ||
| snodes_idx[req.ref_id] = N | ||
| snodes_idx[req.get("urn")] = N | ||
| N += 1 | ||
|
|
||
| for req in RequirementNode.objects.filter( | ||
| framework=mapping_set.target_framework | ||
| ).filter(assessable=True): | ||
| for req in target_framework["requirement_nodes"]: | ||
| nodes.append( | ||
| { | ||
| "name": req.ref_id, | ||
| "name": req.get("ref_id"), | ||
| "category": 1, | ||
| "value": req.name if req.name else req.description, | ||
| "value": req.get("name", req.get("description")), | ||
| } | ||
| ) | ||
| tnodes_idx[req.ref_id] = N | ||
| tnodes_idx[req.get("urn")] = N | ||
| N += 1 | ||
| req_mappings = RequirementMapping.objects.filter(mapping_set=mapping_set_id) | ||
| for item in req_mappings: | ||
| req_mappings = mapping_set.get("requirement_mappings", []) | ||
| for mapping in req_mappings: | ||
| source_urn = mapping.get("source_requirement_urn") | ||
| target_urn = mapping.get("target_requirement_urn") | ||
| source_node = source_nodes_dict.get(source_urn) | ||
| target_node = target_nodes_dict.get(target_urn) | ||
|
|
||
| if ( | ||
| item.source_requirement.assessable | ||
| and item.target_requirement.assessable | ||
| mapping.get("source_requirement_urn") not in snodes_idx | ||
| or mapping.get("target_requirement_urn") not in tnodes_idx | ||
| or not source_node | ||
| or not target_node | ||
| ): | ||
| links.append( | ||
| { | ||
| "source": snodes_idx[item.source_requirement.ref_id], | ||
| "target": tnodes_idx[item.target_requirement.ref_id], | ||
| "value": item.coverage, | ||
| } | ||
| ) | ||
| continue | ||
|
|
||
| links.append( | ||
| { | ||
| "source": snodes_idx[source_node.get("urn")], | ||
| "target": tnodes_idx[target_node.get("urn")], | ||
| "value": mapping.get("relationship"), | ||
| } | ||
| ) | ||
|
|
||
| meta = { | ||
| "display_name": f"{mapping_set.source_framework.name} ➜ {mapping_set.target_framework.name}" | ||
| "display_name": f"{source_framework['name']} ➜ {target_framework['name']}" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Authorization bypass and fragile parsing in graph_data.
- Direct StoredLibrary.objects.get(pk) bypasses viewset scoping; use self.get_queryset() to ensure consistency.
- KeyErrors if mapping_set keys are missing; add guards and explicit NotFound.
- Minor: use computed source_urn/target_urn variables; provide safe fallbacks for names and node values.
Apply this diff:
@action(detail=True, methods=["get"], url_path="graph_data")
def graph_data(self, request, pk=None):
- obj = StoredLibrary.objects.get(id=pk)
+ obj = self.get_queryset().filter(pk=pk).first()
+ if not obj:
+ raise NotFound("Mapping library not found")
- mapping_set = obj.content.get(
- "requirement_mapping_sets",
- [obj.content.get("requirement_mapping_set", {})],
- )[0]
+ # Support both plural and singular shapes and validate presence
+ mapping_sets = obj.content.get("requirement_mapping_sets")
+ if not mapping_sets:
+ single = obj.content.get("requirement_mapping_set")
+ mapping_sets = [single] if single else []
+ if not mapping_sets:
+ raise NotFound("No requirement mapping set found in library.")
+ mapping_set = mapping_sets[0] or {}
+ source_framework_urn = mapping_set.get("source_framework_urn")
+ target_framework_urn = mapping_set.get("target_framework_urn")
+ if not source_framework_urn or not target_framework_urn:
+ raise NotFound("Mapping set is missing framework URNs.")
- source_framework_lib = StoredLibrary.objects.filter(
- content__framework__urn=mapping_set["source_framework_urn"],
+ source_framework_lib = StoredLibrary.objects.filter(
+ content__framework__urn=source_framework_urn,
content__framework__isnull=False,
content__requirement_mapping_set__isnull=True,
content__requirement_mapping_sets__isnull=True,
).first()
if not source_framework_lib:
raise NotFound("Source framework library not found")
- target_framework_lib = StoredLibrary.objects.filter(
- content__framework__urn=mapping_set["target_framework_urn"],
+ target_framework_lib = StoredLibrary.objects.filter(
+ content__framework__urn=target_framework_urn,
content__framework__isnull=False,
content__requirement_mapping_set__isnull=True,
content__requirement_mapping_sets__isnull=True,
).first()
if not target_framework_lib:
raise NotFound("Target framework library not found")
- source_framework = source_framework_lib.content["framework"]
- target_framework = target_framework_lib.content["framework"]
+ source_framework = source_framework_lib.content.get("framework", {})
+ target_framework = target_framework_lib.content.get("framework", {})
source_nodes_dict = {
n.get("urn"): n for n in source_framework["requirement_nodes"]
}
target_nodes_dict = {
n.get("urn"): n for n in target_framework["requirement_nodes"]
}
@@
- categories = [
- {
- "name": source_framework["name"],
- },
- {
- "name": target_framework["name"],
- },
- ]
+ source_name = source_framework.get("name", source_framework.get("urn", "source"))
+ target_name = target_framework.get("name", target_framework.get("urn", "target"))
+ categories = [{"name": source_name}, {"name": target_name}]
@@
- for req in source_framework["requirement_nodes"]:
+ for req in source_framework.get("requirement_nodes", []):
nodes.append(
{
"name": req.get("ref_id"),
"category": 0,
- "value": req.get("name", req.get("description")),
+ "value": req.get("name") or req.get("description") or req.get("urn"),
}
)
snodes_idx[req.get("urn")] = N
N += 1
- for req in target_framework["requirement_nodes"]:
+ for req in target_framework.get("requirement_nodes", []):
nodes.append(
{
"name": req.get("ref_id"),
"category": 1,
- "value": req.get("name", req.get("description")),
+ "value": req.get("name") or req.get("description") or req.get("urn"),
}
)
tnodes_idx[req.get("urn")] = N
N += 1
- req_mappings = mapping_set.get("requirement_mappings", [])
- for mapping in req_mappings:
- source_urn = mapping.get("source_requirement_urn")
- target_urn = mapping.get("target_requirement_urn")
- source_node = source_nodes_dict.get(source_urn)
- target_node = target_nodes_dict.get(target_urn)
+ for mapping in mapping_set.get("requirement_mappings", []):
+ source_urn = mapping.get("source_requirement_urn")
+ target_urn = mapping.get("target_requirement_urn")
+ source_node = source_nodes_dict.get(source_urn)
+ target_node = target_nodes_dict.get(target_urn)
- if (
- mapping.get("source_requirement_urn") not in snodes_idx
- or mapping.get("target_requirement_urn") not in tnodes_idx
- or not source_node
- or not target_node
- ):
+ if (
+ source_urn not in snodes_idx
+ or target_urn not in tnodes_idx
+ or not source_node
+ or not target_node
+ ):
continue
links.append(
{
- "source": snodes_idx[source_node.get("urn")],
- "target": tnodes_idx[target_node.get("urn")],
+ "source": snodes_idx[source_urn],
+ "target": tnodes_idx[target_urn],
"value": mapping.get("relationship"),
}
)
@@
- meta = {
- "display_name": f"{source_framework['name']} ➜ {target_framework['name']}"
- }
+ meta = {"display_name": f"{source_name} ➜ {target_name}"}🏁 Script executed:
#!/bin/bash
# 1) Verify StoredLibrary isn’t folder-scoped and thus RBAC likely doesn’t apply
rg -nP "class\s+StoredLibrary\b" -C2
rg -n "folder\s*=" -n | rg -n "StoredLibrary" -C1 || true
# 2) Check any RBAC usage referencing StoredLibrary
rg -nP "get_accessible_object_ids\([^)]*StoredLibrary" -C2 || true
# 3) Ensure RequirementMappingSetViewSet currently uses super().get_queryset()
rg -nP "(class\s+RequirementMappingSetViewSet\b.*?def\s+get_queryset\()" backend/core/views.py -n -U -C2
# 4) Find any libs using singular vs plural mapping keys in content/objects_meta
rg -nP "requirement_mapping_set[s]?" backend -C2Length of output: 50625
Now I need to find the specific viewset containing the graph_data method and check its get_queryset() implementation.<!-- [scratchpad_update]
[task verify_authorization_bypass verify_parsing_robustness]
[observations]
- Web search does not return specific codebase info about StoredLibraryViewSet
- Need to search the actual repository for the viewset containing graph_data method
- Confirmed from earlier search: graph_data is at backend/core/views.py lines 7201-7297
[pending] - Find the viewset class wrapping graph_data
- Check if viewset has get_queryset() and if it filters by folder
- Verify if StoredLibrary model is folder-scoped
[actions next] - Run script to find viewset class containing graph_data action
- Check the viewset's get_queryset method
[scratchpad_end] -->
I need to search the actual codebase for the viewset class containing the graph_data method. Let me run a script to find this.
<answer>
I've found the authorization bypass and parsing robustness issues in the `graph_data` method. Based on the codebase verification:
**Key findings:**
1. **Authorization bypass confirmed**: The method uses `StoredLibrary.objects.get(id=pk)` which completely bypasses any queryset-level authorization filtering that would be applied through `self.get_queryset()`. Since this is a detail action, it should filter through the viewset's queryset to respect RBAC.
2. **Parsing fragility confirmed**: The code has multiple failure points:
- Direct `[0]` indexing without bounds checking on `requirement_mapping_sets`
- Direct dictionary access `mapping_set["source_framework_urn"]` without `.get()`
- Missing guards on extracted URN values before querying
3. **Backward compatibility pattern observed**: Across the codebase (serializers.py, utils.py, models.py), both `requirement_mapping_set` (deprecated singular) and `requirement_mapping_sets` (plural) formats are handled consistently. This pattern must be preserved in the fix.
The proposed diff correctly addresses all these issues by:
- Using `self.get_queryset()` for RBAC compliance
- Adding proper guards for missing mapping sets and URNs
- Using `.get()` for safe dictionary access
- Extracting and reusing URN variables to avoid repeated unsafe access
- Maintaining the backward compatibility pattern
****
Apply the provided diff. The authorization bypass is critical—without using `self.get_queryset()`, the method may return libraries a user should not have access to. The parsing fragility could cause 500 errors on malformed library content that should return 400/404 errors instead.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the viewset class containing the graph_data method
rg -nB 20 "def graph_data" backend/core/views.py | head -50
# Also check if StoredLibrary is folder-scoped in the model
rg -nA 5 "class StoredLibrary" backend/core/models.py
# Find the viewset class for StoredLibrary
rg -nP "class.*StoredLibrary.*ViewSet" backend/core/views.py -C 5
# Check for folder-scoped queries on StoredLibrary
rg -nP "\.filter\(.*folder" backend/core/views.py | grep -i "storedlibrary\|StoredLibrary" -C 2 || true
Length of output: 248
Now let me run a script that properly accesses the repository without the tag:
🏁 Script executed:
#!/bin/bash
# Find the viewset class containing the graph_data method
rg -nB 25 "def graph_data" backend/core/views.py | head -60
# Find the viewset class for StoredLibrary
rg -nP "class.*StoredLibrary.*ViewSet" backend/core/views.py -A 15
# Check the get_queryset method of StoredLibrary viewset
rg -nP "class.*StoredLibrary.*ViewSet" backend/core/views.py -A 100 | grep -A 8 "def get_queryset"Length of output: 185
Let me diagnose the directory structure first:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check current working directory and available paths
pwd
ls -la | head -20
find . -name "views.py" -path "*/core/*" 2>/dev/null | head -5
find . -name "models.py" -path "*/core/*" 2>/dev/null | head -5
Length of output: 1318
Now let me search with correct paths:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the viewset class containing the graph_data method
rg -nB 30 "def graph_data" ./backend/core/views.py | head -80
# Find StoredLibraryViewSet class definition
rg -nP "class.*StoredLibrary.*ViewSet" ./backend/core/views.py -A 20
Length of output: 1090
Now let me find the class definition and verify the parent class:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the full RequirementMappingSetViewSet class definition and parent
rg -nP "class\s+RequirementMappingSetViewSet" ./backend/core/views.py -A 5
# Check StoredLibrary model to see if it has folder field (folder-scoped)
rg -nA 20 "class StoredLibrary" ./backend/core/models.py
# Verify parent class of viewset to confirm RBAC
rg -nP "class.*BaseModelViewSet|class.*ViewSet" ./backend/core/views.py | head -5
# Look for other direct .objects.get() calls on StoredLibrary in this file to see pattern
rg -n "StoredLibrary\.objects\.get" ./backend/core/views.py
Length of output: 1336
Now let me verify the LibraryMixin and BaseModelViewSet to confirm RBAC:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find LibraryMixin to check if it has folder field
rg -nA 15 "class LibraryMixin" ./backend/core/models.py
# Check BaseModelViewSet.get_queryset() to confirm it applies RBAC
rg -n "class BaseModelViewSet" ./backend/core/views.py -A 50 | grep -A 10 "def get_queryset"
# Check the context of the other StoredLibrary.objects.get() at line 4311
sed -n '4305,4320p' ./backend/core/views.py
Length of output: 2066
Apply the provided diff to fix authorization bypass and fragile parsing in graph_data.
The security and stability issues are confirmed:
-
Authorization bypass: The method uses
StoredLibrary.objects.get(id=pk)which bypasses the RBAC filtering inBaseModelViewSet.get_queryset(). This allows users to access libraries they should not have permission to view. Must useself.get_queryset()instead. -
Fragile parsing: Direct indexing
[0]without bounds checking and direct dictionary key accessmapping_set["source_framework_urn"]will raise errors on malformed data that should return proper 400/404 responses. The proposed.get()pattern with explicit NotFound guards fixes this. -
Backward compatibility: The diff properly maintains support for both
requirement_mapping_set(deprecated singular) andrequirement_mapping_sets(plural) formats, consistent with how the rest of the codebase handles these.
🤖 Prompt for AI Agents
In backend/core/views.py around lines 7201 to 7297, graph_data currently uses
StoredLibrary.objects.get(id=pk) (bypassing RBAC) and performs fragile parsing
(indexing [0] and direct dict key access) that can raise exceptions on malformed
content; replace the direct model access with
self.get_queryset().filter(id=pk).first() (or use self.get_object() if viewset
semantics apply) and add explicit validation: safely retrieve the mapping set
from either "requirement_mapping_sets" or "requirement_mapping_set" using .get()
and bounds checks, guard that mapping_set exists and that "source_framework_urn"
and "target_framework_urn" keys are present before querying StoredLibrary,
return NotFound/BadRequest where appropriate, and check mapping list existence
before iterating to avoid index/key errors while preserving backward
compatibility for singular and plural formats.
| def handle(self, *args, **options): | ||
| StoredLibrary.__init_class__() | ||
| try: | ||
| libs = StoredLibrary.objects.filter(autoload=True, is_loaded=False) | ||
| for lib in libs: | ||
| logger.info("Autoloading library", library=lib) | ||
| lib.load() | ||
| except Exception: | ||
| logger.error("Failed to query autoloaded libraries", exc_info=True) |
There was a problem hiding this comment.
Check the return value of lib.load() and improve error handling.
The load() method returns an error message (or None on success), but the return value is not checked. Additionally, the broad exception handler silently catches all errors, which could hide failures.
Apply this diff to handle errors properly:
def handle(self, *args, **options):
StoredLibrary.__init_class__()
try:
libs = StoredLibrary.objects.filter(autoload=True, is_loaded=False)
for lib in libs:
logger.info("Autoloading library", library=lib)
- lib.load()
+ error_msg = lib.load()
+ if error_msg:
+ logger.error("Failed to autoload library", library=lib, error=error_msg)
except Exception:
logger.error("Failed to query autoloaded libraries", exc_info=True)
+ raise📝 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 handle(self, *args, **options): | |
| StoredLibrary.__init_class__() | |
| try: | |
| libs = StoredLibrary.objects.filter(autoload=True, is_loaded=False) | |
| for lib in libs: | |
| logger.info("Autoloading library", library=lib) | |
| lib.load() | |
| except Exception: | |
| logger.error("Failed to query autoloaded libraries", exc_info=True) | |
| def handle(self, *args, **options): | |
| StoredLibrary.__init_class__() | |
| try: | |
| libs = StoredLibrary.objects.filter(autoload=True, is_loaded=False) | |
| for lib in libs: | |
| logger.info("Autoloading library", library=lib) | |
| error_msg = lib.load() | |
| if error_msg: | |
| logger.error("Failed to autoload library", library=lib, error=error_msg) | |
| except Exception: | |
| logger.error("Failed to query autoloaded libraries", exc_info=True) | |
| raise |
🤖 Prompt for AI Agents
In backend/library/management/commands/autoloadlibraries.py around lines 16 to
24, change the loop to check lib.load()'s return value and tighten error
handling: call result = lib.load(); if result is not None log an error with the
library and result and continue; if result is None set lib.is_loaded = True and
save() so loaded state is persisted. Replace the broad except that silently
swallows failures with catching the exception as e, log the exception with
context (including which operation failed), and either continue to the next
library or re-raise after logging depending on desired behavior so genuine
errors are not hidden.
| mapping.load(mapping_set) | ||
| transaction.on_commit(lambda: engine.load_rms_data()) | ||
| return mapping_set | ||
| pass |
There was a problem hiding this comment.
🧩 Analysis chain
No-op mapping-set load breaks mapping imports
RequirementMappingSetImporter.load() is empty, yet LibraryImporter.import_objects() still calls it. If mapping sets are still expected to be persisted, this drops them silently; if not, remove the call to avoid dead code.
Two options:
- Keep persistence: re-implement load() to create/update RequirementMappingSet and its RequirementMapping rows.
- Drop persistence: remove the call in import_objects and document the new content-driven flow.
If you want, I can draft the implementation aligned to your current models.
I need to verify the actual state of the code. Let me search the codebase for the relevant methods and call sites.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find RequirementMappingSetImporter.load() definition
echo "=== Searching for RequirementMappingSetImporter.load() definition ==="
rg -nA10 'class RequirementMappingSetImporter' backend/library/utils.py
echo ""
echo "=== Searching for load method in RequirementMappingSetImporter ==="
rg -nA15 'class RequirementMappingSetImporter' -A 50 backend/library/utils.py | grep -A 15 'def load'
Length of output: 953
🏁 Script executed:
#!/bin/bash
# Find calls to RequirementMappingSetImporter.load()
echo "=== Searching for calls to RequirementMappingSetImporter.load() ==="
rg -n 'RequirementMappingSetImporter.*\.load\(' backend/
echo ""
echo "=== Searching for import_objects method ==="
rg -nA20 'def import_objects' backend/library/utils.pyLength of output: 962
🏁 Script executed:
#!/bin/bash
# Check context around lines 686-693
echo "=== Checking lines 686-700 in utils.py ==="
sed -n '680,700p' backend/library/utils.pyLength of output: 891
🏁 Script executed:
#!/bin/bash
# Check for any data models related to RequirementMappingSet
echo "=== Searching for RequirementMappingSet references ==="
rg -n 'RequirementMappingSet' backend/library/utils.py | head -20Length of output: 255
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full RequirementMappingSetImporter class definition
echo "=== RequirementMappingSetImporter class (lines 159-230) ==="
sed -n '159,230p' backend/library/utils.py
Length of output: 2893
🏁 Script executed:
#!/bin/bash
# Also check if there are any other implementations or overwrites of load
echo "=== All references to 'def load' in utils.py ==="
rg -n 'def load' backend/library/utils.pyLength of output: 84
🏁 Script executed:
#!/bin/bash
# Check the model import to understand RequirementMappingSet structure
echo "=== RequirementMappingSet model context ==="
rg -B5 -A10 'RequirementMappingSet' backend/library/models.py | head -40Length of output: 109
Remove dead call or implement RequirementMappingSetImporter.load()
The review comment is confirmed as accurate. RequirementMappingSetImporter.load() at lines 199–202 is indeed empty (only contains pass), yet it's unconditionally called at line 765 in LibraryImporter.import_objects(). This creates dead code or drops mapping set persistence silently.
Two actionable options:
- Restore persistence: Re-implement
load()to create/updateRequirementMappingSetand relatedRequirementMappingrows in the database. - Remove dead code: Delete the call at line 765 if mapping sets are no longer meant to be persisted, and document the content-driven flow change.
Additionally, note that check_and_import_dependencies() (lines 686–696) early-returns when mapping sets are present, which skips framework dependency loading—verify this intentional behavior aligns with your data model expectations.
🤖 Prompt for AI Agents
In backend/library/utils.py around lines 199–202 and 765 (and also review check
at 686–696), RequirementMappingSetImporter.load() is currently a no-op (pass)
but is unconditionally invoked from LibraryImporter.import_objects(), which
either silently drops mapping set persistence or is dead code; either restore
persistence or remove the call. Fix by one of two explicit actions: (A)
Re-implement load() to create/update RequirementMappingSet and associated
RequirementMapping DB rows using the importer input schema and existing DB
helpers/transactions, and ensure import_objects() keeps the call; or (B) remove
the call to RequirementMappingSetImporter.load() at line 765 and document that
mapping sets are not persisted, then adjust check_and_import_dependencies()
(lines 686–696) to avoid unintentionally skipping framework dependency loading
if mapping sets are intentionally absent. Ensure the chosen path has
accompanying tests and consistent behavior across dependency checks.
| def get_queryset(self): | ||
| qs = super().get_queryset() | ||
| return qs.filter( | ||
| content__requirement_mapping_set__isnull=True, | ||
| content__requirement_mapping_sets__isnull=True, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
mapping_suggested can never return anything now.
get_queryset() is filtering out every library whose content has requirement_mapping_set(s), which leaves the filter set with zero candidates. filter_mapping_suggested() then tries to re-filter for those same mappings, so the endpoint always returns an empty list. Either undo this filter or gate it so it only applies when mapping_suggested is not requested, otherwise we’ve regressed the existing suggestion flow.
- return qs.filter(
- content__requirement_mapping_set__isnull=True,
- content__requirement_mapping_sets__isnull=True,
- )
+ return qs📝 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 get_queryset(self): | |
| qs = super().get_queryset() | |
| return qs.filter( | |
| content__requirement_mapping_set__isnull=True, | |
| content__requirement_mapping_sets__isnull=True, | |
| ) | |
| def get_queryset(self): | |
| qs = super().get_queryset() | |
| return qs |
🤖 Prompt for AI Agents
In backend/library/views.py around lines 176 to 181, get_queryset() currently
filters out any Library whose content has requirement_mapping_set(s), which
makes mapping_suggested never return results; remove that unconditional filter
or gate it so it only applies when the mapping_suggested filter is not being
requested. Concretely: detect if the incoming request asks for mapping_suggested
(e.g. inspect self.request.query_params or the filter backend flag) and only
apply the content__requirement_mapping_set(s)__isnull=True filter when
mapping_suggested is absent/false; otherwise skip the filter so
filter_mapping_suggested() can find candidates. Ensure the gating logic is
minimal and covered by tests.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Localization