Refactor KATalogus client in Rocky #3717
Merged
Conversation
This file contains 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
…on the KATalogusClient
Call methods with organization code
Some refactoring around the client usage
Donnype
changed the title
underdarknl
reviewed
Oct 23, 2024
underdarknl
reviewed
Oct 23, 2024
underdarknl
reviewed
Oct 23, 2024
underdarknl
reviewed
Oct 23, 2024
underdarknl
reviewed
Oct 23, 2024
underdarknl
reviewed
Oct 23, 2024
Checklist for QA:
What works:Adding multiple organisations, creating a plugin variant works. What doesn't work:See below. Bug or feature?:Disabling a plugin gives the following error in the logs. The UI says the plugin is disabled. |
…as this is the only place we can now delete organizations and the pre_delete signal should not be used for this.
# Conflicts: # rocky/account/mixins.py # rocky/katalogus/client.py # rocky/tools/models.py
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
rocky/katalogus/client.py
Outdated
| return [parse_plugin(plugin) for plugin in response.json()] | ||
|
|
||
| def get_plugin(self, organization_code: str, plugin_id: str) -> Plugin: | ||
| response = self.session.get(f"/v1/organisations/{organization_code}/plugins/{quote(plugin_id)}") |
Check notice
Code scanning / SonarCloud
Server-side requests should not be vulnerable to traversing attacks Low
There was a problem hiding this comment.
We should quote or sanitize the organization_code in this url.
Better yet, we should provide this function only for an instance of the client bound to a user (and as such can check for the user's permissions for the requested orga), or do we also need this functionality at locations where we dont have a user?
There was a problem hiding this comment.
I think the KATalogus wrapper serves this purpose, and I (currently) like this service-based design better than forcing backend-choosing-logic behind model specific methods. It would require us to either mock or juggle some global state in testing context (that Django gets away with because they put the necessary effort into managing it). Also: there is nothing preventing me from importing an Organization class, instantiate it with an arbitrary id string, and then access forbidden data through this object, just like there is nothing preventing you from using the "wrong" client and access the wrong data. In essence it's hence a design (pattern) choice, and I think the service-based approach has some advantages of which consistency with other services (e.g. Octopoes repositories, the Bytes implementation, the KATalogus) is not the least.
Perhaps a subsequent issue and discussion topic would be a good idea?
|
@stephanie0x00 I've made several changes at this point and cannot reproduce this. Perhaps you could retry this? I did notice the error refers to an onboarding endpoint? |
Checklist for QA:
What works:Scanning seems to work, enabling/disabling plugins works too. Normalizers run. What doesn't work:
Bug or feature?:n/a |
…ictionary, not a list
|
@stephanie0x00 (hopefully) fixed! |
Checklist for QA:
What works:Looks good now. Onboarding works, can enable and disable boefjes and normalizers. Tasks are scheduled and complete. Findings are created. Generated a normal report and that works too. What doesn't work:n/a Bug or feature?:n/a |
|
underdarknl
approved these changes
Nov 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes
Issue link
Closes #3697
Demo
QA notes
Please verify that we can still properly create organizations, add plugins, settings and enable/disable them from Rocky!
Code Checklist
All the commits in this PR are properly PGP-signed and verified.
This PR only contains functionality relevant to the issue.
I have written unit tests for the changes or fixes I made.
I have checked the documentation and made changes where necessary.
I have performed a self-review of my code and refactored it to the best of my abilities.
For any non-trivial functionality, I have added integration and/or end-to-end tests.
I have included comments in the code to elaborate on what is not self-evident from the code itself, including references to issues and discussions online, or implicit behavior of an interface.
Checklist for code reviewers:
Copy-paste the checklist from the docs/source/templates folder into your comment.
Checklist for QA:
Copy-paste the checklist from the docs/source/templates folder into your comment.