Skip to content

Commit

Permalink
oauthdevice: Rename to auth; simplify interface
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@minor-fixes
minor-fixes committed Aug 16, 2024
1 parent 3751600 commit 10d8b09
Show file tree
Hide file tree
Showing 9 changed files with 322 additions and 190 deletions.
5 changes: 2 additions & 3 deletions cmd/engflow_auth/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@ go_library(
importpath = "github.com/EngFlow/auth/cmd/engflow_auth",
visibility = ["//visibility:private"],
deps = [
"//internal/auth",
"//internal/autherr",
"//internal/browser",
"//internal/buildstamp",
"//internal/oauthdevice",
"//internal/oauthtoken",
"@com_github_engflow_credential_helper_go//:credential-helper-go",
"@com_github_urfave_cli_v2//:cli",
Expand All @@ -29,9 +29,8 @@ go_test(
srcs = ["main_test.go"],
embed = [":engflow_auth_lib"],
deps = [
"//internal/auth",
"//internal/autherr",
"//internal/browser",
"//internal/oauthdevice",
"//internal/oauthtoken",
"@com_github_stretchr_testify//assert",
"@org_golang_x_oauth2//:oauth2",
Expand Down
29 changes: 4 additions & 25 deletions cmd/engflow_auth/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ import (
"strings"
"time"

"github.com/EngFlow/auth/internal/auth"
"github.com/EngFlow/auth/internal/autherr"
"github.com/EngFlow/auth/internal/browser"
"github.com/EngFlow/auth/internal/buildstamp"
"github.com/EngFlow/auth/internal/oauthdevice"
"github.com/EngFlow/auth/internal/oauthtoken"
"github.com/urfave/cli/v2"

Expand All @@ -44,8 +44,7 @@ const (
)

type appState struct {
browserOpener browser.Opener
authenticator oauthdevice.Authenticator
authenticator auth.Backend
tokenStore oauthtoken.LoadStorer
}

Expand Down Expand Up @@ -194,7 +193,7 @@ func (r *appState) login(cliCtx *cli.Context) error {
}
}

authRes, err := r.authenticator.FetchCode(ctx, oauthURL)
token, err := r.authenticator.Authenticate(ctx, clusterURL)
if err != nil {
if oauthErr := (*oauth2.RetrieveError)(nil); errors.Is(err, autherr.UnexpectedHTML) || errors.As(err, &oauthErr) {
return autherr.CodedErrorf(
Expand All @@ -209,24 +208,6 @@ Visit %s for help.`,
}
return autherr.CodedErrorf(autherr.CodeAuthFailure, "failed to generate device code: %w", err)
}
// The "complete" URI that includes the device code pre-populated is ideal,
// but technically optional. Prefer it, but fall back to the required URL in
// the response if necessary.
verificationURLStr := authRes.VerificationURIComplete
if verificationURLStr == "" {
verificationURLStr = authRes.VerificationURI
}
verificationURL, err := url.Parse(verificationURLStr)
if err != nil {
return autherr.CodedErrorf(autherr.CodeAuthFailure, "failed to parse authentication URL: %w", err)
}
if err := r.browserOpener.Open(verificationURL); err != nil {
return autherr.CodedErrorf(autherr.CodeAuthFailure, "failed to open browser to perform authentication: %w", err)
}
token, err := r.authenticator.FetchToken(ctx, authRes)
if err != nil {
return autherr.CodedErrorf(autherr.CodeAuthFailure, "failed to obtain auth token: %w", err)
}

var storeErrs []error
for _, storeURL := range storeURLs {
Expand Down Expand Up @@ -356,14 +337,12 @@ func main() {
ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt)
defer cancel()

deviceAuth := oauthdevice.NewAuth(cliClientID, nil)
browserOpener := &browser.StderrPrint{}
deviceAuth := auth.NewDeviceCode(&browser.StderrPrint{}, cliClientID, nil)
tokenStore, err := oauthtoken.NewKeyring()
if err != nil {
exitOnError(autherr.CodedErrorf(autherr.CodeTokenStoreFailure, "failed to open token store: %w", err))
}
root := &appState{
browserOpener: browserOpener,
authenticator: deviceAuth,
tokenStore: oauthtoken.NewCacheAlert(tokenStore, os.Stderr),
}
Expand Down
94 changes: 7 additions & 87 deletions cmd/engflow_auth/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,8 @@ import (
"testing"
"time"

"github.com/EngFlow/auth/internal/auth"
"github.com/EngFlow/auth/internal/autherr"
"github.com/EngFlow/auth/internal/browser"
"github.com/EngFlow/auth/internal/oauthdevice"
"github.com/EngFlow/auth/internal/oauthtoken"
"github.com/stretchr/testify/assert"
"golang.org/x/oauth2"
Expand Down Expand Up @@ -78,27 +77,13 @@ func codedErrorContains(t *testing.T, gotErr error, code int, wantMsg string) bo
}

type fakeAuth struct {
res *oauth2.DeviceAuthResponse
fetchCodeErr error
fetchTokenErr error
}

func (f *fakeAuth) FetchCode(ctx context.Context, authEndpint *oauth2.Endpoint) (*oauth2.DeviceAuthResponse, error) {
return f.res, f.fetchCodeErr
}

func (f *fakeAuth) FetchToken(ctx context.Context, authRes *oauth2.DeviceAuthResponse) (*oauth2.Token, error) {
func (f *fakeAuth) Authenticate(ctx context.Context, host *url.URL) (*oauth2.Token, error) {
return nil, f.fetchTokenErr
}

type fakeBrowser struct {
openErr error
}

func (f *fakeBrowser) Open(u *url.URL) error {
return f.openErr
}

func TestRun(t *testing.T) {
expiresInFuture := time.Now().AddDate(0, 0, 7).UTC()
expiryFormat := "2006-01-02T15:04:05Z"
Expand All @@ -108,9 +93,8 @@ func TestRun(t *testing.T) {
args []string

machineInput io.Reader
authenticator oauthdevice.Authenticator
authenticator auth.Backend
tokenStore oauthtoken.LoadStorer
browserOpener browser.Opener

wantCode int
wantErr string
Expand Down Expand Up @@ -211,20 +195,10 @@ func TestRun(t *testing.T) {
{
desc: "login happy path",
args: []string{"login", "cluster.example.com"},
authenticator: &fakeAuth{
res: &oauth2.DeviceAuthResponse{
VerificationURIComplete: "https://cluster.example.com/with/auth/code",
},
},
},
{
desc: "login with alias",
args: []string{"login", "--alias", "cluster.local.example.com", "cluster.example.com"},
authenticator: &fakeAuth{
res: &oauth2.DeviceAuthResponse{
VerificationURIComplete: "https://cluster.example.com/with/auth/code",
},
},
desc: "login with alias",
args: []string{"login", "--alias", "cluster.local.example.com", "cluster.example.com"},
tokenStore: oauthtoken.NewFakeTokenStore(),
wantStored: []string{
"cluster.example.com",
Expand All @@ -234,11 +208,6 @@ func TestRun(t *testing.T) {
{
desc: "login with alias with store errors",
args: []string{"login", "--alias", "cluster.local.example.com", "cluster.example.com"},
authenticator: &fakeAuth{
res: &oauth2.DeviceAuthResponse{
VerificationURIComplete: "https://cluster.example.com/with/auth/code",
},
},
tokenStore: &oauthtoken.FakeTokenStore{
StoreErr: errors.New("token_store_fail"),
},
Expand All @@ -248,38 +217,18 @@ func TestRun(t *testing.T) {
{
desc: "login with host and port",
args: []string{"login", "cluster.example.com:8080"},
authenticator: &fakeAuth{
res: &oauth2.DeviceAuthResponse{
VerificationURIComplete: "https://cluster.example.com:8080/with/auth/code",
},
},
},
{
desc: "login with invalid scheme",
args: []string{"login", "grpcs://cluster.example.com:8080"},
wantCode: autherr.CodeBadParams,
wantErr: "illegal scheme",
},
{
desc: "login code fetch failure",
args: []string{"login", "cluster.example.com"},
authenticator: &fakeAuth{
res: &oauth2.DeviceAuthResponse{
VerificationURIComplete: "https://cluster.example.com/with/auth/code",
},
fetchCodeErr: errors.New("fetch_code_fail"),
},
wantCode: autherr.CodeAuthFailure,
wantErr: "fetch_code_fail",
},
{
desc: "login code fetch RetrieveError",
args: []string{"login", "cluster.example.com"},
authenticator: &fakeAuth{
res: &oauth2.DeviceAuthResponse{
VerificationURIComplete: "https://cluster.example.com/with/auth/code",
},
fetchCodeErr: &oauth2.RetrieveError{},
fetchTokenErr: &oauth2.RetrieveError{},
},
wantCode: autherr.CodeAuthFailure,
wantErr: "This cluster may not support 'engflow_auth login'.\nVisit https://cluster.example.com/gettingstarted for help.",
Expand All @@ -288,35 +237,15 @@ func TestRun(t *testing.T) {
desc: "login code fetch unexpected HTML",
args: []string{"login", "cluster.example.com"},
authenticator: &fakeAuth{
res: &oauth2.DeviceAuthResponse{
VerificationURIComplete: "https://cluster.example.com/with/auth/code",
},
fetchCodeErr: autherr.UnexpectedHTML,
fetchTokenErr: autherr.UnexpectedHTML,
},
wantCode: autherr.CodeAuthFailure,
wantErr: "This cluster may not support 'engflow_auth login'.\nVisit https://cluster.example.com/gettingstarted for help.",
},
{
desc: "login browser open failure",
args: []string{"login", "cluster.example.com"},
authenticator: &fakeAuth{
res: &oauth2.DeviceAuthResponse{
VerificationURIComplete: "https://cluster.example.com/with/auth/code",
},
},
browserOpener: &fakeBrowser{
openErr: errors.New("browser_open_fail"),
},
wantCode: autherr.CodeAuthFailure,
wantErr: "browser_open_fail",
},
{
desc: "login token fetch failure",
args: []string{"login", "cluster.example.com"},
authenticator: &fakeAuth{
res: &oauth2.DeviceAuthResponse{
VerificationURIComplete: "https://cluster.example.com/with/auth/code",
},
fetchTokenErr: errors.New("fetch_token_fail"),
},
wantCode: autherr.CodeAuthFailure,
Expand All @@ -325,11 +254,6 @@ func TestRun(t *testing.T) {
{
desc: "login token store failure",
args: []string{"login", "cluster.example.com"},
authenticator: &fakeAuth{
res: &oauth2.DeviceAuthResponse{
VerificationURIComplete: "https://cluster.example.com/with/auth/code",
},
},
tokenStore: &oauthtoken.FakeTokenStore{
StoreErr: errors.New("token_store_fail"),
},
Expand Down Expand Up @@ -494,13 +418,9 @@ func TestRun(t *testing.T) {
stderr := bytes.NewBuffer(nil)

root := &appState{
browserOpener: tc.browserOpener,
authenticator: tc.authenticator,
tokenStore: tc.tokenStore,
}
if root.browserOpener == nil {
root.browserOpener = &fakeBrowser{}
}
if root.authenticator == nil {
root.authenticator = &fakeAuth{}
}
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
github.com/xrash/smetrics v0.0.0-20240312152122-5f08fbb34913 // indirect
golang.org/x/sys v0.8.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
Expand Down
25 changes: 25 additions & 0 deletions internal/auth/BUILD
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
load("@rules_go//go:def.bzl", "go_library", "go_test")

go_library(
name = "auth",
srcs = ["authenticator.go"],
importpath = "github.com/EngFlow/auth/internal/auth",
visibility = ["//:__subpackages__"],
deps = [
"//internal/autherr",
"//internal/browser",
"@org_golang_x_oauth2//:oauth2",
],
)

go_test(
name = "auth_test",
srcs = ["authenticator_test.go"],
embed = [":auth"],
deps = [
"//internal/browser",
"@com_github_stretchr_testify//assert",
"@com_github_stretchr_testify//mock",
"@org_golang_x_oauth2//:oauth2",
],
)
Loading

0 comments on commit 10d8b09

Please sign in to comment.