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

Add auth support to [Reddit] badges #10790

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add auth support to [Reddit] badges #10790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions .github/actions/service-tests/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ runs:
OBS_USER: '${{ inputs.obs-user }}'
OBS_PASS: '${{ inputs.obs-pass }}'
PEPY_KEY: '${{ inputs.pepy-key }}'
REDDIT_CLIENT_ID: '${{ inputs.reddit-client-id }}'
REDDIT_CLIENT_SECRET: '${{ inputs.reddit-client-secret }}'
SL_INSIGHT_USER_UUID: '${{ inputs.sl-insight-user-uuid }}'
SL_INSIGHT_API_TOKEN: '${{ inputs.sl-insight-api-token }}'
TWITCH_CLIENT_ID: '${{ inputs.twitch-client-id }}'
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/coveralls-code-coverage.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ jobs:
OBS_USER: '${{ secrets.SERVICETESTS_OBS_USER }}'
OBS_PASS: '${{ secrets.SERVICETESTS_OBS_PASS }}'
PEPY_KEY: '${{ secrets.SERVICETESTS_PEPY_KEY }}'
REDDIT_CLIENT_ID: '${{ secrets.SERVICETESTS_REDDIT_CLIENT_ID }}'
REDDIT_CLIENT_SECRET: '${{ secrets.SERVICETESTS_REDDIT_CLIENT_SECRET }}'
SL_INSIGHT_USER_UUID: '${{ secrets.SERVICETESTS_SL_INSIGHT_USER_UUID }}'
SL_INSIGHT_API_TOKEN: '${{ secrets.SERVICETESTS_SL_INSIGHT_API_TOKEN }}'
TWITCH_CLIENT_ID: '${{ secrets.SERVICETESTS_TWITCH_CLIENT_ID }}'
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/daily-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ jobs:
OBS_USER: '${{ secrets.SERVICETESTS_OBS_USER }}'
OBS_PASS: '${{ secrets.SERVICETESTS_OBS_PASS }}'
PEPY_KEY: '${{ secrets.SERVICETESTS_PEPY_KEY }}'
REDDIT_CLIENT_ID: '${{ secrets.SERVICETESTS_REDDIT_CLIENT_ID }}'
REDDIT_CLIENT_SECRET: '${{ secrets.SERVICETESTS_REDDIT_CLIENT_SECRET }}'
SL_INSIGHT_USER_UUID: '${{ secrets.SERVICETESTS_SL_INSIGHT_USER_UUID }}'
SL_INSIGHT_API_TOKEN: '${{ secrets.SERVICETESTS_SL_INSIGHT_API_TOKEN }}'
TWITCH_CLIENT_ID: '${{ secrets.SERVICETESTS_TWITCH_CLIENT_ID }}'
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/deploy-review-app.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ jobs:
OBS_USER=${{ secrets.SERVICETESTS_OBS_USER }}
OBS_PASS=${{ secrets.SERVICETESTS_OBS_PASS }}
PEPY_KEY=${{ secrets.SERVICETESTS_PEPY_KEY }}
REDDIT_CLIENT_ID=${{ secrets.SERVICETESTS_REDDIT_CLIENT_ID }}
REDDIT_CLIENT_SECRET=${{ secrets.SERVICETESTS_REDDIT_CLIENT_SECRET }}
SL_INSIGHT_API_TOKEN=${{ secrets.SERVICETESTS_SL_INSIGHT_USER_UUID }}
SL_INSIGHT_USER_UUID=${{ secrets.SERVICETESTS_SL_INSIGHT_API_TOKEN }}
TWITCH_CLIENT_ID=${{ secrets.SERVICETESTS_TWITCH_CLIENT_ID }}
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/test-services-22.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ jobs:
obs-user: '${{ secrets.SERVICETESTS_OBS_USER }}'
obs-pass: '${{ secrets.SERVICETESTS_OBS_PASS }}'
pepy-key: '${{ secrets.SERVICETESTS_PEPY_KEY }}'
reddit-client-id: '${{ secrets.SERVICETESTS_REDDIT_CLIENT_ID }}'
reddit-client-secret: '${{ secrets.SERVICETESTS_REDDIT_CLIENT_SECRET }}'
sl-insight-user-uuid: '${{ secrets.SERVICETESTS_SL_INSIGHT_USER_UUID }}'
sl-insight-api-token: '${{ secrets.SERVICETESTS_SL_INSIGHT_API_TOKEN }}'
twitch-client-id: '${{ secrets.SERVICETESTS_TWITCH_CLIENT_ID }}'
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/test-services.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ jobs:
obs-user: '${{ secrets.SERVICETESTS_OBS_USER }}'
obs-pass: '${{ secrets.SERVICETESTS_OBS_PASS }}'
pepy-key: '${{ secrets.SERVICETESTS_PEPY_KEY }}'
reddit-client-id: '${{ secrets.SERVICETESTS_REDDIT_CLIENT_ID }}'
reddit-client-secret: '${{ secrets.SERVICETESTS_REDDIT_CLIENT_SECRET }}'
sl-insight-user-uuid: '${{ secrets.SERVICETESTS_SL_INSIGHT_USER_UUID }}'
sl-insight-api-token: '${{ secrets.SERVICETESTS_SL_INSIGHT_API_TOKEN }}'
twitch-client-id: '${{ secrets.SERVICETESTS_TWITCH_CLIENT_ID }}'
Expand Down
2 changes: 2 additions & 0 deletions config/custom-environment-variables.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ private:
opencollective_token: 'OPENCOLLECTIVE_TOKEN'
pepy_key: 'PEPY_KEY'
postgres_url: 'POSTGRES_URL'
reddit_client_id: 'REDDIT_CLIENT_ID'
reddit_client_secret: 'REDDIT_CLIENT_SECRET'
sentry_dsn: 'SENTRY_DSN'
sl_insight_userUuid: 'SL_INSIGHT_USER_UUID'
sl_insight_apiToken: 'SL_INSIGHT_API_TOKEN'
Expand Down
2 changes: 2 additions & 0 deletions config/local-shields-io-production.template.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ private:
gh_client_id: ...
gh_client_secret: ...
gitlab_token: ...
reddit_client_id: ...
reddit_client_secret: ...
sentry_dsn: ...
shields_secret: ...
sl_insight_userUuid: ...
Expand Down
2 changes: 2 additions & 0 deletions config/local.template.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ private:
gitlab_token: '...'
obs_user: '...'
obs_pass: '...'
reddit_client_id: '...'
reddit_client_secret: '...'
twitch_client_id: '...'
twitch_client_secret: '...'
weblate_api_key: '...'
Expand Down
2 changes: 2 additions & 0 deletions core/server/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,8 @@ const privateConfigSchema = Joi.object({
opencollective_token: Joi.string(),
pepy_key: Joi.string(),
postgres_url: Joi.string().uri({ scheme: 'postgresql' }),
reddit_client_id: Joi.string(),
reddit_client_secret: Joi.string(),
sentry_dsn: Joi.string(),
sl_insight_userUuid: Joi.string(),
sl_insight_apiToken: Joi.string(),
Expand Down
1 change: 1 addition & 0 deletions doc/production-hosting.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ Production hosting is managed by the Shields ops team:
| Cloudflare (CDN) | Access management | @espadrine |
| Cloudflare (CDN) | Admin access | @calebcartwright, @chris48s, @espadrine, @paulmelnikow, @PyvesB |
| Twitch | OAuth app | @PyvesB |
| Reddit | OAuth app | @chris48s, @PyvesB |
Copy link
Member Author

Choose a reason for hiding this comment

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

I reused the account that had been created alongside #9817 (comment) and shared the login details in Keybase.

| Discord | OAuth app | @PyvesB |
| YouTube | Account owner | @PyvesB |
| GitLab | Account owner | @calebcartwright |
Expand Down
11 changes: 11 additions & 0 deletions doc/server-secrets.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,17 @@ Create an account, sign in and obtain generate a key on your
`PYPI_URL` can be used to optionally send all the PyPI requests to a Self-hosted Pypi registry,
users can also override this by query parameter `pypiBaseUrl`.

### Reddit

Using a token for Reddit is optional but will allow higher API rates.

- `REDDIT_CLIENT_ID` (yml: `private.reddit_client_id`)
- `REDDIT_CLIENT_SECRET` (yml: `private.reddit_client_secret`)

Register to use the API using [this form](https://support.reddithelp.com/hc/en-us/requests/new?ticket_form_id=14868593862164)
and create an app in the [Reddit preferences page](https://www.reddit.com/prefs/apps)
in order to obtain a client id and a client secret for making Reddit API calls.

### SymfonyInsight (formerly Sensiolabs)

- `SL_INSIGHT_USER_UUID` (yml: `private.sl_insight_userUuid`)
Expand Down
91 changes: 91 additions & 0 deletions services/reddit/reddit-base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import Joi from 'joi'
import { BaseJsonService } from '../index.js'

const tokenSchema = Joi.object({
access_token: Joi.string().required(),
expires_in: Joi.number(),
scope: Joi.string(),
token_type: Joi.string(),
})
Comment on lines +4 to +9
Copy link
Member

Choose a reason for hiding this comment

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

The expires_in value is used. The scope and token_type values are discarded. Suggest

const tokenSchema = Joi.object({
  access_token: Joi.string().required(),
  expires_in: Joi.number().required(),
}).required()

here.


// Abstract class for Reddit badges
// Authorization flow based on https://github.com/reddit-archive/reddit/wiki/OAuth2#application-only-oauth.
export default class RedditBase extends BaseJsonService {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is partly based on https://github.com/badges/shields/blob/master/services/twitch/twitch-base.js. However:

  • there are some annoying differences compared to Twitch's OAuth flow.
  • the logic in the Twitch code is partly broken. For example, the loop here does not refresh the request object, so we're querying new access tokens but still using the old one in our retries, which defeats the whole purpose of the retry loop. Also, I noticed whilst experimenting that err.response could be undefined in some cases, making the conditional blow up.

I decided to just try to make things work for Reddit as a fist iteration, and will came back to rework things at a later point.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah agreed. Last time I looked at the code for twitch I thought there are some issues that want fixing. Lets not copy the approach.

static category = 'social'

static auth = {
userKey: 'reddit_client_id',
passKey: 'reddit_client_secret',
authorizedOrigins: ['https://www.reddit.com'],
isRequired: false,
}

constructor(...args) {
super(...args)
if (!RedditBase._redditToken && this.authHelper.isConfigured) {
RedditBase._redditToken = this._getNewToken()
}
}

async _getNewToken() {
const tokenRes = await super._requestJson(
this.authHelper.withBasicAuth({
schema: tokenSchema,
url: 'https://www.reddit.com/api/v1/access_token',
options: {
method: 'POST',
body: 'grant_type=client_credentials',
},
httpErrors: {
401: 'invalid token',
},
}),
)

// replace the token when we are 80% near the expire time
// 2147483647 is the max 32-bit value that is accepted by setTimeout(), it's about 24.9 days
const replaceTokenMs = Math.min(
tokenRes.expires_in * 1000 * 0.8,
2147483647,
)
const timeout = setTimeout(() => {
RedditBase._redditToken = this._getNewToken()
}, replaceTokenMs)

// do not block program exit
timeout.unref()
Comment on lines +51 to +56
Copy link
Member

Choose a reason for hiding this comment

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

This approach using setTimeout() isn't wrong, and I'm not saying we have to change it.
That said, just as a data point: We implemented something quite similar to this in the JWT auth helper.

shields/core/base-service/auth-helper.js

Lines 256 to 318 in 41d072e

static _isJwtValid(expiry) {
// we consider the token valid if the expiry
// datetime is later than (now + 1 minute)
return dayjs.unix(expiry).isAfter(dayjs().add(1, 'minutes'))
}
async _getJwt(loginEndpoint) {
const { _user: username, _pass: password } = this
// attempt to get JWT from cache
if (
jwtCache?.[loginEndpoint]?.[username]?.token &&
jwtCache?.[loginEndpoint]?.[username]?.expiry &&
this.constructor._isJwtValid(jwtCache[loginEndpoint][username].expiry)
) {
// cache hit
return jwtCache[loginEndpoint][username].token
}
// cache miss - request a new JWT
const originViolation = !this.isAllowedOrigin(loginEndpoint)
if (originViolation) {
throw new InvalidParameter({
prettyMessage: 'requested origin not authorized',
})
}
const { buffer } = await checkErrorResponse({})(
await fetch(loginEndpoint, {
method: 'POST',
form: { username, password },
}),
)
const json = validate(
{
ErrorClass: InvalidResponse,
prettyErrorMessage: 'invalid response data from auth endpoint',
},
parseJson(buffer),
Joi.object({ token: Joi.string().required() }).required(),
)
const token = json.token
const expiry = this.constructor._getJwtExpiry(token)
// store in the cache
if (!(loginEndpoint in jwtCache)) {
jwtCache[loginEndpoint] = {}
}
jwtCache[loginEndpoint][username] = { token, expiry }
return token
}
async _getJwtAuthHeader(loginEndpoint) {
if (!this.isConfigured) {
return undefined
}
const token = await this._getJwt(loginEndpoint)
return { Authorization: `Bearer ${token}` }
}

One thing that is quite nice about that approach is that the control flow is a bit more linear.
Dunno if you reckon that is any better/worse? If you just want to roll with this then I think it is ok.


return tokenRes.access_token
}

async _requestJson(request) {
if (!this.authHelper.isConfigured) {
return super._requestJson(request)
}

request = await this.addBearerAuthHeader(request)
try {
return await super._requestJson(request)
} catch (err) {
if (err.response && err.response.statusCode === 401) {
// if the token is expired or has been revoked, retry once
RedditBase._redditToken = this._getNewToken()
request = await this.addBearerAuthHeader(request)
return super._requestJson(request)
}
// cannot recover
throw err
}
}

async addBearerAuthHeader(request) {
return {
...request,
options: {
headers: {
Authorization: `Bearer ${await RedditBase._redditToken}`,
},
},
}
}
}
14 changes: 7 additions & 7 deletions services/reddit/subreddit-subscribers.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
import Joi from 'joi'
import { optionalNonNegativeInteger } from '../validators.js'
import { metric } from '../text-formatters.js'
import { BaseJsonService, NotFound, pathParams } from '../index.js'
import { NotFound, pathParams } from '../index.js'
import RedditBase from './reddit-base.js'

const schema = Joi.object({
data: Joi.object({
subscribers: optionalNonNegativeInteger,
}).required(),
}).required()

export default class RedditSubredditSubscribers extends BaseJsonService {
static category = 'social'

export default class RedditSubredditSubscribers extends RedditBase {
static route = {
base: 'reddit/subreddit-subscribers',
pattern: ':subreddit',
Expand All @@ -29,8 +28,6 @@ export default class RedditSubredditSubscribers extends BaseJsonService {
},
}

static _cacheLength = 7200
Copy link
Member

Choose a reason for hiding this comment

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

Related to the question about what rate limit we get with authentication, are we confident that we'd have enough rate limit to get away with completely ditching this?


static defaultBadgeData = {
label: 'reddit',
namedLogo: 'reddit',
Expand All @@ -49,7 +46,10 @@ export default class RedditSubredditSubscribers extends BaseJsonService {
async fetch({ subreddit }) {
return this._requestJson({
schema,
url: `https://www.reddit.com/r/${subreddit}/about.json`,
// API requests with a bearer token should be made to https://oauth.reddit.com, NOT www.reddit.com.
url: this.authHelper.isConfigured
? `https://oauth.reddit.com/r/${subreddit}/about.json`
: `https://www.reddit.com/r/${subreddit}/about.json`,
httpErrors: {
404: 'subreddit not found',
403: 'subreddit is private',
Expand Down
21 changes: 20 additions & 1 deletion services/reddit/subreddit-subscribers.tester.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import { noToken } from '../test-helpers.js'
import { isMetric } from '../test-validators.js'
import { createServiceTester } from '../tester.js'
import _serviceClass from './subreddit-subscribers.service.js'
export const t = await createServiceTester()
const noRedditToken = noToken(_serviceClass)
const hasRedditToken = () => !noRedditToken()

t.create('subreddit-subscribers (valid subreddit)')
.get('/drums.json')
Expand Down Expand Up @@ -30,7 +34,8 @@ t.create('subreddit-subscribers (private sub)')
message: 'subreddit is private',
})

t.create('subreddit-subscribers (private sub)')
t.create('subreddit-subscribers (private sub, without token)')
.skipWhen(hasRedditToken)
.get('/centuryclub.json')
.intercept(nock =>
nock('https://www.reddit.com/r')
Expand All @@ -41,3 +46,17 @@ t.create('subreddit-subscribers (private sub)')
label: 'reddit',
message: 'subreddit not found',
})

t.create('subreddit-subscribers (private sub, with token)')
.skipWhen(noRedditToken)
.get('/centuryclub.json')
.intercept(nock =>
nock('https://oauth.reddit.com/r')
.get('/centuryclub/about.json')
.reply(200, { kind: 't5', data: {} }),
)
.networkOn() // API /access_token may or may not be called depending on whether another test ran before and cached the token. Rather than conditionally intercepting it, let it go through and only mock the API call we're validating specific behaviour against.
.expectBadge({
label: 'reddit',
message: 'subreddit not found',
})
14 changes: 7 additions & 7 deletions services/reddit/user-karma.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import Joi from 'joi'
import { anyInteger } from '../validators.js'
import { metric } from '../text-formatters.js'
import { BaseJsonService, pathParams } from '../index.js'
import { pathParams } from '../index.js'
import RedditBase from './reddit-base.js'

const schema = Joi.object({
data: Joi.object({
Expand All @@ -10,9 +11,7 @@ const schema = Joi.object({
}).required(),
}).required()

export default class RedditUserKarma extends BaseJsonService {
static category = 'social'

export default class RedditUserKarma extends RedditBase {
static route = {
base: 'reddit/user-karma',
pattern: ':variant(link|comment|combined)/:user',
Expand All @@ -37,8 +36,6 @@ export default class RedditUserKarma extends BaseJsonService {
},
}

static _cacheLength = 7200

static defaultBadgeData = {
label: 'reddit karma',
namedLogo: 'reddit',
Expand All @@ -61,7 +58,10 @@ export default class RedditUserKarma extends BaseJsonService {
async fetch({ user }) {
return this._requestJson({
schema,
url: `https://www.reddit.com/u/${user}/about.json`,
// API requests with a bearer token should be made to https://oauth.reddit.com, NOT www.reddit.com.
url: this.authHelper.isConfigured
? `https://oauth.reddit.com/u/${user}/about.json`
: `https://www.reddit.com/u/${user}/about.json`,
httpErrors: {
404: 'user not found',
},
Expand Down
Loading
Loading