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

[Junio C Hamano <gitster@pobox.com>]

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

> 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?

> 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.

> 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".

>  	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"?

>  		} 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.