Bug 226999 - [git-webkit] Handle auth failures
Summary: [git-webkit] Handle auth failures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-14 16:33 PDT by Jonathan Bedard
Modified: 2021-06-15 12:05 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.75 KB, patch)
2021-06-14 16:42 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (2.69 KB, patch)
2021-06-15 08:56 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch for landing (5.23 KB, patch)
2021-06-15 11:37 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2021-06-14 16:33:50 PDT
When we get a 400 error but we didn't prompt the user, we should prompt the user for credentials before giving up.
Comment 1 Radar WebKit Bug Importer 2021-06-14 16:34:13 PDT
<rdar://problem/79313850>
Comment 2 Jonathan Bedard 2021-06-14 16:42:14 PDT
Created attachment 431385 [details]
Patch
Comment 3 Stephanie Lewis 2021-06-14 16:50:13 PDT
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?
Comment 4 Stephanie Lewis 2021-06-14 16:51:36 PDT
Does getting credentials prompt?  Because if so we should figure out a better way to know their required than always fetching them
Comment 5 dewei_zhu 2021-06-14 16:57:28 PDT
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`?
Comment 6 Jonathan Bedard 2021-06-14 17:09:20 PDT
(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
Comment 7 Jonathan Bedard 2021-06-14 17:14:32 PDT
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.
Comment 8 Jonathan Bedard 2021-06-14 17:18:43 PDT
(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.
Comment 9 Alexey Proskuryakov 2021-06-14 17:23:33 PDT
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?
Comment 10 Jonathan Bedard 2021-06-14 17:29:01 PDT
(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
Comment 11 Jonathan Bedard 2021-06-15 08:56:58 PDT
Created attachment 431442 [details]
Patch
Comment 12 dewei_zhu 2021-06-15 10:30:24 PDT
Comment on attachment 431442 [details]
Patch

r=me
Comment 13 Stephanie Lewis 2021-06-15 10:30:47 PDT
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
Comment 14 Jonathan Bedard 2021-06-15 10:45:53 PDT
(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.
Comment 15 Jonathan Bedard 2021-06-15 11:37:03 PDT
Created attachment 431458 [details]
Patch for landing
Comment 16 EWS 2021-06-15 12:05:39 PDT
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].