Merge branch 'main' into matifali/devcontainer

matifali · web-flow · commit f15d2981c09b · 2023-07-05T18:06:36.000+03:00
diff --git a/cli/login.go b/cli/login.go
@@ -41,16 +41,18 @@ func (r *RootCmd) login() *clibase.Cmd {
 	const firstUserTrialEnv = "CODER_FIRST_USER_TRIAL"
 
 	var (
-		email    string
-		username string
-		password string
-		trial    bool
+		email              string
+		username           string
+		password           string
+		trial              bool
+		useTokenForSession bool
 	)
 	cmd := &clibase.Cmd{
 		Use:        "login <url>",
 		Short:      "Authenticate with Coder deployment",
 		Middleware: clibase.RequireRangeArgs(0, 1),
 		Handler: func(inv *clibase.Invocation) error {
+			ctx := inv.Context()
 			rawURL := ""
 			if len(inv.Args) == 0 {
 				rawURL = r.clientURL.String()
@@ -89,7 +91,7 @@ func (r *RootCmd) login() *clibase.Cmd {
 				_, _ = fmt.Fprintln(inv.Stderr, cliui.DefaultStyles.Warn.Render(err.Error()))
 			}
 
-			hasInitialUser, err := client.HasFirstUser(inv.Context())
+			hasInitialUser, err := client.HasFirstUser(ctx)
 			if err != nil {
 				return xerrors.Errorf("Failed to check server %q for first user, is the URL correct and is coder accessible from your browser? Error - has initial user: %w", serverURL.String(), err)
 			}
@@ -182,7 +184,7 @@ func (r *RootCmd) login() *clibase.Cmd {
 					trial = v == "yes" || v == "y"
 				}
 
-				_, err = client.CreateFirstUser(inv.Context(), codersdk.CreateFirstUserRequest{
+				_, err = client.CreateFirstUser(ctx, codersdk.CreateFirstUserRequest{
 					Email:    email,
 					Username: username,
 					Password: password,
@@ -191,7 +193,7 @@ func (r *RootCmd) login() *clibase.Cmd {
 				if err != nil {
 					return xerrors.Errorf("create initial user: %w", err)
 				}
-				resp, err := client.LoginWithPassword(inv.Context(), codersdk.LoginWithPasswordRequest{
+				resp, err := client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{
 					Email:    email,
 					Password: password,
 				})
@@ -235,7 +237,7 @@ func (r *RootCmd) login() *clibase.Cmd {
 					Secret: true,
 					Validate: func(token string) error {
 						client.SetSessionToken(token)
-						_, err := client.User(inv.Context(), codersdk.Me)
+						_, err := client.User(ctx, codersdk.Me)
 						if err != nil {
 							return xerrors.New("That's not a valid token!")
 						}
@@ -245,11 +247,27 @@ func (r *RootCmd) login() *clibase.Cmd {
 				if err != nil {
 					return xerrors.Errorf("paste token prompt: %w", err)
 				}
+			} else if !useTokenForSession {
+				// If a session token is provided on the cli, use it to generate
+				// a new one. This is because the cli `--token` flag provides
+				// a token for the command being invoked. We should not store
+				// this token, and `/logout` should not delete it.
+				// /login should generate a new token and store that.
+				client.SetSessionToken(sessionToken)
+				// Use CreateAPIKey over CreateToken because this is a session
+				// key that should not show on the `tokens` page. This should
+				// match the same behavior of the `/cli-auth` page for generating
+				// a session token.
+				key, err := client.CreateAPIKey(ctx, "me")
+				if err != nil {
+					return xerrors.Errorf("create api key: %w", err)
+				}
+				sessionToken = key.Key
 			}
 
 			// Login to get user data - verify it is OK before persisting
 			client.SetSessionToken(sessionToken)
-			resp, err := client.User(inv.Context(), codersdk.Me)
+			resp, err := client.User(ctx, codersdk.Me)
 			if err != nil {
 				return xerrors.Errorf("get user: %w", err)
 			}
@@ -293,6 +311,11 @@ func (r *RootCmd) login() *clibase.Cmd {
 			Description: "Specifies whether a trial license should be provisioned for the Coder deployment or not.",
 			Value:       clibase.BoolOf(&trial),
 		},
+		{
+			Flag:        "use-token-as-session",
+			Description: "By default, the CLI will generate a new session token when logging in. This flag will instead use the provided token as the session token.",
+			Value:       clibase.BoolOf(&useTokenForSession),
+		},
 	}
 	return cmd
 }
diff --git a/cli/login_test.go b/cli/login_test.go
@@ -197,6 +197,7 @@ func TestLogin(t *testing.T) {
 		<-doneChan
 	})
 
+	// TokenFlag should generate a new session token and store it in the session file.
 	t.Run("TokenFlag", func(t *testing.T) {
 		t.Parallel()
 		client := coderdtest.New(t, nil)
@@ -206,6 +207,7 @@ func TestLogin(t *testing.T) {
 		require.NoError(t, err)
 		sessionFile, err := cfg.Session().Read()
 		require.NoError(t, err)
-		require.Equal(t, client.SessionToken(), sessionFile)
+		// This **should not be equal** to the token we passed in.
+		require.NotEqual(t, client.SessionToken(), sessionFile)
 	})
 }
diff --git a/cli/testdata/coder_login_--help.golden b/cli/testdata/coder_login_--help.golden
@@ -19,5 +19,9 @@ Authenticate with Coder deployment
           Specifies a username to use if creating the first user for the
           deployment.
 
+      --use-token-as-session bool
+          By default, the CLI will generate a new session token when logging in.
+          This flag will instead use the provided token as the session token.
+
 ---
 Run `coder --help` for a list of global options.
diff --git a/codersdk/apikey.go b/codersdk/apikey.go
@@ -78,7 +78,10 @@ func (c *Client) CreateToken(ctx context.Context, userID string, req CreateToken
 }
 
 // CreateAPIKey generates an API key for the user ID provided.
-// DEPRECATED: use CreateToken instead.
+// CreateToken should be used over CreateAPIKey. CreateToken allows better
+// tracking of the token's usage and allows for custom expiration.
+// Only use CreateAPIKey if you want to emulate the session created for
+// a browser like login.
 func (c *Client) CreateAPIKey(ctx context.Context, user string) (GenerateAPIKeyResponse, error) {
 	res, err := c.Request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/users/%s/keys", user), nil)
 	if err != nil {
diff --git a/docs/cli/login.md b/docs/cli/login.md
@@ -47,3 +47,11 @@ Specifies whether a trial license should be provisioned for the Coder deployment
 | Environment | <code>$CODER_FIRST_USER_USERNAME</code> |
 
 Specifies a username to use if creating the first user for the deployment.
+
+### --use-token-as-session
+
+|      |                   |
+| ---- | ----------------- |
+| Type | <code>bool</code> |
+
+By default, the CLI will generate a new session token when logging in. This flag will instead use the provided token as the session token.
diff --git a/pty/pty_windows.go b/pty/pty_windows.go
@@ -65,10 +65,13 @@ func newPty(opt ...Option) (*ptyWindows, error) {
 		0,
 		uintptr(unsafe.Pointer(&pty.console)),
 	)
-	if int32(ret) < 0 {
+	// CreatePseudoConsole returns S_OK on success, as per:
+	// https://learn.microsoft.com/en-us/windows/console/createpseudoconsole
+	if windows.Handle(ret) != windows.S_OK {
 		_ = pty.Close()
 		return nil, xerrors.Errorf("create pseudo console (%d): %w", int32(ret), err)
 	}
+
 	return pty, nil
 }
 
@@ -134,39 +137,65 @@ func (p *ptyWindows) Resize(height uint16, width uint16) error {
 		Y: int16(height),
 		X: int16(width),
 	})))))
-	if ret != 0 {
+	if windows.Handle(ret) != windows.S_OK {
 		return err
 	}
 	return nil
 }
 
-func (p *ptyWindows) Close() error {
-	p.closeMutex.Lock()
-	defer p.closeMutex.Unlock()
-	if p.closed {
-		return nil
-	}
-	p.closed = true
-
+// closeConsoleNoLock closes the console handle, and sets it to
+// windows.InvalidHandle. It must be called with p.closeMutex held.
+func (p *ptyWindows) closeConsoleNoLock() error {
 	// if we are running a command in the PTY, the corresponding *windowsProcess
 	// may have already closed the PseudoConsole when the command exited, so that
 	// output reads can get to EOF.  In that case, we don't need to close it
 	// again here.
 	if p.console != windows.InvalidHandle {
+		// ClosePseudoConsole has no return value and typically the syscall
+		// returns S_FALSE (a success value). We could ignore the return value
+		// and error here but we handle anyway, it just in case.
+		//
+		// Note that ClosePseudoConsole is a blocking system call and may write
+		// a final frame to the output buffer (p.outputWrite), so there must be
+		// a consumer (p.outputRead) to ensure we don't block here indefinitely.
+		//
+		// https://docs.microsoft.com/en-us/windows/console/closepseudoconsole
 		ret, _, err := procClosePseudoConsole.Call(uintptr(p.console))
-		if ret < 0 {
-			return xerrors.Errorf("close pseudo console: %w", err)
+		if winerrorFailed(ret) {
+			return xerrors.Errorf("close pseudo console (%d): %w", ret, err)
 		}
 		p.console = windows.InvalidHandle
 	}
 
-	// We always have these files
-	_ = p.outputRead.Close()
-	_ = p.inputWrite.Close()
-	// These get closed & unset if we Start() a new process.
+	return nil
+}
+
+func (p *ptyWindows) Close() error {
+	p.closeMutex.Lock()
+	defer p.closeMutex.Unlock()
+	if p.closed {
+		return nil
+	}
+
+	// Close the pseudo console, this will also terminate the process attached
+	// to this pty. If it was created via Start(), this also unblocks close of
+	// the readers below.
+	err := p.closeConsoleNoLock()
+	if err != nil {
+		return err
+	}
+
+	// Only set closed after the console has been successfully closed.
+	p.closed = true
+
+	// Close the pipes ensuring that the writer is closed before the respective
+	// reader, otherwise closing the reader may block indefinitely. Note that
+	// outputWrite and inputRead are unset when we Start() a new process.
 	if p.outputWrite != nil {
 		_ = p.outputWrite.Close()
 	}
+	_ = p.outputRead.Close()
+	_ = p.inputWrite.Close()
 	if p.inputRead != nil {
 		_ = p.inputRead.Close()
 	}
@@ -184,15 +213,13 @@ func (p *windowsProcess) waitInternal() {
 		// c.f. https://devblogs.microsoft.com/commandline/windows-command-line-introducing-the-windows-pseudo-console-conpty/
 		p.pw.closeMutex.Lock()
 		defer p.pw.closeMutex.Unlock()
-		if p.pw.console != windows.InvalidHandle {
-			ret, _, err := procClosePseudoConsole.Call(uintptr(p.pw.console))
-			if ret < 0 && p.cmdErr == nil {
-				// if we already have an error from the command, prefer that error
-				// but if the command succeeded and closing the PseudoConsole fails
-				// then record that error so that we have a chance to see it
-				p.cmdErr = err
-			}
-			p.pw.console = windows.InvalidHandle
+
+		err := p.pw.closeConsoleNoLock()
+		// if we already have an error from the command, prefer that error
+		// but if the command succeeded and closing the PseudoConsole fails
+		// then record that error so that we have a chance to see it
+		if err != nil && p.cmdErr == nil {
+			p.cmdErr = err
 		}
 	}()
 
@@ -225,3 +252,11 @@ func (p *windowsProcess) killOnContext(ctx context.Context) {
 		p.Kill()
 	}
 }
+
+// winerrorFailed returns true if the syscall failed, this function
+// assumes the return value is a 32-bit integer, like HRESULT.
+//
+// https://learn.microsoft.com/en-us/windows/win32/api/winerror/nf-winerror-failed
+func winerrorFailed(r1 uintptr) bool {
+	return int32(r1) < 0
+}
diff --git a/pty/ptytest/ptytest.go b/pty/ptytest/ptytest.go
@@ -354,7 +354,13 @@ type PTY struct {
 func (p *PTY) Close() error {
 	p.t.Helper()
 	pErr := p.PTY.Close()
-	eErr := p.outExpecter.close("close")
+	if pErr != nil {
+		p.logf("PTY: Close failed: %v", pErr)
+	}
+	eErr := p.outExpecter.close("PTY close")
+	if eErr != nil {
+		p.logf("PTY: close expecter failed: %v", eErr)
+	}
 	if pErr != nil {
 		return pErr
 	}
@@ -398,7 +404,13 @@ type PTYCmd struct {
 func (p *PTYCmd) Close() error {
 	p.t.Helper()
 	pErr := p.PTYCmd.Close()
-	eErr := p.outExpecter.close("close")
+	if pErr != nil {
+		p.logf("PTYCmd: Close failed: %v", pErr)
+	}
+	eErr := p.outExpecter.close("PTYCmd close")
+	if eErr != nil {
+		p.logf("PTYCmd: close expecter failed: %v", eErr)
+	}
 	if pErr != nil {
 		return pErr
 	}
