VAULT-5885: Fix erroneous success message in case of two-phase MFA, and provide MFA information in table format (hashicorp#15428)

* VAULT-5885: Fix erroneous success message in case of two-phase MFA, and provide MFA information in table format

* VAULT-5885 Add changelog

* VAULT-5885 Update changelog as per PR comments

* VAULT-5885 Update changelog category to just 'auth'

* VAULT-5885 Hide useless token info in two-phase MFA case

* VAULT-5885 Update changelog to reflect token info now no longer present

* VAULT-5885 split up changelog into three blocks

Author: VioletHynes

changelog/15428.txt

@@ -0,0 +1,9 @@
+```release-note:bug
+auth: Fixed erroneous success message when using vault login in case of two-phase MFA
+```
+```release-note:bug
+auth: Fixed erroneous token information being displayed when using vault login in case of two-phase MFA
+```
+```release-note:bug
+auth: Fixed two-phase MFA information missing from table format when using vault login
+```

command/format.go

@@ -385,21 +385,32 @@ func (t TableFormatter) OutputSecret(ui cli.Ui, secret *api.Secret) error {
 	}
 
 	if secret.Auth != nil {
-		out = append(out, fmt.Sprintf("token %s %s", hopeDelim, secret.Auth.ClientToken))
-		out = append(out, fmt.Sprintf("token_accessor %s %s", hopeDelim, secret.Auth.Accessor))
-		// If the lease duration is 0, it's likely a root token, so output the
-		// duration as "infinity" to clear things up.
-		if secret.Auth.LeaseDuration == 0 {
-			out = append(out, fmt.Sprintf("token_duration %s %s", hopeDelim, "∞"))
-		} else {
-			out = append(out, fmt.Sprintf("token_duration %s %v", hopeDelim, humanDurationInt(secret.Auth.LeaseDuration)))
-		}
-		out = append(out, fmt.Sprintf("token_renewable %s %t", hopeDelim, secret.Auth.Renewable))
-		out = append(out, fmt.Sprintf("token_policies %s %q", hopeDelim, secret.Auth.TokenPolicies))
-		out = append(out, fmt.Sprintf("identity_policies %s %q", hopeDelim, secret.Auth.IdentityPolicies))
-		out = append(out, fmt.Sprintf("policies %s %q", hopeDelim, secret.Auth.Policies))
-		for k, v := range secret.Auth.Metadata {
-			out = append(out, fmt.Sprintf("token_meta_%s %s %v", k, hopeDelim, v))
+		if secret.Auth.MFARequirement != nil {
+			out = append(out, fmt.Sprintf("mfa_request_id %s %s", hopeDelim, secret.Auth.MFARequirement.MFARequestID))
+
+			for k, constraintSet := range secret.Auth.MFARequirement.MFAConstraints {
+				for _, constraint := range constraintSet.Any {
+					out = append(out, fmt.Sprintf("mfa_constraint_%s_%s_id %s %s", k, constraint.Type, hopeDelim, constraint.ID))
+					out = append(out, fmt.Sprintf("mfa_constraint_%s_%s_uses_passcode %s %t", k, constraint.Type, hopeDelim, constraint.UsesPasscode))
+				}
+			}
+		} else { // Token information only makes sense if no further MFA requirement (i.e. if we actually have a token)
+			out = append(out, fmt.Sprintf("token %s %s", hopeDelim, secret.Auth.ClientToken))
+			out = append(out, fmt.Sprintf("token_accessor %s %s", hopeDelim, secret.Auth.Accessor))
+			// If the lease duration is 0, it's likely a root token, so output the
+			// duration as "infinity" to clear things up.
+			if secret.Auth.LeaseDuration == 0 {
+				out = append(out, fmt.Sprintf("token_duration %s %s", hopeDelim, "∞"))
+			} else {
+				out = append(out, fmt.Sprintf("token_duration %s %v", hopeDelim, humanDurationInt(secret.Auth.LeaseDuration)))
+			}
+			out = append(out, fmt.Sprintf("token_renewable %s %t", hopeDelim, secret.Auth.Renewable))
+			out = append(out, fmt.Sprintf("token_policies %s %q", hopeDelim, secret.Auth.TokenPolicies))
+			out = append(out, fmt.Sprintf("identity_policies %s %q", hopeDelim, secret.Auth.IdentityPolicies))
+			out = append(out, fmt.Sprintf("policies %s %q", hopeDelim, secret.Auth.Policies))
+			for k, v := range secret.Auth.Metadata {
+				out = append(out, fmt.Sprintf("token_meta_%s %s %v", k, hopeDelim, v))
+			}
 		}
 	}
 

command/login.go

@@ -240,6 +240,10 @@ func (c *LoginCommand) Run(args []string) int {
 		c.UI.Warn(wrapAtLength("A login request was issued that is subject to "+
 			"MFA validation. Please make sure to validate the login by sending another "+
 			"request to sys/mfa/validate endpoint.") + "\n")
+
+		// We return early to prevent success message from being printed
+		c.checkForAndWarnAboutLoginToken()
+		return OutputSecret(c.UI, secret)
 	}
 
 	// Unset any previous token wrapping functionality. If the original request
@@ -302,15 +306,7 @@ func (c *LoginCommand) Run(args []string) int {
 			return 2
 		}
 
-		// Warn if the VAULT_TOKEN environment variable is set, as that will take
-		// precedence. We output as a warning, so piping should still work since it
-		// will be on a different stream.
-		if os.Getenv("VAULT_TOKEN") != "" {
-			c.UI.Warn(wrapAtLength("WARNING! The VAULT_TOKEN environment variable "+
-				"is set! This takes precedence over the value set by this command. To "+
-				"use the value set by this command, unset the VAULT_TOKEN environment "+
-				"variable or set it to the token displayed below.") + "\n")
-		}
+		c.checkForAndWarnAboutLoginToken()
 	} else if !c.flagTokenOnly {
 		// If token-only the user knows it won't be stored, so don't warn
 		c.UI.Warn(wrapAtLength(
@@ -372,3 +368,14 @@ func (c *LoginCommand) extractToken(client *api.Client, secret *api.Secret, unwr
 		return nil, false, fmt.Errorf("no auth or wrapping info in response")
 	}
 }
+
+// Warn if the VAULT_TOKEN environment variable is set, as that will take
+// precedence. We output as a warning, so piping should still work since it
+// will be on a different stream.
+func (c *LoginCommand) checkForAndWarnAboutLoginToken() {
+	if os.Getenv("VAULT_TOKEN") != "" {
+		c.UI.Warn(wrapAtLength("WARNING! The VAULT_TOKEN environment variable "+
+			"is set! The value of this variable will take precedence; if this is unwanted "+
+			"please unset VAULT_TOKEN or update its value accordingly.") + "\n")
+	}
+}