Rewrite and test Github auth logic, separating standard and search quota #1205

paulmelnikow · 2017-10-25T04:58:40Z

Summary:

Create new priority queue-based token cycling logic which correctly handles simultaneous requests using the same token
Correctly support search API rate limits, which are separate from core API rate limits (Github search has custom rate limits #1119)
When tokens are exhausted, return an error instead of attempting fallback to our API key
Use a static token in development, when one is configured (replacing When a global gh_token is configured, always use it #1118)
Split up Github auth logic into separate modules which can be tested in isolation

Todo:

Test persistence
Integration test?
Incorporate recent changes to lib/github-auth.js
Catch persistence errors and send them to sentry
Rework TokenProvider / TokenPool abstraction which may not be so helpful. Github has two pools so things feel a little bit off.
Update token persistence for these changes to the api provider
Wait for merge of TokenPool: Add debug logging helper + return correct error #2400

To handle in follow on work:

Log / emit to sentry when tokens are exhausted
(maybe) Consolidate reserve logic, probably in Token

lib/github-auth/acceptor.js

# Conflicts: # lib/github-auth.js # package-lock.json # package.json # server.js

# Conflicts: # package-lock.json # package.json

# Conflicts: # package-lock.json

# Conflicts: # lib/github-auth.js

# Conflicts: # lib/github-auth.js # package-lock.json # package.json # server.js

paulmelnikow · 2018-03-04T01:35:32Z

I'm eager to get this shipped, though I'm hesitant to do it until I get access to logs…

LVMBDV · 2018-06-15T16:17:21Z

lib/github-auth/acceptor.js

+  }));
+}
+
+function constEq(a, b) {


I'm new to node in general but why not use crypto.timingSafeEqual(a, b) instead?

I didn't write this, just moved it. Yes, happy to switch that over. It seems like that solves the same problem.

This was designed as part of a rewrite of the Github auth code in #1205 which is stalled because I don't want to deploy it without access to server logs. The need for token rotation came up recently in #541, so I picked up this code, removed the github-specific bits, and pulled it in. Ordinarily I wouldn't merge helper code without a use, though sadly it seems like the best way to move forward this rewrite.

paulmelnikow · 2018-07-26T17:48:32Z

Since we have access to errors through Sentry, I think I will revive this and see if we can't nudge it forward.

calebcartwright

I'm a little over half way through, but figured I'd go ahead and share my current thoughts/questions

.circleci/config.yml

calebcartwright · 2019-01-09T01:47:33Z

lib/fs-token-persistence.js

@@ -23,21 +22,28 @@ class FsTokenPersistence extends TokenPersistence {
    }

    const tokens = JSON.parse(contents)
-    tokens.forEach(tokenString => {
-      githubAuth.addGithubToken(tokenString)
-    })


My first time looking at this part of the code, but it looks like this change here decouples this persistence engine from github specifics, which creates the possibility of this being reusable for other services?

I've no idea if there's a similar need in other services, but 👍 either way!

lib/fs-token-persistence.spec.js

calebcartwright · 2019-01-09T01:58:28Z

lib/fs-token-persistence.spec.js

        expected.push(newToken)

        await persistence.initialize()
-        githubAuth.addGithubToken(newToken)
        await persistence.noteTokenAdded(newToken)


i know that the function name noteTokenAdded here wasn't something modified in this PR, but it felt a little strange to me. seeing this for the first time I wasn't clear what exactly the function does from the name

Should we change it to add()?

Yeah I'd vote for add() or addToken() (and definitely add() if we can/will add things other than tokens)

services/github/auth/acceptor.spec.js

services/github/github-api-provider.js

services/github/github-api-provider.spec.js

calebcartwright · 2019-01-09T02:44:10Z

I've made it through my first pass and added some inline comments. Although beware, there are more questions for my own understanding than there is useful feedback for you 😁

paulmelnikow · 2019-01-09T16:52:40Z

Although beware, there are more questions for my own understanding than

Avoiding knowledge bottlenecks is fully half of why we do this :)

calebcartwright · 2019-01-09T20:06:54Z

I think this is good to go, will do another pass here in a bit and approve

calebcartwright · 2019-01-09T22:49:36Z

services/github/auth/acceptor.spec.js

+      body: { shieldsSecret: fakeShieldsSecret, token: fakeAccessToken },
+    })
+
+    expect(onTokenAccepted).to.have.been.calledWith(fakeAccessToken)


calebcartwright

This was a herculean effort! ✨ 🎉 👏

It looks good to me as is, only pending comment was the potential rename of noteTokenAdded function to add, but that's not a blocker

paulmelnikow · 2019-01-09T23:07:38Z

Thanks so much! Will be glad to close this out… and see if it really works. Let’s one of us tackle the renames in a follow-on.

paulmelnikow · 2019-01-10T17:32:58Z

I'll merge and deploy this in the afternoon when we get past peak usage time, in case anything breaks.

shields-deployment · 2019-01-11T02:30:26Z

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

The TokenProvider abstraction was refactored away during #1205 and is now obsolete. Other users of token pooling should use TokenPool and TokenPersistence directly.

Rewrite and test Github auth logic

202b508

paulmelnikow added bug Bugs in badges and the frontend core Server, BaseService, GitHub auth labels Oct 25, 2017

paulmelnikow commented Oct 25, 2017

View reviewed changes

lib/github-auth/acceptor.js Outdated Show resolved Hide resolved

Clean lint

a105e98

paulmelnikow mentioned this pull request Oct 26, 2017

When a global gh_token is configured, always use it #1118

Merged

paulmelnikow added 12 commits October 31, 2017 23:35

Merge branch 'master' into github-token-pool-2

054314a

# Conflicts: # lib/github-auth.js # package-lock.json # package.json # server.js

Clean diff

e045dbf

Rm unused

8617f46

Adjust limits

815ec67

Readability improvements

224266d

Simplify reserveFraction

d138ce0

Validate tokens

188d389

Add an integration test

d99d095

Renames + docs

71ccb19

TokenPersistence: Tests and fixes

ee5dba1

Merge branch 'master' into github-token-pool-2

06047cd

# Conflicts: # package-lock.json # package.json

Merge branch 'master' into github-token-pool-2

7cc3ea6

# Conflicts: # package-lock.json

paulmelnikow changed the title ~~Rewrite and test Github auth logic~~ [WIP] Rewrite and test Github auth logic Nov 2, 2017

paulmelnikow added 4 commits November 10, 2017 00:07

Merge branch 'master' into github-token-pool-2

8f587bd

# Conflicts: # lib/github-auth.js

Apply fix from #1244

139b8ad

Merge branch 'master' into github-token-pool-2

c49f6d9

# Conflicts: # lib/github-auth.js # package-lock.json # package.json # server.js

Update package-lock

afe0da5

espadrine force-pushed the master branch from 203d6d5 to 7e668c7 Compare December 3, 2017 18:06

paulmelnikow mentioned this pull request Jun 15, 2018

Badge request: Gitlab CI and Gitlab #541

Closed

LVMBDV reviewed Jun 15, 2018

View reviewed changes

paulmelnikow mentioned this pull request Jun 16, 2018

Generalized rotating token pool #1736

Merged

paulmelnikow removed the on-hold Deferred in favor of another approach, blocked on preceding efforts, stale, or abandoned label Jan 4, 2019

paulmelnikow added 2 commits January 4, 2019 18:52

Fix integ tests

6fda89e

Add debug output for a flaky test

c6576fa

paulmelnikow mentioned this pull request Jan 5, 2019

Github search has custom rate limits #1119

Closed

Merge branch 'master' into github-token-pool-2

cebe1c8

calebcartwright reviewed Jan 9, 2019

View reviewed changes

services/github/github-api-provider.js Show resolved Hide resolved

calebcartwright reviewed Jan 9, 2019

View reviewed changes

services/github/github-api-provider.spec.js Show resolved Hide resolved

paulmelnikow added 5 commits January 9, 2019 11:29

Merge branch 'master' into github-token-pool-2

023953d

Remove obsolete secret.json setup

78773e0

Mock creds using sinon

7320221

Reset circle config

ac4619c

Add test for add-token endpoint

fd63acc

calebcartwright reviewed Jan 9, 2019

View reviewed changes

calebcartwright approved these changes Jan 9, 2019

View reviewed changes

Merge branch 'master' into github-token-pool-2

f3b9191

Merge branch 'master' into github-token-pool-2

4dd8cd4

paulmelnikow merged commit c4efdc8 into badges:master Jan 11, 2019

paulmelnikow deleted the github-token-pool-2 branch January 11, 2019 02:30

This was referenced Jan 11, 2019

Crash in modified GitHub auth #2728

Closed

Cleaning up the GitHub auth #2733

Open

paulmelnikow added a commit that referenced this pull request Jan 22, 2019

Remove unused TokenProvider code

44f4d08

The TokenProvider abstraction was refactored away during #1205 and is now obsolete. Other users of token pooling should use TokenPool and TokenPersistence directly.

paulmelnikow mentioned this pull request Jan 22, 2019

Remove unused TokenProvider code #2844

Merged

paulmelnikow added a commit that referenced this pull request Jan 23, 2019

Remove unused TokenProvider code (#2844)

bbc10c5

The TokenProvider abstraction was refactored away during #1205 and is now obsolete. Other users of token pooling should use TokenPool and TokenPersistence directly.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite and test Github auth logic, separating standard and search quota #1205

Rewrite and test Github auth logic, separating standard and search quota #1205

paulmelnikow commented Oct 25, 2017 •

edited

Loading

paulmelnikow commented Mar 4, 2018

LVMBDV Jun 15, 2018

paulmelnikow Jun 16, 2018

paulmelnikow commented Jul 26, 2018

calebcartwright left a comment

calebcartwright Jan 9, 2019

calebcartwright Jan 9, 2019

paulmelnikow Jan 9, 2019

calebcartwright Jan 9, 2019

calebcartwright commented Jan 9, 2019

paulmelnikow commented Jan 9, 2019

calebcartwright commented Jan 9, 2019

calebcartwright Jan 9, 2019

calebcartwright left a comment

paulmelnikow commented Jan 9, 2019

paulmelnikow commented Jan 10, 2019

shields-deployment bot commented Jan 11, 2019

Rewrite and test Github auth logic, separating standard and search quota #1205

Rewrite and test Github auth logic, separating standard and search quota #1205

Conversation

paulmelnikow commented Oct 25, 2017 • edited Loading

paulmelnikow commented Mar 4, 2018

LVMBDV Jun 15, 2018

Choose a reason for hiding this comment

paulmelnikow Jun 16, 2018

Choose a reason for hiding this comment

paulmelnikow commented Jul 26, 2018

calebcartwright left a comment

Choose a reason for hiding this comment

calebcartwright Jan 9, 2019

Choose a reason for hiding this comment

calebcartwright Jan 9, 2019

Choose a reason for hiding this comment

paulmelnikow Jan 9, 2019

Choose a reason for hiding this comment

calebcartwright Jan 9, 2019

Choose a reason for hiding this comment

calebcartwright commented Jan 9, 2019

paulmelnikow commented Jan 9, 2019

calebcartwright commented Jan 9, 2019

calebcartwright Jan 9, 2019

Choose a reason for hiding this comment

calebcartwright left a comment

Choose a reason for hiding this comment

paulmelnikow commented Jan 9, 2019

paulmelnikow commented Jan 10, 2019

shields-deployment bot commented Jan 11, 2019

paulmelnikow commented Oct 25, 2017 •

edited

Loading