Adjust `Security::StoreReportService` to look up findings using UUIDv5, attempt 2
What does this MR do?
Adjusts the logic present in Security::StoreReportService#create_or_find_vulnerability_finding
to match the following flowchart so that we use UUIDv5 whenever possible.
Query details
Selecting a Vulnerabilities::Finding by UUID
https://postgres.ai/console/gitlab/gitlab-production-tunnel/sessions/1758/commands/5930
Sample query
SELECT * FROM vulnerability_occurrences WHERE project_id = 23762767 AND uuid = '6892fc62-ed64-5494-bb8d-83e418f9deda' LIMIT 1;
Full execution plan
Limit (cost=0.56..3.58 rows=1 width=1131) (actual time=15.098..15.099 rows=0 loops=1)
Buffers: shared read=4
I/O Timings: read=15.031
-> Index Scan using index_vulnerability_occurrences_on_uuid on public.vulnerability_occurrences (cost=0.56..3.58 rows=1 width=1131) (actual time=15.096..15.097 rows=0 loops=1)
Index Cond: ((vulnerability_occurrences.uuid)::text = '6892fc62-ed64-5494-bb8d-83e418f9deda'::text)
Filter: (vulnerability_occurrences.project_id = 23762767)
Rows Removed by Filter: 0
Buffers: shared read=4
I/O Timings: read=15.031
Summary
Time: 15.297 ms
- planning: 0.167 ms
- execution: 15.130 ms
- I/O read: 15.031 ms
- I/O write: N/A
Shared buffers:
- hits: 0 from the buffer pool
- reads: 4 (~32.00 KiB) from the OS file cache, including disk I/O
- dirtied: 0
- writes: 0
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. - [-] Tested in all supported browsers
- [-] Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
- [-] Label as security and @ mention
@gitlab-com/gl-security/appsec
- [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
- [-] Security reports checked/validated by a reviewer from the AppSec team
Related to #292236 (closed)
Merge request reports
Activity
changed milestone to %13.9
added databasereview pending label
1 Message CHANGELOG missing: If you want to create a changelog entry for GitLab FOSS, run the following:
bin/changelog -m 52231 "Adjust \`Security::StoreReportService\` to look up findings using UUIDv5, attempt 2"
If you want to create a changelog entry for GitLab EE, run the following instead:
bin/changelog --ee -m 52231 "Adjust \`Security::StoreReportService\` to look up findings using UUIDv5, attempt 2"
If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.
Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.
To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
Category Reviewer Maintainer backend Pavel Shutsin ( @pshutsin
) (UTC+3)Dmitriy 'DZ' Zaporozhets ( @dzaporozhets
) (UTC+2)database Marius Bobin ( @mbobin
) (UTC+2)Toon Claes ( @toon
) (UTC+1)If needed, you can retry the
danger-review
job that generated this comment.Generated by
DangerEdited by 🤖 GitLab Bot 🤖mentioned in issue #292236 (closed)
- Resolved by Mayra Cabrera
Hi @alexkalderimis @alexbuijs - this is a second attempt for !50283 (merged) which failed after merge due to usage of
attributes_for
in some of the related specs. I opened up MRs for those tests in #299420 which I hope to have merged within 24 hours but I'd like to start the review process for this ASAP.I don't recall making any changes from the previous attempt but better to have another pair of eyes to comb through the changes.
Edited by Michał Zając
assigned to @alexkalderimis and @alexbuijs and unassigned @Quintasan
added databasereviewed label and removed databasereview pending label
assigned to @mayra-cabrera and unassigned @alexbuijs
- Resolved by Michał Zając
- Resolved by Michał Zając
Thanks @Quintasan! One question from database side
assigned to @Quintasan and unassigned @mayra-cabrera
assigned to @mayra-cabrera and unassigned @Quintasan
- Resolved by Michał Zając
- Resolved by Dmytro Zaporozhets (DZ)
This looks good to me, but I would want someone else with Domain expertise to sign off on the UUID components (they seem comprehensive, but I'm not familiar with findings), and I think it makes sense to also test the retrieval of a finding by UUID5
Back to you @Quintasan
assigned to @Quintasan and unassigned @alexkalderimis
added databaseapproved label and removed databasereviewed label
unassigned @mayra-cabrera
assigned to @minac and unassigned @Quintasan
- Resolved by Michał Zając
assigned to @Quintasan and unassigned @minac
added 4232 commits
-
7200a393...77dfaba7 - 4229 commits from branch
master
- 93fbc5e7 - Look up Findings by UUIDv5
- 9a877966 - Test UUIDv5 lookup
- eb7e5f43 - Perform the finding lookup twice on conflict
Toggle commit list-
7200a393...77dfaba7 - 4229 commits from branch
added 269 commits
-
eb7e5f43...c956f538 - 266 commits from branch
master
- 7bdd5c2b - Look up Findings by UUIDv5
- 382b0702 - Test UUIDv5 lookup
- 22e97b86 - Perform the finding lookup twice on conflict
Toggle commit list-
eb7e5f43...c956f538 - 266 commits from branch
assigned to @dzaporozhets and unassigned @Quintasan
changed milestone to %13.10
added missed:13.9 label
requested review from @dzaporozhets
- Resolved by Michał Zając
- Resolved by Michał Zając
@Quintasan thanks, the code looks good. My only suggestion would be to move v5 string building in some sort of method so we ensure its the same across all specs. I see it was already suggested in !52231 (comment 510158719). I believe it should be in same MR though as it directly related to the new code introduced here.
assigned to @Quintasan
unassigned @dzaporozhets
added 1014 commits
-
22e97b86...f962e3ad - 1010 commits from branch
master
- 53eb13e3 - Look up Findings by UUIDv5
- 90eb9613 - Test UUIDv5 lookup
- f990f0ed - Perform the finding lookup twice on conflict
- 64f33513 - Introduce Security::VulnerabilityUUID helper
Toggle commit list-
22e97b86...f962e3ad - 1010 commits from branch
added 138 commits
-
64f33513...48a16d4c - 134 commits from branch
master
- a942d9f3 - Look up Findings by UUIDv5
- 3018dbc4 - Test UUIDv5 lookup
- 91e7e999 - Perform the finding lookup twice on conflict
- 415453f3 - Introduce Security::VulnerabilityUUID helper
Toggle commit list-
64f33513...48a16d4c - 134 commits from branch
@dzaporozhets I added a helper method for the UUID calculation, back to you.
assigned to @dzaporozhets and unassigned @Quintasan
@Quintasan thanks, LGTM!
enabled an automatic merge when the pipeline for d8ccd6f3 succeeds
mentioned in commit 8b6129e7
added workflowstaging label and removed workflowin review label
added workflowcanary label and removed workflowstaging label
added releasedcandidate label