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.

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

RSDK-9295 - refactor disable-web-browser name and behavior #4653

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+15 −8
Open

RSDK-9295 - refactor disable-web-browser name and behavior #4653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9cb2757
refactor disable-web-browser name and behavior
stuqdog Dec 23, 2024
8cb828f
minor formatting fix
stuqdog Dec 23, 2024
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
5 changes: 3 additions & 2 deletions cli/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,8 +371,9 @@ var app = &cli.App{
HideHelpCommand: true,
Flags: []cli.Flag{
&cli.BoolFlag{
Name: loginFlagDisableBrowser,
Usage: "prevent opening the default browser during login",
Name: loginFlagDisableBrowser,
Aliases: []string{"no-browser"}, // ease of use alias, not related to backwards compatibility
Usage: "prevent opening the default browser during login",
},
},
Action: createCommandWithT[loginActionArgs](LoginAction),
Expand Down
18 changes: 12 additions & 6 deletions cli/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func (c *viamClient) loginAction(cCtx *cli.Context) error {
return errors.New("error while refreshing token, logging out. Please log in again")
}
} else {
t, err = c.authFlow.loginAsUser(c.c.Context)
t, err = c.authFlow.loginAsUser(c.c)
if err != nil {
debugf(c.c.App.Writer, globalArgs.Debug, "Login error: %v", err)

Expand Down Expand Up @@ -660,7 +660,8 @@ func newCLIAuthFlowWithAuthDomain(authDomain, audience, clientID string, console
}
}

func (a *authFlow) loginAsUser(ctx context.Context) (*token, error) {
func (a *authFlow) loginAsUser(c *cli.Context) (*token, error) {
ctx := c.Context
Comment on lines -663 to +664
Copy link
Member

Choose a reason for hiding this comment

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

For my own knowledge more than anything, why the change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question! The cli.Context has an io.Writer on it that allows us to write warning messages to terminal when connecting to the browser fails.

discovery, err := a.loadOIDiscoveryEndpoint(ctx)
if err != nil {
return nil, errors.Wrapf(err, "failed retrieving discovery endpoint")
Expand All @@ -673,8 +674,9 @@ func (a *authFlow) loginAsUser(ctx context.Context) (*token, error) {

err = a.directUser(deviceCode)
if err != nil {
return nil, fmt.Errorf("unable to open the browser to complete the login flow due to %w."+
"You can use the --%s flag to skip this behavior", err, loginFlagDisableBrowser)
warningf(c.App.ErrWriter, "unable to open the browser to complete the login flow due to %w. "+
"Please go to the provided URL to log in; you can use the --%s flag to skip this warning in the future",
err, loginFlagDisableBrowser)
}

token, err := a.waitForUser(ctx, deviceCode, discovery)
Expand Down Expand Up @@ -741,9 +743,13 @@ func (a *authFlow) makeDeviceCodeRequest(ctx context.Context, discovery *openIDD
}

func (a *authFlow) directUser(code *deviceCodeResponse) error {
infof(a.console, `You can log into Viam through the opened browser window or follow the URL below.
suggestedLoginMethods := ""
if !a.disableBrowserOpen {
suggestedLoginMethods = " through the opened browser window or"
}
infof(a.console, `You can log into Viam%s by following the URL below.
Ensure the code in the URL matches the one shown in your browser.
%s`, code.VerificationURIComplete)
%s`, suggestedLoginMethods, code.VerificationURIComplete)

if a.disableBrowserOpen {
return nil
Expand Down
Loading