Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use Basic auth for GitHub (#1705) #1729

Merged
merged 1 commit into from
Aug 16, 2020
Merged

Use Basic auth for GitHub (#1705) #1729

merged 1 commit into from
Aug 16, 2020

Conversation

joeytwiddle
Copy link
Contributor

@joeytwiddle joeytwiddle commented Aug 9, 2020 •
edited
Loading

I don't know if this will work, since I don't have a full test setup.

However this is how we did Basic authentication in our app's fetch requests. So fingers crossed it will work for GitHub too!

Addresses issue #1705

@joeytwiddle joeytwiddle force-pushed the repomanager-basic-auth branch from 936e319 to 5824f41 Compare August 9, 2020 06:50
@Martii Martii mentioned this pull request Aug 11, 2020
Closed
@Martii
Copy link
Member

Martii commented Aug 11, 2020 •
edited
Loading

@joeytwiddle

Had some more dedicated time to think about this... seems syntactically correct although I'm wondering where you got the Basic authorization header syntax from? I see a modified header at https://developer.github.com/v3/auth/#using-the-oauth-authorizations-api-with-two-factor-authentication but nothing regarding actual basic authorization headers. Thanks for citing please.

We'll test it for a while directly on pro and if everything goes well (you'll have to tell me and I'll watch the logs/possible restarts) will merge it unless @sizzlemctwizzle or other steps in with any additional concerns.

@Martii
Copy link
Member

Martii commented Aug 16, 2020 •
edited
Loading

This appears to be working ... specifically not emailing us... going to merge since there is no other response. Doesn't seem to hurt things in the few webhook tests I've done (although shouldn't be related but one never knows with GH) and I'm sure someone would have barked by now. Would be nice to know the citation on where the Basic header came from... still can't confirm it.

@Martii Martii merged commit 557e66d into OpenUserJS:master Aug 16, 2020
@Martii Martii added the needs testing Anyone can add this but it is primarily there for the Assignee indicating that Testers are wanted. label Aug 16, 2020
@joeytwiddle
Copy link
Contributor Author

Some documentation:

I'm sure there are plenty more docs and tutorials out there. But not sure which is the "best one".

The scheme we used in our own projects (and I think the most commonly used) was to send "Basic <username+pass>" to request a token, and thereafter use "Bearer " for all subsequent requests.

This takes a little more code because of the need to persist the token between requests, and the possibility that it might one day expire.

@Martii
Copy link
Member

Martii commented Aug 22, 2020

Thanks... and here I thought it was a GH specific item. I've done basic auth in PHP before just didn't recognize that GH was asking for that.

@Martii Martii added this to the #1705 milestone Apr 11, 2021
Martii added a commit to Martii/OpenUserJS.org that referenced this pull request Apr 13, 2021
@Martii
Additional dep
59c3db3 
* Partially migrate to newer dep for GH API
* Adding some error protection, **but not all**, to the GH API deprecation of QSP's. This is probably temporary until a broader fix is implemented. GH importing may have to be disabled after May 5th, 2021... webhooks might be affected if the old (or new) Promise rejection fails. i.e. no more webhooks... but we'll see after the 5th of next month if not addressed.

Applies to OpenUserJS#1705

NOTE(S):
* Due to the nature of the dependency with Promises it is currently contrary to the STYLEGUIDE.md See OpenUserJS#1556 as is OpenUserJS#1729 with ES6+ syntax.
* Doing what I can to avert this chaos but the Code is spread out all over the place.
@Martii Martii mentioned this pull request Apr 13, 2021
Merged
Martii added a commit that referenced this pull request Apr 13, 2021
@Martii
Additional dep (#1793)
e843506 
* Partially migrate to newer dep for GH API
* Adding some error protection, **but not all**, to the GH API deprecation of QSP's. This is probably temporary until a broader fix is implemented. GH importing may have to be disabled after May 5th, 2021... webhooks might be affected if the old (or new) Promise rejection fails. i.e. no more webhooks... but we'll see after the 5th of next month if not addressed.

Applies to #1705

NOTE(S):
* Due to the nature of the dependency with Promises it is currently contrary to the STYLEGUIDE.md See #1556 as is #1729 with ES6+ syntax.
* Doing what I can to avert this chaos but the Code is spread out all over the place.

Auto-merge
@Martii Martii mentioned this pull request Apr 16, 2021
Open
Martii added a commit to Martii/OpenUserJS.org that referenced this pull request Apr 17, 2021
@Martii
More error checking and casing
26c30a3 
* Translate GH's 403 to 503
* If/when the dep gets updated, gracefully handle GH's 403 as much as possible at authenticated and non-authenticated requests.
* The *@octokit/auth-token* dep returns lower casings for `authentication` and `basic`... not sure if matching this makes a difference but today I've been getting 45 maximum on unauthenticated GH rate limiting with this. Post OpenUserJS#1729

Applies to OpenUserJS#1705 OpenUserJS#37
@Martii Martii mentioned this pull request Apr 17, 2021
Merged
Martii added a commit that referenced this pull request Apr 17, 2021
@Martii
More error checking and casing (#1799)
66c9082 
* Translate GH's 403 to 503
* If/when the dep gets updated, gracefully handle GH's 403 as much as possible at authenticated and non-authenticated requests.
* The *@octokit/auth-token* dep returns lower casings for `authentication` and `basic`... not sure if matching this makes a difference but today I've been getting 45 maximum on unauthenticated GH rate limiting with this. Post #1729

Applies to #1705 #37

Auto-merge
Martii added a commit to Martii/OpenUserJS.org that referenced this pull request Apr 17, 2021
@Martii
Correct console messages, comments, and some bugs
c5186cc 
* Authentication confirmation doesn't happen at all on object construction. This routine is instead used to store the values for later usage. A call to a URL will show if there is a higher rate limit. In the newer dep `authorization` is calculated as well.
* Clean up and add existence checks in repoManager. Don't necessarily have to terminate the app if API Keys for GH aren't there... should just use lower rate limit... although other features might depend on those values. Appears to be part of the webhook but not confirmed.

Applies to OpenUserJS#1705 OpenUserJS#37  and post OpenUserJS#1729
@Martii Martii mentioned this pull request Apr 17, 2021
Merged
Martii added a commit that referenced this pull request Apr 17, 2021
@Martii
Correct console messages, comments, and some bugs (#1800)
a303691 
* Authentication confirmation doesn't happen at all on object construction. This routine is instead used to store the values for later usage. A call to a URL will show if there is a higher rate limit. In the newer dep `authorization` is calculated as well.
* Clean up and add existence checks in repoManager. Don't necessarily have to terminate the app if API Keys for GH aren't there... should just use lower rate limit... although other features might depend on those values. Appears to be part of the webhook but not confirmed.

Applies to #1705 #37  and post #1729

Auto-merge
Martii added a commit to Martii/OpenUserJS.org that referenced this pull request May 19, 2021
@Martii
Flip auth type
5d6b36a 
* I'm guessing the failure that was encountered at OpenUserJS#1705 (comment) may have been a fluke on the GH back-end.
* GH Docs and blog posts are a little too vague for my tastes with `username` and `password`... however upon manual deconstruction and inspection of the *github* dep at https://github.com/OpenUserJS/rest.js/blob/29dedb3022649c27ac1dad79326270b6b2a48047/index.js#L730 it uses nearly what the maintainer mentioned at [the discussion](octokit/octokit.js#2070) and what joeytwiddle did at OpenUserJS#1729 in the recently discovered dead code. *nodes* default for `Buffer` is `utf8` whereas *github*@0.2.4 via commit ref uses `ascii`. Since hashes usually only contain ASCII should not be an issue.
* Newest dep at the time of testing with *@octokit/rest* for this seems to be unstable so going with this for a while since it currently gives the higher rate limit. This could change depending on what they do in the back-end.

Applies to OpenUserJS#1705

NOTE(s)
* a GH url changed recently from https://github.com/octokit/rest.js to https://github.com/octokit/octokit.js ... may affect things even further.
@Martii Martii mentioned this pull request May 19, 2021
Merged
Martii added a commit that referenced this pull request May 19, 2021
@Martii
Flip auth type (#1811)
503d2ff 
* I'm guessing the failure that was encountered at #1705 (comment) may have been a fluke on the GH back-end.
* GH Docs and blog posts are a little too vague for my tastes with `username` and `password`... however upon manual deconstruction and inspection of the *github* dep at https://github.com/OpenUserJS/rest.js/blob/29dedb3022649c27ac1dad79326270b6b2a48047/index.js#L730 it uses nearly what the maintainer mentioned at [the discussion](octokit/octokit.js#2070) and what joeytwiddle did at #1729 in the recently discovered dead code. *nodes* default for `Buffer` is `utf8` whereas *github*@0.2.4 via commit ref uses `ascii`. Since hashes usually only contain ASCII should not be an issue.
* Newest dep at the time of testing with *@octokit/rest* for this seems to be unstable so going with this for a while since it currently gives the higher rate limit. This could change depending on what they do in the back-end.

Applies to #1705

NOTE(s)
* a GH url changed recently from https://github.com/octokit/rest.js to https://github.com/octokit/octokit.js ... may affect things even further.

Auto-merge
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@sizzlemctwizzle sizzlemctwizzle

@Martii Martii

Labels
needs testing Anyone can add this but it is primarily there for the Assignee indicating that Testers are wanted.
Milestone
#1705
Development

Successfully merging this pull request may close these issues.
3 participants
@joeytwiddle @Martii @sizzlemctwizzle