Skip to content

simplify register #453

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
patelspratik wants to merge 1 commit into
base: main
Choose a base branch
from
+1,189 −757
Draft

simplify register #453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions pkg/auth/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,9 @@ type Auth struct {

const BrevAPIKeyPrefix = "bak-"

const MissingAPIKeyOrgIDMessage = "api key auth requires an org id; run brev login --api-key <api-key> --org-id <org-id>"
const AccessKeyEnvVar = "BREV_ACCESS_KEY"

const MissingAPIKeyOrgIDMessage = "org id missing, please login again; run 'brev login --api-key <api-key>'"

type APIKeyAuthStore interface {
GetAuthTokens() (*entity.AuthTokens, error)
Expand Down Expand Up @@ -142,6 +144,9 @@ func IsBrevAPIKey(token string) bool {
}

func IsAPIKeyAuthStore(authTokensProvider APIKeyAuthStore) bool {
if strings.TrimSpace(os.Getenv(AccessKeyEnvVar)) != "" {
return true
}
tokens, err := authTokensProvider.GetAuthTokens()
if err != nil {
return false
Expand All @@ -153,6 +158,20 @@ func IsAPIKeyAuthStore(authTokensProvider APIKeyAuthStore) bool {
}

func GetAPIKeyOrgID(authTokensProvider APIKeyAuthStore) (string, error) {
if envKey := strings.TrimSpace(os.Getenv(AccessKeyEnvVar)); envKey != "" {
tokens, err := authTokensProvider.GetAuthTokens()
if err != nil {
return "", breverrors.WrapAndTrace(err)
}
if tokens == nil || tokens.APIKey != envKey {
return "", breverrors.NewValidationError(MissingAPIKeyOrgIDMessage)
}
orgID := strings.TrimSpace(tokens.APIKeyOrgID)
if orgID == "" {
return "", breverrors.NewValidationError(MissingAPIKeyOrgIDMessage)
}
return orgID, nil
}
tokens, err := authTokensProvider.GetAuthTokens()
if err != nil {
return "", breverrors.WrapAndTrace(err)
Expand Down Expand Up @@ -204,6 +223,9 @@ func (t Auth) GetFreshAccessTokenOrLogin() (string, error) {

// Gets fresh access token or returns nil and saves to store
func (t Auth) GetFreshAccessTokenOrNil() (string, error) {
if key := strings.TrimSpace(os.Getenv(AccessKeyEnvVar)); key != "" {
return key, nil
}
tokens, err := t.getSavedTokensOrNil()
if err != nil {
return "", breverrors.WrapAndTrace(err)
Expand All @@ -217,7 +239,6 @@ func (t Auth) GetFreshAccessTokenOrNil() (string, error) {
return apiKey, nil
}

// should always at least have access token?
if tokens.AccessToken == "" {
breverrors.GetDefaultErrorReporter().ReportMessage("access token is an empty string but shouldn't be")
}
Expand Down Expand Up @@ -301,10 +322,6 @@ func (t Auth) LoginWithAPIKey(apiKey string, orgID string) error {
if !IsBrevAPIKey(apiKey) {
return breverrors.NewValidationError(fmt.Sprintf("api key must start with %s", BrevAPIKeyPrefix))
}
orgID = strings.TrimSpace(orgID)
if orgID == "" {
return breverrors.NewValidationError(MissingAPIKeyOrgIDMessage)
}

tokens, err := t.getSavedTokensOrNil()
if err != nil {
Expand Down
90 changes: 69 additions & 21 deletions pkg/auth/auth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,6 @@ func TestIsAccessTokenValid(t *testing.T) {
if !assert.False(t, res) {
return
}

// expiredToken := "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImdLTXBESXlRc0ZXSF9zYWdiT2oyViJ9.eyJpc3MiOiJodHRwczovL2JyZXZkZXYudXMuYXV0aDAuY29tLyIsInN1YiI6Imdvb2dsZS1vYXV0aDJ8MTAxNzY0NjMwNTEwODYxNDk5MTgwIiwiYXVkIjpbImh0dHBzOi8vYnJldmRldi51cy5hdXRoMC5jb20vYXBpL3YyLyIsImh0dHBzOi8vYnJldmRldi51cy5hdXRoMC5jb20vdXNlcmluZm8iXSwiaWF0IjoxNjM4NTYyMzY4LCJleHAiOjE2Mzg2NDg3NjgsImF6cCI6IkphcUpSTEVzZGF0NXc3VGIwV3FtVHh6SWVxd3FlcG1rIiwic2NvcGUiOiJvcGVuaWQgcHJvZmlsZSBlbWFpbCBvZmZsaW5lX2FjY2VzcyJ9.YCiO-som26ehT91qGAX5ZfrtVg4eYwamnlMRoCuUljXmg8Nf-ArDyoG32CqZkQ6YJ5XnzrVX9bVk5ZNHP_AFSE9SJvYL6MchoN09nR84WTbevRBCtZedIZUk5ULg6rWo5mszGr-S2gi08od4iTzXtKySPx1JnT60muRj_k9VV3MyixqvngEz5NvmFDdA8glGes5_iOuiBidmjOJzi_CVfKJ9s48BhlxzciSXFC0_DUBnT9OThjYjUP-22ohOuWwJWomRUv6gMSq78hJOALc330LwvmEsLdzlP7a3otIYM43hTtAVJ9QEL6M08GKqm3PdikzTxiGdfuQUhgMDlXygbQ"
// res, err := isAccessTokenValid(expiredToken)
// if !assert.Nil(t, err) {
// return
// }
// if !assert.False(t, res) {
// return
// }
}

type MockAuthStore struct {
Expand All @@ -48,6 +39,7 @@ type MockAuthStore struct {
func (m *MockAuthStore) SaveAuthTokens(tokens entity.AuthTokens) error {
m.saved = tokens
m.didSave = true
m.authTokens = &tokens // write-then-read consistent (mirrors a real store)
return nil
}

Expand Down Expand Up @@ -110,6 +102,7 @@ func (s *sideEffectingTokenStore) GetAccessToken() (string, error) {
}

func TestIsAPIKeyAuthStore_ReadsSavedTokensWithoutAccessTokenSideEffects(t *testing.T) {
t.Setenv(AccessKeyEnvVar, "")
s := &sideEffectingTokenStore{
tokens: &entity.AuthTokens{APIKey: testAPIKey},
}
Expand All @@ -119,6 +112,7 @@ func TestIsAPIKeyAuthStore_ReadsSavedTokensWithoutAccessTokenSideEffects(t *test
}

func TestIsAPIKeyAuthStore_LegacyCredentialsAreNotAPIKeyAuth(t *testing.T) {
t.Setenv(AccessKeyEnvVar, "")
s := &sideEffectingTokenStore{
tokens: &entity.AuthTokens{
AccessToken: validToken,
Expand All @@ -130,6 +124,35 @@ func TestIsAPIKeyAuthStore_LegacyCredentialsAreNotAPIKeyAuth(t *testing.T) {
assert.False(t, s.getAccessTokenCalled)
}

func TestIsAPIKeyAuthStore_EnvKeyIsAPIKeyEvenWhenNotPersisted(t *testing.T) {
t.Setenv(AccessKeyEnvVar, testAPIKey)
s := &sideEffectingTokenStore{tokens: nil} // nothing persisted
assert.True(t, IsAPIKeyAuthStore(s))
}

func TestGetAPIKeyOrgID_EnvKeyMismatchingPersistedRejects(t *testing.T) {
t.Setenv(AccessKeyEnvVar, BrevAPIKeyPrefix+"env-key")
s := &sideEffectingTokenStore{tokens: &entity.AuthTokens{
APIKey: BrevAPIKeyPrefix + "persisted-key",
APIKeyOrgID: "org-persisted",
}}
_, err := GetAPIKeyOrgID(s)
assert.Error(t, err)
assert.Contains(t, err.Error(), "org id missing")
}

// When the env key matches the persisted key, its persisted org is valid.
func TestGetAPIKeyOrgID_EnvKeyMatchingPersistedReturnsOrg(t *testing.T) {
t.Setenv(AccessKeyEnvVar, testAPIKey)
s := &sideEffectingTokenStore{tokens: &entity.AuthTokens{
APIKey: testAPIKey,
APIKeyOrgID: "org-test",
}}
orgID, err := GetAPIKeyOrgID(s)
assert.NoError(t, err)
assert.Equal(t, "org-test", orgID)
}

type cliAuthStore struct {
tokens *entity.AuthTokens
user *entity.User
Expand Down Expand Up @@ -238,6 +261,43 @@ func TestGetFreshAccessTokenOrNil_APIKeyOnlyCredentialReturnsAPIKey(t *testing.T
assert.False(t, s.didSave)
}

// Closest credential wins: BREV_ACCESS_KEY takes precedence over saved
// tokens (flag/env before persisted), matching other CLIs. The global
// --api-key flag handler populates this env var before the auth chain runs.
func TestGetFreshAccessTokenOrNil_EnvVarTakesPrecedenceOverSaved(t *testing.T) {
t.Setenv(AccessKeyEnvVar, BrevAPIKeyPrefix+"env-key")
s := MockAuthStore{authTokens: &entity.AuthTokens{APIKey: testAPIKey}}
a := Auth{authStore: &s, oauth: &MockOauth{}, accessTokenValidator: func(string) (bool, error) {
t.Fatal("env key must short-circuit before touching saved credentials")
return false, nil
}}

res, err := a.GetFreshAccessTokenOrNil()
assert.NoError(t, err)
assert.Equal(t, BrevAPIKeyPrefix+"env-key", res, "BREV_ACCESS_KEY must win over saved tokens")
}

// With no saved credential, BREV_ACCESS_KEY authenticates headless/CI commands.
func TestGetFreshAccessTokenOrNil_EnvVarFallbackWhenNoSavedTokens(t *testing.T) {
t.Setenv(AccessKeyEnvVar, testAPIKey)
s := MockAuthStore{} // no saved tokens
a := Auth{authStore: &s, oauth: &MockOauth{}}

res, err := a.GetFreshAccessTokenOrNil()
assert.NoError(t, err)
assert.Equal(t, testAPIKey, res, "env var should be used when no credential is saved")
}

func TestGetFreshAccessTokenOrNil_EnvVarEmptyFallsThroughToSaved(t *testing.T) {
t.Setenv(AccessKeyEnvVar, "")
s := MockAuthStore{authTokens: &entity.AuthTokens{APIKey: testAPIKey}}
a := Auth{authStore: &s, oauth: &MockOauth{}}

res, err := a.GetFreshAccessTokenOrNil()
assert.NoError(t, err)
assert.Equal(t, testAPIKey, res, "empty env var should fall through to saved credentials")
}

func TestLoginWithAPIKey_SavesTypedCredential(t *testing.T) {
s := MockAuthStore{}
a := Auth{
Expand Down Expand Up @@ -286,18 +346,6 @@ func TestLoginWithAPIKey_EmptyKeyReturnsError(t *testing.T) {
assert.False(t, s.didSave)
}

func TestLoginWithAPIKey_EmptyOrgIDReturnsError(t *testing.T) {
s := MockAuthStore{}
a := Auth{
authStore: &s,
oauth: &MockOauth{},
}

err := a.LoginWithAPIKey(testAPIKey, "")
assert.Error(t, err)
assert.False(t, s.didSave)
}

func TestStandardLogin_APIKeyCredentialDoesNotProbeOAuthProviders(t *testing.T) {
oldStdout := os.Stdout
t.Cleanup(func() {
Expand Down
54 changes: 15 additions & 39 deletions pkg/cmd/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package cmd

import (
"fmt"
"os"

"github.com/brevdev/brev-cli/pkg/analytics"
"github.com/brevdev/brev-cli/pkg/auth"
Expand Down Expand Up @@ -59,7 +60,6 @@ import (
"github.com/brevdev/brev-cli/pkg/cmd/upgrade"
"github.com/brevdev/brev-cli/pkg/cmd/version"
"github.com/brevdev/brev-cli/pkg/config"
"github.com/brevdev/brev-cli/pkg/entity"
"github.com/brevdev/brev-cli/pkg/featureflag"
"github.com/brevdev/brev-cli/pkg/files"
"github.com/brevdev/brev-cli/pkg/remoteversion"
Expand All @@ -73,6 +73,7 @@ import (

var (
userFlag string
apiKeyFlag string
printVersion bool
noCheckLatest bool
)
Expand All @@ -84,6 +85,8 @@ func NewDefaultBrevCommand() *cobra.Command {
cmd.PersistentFlags().BoolP("help", "h", false, "Help for Brev")

cmd.PersistentFlags().StringVar(&userFlag, "user", "", "Non root user to use for per user configuration of commands run as root")
cmd.PersistentFlags().StringVar(&apiKeyFlag, "api-key", "", "api key to authenticate CLI requests")
_ = cmd.PersistentFlags().MarkHidden("api-key")
cmd.PersistentFlags().BoolVar(&printVersion, "version", false, "Print version output")
cmd.PersistentFlags().BoolVar(&noCheckLatest, "no-check-latest", false, "Do not check for the latest version when printing version")

Expand Down Expand Up @@ -163,6 +166,9 @@ func NewBrevCommand() *cobra.Command { //nolint:funlen,gocognit,gocyclo // defin
fmt.Println(v)
}
}
if apiKeyFlag != "" {
os.Setenv(auth.AccessKeyEnvVar, apiKeyFlag)
}
if userFlag != "" {
_, err := noLoginCmdStore.WithUserID(userFlag)
if err != nil {
Expand Down Expand Up @@ -233,33 +239,21 @@ func NewBrevCommand() *cobra.Command { //nolint:funlen,gocognit,gocyclo // defin

cmds.SetUsageTemplate(usageTemplate)

// In-memory auth for external node commands — never touches credentials.json.
// Pre-fill the cached email so the user sees a confirmation prompt instead of
// having to type it from scratch every time.
cachedEmail, _ := fsStore.GetCachedEmail()
memAuthenticator := auth.StandardLogin("", cachedEmail, nil)
if cachedEmail != "" {
if kas, ok := memAuthenticator.(auth.KasAuthenticator); ok {
kas.ShouldPromptEmail = true
memAuthenticator = kas
}
}
memAuthStore := &emailCachingAuthStore{
MemoryAuthStore: store.NewMemoryAuthStore(),
fileStore: fsStore,
}
memLoginAuth := auth.NewLoginAuth(memAuthStore, memAuthenticator)
memLoginAuth.WithShouldLogin(func() (bool, error) { return true, nil })

// External node commands (register/deregister/enable-ssh/grant-ssh/revoke-ssh)
// read credentials.json and BREV_ACCESS_KEY but never prompt for a login —
// a shared box should not be encouraged to write durable creds.
externalNodeCmdStore := fsStore.WithNoAuthHTTPClient(
store.NewNoAuthHTTPClient(conf.GetBrevAPIURl()),
).WithAuth(memLoginAuth, store.WithDebug(conf.GetDebugHTTP()))
).WithAuth(noLoginAuth, store.WithDebug(conf.GetDebugHTTP()))

err = externalNodeCmdStore.SetForbiddenStatusRetryHandler(func() error {
_, err1 := memLoginAuth.GetAccessToken()
token, err1 := noLoginAuth.GetAccessToken()
if err1 != nil {
return breverrors.WrapAndTrace(err1)
}
if token == "" {
return breverrors.New("not authenticated; set BREV_ACCESS_KEY or run 'brev login --api-key' on a trusted machine")
}
return nil
})
if err != nil {
Expand Down Expand Up @@ -545,22 +539,4 @@ var (
_ store.Auth = auth.NoLoginAuth{}
_ auth.AuthStore = store.FileStore{}
_ auth.AuthStore = &store.MemoryAuthStore{}
_ auth.AuthStore = &emailCachingAuthStore{}
)

// emailCachingAuthStore wraps MemoryAuthStore and persists the login email
// to ~/.brev/cached-email after each successful authentication.
type emailCachingAuthStore struct {
*store.MemoryAuthStore
fileStore *store.FileStore
}

func (e *emailCachingAuthStore) SaveAuthTokens(tokens entity.AuthTokens) error {
if err := e.MemoryAuthStore.SaveAuthTokens(tokens); err != nil {
return breverrors.WrapAndTrace(err)
}
if email := auth.GetEmailFromToken(tokens.AccessToken); email != "" {
_ = e.fileStore.SaveCachedEmail(email)
}
return nil
}
Loading
Loading