Distinguish between authentication failures due to missing vs invalid credentials#12667
Distinguish between authentication failures due to missing vs invalid credentials#12667
Conversation
jtfmumm
force-pushed
the
jtfm/auth-error-messages
branch
from
c2836c8 to
4368739
Compare
|
|
||
| assert_eq!( | ||
| assert!( | ||
| client | ||
| .get(format!("{}/foo", server.uri())) | ||
| .send() | ||
| .await? | ||
| .status(), | ||
| 401 | ||
| .await | ||
| .is_err(), | ||
| "Requests should require credentials" | ||
| ); |
There was a problem hiding this comment.
Why did this one change? There's no policy set here.
There was a problem hiding this comment.
Maybe there's a better way to do it, but the existing approach had been to throw an error for the missing credentials case. There's no policy set here, but the reason for the 401 is because of missing credentials.
There was a problem hiding this comment.
I don't think we can throw an error there though, doesn't downstream logic rely on a successful request? (i.e., in #12667 (comment))
Perhaps I misunderstood the intent of this PR?
There was a problem hiding this comment.
The only tests that broke were cases where we'd want the message to indicate that missing credentials was the cause of failing to find the package. But if we are depending on getting the 401 out of this response downstream, then this approach won't work. In that case, I can try using reqwest extensions to add more context about the 401.
There was a problem hiding this comment.
Ah I understand the goal now, you want to make the case where no credentials were present distinct from the case where the credentials are wrong. Sorry it took me a while to get there. I think the snapshot changes should be like..
hint: An index URL (https://pypi-proxy.fly.dev/basic-auth/simple) could not be queried due to a lack of valid authentication credentials (401 Unauthorized).
to
hint: An index URL (https://pypi-proxy.fly.dev/basic-auth/simple) could not be queried due to missing credentials (401 Unauthorized)
and
hint: An index URL (https://pypi-proxy.fly.dev/basic-auth/simple) could not be queried due to invalid credentials (401 Unauthorized)
I think changing that error path entirely feels too risky and is hopefully unnecessary.
There was a problem hiding this comment.
(fwiw, I think this would have been clearer if the title was something like "Distinguish between authentication failures due to missing vs invalid credentials")
| hint: An index URL (https://pypi-proxy.fly.dev/basic-auth/simple) could not be queried due to a lack of valid authentication credentials (401 Unauthorized). | ||
| "###); | ||
| error: Failed to fetch: `https://pypi-proxy.fly.dev/basic-auth/simple/iniconfig/` | ||
| Caused by: Missing credentials for https://pypi-proxy.fly.dev/basic-auth/simple/iniconfig/ |
There was a problem hiding this comment.
This looks like a regression to me?
There was a problem hiding this comment.
This is a case where the fetch failed because there were no credentials, which is the reason the package was not found (since we were never authenticated, we were not able to search for it). So this case is intentional. Does it still seem like a regression?
There was a problem hiding this comment.
I think so? Doesn't this fail eagerly now? What if there was a second index with the package available?
There was a problem hiding this comment.
(It should be easy to add a test for that, we should probably have coverage regardless)
There was a problem hiding this comment.
I'll add that test.
| uv_snapshot!(context.filters(), context.lock(), @r###" | ||
| uv_snapshot!(context.filters(), context.lock(), @r" | ||
| success: false | ||
| exit_code: 1 | ||
| exit_code: 2 | ||
| ----- stdout ----- | ||
|
|
||
| ----- stderr ----- | ||
| × No solution found when resolving dependencies: | ||
| ╰─▶ Because iniconfig was not found in the package registry and child depends on iniconfig>=2, we can conclude that child's requirements are unsatisfiable. | ||
| And because your workspace requires child, we can conclude that your workspace's requirements are unsatisfiable. | ||
| "###); | ||
| error: Failed to fetch: `https://pypi-proxy.fly.dev/basic-auth/simple/iniconfig/` | ||
| Caused by: Missing credentials for https://pypi-proxy.fly.dev/basic-auth/simple/iniconfig/ | ||
| "); |
There was a problem hiding this comment.
A bit of an aside, but... why weren't we showing the hint from https://github.com/astral-sh/uv/pull/12667/files#diff-82edd36151736f44055f699a34c8b19a63ffc4cf3c86bf5fb34d69f8ac88a957L8471 in this case?
There was a problem hiding this comment.
That's a good question. I'll look into it
jtfmumm
changed the title
|
Closing because we are going to use a different approach. |
2 participants
Prior to this change, unless an index was configured as
authenticate = "always", authentication failures would output the same error message whether the cause was missing credentials or incorrect credentials. This PR ensures that missing credentials lead to a distinct error message.Once #12651 is merged,
Closes #12280