-
Notifications
You must be signed in to change notification settings - Fork 18
Search component #3878
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?
Search component #3878
Conversation
05e5494 to
12a72f6
Compare
12a72f6 to
29f5b08
Compare
|
I wonder whether we should use a POST-based batch request API here instead. The use-case is that you are given a list of vulnerable packages like here. It would be convenient for a user to copy & paste this list to a text field, and find any affected runs. This would also add more value to the returned |
e105312 to
61480f4
Compare
Yes, but I'd like this to be reviewed at first and get it merged, because then I'd be able to build a minimal UI around it. That would reveal probably something else to add to the implementation as well. |
| repositories { | ||
| exclusiveContent { | ||
| forRepository { | ||
| maven("https://repo.gradle.org/gradle/libs-releases/") | ||
| } | ||
| filter { | ||
| includeGroup("org.gradle") | ||
| } | ||
| } | ||
| } |
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 don't see why this special repository declaration should be required at all.
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 guess I copied it from plugin-manager's buildfile. I don't really understand what it means. Also infrastructure-services and admin-config components have it. Maybe @mnonnenmacher could have an answer?
Could this be something left there "historically" for the other components as well? I took the whole repositories block out: buildAllItems pass, all tests pass, manually verified that the feature also works.
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.
This block is only needed if either this project or projects it depends on need to retrieve artifacts from non-standard locations (in this case the Gradle-specific Maven repository).
To find out whether it's really needed we first need to double-check that the list of dependencies is as minimal as it can be, and then simply try whether everything builds also without this block or not.
| api(libs.exposedCore) | ||
|
|
||
| implementation(projects.components.search.apiModel) | ||
| implementation(projects.dao) | ||
| implementation(projects.model) | ||
|
|
||
| routesImplementation(projects.components.authorizationKeycloak.backend) | ||
| routesImplementation(projects.shared.apiModel) | ||
| 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.
From a quick glance, this looks like way too much, and should be double-checked.
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.
It was checked with Claude Code, which inspected the dependencies and concluded that all deps are needed. So how should I double-check it, as you have said earlier that the Gradle task for this is really not working atm.
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 wouldn't trust Claude (or in fact any AI) for this kind of strictly algorithmic task. So I ran ./gradlew -p components/search/backend projectHealth which gave me:
> Task :components:search:backend:projectHealth
/home/sebastian/Development/GitHub/eclipse-apoapsis/ort-server/components/search/backend/build.gradle.kts
Unused dependencies which should be removed:
routesApi(libs.exposedCore)
routesImplementation(ktorLibs.server.auth)
routesImplementation(libs.exposedCore)
routesImplementation(project(":dao"))
routesImplementation(project(":shared:api-model"))
testApi(libs.exposedCore)
testImplementation(ktorLibs.server.auth)
testImplementation(ktorLibs.server.contentNegotiation)
testImplementation(ktorLibs.server.statusPages)
testImplementation(libs.exposedCore)
testImplementation(libs.kotlinxSerializationJson)
testImplementation(libs.ktorOpenApi)
testImplementation(project(":components:authorization:backend"))
testImplementation(project(":components:search:api-model"))
testImplementation(project(":dao"))
testImplementation(project(":model"))
testImplementation(project(":shared:api-model"))
testImplementation(project(":shared:ktor-utils"))
testImplementation(project(":utils:logging"))
testImplementation(project(":utils:test"))
These transitive dependencies should be declared directly:
implementation(libs.exposedDao)
implementation(libs.kotlinxDatetime)
routesImplementation(ktorLibs.http)
routesImplementation(ktorLibs.utils)
routesImplementation(libs.kotlinxDatetime)
testImplementation("io.kotest:kotest-assertions-shared:6.0.7")
testImplementation("io.kotest:kotest-common:6.0.7")
testImplementation("io.mockk:mockk-dsl:1.14.6")
testImplementation(ktorLibs.client.core)
testImplementation(ktorLibs.http)
testImplementation(ktorLibs.utils)
testImplementation(libs.kotestAssertionsCore)
testImplementation(libs.kotestFrameworkEngine)
testImplementation(libs.kotlinxDatetime)
Existing dependencies which should be modified to be as indicated:
api(project(":components:authorization:backend")) (was implementation)
api(project(":components:search:api-model")) (was implementation)
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.
As @lamppu has said earlier, that list certainly cannot be trusted: she tried, and the build broke from a slightest removal of the deps suggested by projectHealth. That's what I meant by saying the task is not functioning as it should.
components/search/backend/src/routes/kotlin/routes/GetRunsWithPackage.kt
Show resolved
Hide resolved
61480f4 to
312d97a
Compare
|
@oheger-bosch could I kindly ask from you a "sanity check" review for this PR, especially regarding the usage of the new db authorization system? As I am not nearly enough familiar with it, I might have omitted some useful methods and workflows from it. I kept all relevant implementation (especially query filtering) in the last commit 312d97a for easier review. I am especially sceptic about the need of If you think it's still needed, I believe I could at least refactor it to an own file, as similar search endpoints (in future PRs) will need this kind of dynamic permissions checking, because they will also use query parameters dynamically. |
Signed-off-by: Jyrki Keisala <[email protected]>
The helper function simplifies testing of search service. Signed-off-by: Jyrki Keisala <[email protected]>
Signed-off-by: Jyrki Keisala <[email protected]>
Signed-off-by: Jyrki Keisala <[email protected]>
| else -> null | ||
| } | ||
|
|
||
| private fun HierarchyFilter.authorizationCondition(ids: AccessibleHierarchyIds): Op<Boolean>? { |
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 is the HierarchyFiler.apply() extension function. I would assume that some of the logic implemented here could be simplified by using this function. You can refer to the repository implementations for organizations, products, and repositories for concrete usage examples.
| val repositoryIdParam = call.request.queryParameters["repositoryId"]?.toLongOrNull() | ||
|
|
||
| return when { | ||
| repositoryIdParam != null && productIdParam != null && organizationIdParam != 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.
This is problematic. Note that for a CompoundHierarchyId, the service does not check whether the single components are consistent. So, a user could specify an organization he has READ access for, but arbitrary product and repository IDs and then gain access to these elements.
The correct approach is to force only a single scope parameter - maybe return a 400 error code if multiple are defined - and invoke the service with a corresponding HierarchyId.
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.
Thanks, that would've been a serious security problem. Now patched by forcing only a single scope parameter.
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.
In #4096 I have added a corresponding warning to the documentation of the authorization service.
@Etsija, for the other authorization checks implemented so far, there was always a specific hierarchy level to be checked. So, there is no support yet for a dynamic evaluation of the level based on the provided parameters. If there are multiple use cases that require such an approach, it certainly makes sense to move this checker implementation to a more central place. |
Enforce hierarchy-based authorization in package search. This is done already on the query level: first, it is determined to which organizations, products and repositories the user has access. Then this hierarchy filter is added to the WHERE clause, so the query executes filtered. This is more efficient than running the query unfiltered, then filtering the results. Signed-off-by: Jyrki Keisala <[email protected]>
312d97a to
4f232b9
Compare
|
@oheger-bosch I believe I managed, based on your valuable input, to considerably simplify the component and make it safer. Once I get an "OK" from you, I will probably merge the changes in my latest commit to the other ones, then ask for approval (well, after me and @sschuberth have cleaned up the deps). |
| ) | ||
|
|
||
| val whereClause = conditions.reduce { acc, expression -> acc and expression } | ||
| val scopeCondition = when (scope) { |
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, this condition should not be required. The condition generated by the HierarchyFilter should already include exactly the hierarchy elements that are visible for the user.
| userId: String, | ||
| call: ApplicationCall | ||
| ): EffectiveRole? { | ||
| val scope = call.parseScope().getOrNull() |
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.
IIUC, if parseScope returns a failed result, then superuser rights are required. Wouldn't it be better to rethrow the exeption (or some other exception), so that the endpoint can return a 400 response in this case?
| val organizationIdParam = call.request.queryParameters["organizationId"]?.toLongOrNull() | ||
| val productIdParam = call.request.queryParameters["productId"]?.toLongOrNull() | ||
| val repositoryIdParam = call.request.queryParameters["repositoryId"]?.toLongOrNull() | ||
| val scopeResult = call.parseScope() |
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.
Related to my previous comment. I would propose to handle invalid scopes in the checker returned by requireScopedReadPermission() and use some exception mapping logic in StatusPages to yield a 400 BAD RESPONSE code.
This PR introduces a new component "search", which is implemented with the sliced architecture. Initial implementation for the service and endpoint returns a list of all ORT run ids which include a given ORT ID. The service can later on be extended to other search functionality, for example to provide all products/repositories that are affected by a given vulnerability.
Details:
regExbased, so for example substring of a package ID can be usedPlease see the commits for details.
NOTE: the filtering part is retained as a separate (last) commit in the PR, for reviewers to more easily review it.
NOTE2: it has manually been tested that the endpoint returns runs properly as an admin. User-filtered results will be verified later with UI code in a real-life situation (but they are tested with service and integration tests).