Skip to content
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.

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

DXCDT-267: Graceful handling of access token regeneration (2/x) #547

Merged
merged 8 commits into from
Dec 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions internal/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ type ClientCredentials struct {
}

// GetAccessTokenFromClientCreds generates an access token from client credentials
func GetAccessTokenFromClientCreds(args ClientCredentials) (Result, error) {
func GetAccessTokenFromClientCreds(ctx context.Context, args ClientCredentials) (Result, error) {
u, err := url.Parse("https://" + args.Domain)
if err != nil {
return Result{}, err
Expand All @@ -305,7 +305,7 @@ func GetAccessTokenFromClientCreds(args ClientCredentials) (Result, error) {
},
}

resp, err := credsConfig.Token(context.Background())
resp, err := credsConfig.Token(ctx)
if err != nil {
return Result{}, err
}
Expand Down
36 changes: 25 additions & 11 deletions internal/cli/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/spf13/pflag"

"github.com/auth0/auth0-cli/internal/analytics"
"github.com/auth0/auth0-cli/internal/ansi"
"github.com/auth0/auth0-cli/internal/auth"
"github.com/auth0/auth0-cli/internal/auth0"
"github.com/auth0/auth0-cli/internal/buildinfo"
Expand Down Expand Up @@ -109,11 +110,14 @@ func (t *Tenant) hasExpiredToken() bool {

func (t *Tenant) regenerateAccessToken(ctx context.Context, c *cli) error {
if t.authenticatedWithClientCredentials() {
token, err := auth.GetAccessTokenFromClientCreds(auth.ClientCredentials{
ClientID: t.ClientID,
ClientSecret: t.ClientSecret,
Domain: t.Domain,
})
token, err := auth.GetAccessTokenFromClientCreds(
ctx,
auth.ClientCredentials{
ClientID: t.ClientID,
ClientSecret: t.ClientSecret,
Domain: t.Domain,
},
)
if err != nil {
return err
}
Expand Down Expand Up @@ -207,18 +211,28 @@ func (c *cli) prepareTenant(ctx context.Context) (Tenant, error) {
return Tenant{}, err
}

if t.AccessToken == "" || (scopesChanged(t) && t.authenticatedWithDeviceCodeFlow()) {
return RunLoginAsUser(ctx, c, true)
if scopesChanged(t) && t.authenticatedWithDeviceCodeFlow() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the empty access token check here enables more specific error messaging and guidance. Further, it didn't make sense to have both cases handled here because we always attempt to regenerate the access token no matter what below.

c.renderer.Warnf("Required scopes have changed. Please log in to re-authorize the CLI.\n")
return RunLoginAsUser(ctx, c)
}

if !t.hasExpiredToken() {
if t.AccessToken != "" && !t.hasExpiredToken() {
return t, nil
}

if err := t.regenerateAccessToken(ctx, c); err != nil {
// Ask and guide the user through the login process.
c.renderer.Errorf("failed to renew access token, %s", err)
return RunLoginAsUser(ctx, c, true)
if t.authenticatedWithClientCredentials() {
return t, fmt.Errorf(
"failed to fetch access token using client credentials.\n\n"+
"This may occur if the designated application has been deleted or the client secret has been rotated.\n\n"+
"Please re-authenticate by running: %s",
ansi.Bold("auth0 login --domain <tenant-domain --client-id <client-id> --client-secret <client-secret>"),
)
}

c.renderer.Warnf("Failed to renew access token. Please log in to re-authorize the CLI.\n")

return RunLoginAsUser(ctx, c)
}

if err := c.addTenant(t); err != nil {
Expand Down
45 changes: 20 additions & 25 deletions internal/cli/login.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,13 @@ auth0 login --domain <tenant-domain> --client-id <client-id> --client-secret <cl
return err
}
} else {
if _, err := RunLoginAsUser(ctx, cli, false); err != nil {
welcomeMessage := fmt.Sprintf(
"%s\n\n%s\n\n",
"✪ Welcome to the Auth0 CLI 🎊",
"If you don't have an account, please create one here: https://auth0.com/signup.",
)
cli.renderer.Output(welcomeMessage)
if _, err := RunLoginAsUser(ctx, cli); err != nil {
return err
}
}
Expand All @@ -85,7 +91,6 @@ auth0 login --domain <tenant-domain> --client-id <client-id> --client-secret <cl
cmd.SetHelpFunc(func(cmd *cobra.Command, args []string) {
_ = cmd.Flags().MarkHidden("tenant")
_ = cmd.Flags().MarkHidden("json")
_ = cmd.Flags().MarkHidden("no-input")
cmd.Parent().HelpFunc()(cmd, args)
})

Expand All @@ -94,28 +99,13 @@ auth0 login --domain <tenant-domain> --client-id <client-id> --client-secret <cl

// RunLoginAsUser runs the login flow guiding the user through the process
// by showing the login instructions, opening the browser.
// Use `expired` to run the login from other commands setup:
// this will only affect the messages.
func RunLoginAsUser(ctx context.Context, cli *cli, expired bool) (Tenant, error) {
message := fmt.Sprintf(
"%s\n\n%s\n\n",
"✪ Welcome to the Auth0 CLI 🎊",
"If you don't have an account, please create one here: https://auth0.com/signup.",
)

if expired {
message = "Please sign in to re-authorize the CLI."
cli.renderer.Warnf(message)
} else {
cli.renderer.Output(message)
}
Comment on lines -99 to -111
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracting the error messaging out of the RunLoginAsUser is an intentional decision to keep the function focused. Further, each invocation of RunLoginAsUser comes with a unique message, that is, there is no duplication of messages with the removal of this code. This allows the messaging to stay specific to the situation and make the function more focused.


func RunLoginAsUser(ctx context.Context, cli *cli) (Tenant, error) {
state, err := cli.authenticator.Start(ctx)
if err != nil {
return Tenant{}, fmt.Errorf("Failed to start the authentication process: %w.", err)
}

message = fmt.Sprintf("Your device confirmation code is: %s\n\n", ansi.Bold(state.UserCode))
message := fmt.Sprintf("Your device confirmation code is: %s\n\n", ansi.Bold(state.UserCode))
cli.renderer.Output(message)

if cli.noInput {
Expand Down Expand Up @@ -209,13 +199,18 @@ func RunLoginAsMachine(ctx context.Context, inputs LoginInputs, cli *cli, cmd *c
return err
}

token, err := auth.GetAccessTokenFromClientCreds(auth.ClientCredentials{
ClientID: inputs.ClientID,
ClientSecret: inputs.ClientSecret,
Domain: inputs.Domain,
})
token, err := auth.GetAccessTokenFromClientCreds(
ctx,
auth.ClientCredentials{
ClientID: inputs.ClientID,
ClientSecret: inputs.ClientSecret,
Domain: inputs.Domain,
},
)
if err != nil {
return err
return fmt.Errorf(
"failed to fetch access token using client credentials. \n\n"+
"Ensure that the provided client-id, client-secret and domain are correct. \n\nerror: %w\n", err)
}

t := Tenant{
Expand Down