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.

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

allow calling [github] without auth #9427

Merged
merged 3 commits into from
Aug 7, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions services/github/github-api-provider.integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,32 @@ describe('Github API provider', function () {

let githubApiProvider

context('without token pool', function () {
context('with no auth', function () {
before(function () {
githubApiProvider = new GithubApiProvider({
baseUrl,
withPooling: false,
authType: GithubApiProvider.AUTH_TYPES.NO_AUTH,
})
})

it('should be able to run 10 requests', async function () {
this.timeout('20s')
for (let i = 0; i < 10; ++i) {
const { res } = await githubApiProvider.fetch(
fetch,
'/repos/rust-lang/rust',
{},
)
expect(res.statusCode).to.equal(200)
}
})
})

context('with global token', function () {
before(function () {
githubApiProvider = new GithubApiProvider({
baseUrl,
authType: GithubApiProvider.AUTH_TYPES.GLOBAL_TOKEN,
globalToken: token,
reserveFraction,
})
Expand All @@ -30,7 +51,12 @@ describe('Github API provider', function () {
it('should be able to run 10 requests', async function () {
this.timeout('20s')
for (let i = 0; i < 10; ++i) {
await githubApiProvider.fetch(fetch, '/repos/rust-lang/rust', {})
const { res } = await githubApiProvider.fetch(
fetch,
'/repos/rust-lang/rust',
{},
)
expect(res.statusCode).to.equal(200)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I was working on this, I realised this test never actually asserted the calls were successful! 😬

}
})
})
Expand All @@ -40,7 +66,7 @@ describe('Github API provider', function () {
before(function () {
githubApiProvider = new GithubApiProvider({
baseUrl,
withPooling: true,
authType: GithubApiProvider.AUTH_TYPES.TOKEN_POOL,
reserveFraction,
})
githubApiProvider.addToken(token)
Expand Down
28 changes: 20 additions & 8 deletions services/github/github-api-provider.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,25 +33,31 @@ const bodySchema = Joi.object({

// Provides an interface to the Github API. Manages the base URL.
class GithubApiProvider {
static AUTH_TYPES = {
NO_AUTH: 'No Auth',
GLOBAL_TOKEN: 'Global Token',
TOKEN_POOL: 'Token Pool',
}

// reserveFraction: The amount of much of a token's quota we avoid using, to
// reserve it for the user.
constructor({
baseUrl,
withPooling = true,
authType = this.constructor.AUTH_TYPES.NO_AUTH,
onTokenInvalidated = tokenString => {},
globalToken,
reserveFraction = 0.25,
restApiVersion,
}) {
Object.assign(this, {
baseUrl,
withPooling,
authType,
onTokenInvalidated,
globalToken,
reserveFraction,
})

if (this.withPooling) {
if (this.authType === this.constructor.AUTH_TYPES.TOKEN_POOL) {
this.standardTokens = new TokenPool({ batchSize: 25 })
this.searchTokens = new TokenPool({ batchSize: 5 })
this.graphqlTokens = new TokenPool({ batchSize: 25 })
Expand All @@ -60,7 +66,7 @@ class GithubApiProvider {
}

addToken(tokenString) {
if (this.withPooling) {
if (this.authType === this.constructor.AUTH_TYPES.TOKEN_POOL) {
this.standardTokens.add(tokenString)
this.searchTokens.add(tokenString)
this.graphqlTokens.add(tokenString)
Expand Down Expand Up @@ -157,7 +163,7 @@ class GithubApiProvider {

let token
let tokenString
if (this.withPooling) {
if (this.authType === this.constructor.AUTH_TYPES.TOKEN_POOL) {
try {
token = this.tokenForUrl(url)
} catch (e) {
Expand All @@ -167,7 +173,7 @@ class GithubApiProvider {
})
}
tokenString = token.id
} else {
} else if (this.authType === this.constructor.AUTH_TYPES.GLOBAL_TOKEN) {
tokenString = this.globalToken
}

Expand All @@ -176,14 +182,20 @@ class GithubApiProvider {
...{
headers: {
'User-Agent': userAgent,
Authorization: `token ${tokenString}`,
'X-GitHub-Api-Version': this.restApiVersion,
...options.headers,
},
},
}
if (
this.authType === this.constructor.AUTH_TYPES.TOKEN_POOL ||
this.authType === this.constructor.AUTH_TYPES.GLOBAL_TOKEN
) {
mergedOptions.headers.Authorization = `token ${tokenString}`
}

const response = await requestFetcher(`${baseUrl}${url}`, mergedOptions)
if (this.withPooling) {
if (this.authType === this.constructor.AUTH_TYPES.TOKEN_POOL) {
if (response.res.statusCode === 401) {
this.invalidateToken(token)
} else if (response.res.statusCode < 500) {
Expand Down
6 changes: 5 additions & 1 deletion services/github/github-api-provider.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ describe('Github API provider', function () {

let mockStandardToken, mockSearchToken, mockGraphqlToken, provider
beforeEach(function () {
provider = new GithubApiProvider({ baseUrl, reserveFraction })
provider = new GithubApiProvider({
baseUrl,
authType: GithubApiProvider.AUTH_TYPES.TOKEN_POOL,
reserveFraction,
})

mockStandardToken = { update: sinon.spy(), invalidate: sinon.spy() }
sinon.stub(provider.standardTokens, 'next').returns(mockStandardToken)
Expand Down
9 changes: 8 additions & 1 deletion services/github/github-auth-service.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,14 @@ class GithubAuthV4Service extends BaseGraphqlService {
`,
)

return super._requestGraphql({ ...attrs, ...{ url, query } })
return super._requestGraphql({
...attrs,
...{
url,
query,
httpErrorMessages: { 401: 'auth required for graphql api' },
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The v4/GraphQL API doesn't allow anonymous access - a token is required

},
})
}
}

Expand Down
1 change: 1 addition & 0 deletions services/github/github-auth-service.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ describe('GithubAuthV3Service', function () {
)
const githubApiProvider = new GithubApiProvider({
baseUrl: 'https://github-api.example.com',
authType: GithubApiProvider.AUTH_TYPES.TOKEN_POOL,
restApiVersion: '2022-11-28',
})
const mockToken = { update: sinon.mock(), invalidate: sinon.mock() }
Expand Down
15 changes: 12 additions & 3 deletions services/github/github-constellation.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,28 @@ class GithubConstellation {
this._debugEnabled = config.service.debug.enabled
this._debugIntervalSeconds = config.service.debug.intervalSeconds

let authType = GithubApiProvider.AUTH_TYPES.NO_AUTH

const { postgres_url: pgUrl, gh_token: globalToken } = config.private
if (pgUrl) {
log.log('Token persistence configured with dbUrl')
log.log('Github Token persistence configured with pgUrl')
this.persistence = new SqlTokenPersistence({
url: pgUrl,
table: 'github_user_tokens',
})
authType = GithubApiProvider.AUTH_TYPES.TOKEN_POOL
}

if (globalToken) {
authType = GithubApiProvider.AUTH_TYPES.GLOBAL_TOKEN
}

log.log(`Github using auth type: ${authType}`)

this.apiProvider = new GithubApiProvider({
baseUrl: config.service.baseUri,
globalToken,
withPooling: !globalToken,
authType,
onTokenInvalidated: tokenString => this.onTokenInvalidated(tokenString),
restApiVersion: config.service.restApiVersion,
})
Expand All @@ -52,7 +61,7 @@ class GithubConstellation {
}

async initialize(server) {
if (!this.apiProvider.withPooling) {
if (this.apiProvider.authType !== GithubApiProvider.AUTH_TYPES.TOKEN_POOL) {
return
}

Expand Down
8 changes: 6 additions & 2 deletions services/github/github-search.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import Joi from 'joi'
import { metric } from '../text-formatters.js'
import { nonNegativeInteger } from '../validators.js'
import { GithubAuthV3Service } from './github-auth-service.js'
import { httpErrorsFor, documentation } from './github-helpers.js'
import { documentation } from './github-helpers.js'

const schema = Joi.object({ total_count: nonNegativeInteger }).required()

Expand Down Expand Up @@ -49,7 +49,11 @@ export default class GithubSearch extends GithubAuthV3Service {
},
},
schema,
httpErrors: httpErrorsFor('repo not found'),
httpErrors: {
401: 'auth required for search api',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The search API doesn't allow anonymous access - a token is required

404: 'repo not found',
422: 'repo not found',
},
})
return this.constructor.render({ query, totalCount })
}
Expand Down
Loading