Remove fallback to global credentials in batches#41078
Conversation
7748b2a to
8fc9580
Compare
|
|
||
| // Construct changeset source. | ||
| b.css, err = b.sourcer.ForRepo(ctx, b.tx, b.repo) | ||
| b.css, err = b.sourcer.ForUser(ctx, b.tx, job.UserID, b.repo) |
There was a problem hiding this comment.
No behavioral change here, we still only allow using credentials from the initiating user, or global credentials.
| } | ||
|
|
||
| css, err = sources.WithAuthenticatorForChangeset(ctx, s, css, ch, repo, false) | ||
| css, err := sourcer.ForChangeset(ctx, s, ch) |
There was a problem hiding this comment.
The ForChangeset helper now only returns authenticated sources, so we don't need to do the authentication as step 2. The algorithm for determining it is unchanged.
Nice side-effect: Not possible to forget authentication anymore :)
|
|
||
| css, err := s.sourcer.ForExternalService(ctx, s.store, store.GetExternalServiceIDsOpts{ | ||
| // Get a changeset source for the external service and use the given authenticator. | ||
| css, err := s.sourcer.ForExternalService(ctx, s.store, &auth.OAuthBearerToken{Token: token}, store.GetExternalServiceIDsOpts{ |
There was a problem hiding this comment.
No behavioral change here, we still only allow using the current authenticator. No fallback, just consolidated into one call to make sure it's not possible to forget and accidentally fall back to external service auth.
| } | ||
|
|
||
| css, err := s.sourcer.ForExternalService(ctx, s.store, store.GetExternalServiceIDsOpts{ | ||
| // Get a changeset source for the external service and use the given authenticator. |
There was a problem hiding this comment.
No behavioral change here, we still only allow using the current authenticator. No fallback, just consolidated into one call to make sure it's not possible to forget and accidentally fall back to external service auth.
58a8f53 to
9d089f7
Compare
There was a problem hiding this comment.
this test is fully covered by the sourcer testsuite
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff f963915...6a5ce2d.
|
courier-new
left a comment
There was a problem hiding this comment.
The changes read SO WELL! It's a loooot clearer from the code how we use the two different authentication types, now. Thanks for the added code comments, as well! 🙌
There was a problem hiding this comment.
This is something I haven't thought much about before, that a repo could be tracked by multiple external services. I feel like I remember this coming up in conversation before once but it's very fuzzy, and I'm curious but having a bit of trouble tracing in the code how we handle this in the context of Batch Changes!
I see that in loadExternalService we prioritize a globally-configured external service over a user-owned one, but after that it looks like we just return the first one that's for a BC-compatible code host type. Does that mean that even if a repo is associated with many external services across multiple different code hosts, Batch Changes will only ever acknowledge it on the (compatible) external service that was added to Sourcegraph first?
There was a problem hiding this comment.
Yes, that's correct. That's always been the case and is a problem of our external service model - they all have config flags for the code host, which technically should be the same on all configs but mightn't be. Like the TLS cert for a GHE instance. If one of them doesn't have it but they need it, it would be misconfiguration, but we don't really surface that well. 2 years ago Thorsten and I pitched to introduce a code host model to the product that would capture all global settings for it, like URL, TLS, ssh mode etc, and then an external service would be just the auth part and the repo scope. Then batch changes would only look at the code host for creating the client but.. we don't have that so that's the best we can do for now :)
Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
…ert.tsx Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
8a3231f to
a005279
Compare
This PR completes the deprecation of external service tokens to be used with batch changes.
From now on, all
ChangesetSourcesreturned fromSourcermethods are ensured to be properly authenticated with either a site credential or user credential, where applicable. This will also make sure that we will never accidentally use other tokens. That also means that we don't need so strict constraints on the external service anymore, so we can simply read therepo.Sources.CloneURLfield and don't have to check if an external service has a token configured. Overall, the logic got much simpler and I was able to eliminate many now redundant tests, as they all just use the already tested sourcer methods now.Closes https://github.com/sourcegraph/sourcegraph/issues/25394
Test plan
Verified locally that things still work, also adjusted the test suite accordingly.