Skip to content

Commit

Permalink
CUS-384: fix -store flag, make test less fake-reliant (#40)
Browse files Browse the repository at this point in the history
In #37, we thought App.Before was called after subcommand flag parsing,
but it is not. This meant the -store flag did not work. We need the
flags
to be parsed before we construct the object graph in appState.build.
Instead, call appState.build from the Command.Before hook.

Our tests in main_test.go should have caught this, but they fake
appState.tokenStore so the -store flag is ignored. This PR now makes
the test use the real LoadStorer implementations in most cases, but
calls keyring.MockInit ahead of time and uses a temporary user
configuration
directory to avoid side effects.
  • Loading branch information
jayconrod authored Aug 23, 2024
1 parent 1363f11 commit d29b577
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 12 deletions.
1 change: 1 addition & 0 deletions cmd/engflow_auth/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ go_test(
"//internal/oauthdevice",
"//internal/oauthtoken",
"@com_github_stretchr_testify//assert",
"@com_github_zalando_go_keyring//:go-keyring",
"@org_golang_x_oauth2//:oauth2",
],
)
Expand Down
30 changes: 21 additions & 9 deletions cmd/engflow_auth/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ const (
type appState struct {
// These vars are initialized by `build()` iff they are not pre-populated;
// they should be pre-populated in tests and left nil otherwise.
userConfigDir string
browserOpener browser.Opener
authenticator oauthdevice.Authenticator
tokenStore oauthtoken.LoadStorer
Expand All @@ -65,10 +66,13 @@ type ExportedToken struct {
}

func (r *appState) build(cliCtx *cli.Context) error {
if cliCtx.NArg() < 1 {
return autherr.CodedErrorf(autherr.CodeUnknownSubcommand, "no subcommand provided; expected at least one subcommand")
if r.userConfigDir == "" {
configDir, err := os.UserConfigDir()
if err != nil {
return autherr.CodedErrorf(autherr.CodeTokenStoreFailure, "failed to discover user's config dir: %w", err)
}
r.userConfigDir = configDir
}

if r.authenticator == nil {
r.authenticator = oauthdevice.NewAuth(cliClientID, nil)
}
Expand All @@ -81,11 +85,7 @@ func (r *appState) build(cliCtx *cli.Context) error {
return autherr.CodedErrorf(autherr.CodeTokenStoreFailure, "failed to open keyring-based token store: %w", err)
}

userConfigDir, err := os.UserConfigDir()
if err != nil {
return autherr.CodedErrorf(autherr.CodeTokenStoreFailure, "failed to discover user's config dir: %w", err)
}
tokensDir := filepath.Join(userConfigDir, "engflow_auth", "tokens")
tokensDir := filepath.Join(r.userConfigDir, "engflow_auth", "tokens")
fileStore, err := oauthtoken.NewFileTokenStore(tokensDir)
if err != nil {
return autherr.CodedErrorf(autherr.CodeTokenStoreFailure, "failed to open file-based token store: %w", err)
Expand Down Expand Up @@ -403,14 +403,26 @@ Erases the credentials for the named cluster from the local machine.`),
Action: root.version,
},
},
Before: root.build,
Before: func(cliCtx *cli.Context) error {
if cliCtx.NArg() < 1 {
return autherr.CodedErrorf(autherr.CodeUnknownSubcommand, "no subcommand provided; expected at least one subcommand")
}
return nil
},
ExitErrHandler: func(cCtx *cli.Context, err error) {
// The default handler will call os.Exit(); we want to do nothing so
// that the error is returned to the caller of app.RunContext(), and
// we will take care of calling os.Exit().
},
}

// Call root.build after command-line parsing for whichever subcommand gets
// called. We need to know the value of the -store flag (if defined).
// It's available when Command.Before is called but not App.Before.
for _, cmd := range app.Commands {
cmd.Before = root.build
}

// Ensure that all usage errors get an error code, for consistency with
// other error exit conditions.
usageErrWrapper := func(cliCtx *cli.Context, err error, isSubcommand bool) error {
Expand Down
10 changes: 7 additions & 3 deletions cmd/engflow_auth/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,15 @@ import (
"github.com/EngFlow/auth/internal/oauthdevice"
"github.com/EngFlow/auth/internal/oauthtoken"
"github.com/stretchr/testify/assert"
"github.com/zalando/go-keyring"
"golang.org/x/oauth2"
)

func init() {
// Tests should not interact with the user's real keyring.
keyring.MockInit()
}

func codedErrorContains(t *testing.T, gotErr error, code int, wantMsg string) bool {
t.Helper()
// Handle cases where expecting an error but getting none, or vice versa
Expand Down Expand Up @@ -529,6 +535,7 @@ func TestRun(t *testing.T) {
stderr := bytes.NewBuffer(nil)

root := &appState{
userConfigDir: t.TempDir(),
browserOpener: tc.browserOpener,
authenticator: tc.authenticator,
tokenStore: tc.tokenStore,
Expand All @@ -539,9 +546,6 @@ func TestRun(t *testing.T) {
if root.authenticator == nil {
root.authenticator = &fakeAuth{}
}
if root.tokenStore == nil {
root.tokenStore = oauthtoken.NewFakeTokenStore()
}

app := makeApp(root)

Expand Down

0 comments on commit d29b577

Please sign in to comment.