feat: add new parameter source_uuid to secret scan

[alina-tuholukova-gg]

Add new parameter source_uuid, when provided, the scan will also create incidents on GIM dashboard

Context

When calling ggshield secret scan with --source-uuid, the scan will also create incidents on GIM dashboard.

Validation

To validate change:

• Create a custom source on GIM
• Copy its uuid
• Call ggshield secret scan with --source-uuid on some documents that have secrets
• Check that the corresponding issues are created on GIM

PR check list

• As much as possible, the changes include tests (unit and/or functional)
• If the changes affect the end user (new feature, behavior change, bug fix) then the PR has a changelog entry (see doc/dev/getting-started.md). If the changes do not affect the end user, then the skip-changelog label has been added to the PR.

[alina-tuholukova-gg]

this is just to check the pipeline with the new py-gitguardian

[agateau-gg]

In certain situations, for example when scanning commit ranges, ggshield creates multiple SecretScanner instances. This is why we have the if check_api_key: at the start of the method: we only check the API key validity if it has not already be done by the caller.

I think these new checks should be done in check_client_api_key(). It does not have access to the config though, so you need to extend the function to accept a source_uuid optional argument.

[agateau-gg]

Sorry, looking at the code, I realize my suggestion was not very good. Plus there is already some code that calls client.api_tokens() in SecretScanner.__init__(), which is not ideal.

Here is a better (I think, but do tell me if you disagree!) suggestion:

• Change check_client_api_key() to take a mandatory second argument that would be a set of the required scopes.

• Change check_client_api_key() callers to pass this second argument:

  • oauth.py can pass an empty set (it only cares if the key is valid)
  • secret_scanner.py and repo.py can generate the required set doing something like this:

scopes = {TokenScope.SCAN}
if secret_config.with_incident_details:
    scopes.add(TokenScope.INCIDENTS_READ)
if secret_config.source_uuid:
    scopes.add(TokenScope.SCAN_CREATE_INCIDENTS)
check_client_api_key(client, scopes)

What do you think?

[corenting]

do we want to officially document the option for now, or do we prefer to mark it as beta or hidden?

[mathieubellon]

I prefer we officially document and mark it as beta

[alina-tuholukova-gg]

Sorry, looking at the code, I realize my suggestion was not very good. Plus there is already some code that calls client.api_tokens() in SecretScanner.__init__(), which is not ideal.

Here is a better (I think, but do tell me if you disagree!) suggestion:

• Change check_client_api_key() to take a mandatory second argument that would be a set of the required scopes.

• Change check_client_api_key() callers to pass this second argument:

  • oauth.py can pass an empty set (it only cares if the key is valid)
  • secret_scanner.py and repo.py can generate the required set doing something like this:

scopes = {TokenScope.SCAN}
if secret_config.with_incident_details:
    scopes.add(TokenScope.INCIDENTS_READ)
if secret_config.source_uuid:
    scopes.add(TokenScope.SCAN_CREATE_INCIDENTS)
check_client_api_key(client, scopes)

What do you think?

I did the change, tell me if it's what you had in mind :)
When the MR is ok for you, I'll rebase

[agateau-gg]

Looks nicer now. Just one last change and this can go in.

[agateau-gg]

nitpick: typos

Suggested change

	dashboard on the correspoding source. Note, that the token should have the scope `scan:create-incidents`.
	dashboard on the corresponding source. Note that the token should have the scope `scan:create-incidents`.

[agateau-gg]

It's a good idea to factorize this code, but I would prefer if it were kept in the secret vertical. Maybe you can move this function in verticals/secret_scanner.py?

[agateau-gg]

Looks good, thanks!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.95%. Comparing base (917c935) to head (57d8f9e).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1109      +/-   ##
==========================================
+ Coverage   91.90%   91.95%   +0.05%     
==========================================
  Files         144      144              
  Lines        6102     6130      +28     
==========================================
+ Hits         5608     5637      +29     
+ Misses        494      493       -1     

Flag	Coverage Δ
unittests	91.95% <100.00%> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.