Skip to content

login: handle non-tty scenario consistently - #5401

Merged
thaJeztah merged 1 commit into
docker:masterfrom
laurazard:login-non-tty-standardize
Sep 3, 2024
Merged

login: handle non-tty scenario consistently#5401
thaJeztah merged 1 commit into
docker:masterfrom
laurazard:login-non-tty-standardize

Conversation

@laurazard

@laurazardlaurazard commented Sep 3, 2024

Copy link
Copy Markdown
Member

Running docker login in a non-interactive environment sometimes errors out if no username/password is provided.

Before:

--username--passwordResult
Error: Cannot perform an interactive login from a non TTY device
Error: Cannot perform an interactive login from a non TTY device
hangs

After:

--username--passwordResult
Error: Cannot perform an interactive login from a non TTY device
Error: Cannot perform an interactive login from a non TTY device
Error: Cannot perform an interactive login from a non TTY device

It's worth calling out a separate scenario – if there are previous, valid credentials, then running docker login with no username or password provided will use the previously stored credentials, and not error out.

cat ~/.docker/config.json{ "auths": { "https://index.docker.io/v1/": { "auth": "xxxxxxxxxxx" } }}⭑ docker login 0>/dev/nullAuthenticating with existing credentials...Login Succeeded

- What I did

This PR makes this handling consistent, and also adds some tests.

- How I did it

Made the non-tty handling consistent by also erroring out when a user runs docker login --password bork (with no username) when stdin is not a tty.

Also applied the same non-interactive handling logic to the new web-based login flow, which means that now, if there are no prior credentials stored and a user runs docker login, instead of initiating the new web-based login flow, an error is returned.

- How to verify it

Run the added tests, or

  • docker login 0>/dev/null
  • docker login -u [username] -p [password] 0>/dev/null
  • docker login -u [username] 0>/dev/null
  • docker login -p [password] 0>/dev/null

- Description for the changelog

`docker login` now returns an error instead of hanging if called non-interactively with `--password` or `--password-stdin` but without `--user`.

- A picture of a cute animal (not mandatory but encouraged)

Running `docker login` in a non-interactive environment sometimes errors
out if no username/pwd is provided. This handling is somewhat
inconsistent – this commit addresses that.
Before:
| `--username` | `--password` | Result |
|:------------:|:------------:| ------------------------------------------------------------------ |
| ✅ | ✅ | ✅ |
| ❌ | ❌ | `Error: Cannot perform an interactive login from a non TTY device` |
| ✅ | ❌ | `Error: Cannot perform an interactive login from a non TTY device` |
| ❌ | ✅ | hangs |
After:
| `--username` | `--password` | Result |
|:------------:|:------------:| ------------------------------------------------------------------ |
| ✅ | ✅ | ✅ |
| ❌ | ❌ | `Error: Cannot perform an interactive login from a non TTY device` |
| ✅ | ❌ | `Error: Cannot perform an interactive login from a non TTY device` |
| ❌ | ✅ | `Error: Cannot perform an interactive login from a non TTY device` |
It's worth calling out a separate scenario – if there are previous,
valid credentials, then running `docker login` with no username or
password provided will use the previously stored credentials, and not
error out.
```console
cat ~/.docker/config.json
{
"auths": {
"https://index.docker.io/v1/": {
"auth": "xxxxxxxxxxx"
}
}
}
⭑ docker login 0>/dev/null
Authenticating with existing credentials...
Login Succeeded
```
This commit also applies the same non-interactive handling logic to the
new web-based login flow, which means that now, if there are no prior
credentials stored and a user runs `docker login`, instead of initiating
the new web-based login flow, an error is returned.
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
@codecov-commenter

codecov-commenter commented Sep 3, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.73%. Comparing base (092b5f0) to head (bbb6e76).
Report is 548 commits behind head on master.

Additional details and impacted files
@@ Coverage Diff @@## master #5401 +/- ##
==========================================
- Coverage 60.94% 59.73% -1.22% 
==========================================
Files 304 345 +41 Lines 21346 23394 +2048 ==========================================
+ Hits 13010 13974 +964 - Misses 7407 8450 +1043 - Partials 929 970 +41 

@vvolandvvoland added this to the 28.0.0 milestone Sep 3, 2024

@thaJeztahthaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@laurazard@codecov-commenter@thaJeztah@vvoland