Issue/2323: Filter portfolio metrics in portfolio access control mode to match team's accesses

[mulder999]

Description

Filter portfolio metrics in portfolio access control mode to match team's accesses

Addressed Issue

Fixes #2323

Additional Details

Sum in real time the matching project's metrics

Checklist

• I have read and understand the contributing guidelines
• This PR fixes a defect, and I have provided tests to verify that the fix is effective
• This PR implements an enhancement, and I have provided tests to verify that it works as intended
• This PR introduces changes to the database model, and I have added corresponding update logic
• This PR introduces new or alters existing behavior, and I have updated the documentation accordingly

[valentijnscholten]

Looks smart as it avoids having to do a complexer query to get exactly the latest metrics for each project in one go.

[mulder999]

Sweet. Thanks @valentijnscholten for all the comments. Let me know if you see anything else.

[valentijnscholten]

Thinking out loud here. Could/should it be a background job that gathers the metrics for each users?
I was about to say for each Team, but you can have users that are in no team. The metrics for all projects are already calculated, so just adding them up for each user should be quite fast, even for 1000 users? Not sure what kind of numbers DT aims to support without requiring enterprise funding :-)
The downside might be that on every project metrics update, all users with access to that project need their metrics updated as well.

In Defect Dojo we had these type of metrics all calculated on the fly (by the DB) and that performed quite well even for 1M vulnerabilties.

[mulder999]

Thinking out loud here. Could/should it be a background job that gathers the metrics for each users? I was about to say for each Team, but you can have users that are in no team. The metrics for all projects are already calculated, so just adding them up for each user should be quite fast, even for 1000 users? Not sure what kind of numbers DT aims to support without requiring enterprise funding :-) The downside might be that on every project metrics update, all users with access to that project need their metrics updated as well.

In Defect Dojo we had these type of metrics all calculated on the fly (by the DB) and that performed quite well even for 1M vulnerabilties.

Adding metrics computation should be ultra fast indeed. I tend to believe building and maintaining any sort of long term cache would not be useful for this usage. However a short term cache (redis alike) when the user is currently connected and navigating back and force could be useful to avoid repeating same query over and over.

[nscuro]

@valentijnscholten Could/should it be a background job that gathers the metrics for each users?

I fear that would not scale well. Metrics are already one of the main bottlenecks we have when it comes to exhausting the thread pool.

In Defect Dojo we had these type of metrics all calculated on the fly (by the DB) and that performed quite well even for 1M vulnerabilties.

Would you be willing to share some more insights, or links to where this is done in DD? Maybe there are some improvements we can make to the data model so it performs better for these ad-hoc queries.

@mulder999 Adding metrics computation should be ultra fast indeed. [...] However a short term cache (redis alike) when the user is currently connected and navigating back and force could be useful to avoid repeating same query over and over.

The "ultra fast" part we'd have to test. But agreed, caching would be a good way to optimize. Even such short-term storage could be realized using the database, we don't necessarily have to introduce a dependency on a 3rd party cache.

[valentijnscholten]

Would you be willing to share some more insights, or links to where this is done in DD? Maybe there are some improvements we can make to the data model so it performs better for these ad-hoc queries.

The thing in the datamodel that does look weird to me is the alias part, that's gonna give a bit of trouble I think for some usecases. I think it doesn't lend itself very well to let the database handle the counting (and other queries that need to deduplicate aliases, etc). From a vulnerability record you can't easily get all its ids/aliases as you have to do a lot of comparisons in code (or queries):

If the aliases table was more like a join table to map vuln_ids onto vulnerabilities and with source and vuln_id I think all the joining and counting and deduplication would become a lot easier. If you truely want to deduplicate vulnerabilities based on their aliases, it might be best to model that data like that:

Don't have an ER modeling tool handy, but the idea is to make vulnerabilities unique. And have multiple aliases attached to the vulnerability. So if a vulnerability has multiple aliases, you have multiple records. The vuln_id is then CVExxxx, GHSAxxx, depending on the source. Thinking out loud there could be sources that report multiple ids/aliases for the same vulnerability, if needed those could be duplicated:

Just thinking out loud here, so don't pick on the details :-)

About DD: DD has a datamodel similar to the above where a unique vulnerability is one database records with multiple aliase records (ids) attached to it in a separate table.
In DD you have Product Type -> Product -> Engagement -> Test -> Finding. So 5 tables joined together. Even with 1M findings and ~10000 tests and 1000 engagements and 100 products, this performs quite OK when doing a SELECT COUNT(ID) FROM .... JOIN ..... JOIN .... GROUP BY Product for example. IIRC the query takes about 500ms on a stock mysql running on my laptop and that mysql instance has not been tuned or anything.

The queries in DT would look quite similar, just count a bunch of vulnerabilities and group them by project and maybe severity. Even with 10000 projects and 1M vulnerabilities it should be quite fast. Unless there's something very different from the DD case.
Even if the query takes 1s or 5s or 10s it would still be OK for the background job to populate the metrics and keep track of historical metrics. When a user logs in it should still be quite fast to just query that single table with metrics, even if the user has 10000 projects in its ACL. Just make sure the ACL is handled by WHERE/JOIN clauses and not 10000 projects loaded into memory and then serialized in a list of 10000 ids.

In DD permissions are more complex and involve some more joining for users that are not superusers/admins. Similar to the ACL part in DT.

[stevespringett]

@valentijnscholten join tables with foreign keys were specifically not chosen as a design pattern due to the performance impact they will have.

[valentijnscholten]

I am not convinced doing counting and alias handling in code and doing 1+N queries or N * M queries is going to be faster than 1 or 2 extra joins. There's also great benefit in simpler code and simpler queries.

[valentijnscholten]

@valentijnscholten Could/should it be a background job that gathers the metrics for each users?

I fear that would not scale well. Metrics are already one of the main bottlenecks we have when it comes to exhausting the thread pool.
If you want the "filtered by ACL" metrics to also show the historical metrics, then some kind of background task is needed that records these historical metrics. An alternative would be if the portfolio metrics were shown in realtime based on an aggregation of the project metrics. But that would mean metrics of deleted projects are no longer shown in the historical portfolio metrics. And we would need some kind of logic to aggregate the project metrics timeseries as timestamps might differ between projects.

[melba-lopez]

@valentijnscholten its been some time since this PR has been updated. Can this PR be closed? -If not, could you update your branch?-

[valentijnscholten]

This is not my PR.

[melba-lopez]

I understand @valentijnscholten. You were very active in this PR and was not sure if there were other conversations happening behind the scenes with @mulder999. @mulder999 please let me know if you will be updating this PR or if I should close it.

[mulder999]

Hi @melba-lopez, thanks for asking. The PR was complete but did not reach community consensus. Would be happy to solve conflicts if it has any chance to be considered for merge in the trunk.

[mulder999]

@melba-lopez PR has been updated. Anything else needed to remove the "pending more information" tag ?

[melba-lopez]

@nscuro @stevespringett i know there were several discussions previously about how this may not work for DT. Can you chime in and let us know if there is anything else you would like to modify?

[mulder999]

@melba-lopez @nscuro @stevespringett The feature is complete and can now also conveniently be enabled from the web interface (default setting: disabled).