Merge pull request #237 from okta/fix_retries_and_error_messages

Fix buggy retry logic
okta · Sep 2, 2024 · e55a600 · e55a600
2 parents deb8118 + 5e2c9eb
commit e55a600
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 54 deletions.
diff --git a/cmd/root/web/web.go b/cmd/root/web/web.go
@@ -86,10 +86,10 @@ func NewWebCommand() *cobra.Command {
 				return err
 			}
 
-			// Warn if there is an issue with okta.yaml
 			_, err = config.OktaConfig()
 			if err != nil {
 				if _, pathError := err.(*fs.PathError); !pathError {
+					// Warn if okta.yaml exists and there is an error with it.
 					webssoauth.ConsolePrint(cfg, "WARNING: issue with %s file. Run `okta-aws-cli debug` command for additional diagnosis.\nError: %+v\n", config.OktaYaml, err)
 				}
 			}
@@ -101,25 +101,22 @@ func NewWebCommand() *cobra.Command {
 
 			for attempt := 1; attempt <= 2; attempt++ {
 				wsa, err := webssoauth.NewWebSSOAuthentication(cfg)
-				if _, ok := err.(*webssoauth.ClassicOrgError); ok {
-					return err
-				}
 				if err != nil {
-					break
+					return err
 				}
 
 				err = wsa.EstablishIAMCredentials()
-				if err == nil {
-					break
-				}
-
 				if apiErr, ok := err.(*okta.APIError); ok {
 					if apiErr.ErrorType == "invalid_grant" && webssoauth.RemoveCachedAccessToken() {
-						webssoauth.ConsolePrint(cfg, "\nCached access token appears to be stale, removing token and retrying device authorization ...\n\n")
+						webssoauth.ConsolePrint(cfg, "Cached access token appears to be stale, removing token and retrying device authorization ...\n\n")
 						continue
 					}
-					break
 				}
+				if err != nil {
+					return err
+				}
+
+				break
 			}
 
 			return err

diff --git a/internal/okta/apierror.go b/internal/okta/apierror.go
@@ -29,11 +29,11 @@ import (
 
 const (
 	// APIErrorMessageBase base API error message
-	APIErrorMessageBase = "the API returned an unknown error"
+	APIErrorMessageBase = "Okta API returned an unknown error"
 	// APIErrorMessageWithErrorDescription API error message with description
-	APIErrorMessageWithErrorDescription = "the API returned an error: %s"
+	APIErrorMessageWithErrorDescription = "Okta API returned an error: %s"
 	// APIErrorMessageWithErrorSummary API error message with summary
-	APIErrorMessageWithErrorSummary = "the API returned an error: %s"
+	APIErrorMessageWithErrorSummary = "Okta API returned an error: %s"
 	// HTTPHeaderWwwAuthenticate Www-Authenticate header
 	HTTPHeaderWwwAuthenticate = "Www-Authenticate"
 )

diff --git a/internal/webssoauth/webssoauth.go b/internal/webssoauth/webssoauth.go
@@ -149,52 +149,29 @@ func (w *WebSSOAuthentication) EstablishIAMCredentials() error {
 	var at *okta.AccessToken
 	var apps []*okta.Application
 	var err error
-	at = w.cachedAccessToken()
-
-	// If there is a cached okta access token, and it isn't expired, but the API
-	// 401s redo the authorize step.
-	for attempt := 1; attempt <= 2; attempt++ {
-		err = nil
-		if at == nil {
-			deviceAuth, err := w.authorize()
-			if err != nil {
-				return err
-			}
-
-			w.promptAuthentication(deviceAuth)
 
-			at, err = w.accessToken(deviceAuth)
-			if err != nil {
-				return err
-			}
-			at.Expiry = time.Now().Add(time.Duration(at.ExpiresIn) * time.Second).Format(time.RFC3339)
-			w.cacheAccessToken(at)
-		}
-		if w.config.FedAppID() != "" {
-			// Alternate path when operator knows their AWS Fed app ID
-			err = w.establishTokenWithFedAppID(clientID, w.config.FedAppID(), at, w.config.AWSRegion())
-			if at != nil && err != nil {
-				// possible bad cached access token, retry
-				at = nil
-				continue
-			}
+	at = w.cachedAccessToken()
+	if at == nil {
+		deviceAuth, err := w.authorize()
+		if err != nil {
 			return err
 		}
 
-		apps, err = w.listFedApps(clientID, at)
-		if at != nil && err != nil {
-			action := "exiting."
-			if attempt == 1 {
-				action = "retrying ..."
-			}
-			w.consolePrint("Listing federation apps failed, %s\n\n", action)
-			// possible bad cached access token, retry
-			at = nil
-			continue
+		w.promptAuthentication(deviceAuth)
+		at, err = w.accessToken(deviceAuth)
+		if err != nil {
+			return err
 		}
-		break
+		at.Expiry = time.Now().Add(time.Duration(at.ExpiresIn) * time.Second).Format(time.RFC3339)
+
+		w.cacheAccessToken(at)
+	}
+
+	if w.config.FedAppID() != "" {
+		return w.establishTokenWithFedAppID(clientID, w.config.FedAppID(), at, w.config.AWSRegion())
 	}
 
+	apps, err = w.listFedApps(clientID, at)
 	if err != nil {
 		return err
 	}
@@ -1075,7 +1052,7 @@ func NewClassicOrgError(orgDomain string) *ClassicOrgError {
 
 // Error Error interface error message
 func (e *ClassicOrgError) Error() string {
-	return fmt.Sprintf("%q is a Classic org, okta-aws-cli is an-OIE only tool", e.orgDomain)
+	return fmt.Sprintf("%q is a Classic org, okta-aws-cli is an OIE only tool", e.orgDomain)
 }
 
 // isClassicOrg Conduct simple check of well known endpoint to determine if the