Auth GitHub in Init

[rosecodym]

Description:

The GitHub source currently applies its authentication configuration as the first step of enumeration. This is incompatible with both targeted scans and scan job reports, and also means that authentication logic has to be duplicated into the validation flow. This PR moves it into Init so that it's available to targeted scans and, eventually, unit-specific scans. This also allows us to remove the copy of the old logic that was in Validate.

As part of the work I've also cleaned up the integration test suite. (Several of them were apparently disabled back when they ran on every push, but now that we're not doing that, we can re-enable them.)

Checklist:

• Tests passing (make test-community)?
• Lint passing (make lint this requires golangci-lint)?

[rosecodym]

The big change in this PR is the removal of connection setup logic from the enumeration logic. They used to be heavily intertwined, which led to weird things like enumerate returning a GitHub App installation client. (This client was nil for all non-app-based configurations.)

[rosecodym]

Apparently, this app doesn't have access to gists - even public ones. However, the test was incidentally passing because it never actually applied this authentication configuration. (That used to happen during enumeration, but this test never enumerated!) So when I moved the authentication to Init, the test started breaking.

Rather than fiddle with the app permissions I just changed the authentication configuration to match what the old test had (inadvertently) been doing.

[rosecodym]

This test was commented-out when it was originally committed. The method it's trying to test doesn't exist, so I just deleted it.

[rosecodym]

Since this repo is public, this test wasn't originally actually testing the authentication configuration that was specified. Changing it to a private repo (visible to the configured credentials) means the logic under test is actually exercised.

[rosecodym]

This test case (and the two following) are trying to scan public repos, so I removed the (effectively unused) authentication configuration to reduce future confusion.

[rosecodym]

This removed code was testing implementation details that no longer exist.

[mcastorina]

This is a really great step towards cleaning up the GitHub source!

[mcastorina]

Nit: This might just be my personal preference, but when checking for empty strings I think it's better to do apiEndpoint == "". It's less ambiguous since len can be used for slices and maps too, so it makes it obvious that apiEndpoint is a string.

[mcastorina]

Nit: I think Go's naming conventions suggest capitalizing acronyms so this would be APIClient.

Reference

[mcastorina]

Nit: inverting this statement will let us de-indent most of the function:

if c.user != "" {
    return nil
}

if user, err := c.getUser(ctx); err != nil {
    return err
} else {
    c.user = user
}
return nil

or even

if c.user != "" {
    return nil
}

user, err := c.getUser(ctx)
if err != nil {
    return err
}

c.user = user
return nil

[mcastorina]

Nit: normally I wouldn't suggest using a constant for the 60, but since it's unit-less I think it would be a good idea to have something like:

const httpTimeoutSeconds = 60
httpClient := common.RetryableHTTPClientTimeout(httpTimeoutSeconds)

[mcastorina]

Love this abstraction!

[mcastorina]

I agree with this comment. Currently enumeration is different based on how we access GitHub, but I wonder if in the future we could always enumerate the same way and ignore any errors. Maybe that's a bit wasteful with rate-limits though.. 🤔