Bug 226999 – [git-webkit] Handle auth failures

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

When we get a 400 error but we didn't prompt the user, we should prompt the user for credentials before giving up.

[Radar WebKit Bug Importer]

<rdar://problem/79313850>

[Jonathan Bedard]

Created attachment 431385 [details]
Patch

[Stephanie Lewis]

Comment on attachment 431385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431385&action=review

> Tools/ChangeLog:10
> +        (credentials): We should attempt to retrieve credentials, even if they

Why?

[Stephanie Lewis]

Does getting credentials prompt?  Because if so we should figure out a better way to know their required than always fetching them

Comment on attachment 431385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431385&action=review

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/remote/git_hub.py:100
> +        if authenticated is None and not auth and response.status_code - (response.status_code % 100) == 400:

Is `response.status_code - (response.status_code % 100) == 400` equivalent to `response.status_code  // 100 == 4`?

[Jonathan Bedard]

(In reply to dewei_zhu from comment #5)
> Comment on attachment 431385 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431385&action=review
> 
> > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/remote/git_hub.py:100
> > +        if authenticated is None and not auth and response.status_code - (response.status_code % 100) == 400:
> 
> Is `response.status_code - (response.status_code % 100) == 400` equivalent
> to `response.status_code  // 100 == 4`?

Yes. Do you prefer the `response.status_code  // 100 == 4` version? No particular reason I used one over the other

[Jonathan Bedard]

Comment on attachment 431385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431385&action=review

>> Tools/ChangeLog:10
>> +        (credentials): We should attempt to retrieve credentials, even if they
> 
> Why?

Because retrieving credentials is a cheap operation. If they aren't there, we won't prompt. But we should be retrieving them (or at least trying to) even if they aren't required.

[Jonathan Bedard]

(In reply to Stephanie Lewis from comment #4)
> Does getting credentials prompt?  Because if so we should figure out a
> better way to know their required than always fetching them

No, getting credentials does not, generally prompt. You might get a prompt for keychain if a particular credential is available, but not accessible to Python. That case should be pretty rare though, because in most cases, it's Python that put the credential there in the first place.

The only case where you will get a command line prompt is the one where we don't have a credential and we need one. Right now "needing" one means that Python specifically says "hey, I need a credential to do this thing". After this patch, "needing" one means either Python specifically asked, or we got a 400 error on something.

[Alexey Proskuryakov]

Comment on attachment 431385 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431385&action=review

>>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/remote/git_hub.py:100
>>> +        if authenticated is None and not auth and response.status_code - (response.status_code % 100) == 400:
>> 
>> Is `response.status_code - (response.status_code % 100) == 400` equivalent to `response.status_code  // 100 == 4`?
> 
> Yes. Do you prefer the `response.status_code  // 100 == 4` version? No particular reason I used one over the other

// looks cleaner to me. Is this script Python 3 only?

[Jonathan Bedard]

(In reply to Alexey Proskuryakov from comment #9)
> Comment on attachment 431385 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431385&action=review
> 
> >>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/remote/git_hub.py:100
> >>> +        if authenticated is None and not auth and response.status_code - (response.status_code % 100) == 400:
> >> 
> >> Is `response.status_code - (response.status_code % 100) == 400` equivalent to `response.status_code  // 100 == 4`?
> > 
> > Yes. Do you prefer the `response.status_code  // 100 == 4` version? No particular reason I used one over the other
> 
> // looks cleaner to me. Is this script Python 3 only?

Not yet, buildbot still runs it as Python 2 in a few places

[Jonathan Bedard]

Created attachment 431442 [details]
Patch

Comment on attachment 431442 [details]
Patch

r=me

[Stephanie Lewis]

I still don't understand why we need credentials if we say we don't need them.  And your commit log should probably explain that too

[Jonathan Bedard]

(In reply to Stephanie Lewis from comment #13)
> I still don't understand why we need credentials if we say we don't need
> them.  And your commit log should probably explain that too

It's more in the case that we don't know if we need them.

Consider the case where we are grabbing a commit from a repository. We don't know in advance if that repository is private or public. If the repository is public, we don't need credentials. If it's private, we do. However, if we have credentials available (as in, we can access those credentials without prompting the user) it's better that we use the credentials. This is for two reasons: First, if we happen to be accessing a private repo, we saved ourselves a request. Second, though, is that if we're accessing a public repo with a credentialed request, that request does not count against the rate limit for your current IP address.

[Jonathan Bedard]

Created attachment 431458 [details]
Patch for landing

[EWS]

Committed r278890 (238832@main): <https://commits.webkit.org/238832@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431458 [details].