Re: [PATCH] [RFC] http: reauthenticate on 401 Unauthorized

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 12, 2023, 21:59 UTC
Message-ID: <xmqqmt147362.fsf@gitster.g>
In-Reply-To: <pull.1521.git.git.1686474351611.gitgitgadget@gmail.com>

"M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

> From: M Hickford <mirth.hickford@gmail.com>
>
> A credential helper may return a bad credential if the user's password
> has changed or a personal access token has expired. The user gets
> an HTTP 401 Unauthorized error. The user invariably retries the command.

... and no matter how many times the user retries, the command will never succeed? Is that the problem the patch tries to solve?

Show 8 quoted lines

> To spare the user from retrying the command, in case of HTTP 401
> Unauthorized, call `credential fill` again and reauthenticate. This will
> succeed if a helper generates a fresh credential or the user enters a
> valid password.
>
> Keep current behaviour of asking user for username and password at
> most once. Sanity check that second credential differs from first before
> trying it.

Soon after changing the password is probably the time it is more likely that you would mistype your password, than after you got used to typing it over and over again. I can understand the wish to avoid asking for correct password forever, but giving just one attempt feels a bit cruel for that reason.

Show 12 quoted lines

> diff --git a/credential.h b/credential.h
> index b8e2936d1dc..c176b05981a 100644
> --- a/credential.h
> +++ b/credential.h
> @@ -134,7 +134,9 @@ struct credential {
>  		 configured:1,
>  		 quit:1,
>  		 use_http_path:1,
> -		 username_from_proto:1;
> +		 username_from_proto:1,
> +		 /* Whether the user has been prompted for username or password. */
> +		 getpass:1;

Mental note: the comment here says "prompted".

Show 16 quoted lines

>  	char *username;
>  	char *password;
> diff --git a/http.c b/http.c
> index bb58bb3e6a3..d2897c4d9d1 100644
> --- a/http.c
> +++ b/http.c
> @@ -1732,7 +1732,11 @@ static int handle_curl_result(struct slot_results *results)
>  	else if (results->http_code == 401) {
>  		if (http_auth.username && http_auth.password) {
>  			credential_reject(&http_auth);
> -			return HTTP_NOAUTH;
> +			if (http_auth.getpass) {
> +				/* Previously prompted user, don't prompt again. */
> +				return HTTP_NOAUTH;
> +			}
> +			return HTTP_REAUTH;

And here we also see "prompted" again. Perhaps it will help make the result easier to read if we renamed the new member from "getpass" to another phrase that contains "prompt"?

Show 10 quoted lines

>  		} else {
>  			http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
>  			if (results->auth_avail) {
> @@ -2125,6 +2129,9 @@ static int http_request_reauth(const char *url,
>  			       struct http_get_options *options)
>  {
>  	int ret = http_request(url, result, target, options);
> +	int reauth = 0;
> +	char* first_username;
> +	char* first_password;

In our codebase, asterisk sticks to the variable, not type.

Thanks.