-
Notifications
You must be signed in to change notification settings - Fork 17
Initial backend support for managing vulnerability resolutions on the server #3846
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Initial backend support for managing vulnerability resolutions on the server #3846
Conversation
779209d to
2388667
Compare
`UserDisplayName` can then be used in components. Signed-off-by: Johanna Lamppu <[email protected]>
2388667 to
a03cd6b
Compare
This is for cases where for example new resolutions have been added for items found in the run, so that the message can be shown in the UI. Relates to eclipse-apoapsis#1009. Signed-off-by: Johanna Lamppu <[email protected]>
de95600 to
e5e9804
Compare
components/resolutions/api-model/src/commonMain/kotlin/PostVulnerabilityResolution.kt
Show resolved
Hide resolved
.../backend/src/routes/kotlin/routes/vulnerabilities/DeleteVulnerabilityResolutionDefinition.kt
Outdated
Show resolved
Hide resolved
71e30cd to
fedd1fb
Compare
| /** | ||
| * A flag to indicate if the results of the run are outdated, e.g. because of a new resolution. | ||
| */ | ||
| val outdated: Boolean = false, | ||
|
|
||
| /** | ||
| * A message describing why the results of the run are outdated. | ||
| */ | ||
| val outdatedMessage: String? = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need both outdated and outdatedMessage? Theoretically, we could define that a run is outdated if outdatedMessage is non-null, and never allow outdatedMessage to be null for and outdated run (i.e. outdated runs always need to provide a message, which is probably good practice anyway).
On the other hand, being explicit with outdated also has its advantages, but complicates the logic a bit, I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, this was something I was wondering about too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume there is a limited list of reasons why a run can get outdated so I would suggest to create an enum for that, then it could be outdated: List<OutdatedReason> with, for example, OutdatedReason.ISSUE_RESOLUTIONS_CHANGED.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What your opinion @mnonnenmacher about having both these properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong opinion but I have a tendency to only have one property with the list of reasons and if it is empty or null it means the run is not outdated.
| /** | ||
| * Possible reasons for resolving an [Vulnerability] using a [VulnerabilityResolution]. | ||
| */ | ||
| enum class VulnerabilityResolutionReason { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see the PR discussion in ORT to "Rework VulnerabilityResolutionReason for CRA / DORA requirements". So these enums are likely to change. I was briefly wondering whether we should already use that we believe to be "the right solution" here, abstracting away what ORT currently uses until the PR is merged. But it's probably easier (for the developers) to keep these in sync with ORT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid that that would cause some errors when the vulnerability resolutions get mapped to the ORT model's VulnerabilityResolution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not add the new reasons already but leave out the redundant _VULNERABILITY suffix.
| api(projects.model) | ||
| api(projects.shared.apiModel) | ||
|
|
||
| implementation(libs.exposedCore) | ||
| implementation(libs.exposedKotlinDatetime) | ||
|
|
||
| implementation(projects.components.resolutions.apiModel) | ||
| implementation(projects.dao) | ||
| implementation(projects.services.ortRunService) | ||
|
|
||
| routesImplementation(projects.components.authorizationKeycloak.backend) | ||
| routesImplementation(projects.shared.apiMappings) | ||
| routesImplementation(projects.shared.ktorUtils) | ||
|
|
||
| routesImplementation(ktorLibs.server.auth) | ||
| routesImplementation(ktorLibs.server.core) | ||
| routesImplementation(libs.ktorOpenApi) | ||
|
|
||
| testImplementation(testFixtures(projects.clients.keycloak)) | ||
| testImplementation(testFixtures(projects.dao)) | ||
| testImplementation(testFixtures(projects.shared.ktorUtils)) | ||
|
|
||
| testImplementation(ktorLibs.serialization.kotlinx.json) | ||
| testImplementation(ktorLibs.server.auth) | ||
| testImplementation(ktorLibs.server.contentNegotiation) | ||
| testImplementation(ktorLibs.server.statusPages) | ||
| testImplementation(ktorLibs.server.testHost) | ||
| testImplementation(libs.kotestAssertionsKtor) | ||
| testImplementation(libs.kotestRunnerJunit5) | ||
| testImplementation(libs.kotlinxSerializationJson) | ||
| testImplementation(libs.mockk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of dependencies in here that do not seem to be used (right now). I believe we should keep dependencies at a minimum and only extend as needed. To identify unsued dependencies (and other issues with dependency declarations), run the projectHealth Gradle task on any project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was exactly the task I was also missing, thanks! But what's the "project" here? So if I want to do projectHealth for, say, components/secrets, how would I use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what's the "project" here?
A Gradle project. So basically, any directory that contains a build.gradle.kts file. Or phrased differently, anything that's included in settings.gradle.kts:
ort-server/settings.gradle.kts
Lines 26 to 99 in 06c875d
| include(":api:v1:client") | |
| include(":api:v1:mapping") | |
| include(":api:v1:model") | |
| include(":cli") | |
| include(":clients:keycloak") | |
| include(":components:admin-config:api-model") | |
| include(":components:admin-config:backend") | |
| include(":components:authorization:api-model") | |
| include(":components:authorization:backend") | |
| include(":components:authorization-keycloak:api-model") | |
| include(":components:authorization-keycloak:backend") | |
| include(":components:infrastructure-services:api-model") | |
| include(":components:infrastructure-services:backend") | |
| include(":components:plugin-manager:api-model") | |
| include(":components:plugin-manager:backend") | |
| include(":components:secrets:api-model") | |
| include(":components:secrets:backend") | |
| include(":compositions:secrets-routes") | |
| include(":config") | |
| include(":config:git") | |
| include(":config:github") | |
| include(":config:local") | |
| include(":config:secret-file") | |
| include(":config:spi") | |
| include(":core") | |
| include(":dao") | |
| include(":logaccess") | |
| include(":logaccess:spi") | |
| include(":logaccess:loki") | |
| include(":model") | |
| include(":orchestrator") | |
| include(":secrets") | |
| include(":secrets:azure-keyvault") | |
| include(":secrets:file") | |
| include(":secrets:spi") | |
| include(":secrets:scaleway") | |
| include(":secrets:vault") | |
| include(":services:admin-config") | |
| include(":services:content-management") | |
| include(":services:hierarchy") | |
| include(":services:ort-run") | |
| include(":services:report-storage") | |
| include(":shared:api-mappings") | |
| include(":shared:api-model") | |
| include(":shared:ktor-utils") | |
| include(":shared:ort-test-data") | |
| include(":shared:package-curation-providers") | |
| include(":shared:plugin-info") | |
| include(":shared:reporters") | |
| include(":storage") | |
| include(":storage:azure-blob") | |
| include(":storage:database") | |
| include(":storage:s3") | |
| include(":storage:spi") | |
| include(":tasks") | |
| include(":transport") | |
| include(":transport:activemqartemis") | |
| include(":transport:azure-servicebus") | |
| include(":transport:kubernetes") | |
| include(":transport:rabbitmq") | |
| include(":transport:spi") | |
| include(":transport:sqs") | |
| include(":utils:config") | |
| include(":utils:logging") | |
| include(":utils:system") | |
| include(":utils:test") | |
| include(":workers:advisor") | |
| include(":workers:analyzer") | |
| include(":workers:common") | |
| include(":workers:config") | |
| include(":workers:evaluator") | |
| include(":workers:notifier") | |
| include(":workers:reporter") | |
| include(":workers:scanner") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might need some help with this as I seem to get a lot of dependencies in the "Unused dependencies which should be removed", but if I remove them, the build breaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, me too, seems as if the task really isn't as intelligent as it should. It suggests removing almost all of the dependencies for my component, but totally breaks even if I remove a couple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The short answer here is: The DAGP plugin does not (fully) support Kotlin/Multiplatform projects yet, only Kotlin/JVM projects.
components/resolutions/backend/src/main/kotlin/VulnerabilityResolutionDefinitionService.kt
Show resolved
Hide resolved
...esolutions/backend/src/routes/kotlin/routes/vulnerabilities/DeleteVulnerabilityResolution.kt
Outdated
Show resolved
Hide resolved
components/resolutions/backend/src/main/kotlin/VulnerabilityResolutionDefinitionService.kt
Show resolved
Hide resolved
...resolutions/backend/src/routes/kotlin/routes/vulnerabilities/PatchVulnerabilityResolution.kt
Outdated
Show resolved
Hide resolved
...solutions/backend/src/routes/kotlin/routes/vulnerabilities/RestoreVulnerabilityResolution.kt
Outdated
Show resolved
Hide resolved
Add table for storing events regarding items such as resolutions. Relates to eclipse-apoapsis#1009. Signed-off-by: Johanna Lamppu <[email protected]>
The vulnerability resolution definitions will be used to allow users to make vulnerability resolutions on the server, after which they will be injected into repository configuration on new runs. In the first implementation, the scope of the resolutions will be the repository. The definition allows to add multiple ID matchers for cases when the same vulnerability is found with different external IDs from different advisors. Relates to eclipse-apoapsis#1009. Signed-off-by: Johanna Lamppu <[email protected]>
5732f82 to
60346f6
Compare
| /** | ||
| * A flag to indicate if the results of the run are outdated, e.g. because of a new resolution. | ||
| */ | ||
| val outdated: Boolean = false, | ||
|
|
||
| /** | ||
| * A message describing why the results of the run are outdated. | ||
| */ | ||
| val outdatedMessage: String? = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume there is a limited list of reasons why a run can get outdated so I would suggest to create an enum for that, then it could be outdated: List<OutdatedReason> with, for example, OutdatedReason.ISSUE_RESOLUTIONS_CHANGED.
| /* | ||
| * A table to store change log entries representing user-performed change events. | ||
| */ | ||
| object ChangeLogTable : Table("change_log") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered using the event sourcing approach as in the plugin manager? This would provide the audit log but also store details about what exactly was done, not just a generic type like CREATE.
I'm skeptical about having a global change log. It can grow very large if more entities start using it and different entities likely have different requirements, for example, different actions or different data that should be stored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So do you think it could be something like ResolutionsEventStore, or do you think there should be a separate one for each type of resolution? The changes can be stored as well if so wanted, I didn't add that yet, as the answer I got for that question in the issue was that it's not necessarily needed yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an event log without details about the event is not so useful, because in most cases when you would look into an event log you want to find a specific change. About the event store, I would have a tendency towards separate stores because the different kinds of resolutions carry different data. A single store could also work, but maybe be more difficult to implement than the separate stores.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lamppu We could also have a meeting to talk about the implementation details if that would help.
| /** | ||
| * Possible reasons for resolving an [Vulnerability] using a [VulnerabilityResolution]. | ||
| */ | ||
| enum class VulnerabilityResolutionReason { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not add the new reasons already but leave out the redundant _VULNERABILITY suffix.
| testImplementation(ortLibs.scanner) | ||
|
|
||
| testRuntimeOnly(libs.logback) | ||
| testRuntimeOnly(libs.testContainers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all these dependencies really going to be used? For example, the report storage service does not seem to be related to resolutions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on whether I should ignore the output in the "These transitive dependencies should be declared directly" section of the projectHealth task. Also see this conversation above. I still need some guidance on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lamppu, let's try to have a "guidance session" so time this week! Please continue to remind me about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above. We'll have to clean these up manually for now.
| /** | ||
| * Service class for managing vulnerability resolution definitions. | ||
| */ | ||
| class VulnerabilityResolutionDefinitionService(private val db: Database, private val ortRunService: OrtRunService) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why "definitions"? It only seems to make all the names longer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same question, and @lamppu explained the naming is to prevent clashing with some already existing classes for VulnerabilityResolution (through ORT core library of resolutions, or local resolutions defined at the repository with a file).
| provenanceSnippetChoices: List<ProvenanceSnippetChoices> | ||
| ): RepositoryConfiguration = db.blockingQuery { | ||
| RepositoryConfigurationDao.new { | ||
| val ortRun = OrtRunDao[ortRunId].mapToModel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repository configuration is the representation of the .ort.yml file, adding the resolutions here can create confusion. A cleaner approach would be to make a custom ResolutionProvider implementation for the server that takes resolutions from the database into account.
The class naming issue was already discussed and brought up by another reviewer, no need for me to block the PR.
Add a new component for handling resolutions. Signed-off-by: Johanna Lamppu <[email protected]>
Add support for creating vulnerability resolutions on the server. In the initial implementation, the resolutions are created in the scope of the repository. Relates to eclipse-apoapsis#1009. Signed-off-by: Johanna Lamppu <[email protected]>
Inject vulnerability resolutions that have been made for the repository on the server into the repository configuration. Also save the connection between the vulnerability resolution definition and the repository configuration in order to be able to connect the applied resolution to the definition in a later phase when returning vulnerabilities for a run. Relates to eclipse-apoapsis#1009. Signed-off-by: Johanna Lamppu <[email protected]>
If the vulnerability resolution has been defined in the server, add the definition for the resolution in the vulnerabilities for run query. Relates to eclipse-apoapsis#1009. Signed-off-by: Johanna Lamppu <[email protected]>
If there have been new vulnerability resolution definitions made on the server since the run, add the matching definitions to the vulnerabilities response, so that it can be reflected on the UI that with a new run, the particular vulnerability would be resolved. Relates to eclipse-apoapsis#1009. Signed-off-by: Johanna Lamppu <[email protected]>
Implement deleting a vulnerability resolution by archiving the item, i.e. with soft deletion. The resolution will be marked as archived, which means that on new runs, the resolution won't be injected in the repository configuration anymore. The resolution will still be a part of the results of any run where it was injected earlier, and the information that it originated from a definition on the server is retained. Relates to eclipse-apoapsis#1009. Signed-off-by: Johanna Lamppu <[email protected]>
Allow to restore a previously archived resolution. After the restoration the resolution will again be injected to the repository configuration of new runs. Relates to eclipse-apoapsis#1009. Signed-off-by: Johanna Lamppu <[email protected]>
Updating a vulnerability resolution will update the definition, and the new updated values will be used when injecting the resolution on new runs. Old runs where the resolution was injected to previously will still retain the information of what the resolution was when the run was made. Relates to eclipse-apoapsis#1009. Signed-off-by: Johanna Lamppu <[email protected]>
60346f6 to
91f0ce9
Compare
This PR adds back-end support for making vulnerability resolutions on the server.
Please see the individual commits for details.