oauthdevice: Rename to `auth`; simplify interface #28

minor-fixes · 2024-08-14T21:38:05Z

This change simplifies the oauthdevice.Authenticator interface from
the two-step FetchCode + FetchToken to a combined single method
Authenticate that essentially chains the two code paths. The former
interface may over-fit the device code auth flow, whereas the new
interface may allow for non-device-code-flows to be implemented in the
future.

Since this change causes the interface to no longer be
device-code-specific, the former package name no longer makes sense.
This change results in the following renames:

package oauthdevice -> package auth
oauthdevice.Authenticator -> auth.Backend
oauthdevice.Auth -> auth.DeviceCode
oauthdevice.NewAuth -> auth.NewDeviceCode

This change moves the browser-opening step from main into
auth.DeviceCode as a consequence (which more accurately reflects
the reality that not all auth flows involve opening a browser). Tests
for main are simpler but tests for auth.DeviceCode are more complex as
a result.

Tests for auth.DeviceCode mock the underlying transport to perform
validation on the HTTP requests and responses, bringing some of the
oauth2 package logic into the test. This results in slight
over-testing, but may allow us to more easily validate engflow_auth
against actual HTTP responses from our oauth endpoint in the future.

The first dependency on github.com/stretchr/testify/mock requires us to add
github.com/stretchr/objx as a dependency, which requires a security approval

minor-fixes · 2024-08-15T16:47:08Z

note to reviewers: Don't feel especially strong about this - I just have a sneaking suspicion that the original interface is too narrow to be futureproof, and thought that the best time to change it was sooner rather than later

edit: Without this change, swapping code paths based on server version may be less clean

rogerhu · 2024-08-15T16:57:49Z

internal/auth/authenticator.go

+		if oauthErr := (*oauth2.RetrieveError)(nil); errors.As(err, &oauthErr) {
+			return nil, err
+		}
+		// BUG(CUS-320): Clusters that are not oauth-aware will return HTML with


btw, is this bug addressed now?

Dunno - do we have any non-oauth-aware backends running, or have we enabled the feature everywhere?

It got merged in https://github.com/EngFlow/engflow/pull/22227.

Will have to see if what version coral has deployed, but I think it's enabled for them.

The check should stay until no backends exist that can return this kind of response IMO

We could rephrase to say that older versions of EngFlow redirected unknown API requests to a login page that eventually responded with 200 and HTML. That shouldn't happen anymore, but we can still guard against it. The new behavior is to return 404 instead when OAuth2 device flow is disabled.

Ok, rephrased

rogerhu · 2024-08-15T17:56:37Z

Seems fine to me though it moves the control flow for opening browser into the interface.

@jayconrod should comment about Golang hygeines.

jayconrod · 2024-08-15T19:23:18Z

internal/auth/authenticator.go

+		if oauthErr := (*oauth2.RetrieveError)(nil); errors.As(err, &oauthErr) {
+			return nil, err
+		}
+		// BUG(CUS-320): Clusters that are not oauth-aware will return HTML with


We could rephrase to say that older versions of EngFlow redirected unknown API requests to a login page that eventually responded with 200 and HTML. That shouldn't happen anymore, but we can still guard against it. The new behavior is to return 404 instead when OAuth2 device flow is disabled.

jayconrod · 2024-08-15T19:24:36Z

internal/auth/authenticator.go

+
+var errUnexpectedHTML = errors.New("request to JSON API returned HTML unexpectedly")
+
+type Backend interface {


Exported definitions should have doc comments if their purpose isn't entirely clear from their names. Doubly so for types.

Added comments for Backend and DeviceCode; I think NewDeviceCode and DeviceCode.Authenticate are fairly obvious from these docs; WDYT?

Looks good. Types are the biggest thing.

jayconrod · 2024-08-15T19:25:52Z

internal/auth/authenticator.go

+	browserOpener browser.Opener
+	clientID      string
+	scopes        []string
+	httpTransport http.RoundTripper


It doesn't seem like this can be initialized by the constructor. What's it for?

Added a comment here, and explicitly set it to nil in the constructor so that it looks more intentional.

jayconrod · 2024-08-15T19:26:10Z

internal/auth/authenticator.go

+}
+
+func (d *DeviceCode) Authenticate(ctx context.Context, host *url.URL) (*oauth2.Token, error) {
+	if d.httpTransport != nil {


What's happening here?

Added a comment

jayconrod · 2024-08-15T19:30:28Z

internal/auth/authenticator_test.go

+	"golang.org/x/oauth2"
+)
+
+func AssertErrorContains(t *testing.T, got error, want string) {


Unexport: this can't be used by other packages.

jayconrod · 2024-08-15T19:31:09Z

go.mod

@@ -20,6 +20,7 @@ require (
 	github.com/godbus/dbus/v5 v5.1.0 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/russross/blackfriday/v2 v2.1.0 // indirect
+	github.com/stretchr/objx v0.5.2 // indirect


Is this covered by security review already?

Just checked - it's not :/

I created https://docs.google.com/spreadsheets/d/1uTKuur4qbow2tRw7qvRfmReSJWjzhfpKtUM3Zlh7Ppg/edit?gid=0#gid=0; will update the PR desc and add security

This change simplifies the `oauthdevice.Authenticator` interface from the two-step `FetchCode` + `FetchToken` to a combined single method `Authenticate` that essentially chains the two code paths. The former interface may over-fit the device code auth flow, whereas the new interface may allow for non-device-code-flows to be implemented in the future. Since this change causes the interface to no longer be device-code-specific, the former package name no longer makes sense. This change results in the following renames: `package oauthdevice` -> `package auth` `oauthdevice.Authenticator` -> `auth.Backend` `oauthdevice.Auth` -> `auth.DeviceCode` `oauthdevice.NewAuth` -> `auth.NewDeviceCode` This change moves the browser-opening step from `main` into `auth.DeviceCode` as a consequence (which more accurately reflects the reality that not all auth flows involve opening a browser). Tests for main are simpler but tests for `auth.DeviceCode` are more complex as a result. Tests for `auth.DeviceCode` mock the underlying transport to perform validation on the HTTP requests and responses, bringing some of the `oauth2` package logic into the test. This results in slight over-testing, but may allow us to more easily validate `engflow_auth` against actual HTTP responses from our oauth endpoint in the future.

jayconrod · 2024-08-16T18:22:48Z

(LGTM by me, but still needs security approval for the new dependency).

This change modifies the HTTP client used by the `DeviceCode` Authenticator implementation to send version information in each request. The version info is sent in the `User-Agent` header, using the format: engflow_auth/vX.Y.Z The version string is generated by the `buildstamp` library and is a semver string in a release binary, or a "pseudoversion" in a dev binary, similar go Go module pseudoversions. Tested: * Added unit tests * `bazel run //cmd/engflow_auth` against dev cluster fails (no buildstamp info) * `bazel run --stamp //cmd/engflow_auth` against dev cluster succeeds Bug: linear/CUS-387

minor-fixes force-pushed the scott/auth_interface branch 4 times, most recently from e3607a3 to 6398213 Compare August 15, 2024 16:41

minor-fixes marked this pull request as ready for review August 15, 2024 16:41

minor-fixes requested review from jayconrod and rogerhu August 15, 2024 16:43

rogerhu reviewed Aug 15, 2024

View reviewed changes

minor-fixes requested a review from rogerhu August 15, 2024 17:05

jayconrod reviewed Aug 15, 2024

View reviewed changes

minor-fixes force-pushed the scott/auth_interface branch from 6398213 to 10d8b09 Compare August 16, 2024 17:31

minor-fixes requested review from jayconrod and a team August 16, 2024 17:32

jayconrod approved these changes Aug 16, 2024

View reviewed changes

jayconrod closed this Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

oauthdevice: Rename to `auth`; simplify interface #28

oauthdevice: Rename to `auth`; simplify interface #28

minor-fixes commented Aug 14, 2024 •

edited

Loading

minor-fixes commented Aug 15, 2024 •

edited

Loading

rogerhu Aug 15, 2024 •

edited

Loading

minor-fixes Aug 15, 2024

rogerhu Aug 15, 2024 •

edited

Loading

minor-fixes Aug 15, 2024 •

edited

Loading

jayconrod Aug 15, 2024

minor-fixes Aug 16, 2024

rogerhu commented Aug 15, 2024

jayconrod Aug 15, 2024

jayconrod Aug 15, 2024

minor-fixes Aug 16, 2024

jayconrod Aug 16, 2024

jayconrod Aug 15, 2024

minor-fixes Aug 16, 2024

jayconrod Aug 15, 2024

minor-fixes Aug 16, 2024

jayconrod Aug 15, 2024

minor-fixes Aug 16, 2024

jayconrod Aug 15, 2024

minor-fixes Aug 16, 2024

jayconrod commented Aug 16, 2024


		var errUnexpectedHTML = errors.New("request to JSON API returned HTML unexpectedly")

		type Backend interface {

oauthdevice: Rename to auth; simplify interface #28

oauthdevice: Rename to auth; simplify interface #28

Conversation

minor-fixes commented Aug 14, 2024 • edited Loading

minor-fixes commented Aug 15, 2024 • edited Loading

rogerhu Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerhu Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

minor-fixes Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerhu commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayconrod commented Aug 16, 2024

oauthdevice: Rename to `auth`; simplify interface #28

oauthdevice: Rename to `auth`; simplify interface #28

minor-fixes commented Aug 14, 2024 •

edited

Loading

minor-fixes commented Aug 15, 2024 •

edited

Loading

rogerhu Aug 15, 2024 •

edited

Loading

rogerhu Aug 15, 2024 •

edited

Loading

minor-fixes Aug 15, 2024 •

edited

Loading