Rename PackageVulnerability's ReferenceUrl to AdvisoryUrl and make it required #7721
Conversation
xavierdecoster
requested review from
joelverhagen,
agr,
loic-sharma,
shishirx34,
ryuyu,
cristinamanum,
skofman1 and
zhhyu
xavierdecoster
requested review from
dannyjdev and
scottbommarito
as code owners
xavierdecoster
force-pushed
the
dev-issue7720
branch
from
ca13a0f to
d2e356c
Compare
xavierdecoster
changed the title
| @@ -41,7 +41,7 @@ public async Task IngestAsync(IReadOnlyList<SecurityAdvisory> advisories) | |||
| { | |||
| GitHubDatabaseKey = advisory.DatabaseId, | |||
| Severity = (PackageVulnerabilitySeverity)Enum.Parse(typeof(PackageVulnerabilitySeverity), advisory.Severity, ignoreCase: true), | |||
| ReferenceUrl = advisory.References.FirstOrDefault()?.Url | |||
| AdvisoryUrl = advisory.GetPermalink() | |||
| }; | |||
There was a problem hiding this comment.
Why do we persist the whole URL? Isn't this a view concern? Seems more flexible in the future to just persist what we know (GhsaId) and allow the caller to choose what to do with it (i.e. call another API, build a URL, etc).
There was a problem hiding this comment.
We could do both, if desired. Though the entity is meant to be GitHub-agnostic...
There was a problem hiding this comment.
Note that both GhsaId and Permalink will be fields retrieved from the GH GraphQL API. I think we are only interested in the AdvisoryUrl, which is what we'll be surfacing in our own protocol and UI.
There was a problem hiding this comment.
Okay, I thought we already non-generic with GitHubDatabaseKey. Should the be "ExternalKey" instead?
There was a problem hiding this comment.
Yeah... I'm kinda wondering about the usefulness of that GitHubDatabaseKey...
It's the one thing GH guaranteed that wouldn't change. But the GraphQL API is using the GhsaId to query for a specific advisory.
See also this comment added by Scott when he modeled the entity.
There was a problem hiding this comment.
I guess it would come down to having some Provider column (in this case GitHub would be the value) so we can identify the source of the data, plus a generic external key. Or alternatively have the approach that Scott described of using a key per provider (see his comment).
xavierdecoster
force-pushed
the
dev-issue7720
branch
from
d2e356c to
3320b90
Compare
The
PackageVulnerability.ReferenceUrlfield is mandatory and should be renamed toPackageVulnerability.AdvisoryUrl. The current db model assumes it is optional. We should enforce this restriction in the db model.In addition, the protocol and ubiqituous language for this feature have been updated:
ReferenceUrlis actually anAdvisoryUrl(which points to a resource to learn more about the security advisory, which in turn may have references to e.g. NIST database)permalinkfield on theSecurityAdvisoryobject, but that's still a few months out. In the mean time, we can work-around it by using the permalink-format provided to us, which purposely was put into an extension method with a comment.Addresses #7720
Rollout plan:
GitHubVulnerabilities2Dbjob on DEV and INTGitHubVulnerabilties2Dbjob to DEV and INT