Skip to content

Commit

Permalink
make authProvider consistent
Browse files Browse the repository at this point in the history
always use the static property
ignoring the instance propety

fixes #4460
  • Loading branch information
mifi committed Sep 6, 2023
1 parent a28fcec commit e040c7b
Show file tree
Hide file tree
Showing 16 changed files with 35 additions and 63 deletions.
4 changes: 2 additions & 2 deletions packages/@uppy/companion/src/companion.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,13 @@ module.exports.app = (optionsArg = {}) => {

const providers = providerManager.getDefaultProviders()

providerManager.addProviderOptions(options, grantConfig)

const { customProviders } = options
if (customProviders) {
providerManager.addCustomProviders(customProviders, providers, grantConfig)
}

providerManager.addProviderOptions(options, grantConfig, providers)

// mask provider secrets from log messages
logger.setMaskables(getMaskableSecrets(options))

Expand Down
6 changes: 3 additions & 3 deletions packages/@uppy/companion/src/server/controllers/connect.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,17 @@ module.exports = function connect (req, res) {
}

const state = oAuthState.encodeState(stateObj, secret)
const { provider, providerGrantConfig } = req.companion
const { providerClass, providerGrantConfig } = req.companion

// pass along grant's dynamic config (if specified for the provider in its grant config `dynamic` section)
const grantDynamicConfig = Object.fromEntries(providerGrantConfig.dynamic?.map(p => [p, req.query[p]]) || [])

const providerName = provider.authProvider
const { authProvider } = providerClass
const qs = queryString({
...grantDynamicConfig,
state,
})

// Now we redirect to grant's /connect endpoint, see `app.use(Grant(grantConfig))`
res.redirect(req.companion.buildURL(`/connect/${providerName}${qs}`, true))
res.redirect(req.companion.buildURL(`/connect/${authProvider}${qs}`, true))
}
2 changes: 1 addition & 1 deletion packages/@uppy/companion/src/server/controllers/logout.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ async function logout (req, res, next) {
const { accessToken } = providerUserSession
const data = await companion.provider.logout({ token: accessToken, providerUserSession, companion })
delete companion.providerUserSession
tokenService.removeFromCookies(res, companion.options, companion.provider.authProvider)
tokenService.removeFromCookies(res, companion.options, companion.providerClass.authProvider)
cleanSession()
res.json({ ok: true, ...data })
} catch (err) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const oAuthState = require('../helpers/oauth-state')
*/
module.exports = function oauthRedirect (req, res) {
const params = qs.stringify(req.query)
const { authProvider } = req.companion.provider
const { authProvider } = req.companion.providerClass
if (!req.companion.options.server.oauthDomain) {
res.redirect(req.companion.buildURL(`/connect/${authProvider}/callback?${params}`, true))
return
Expand Down
2 changes: 1 addition & 1 deletion packages/@uppy/companion/src/server/helpers/jwt.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ module.exports.addToCookiesIfNeeded = (req, res, uppyAuthToken, maxAge) => {
res,
token: uppyAuthToken,
companionOptions: req.companion.options,
authProvider: req.companion.provider.authProvider,
authProvider: req.companion.providerClass.authProvider,
maxAge,
})
}
Expand Down
2 changes: 1 addition & 1 deletion packages/@uppy/companion/src/server/middlewares.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ exports.gentleVerifyToken = (req, res, next) => {
}

exports.cookieAuthToken = (req, res, next) => {
req.companion.authToken = req.cookies[`uppyAuthToken--${req.companion.provider.authProvider}`]
req.companion.authToken = req.cookies[`uppyAuthToken--${req.companion.providerClass.authProvider}`]
return next()
}

Expand Down
2 changes: 1 addition & 1 deletion packages/@uppy/companion/src/server/provider/Provider.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,5 +122,5 @@ class Provider {
}

module.exports = Provider
// OAuth providers are those that have a `static authProvider` set. It means they require OAuth authentication to work
// OAuth providers are those that have an `authProvider` set. It means they require OAuth authentication to work
module.exports.isOAuthProvider = (authProvider) => typeof authProvider === 'string' && authProvider.length > 0
4 changes: 2 additions & 2 deletions packages/@uppy/companion/src/server/provider/box/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ async function list ({ directory, query, token }) {
class Box extends Provider {
constructor (options) {
super(options)
this.authProvider = Box.authProvider
// needed for the thumbnails fetched via companion
this.needsCookieAuth = true
}
Expand Down Expand Up @@ -116,11 +115,12 @@ class Box extends Provider {
})
}

// eslint-disable-next-line class-methods-use-this
async #withErrorHandling (tag, fn) {
return withProviderErrorHandling({
fn,
tag,
providerName: this.authProvider,
providerName: Box.authProvider,
isAuthError: (response) => response.statusCode === 401,
getJsonErrorMessage: (body) => body?.message,
})
Expand Down
8 changes: 2 additions & 6 deletions packages/@uppy/companion/src/server/provider/drive/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,6 @@ async function getStats ({ id, token }) {
* Adapter for API https://developers.google.com/drive/api/v3/
*/
class Drive extends Provider {
constructor (options) {
super(options)
this.authProvider = Drive.authProvider
}

static get authProvider () {
return 'google'
}
Expand Down Expand Up @@ -181,11 +176,12 @@ class Drive extends Provider {
})
}

// eslint-disable-next-line class-methods-use-this
async #withErrorHandling (tag, fn) {
return withProviderErrorHandling({
fn,
tag,
providerName: this.authProvider,
providerName: Drive.authProvider,
isAuthError: (response) => (
response.statusCode === 401
|| (response.statusCode === 400 && response.body?.error === 'invalid_grant') // Refresh token has expired or been revoked
Expand Down
4 changes: 2 additions & 2 deletions packages/@uppy/companion/src/server/provider/dropbox/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ async function userInfo ({ token }) {
class DropBox extends Provider {
constructor (options) {
super(options)
this.authProvider = DropBox.authProvider
this.needsCookieAuth = true
}

Expand Down Expand Up @@ -136,11 +135,12 @@ class DropBox extends Provider {
})
}

// eslint-disable-next-line class-methods-use-this
async #withErrorHandling (tag, fn) {
return withProviderErrorHandling({
fn,
tag,
providerName: this.authProvider,
providerName: DropBox.authProvider,
isAuthError: (response) => response.statusCode === 401,
getJsonErrorMessage: (body) => body?.error_summary,
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,6 @@ async function getMediaUrl ({ token, id }) {
* Adapter for API https://developers.facebook.com/docs/graph-api/using-graph-api/
*/
class Facebook extends Provider {
constructor (options) {
super(options)
this.authProvider = Facebook.authProvider
}

static get authProvider () {
return 'facebook'
}
Expand Down Expand Up @@ -86,11 +81,12 @@ class Facebook extends Provider {
})
}

// eslint-disable-next-line class-methods-use-this
async #withErrorHandling (tag, fn) {
return withProviderErrorHandling({
fn,
tag,
providerName: this.authProvider,
providerName: Facebook.authProvider,
isAuthError: (response) => response.statusCode === 190, // Invalid OAuth 2.0 Access Token
getJsonErrorMessage: (body) => body?.error?.message,
})
Expand Down
20 changes: 6 additions & 14 deletions packages/@uppy/companion/src/server/provider/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,6 @@ const validOptions = (options) => {
return options.server.host && options.server.protocol
}

/**
*
* @param {string} name of the provider
* @param {{server: object, providerOptions: object}} options
* @returns {string} the authProvider for this provider
*/
const providerNameToAuthName = (name, options) => { // eslint-disable-line no-unused-vars
const providers = exports.getDefaultProviders()
return (providers[name] || {}).authProvider
}

/**
* adds the desired provider module to the request object,
* based on the providerName parameter specified
Expand Down Expand Up @@ -97,8 +86,9 @@ module.exports.addCustomProviders = (customProviders, providers, grantConfig) =>

// eslint-disable-next-line no-param-reassign
providers[providerName] = customProvider.module
const { authProvider } = customProvider.module

if (isOAuthProvider(customProvider.module.authProvider)) {
if (isOAuthProvider(authProvider)) {
// eslint-disable-next-line no-param-reassign
grantConfig[providerName] = {
...customProvider.config,
Expand All @@ -116,8 +106,9 @@ module.exports.addCustomProviders = (customProviders, providers, grantConfig) =>
*
* @param {{server: object, providerOptions: object}} companionOptions
* @param {object} grantConfig
* @param {Record<string, typeof Provider>} providers
*/
module.exports.addProviderOptions = (companionOptions, grantConfig) => {
module.exports.addProviderOptions = (companionOptions, grantConfig, providers) => {
const { server, providerOptions } = companionOptions
if (!validOptions({ server })) {
logger.warn('invalid provider options detected. Providers will not be loaded', 'provider.options.invalid')
Expand All @@ -134,7 +125,8 @@ module.exports.addProviderOptions = (companionOptions, grantConfig) => {
const { oauthDomain } = server
const keys = Object.keys(providerOptions).filter((key) => key !== 'server')
keys.forEach((providerName) => {
const authProvider = providerNameToAuthName(providerName, companionOptions)
const authProvider = providers[providerName]?.authProvider

if (isOAuthProvider(authProvider) && grantConfig[authProvider]) {
// explicitly add providerOptions so users don't override other providerOptions.
// eslint-disable-next-line no-param-reassign
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,6 @@ async function getMediaUrl ({ token, id }) {
* Adapter for API https://developers.facebook.com/docs/instagram-api/overview
*/
class Instagram extends Provider {
constructor (options) {
super(options)
this.authProvider = Instagram.authProvider
}

// for "grant"
static getExtraConfig () {
return {
Expand Down Expand Up @@ -86,11 +81,12 @@ class Instagram extends Provider {
return { revoked: false, manual_revoke_url: 'https://www.instagram.com/accounts/manage_access/' }
}

// eslint-disable-next-line class-methods-use-this
async #withErrorHandling (tag, fn) {
return withProviderErrorHandling({
fn,
tag,
providerName: this.authProvider,
providerName: Instagram.authProvider,
isAuthError: (response) => response.statusCode === 190, // Invalid OAuth 2.0 Access Token
getJsonErrorMessage: (body) => body?.error?.message,
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,6 @@ const getRootPath = (query) => (query.driveId ? `drives/${query.driveId}` : 'me/
* Adapter for API https://docs.microsoft.com/en-us/onedrive/developer/rest-api/
*/
class OneDrive extends Provider {
constructor (options) {
super(options)
this.authProvider = OneDrive.authProvider
}

static get authProvider () {
return 'microsoft'
}
Expand Down Expand Up @@ -96,11 +91,12 @@ class OneDrive extends Provider {
})
}

// eslint-disable-next-line class-methods-use-this
async #withErrorHandling (tag, fn) {
return withProviderErrorHandling({
fn,
tag,
providerName: this.authProvider,
providerName: OneDrive.authProvider,
isAuthError: (response) => response.statusCode === 401,
getJsonErrorMessage: (body) => body?.error?.message,
})
Expand Down
8 changes: 2 additions & 6 deletions packages/@uppy/companion/src/server/provider/zoom/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,6 @@ async function findFile ({ client, meetingId, fileId, recordingStart }) {
* Adapter for API https://marketplace.zoom.us/docs/api-reference/zoom-api
*/
class Zoom extends Provider {
constructor (options) {
super(options)
this.authProvider = Zoom.authProvider
}

static get authProvider () {
return 'zoom'
}
Expand Down Expand Up @@ -157,6 +152,7 @@ class Zoom extends Provider {
})
}

// eslint-disable-next-line class-methods-use-this
async #withErrorHandling (tag, fn) {
const authErrorCodes = [
124, // expired token
Expand All @@ -166,7 +162,7 @@ class Zoom extends Provider {
return withProviderErrorHandling({
fn,
tag,
providerName: this.authProvider,
providerName: Zoom.authProvider,
isAuthError: (response) => authErrorCodes.includes(response.statusCode),
getJsonErrorMessage: (body) => body?.message,
})
Expand Down
10 changes: 5 additions & 5 deletions packages/@uppy/companion/test/__tests__/provider-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ describe('Test Provider options', () => {
})

test('adds provider options', () => {
providerManager.addProviderOptions(companionOptions, grantConfig)
providerManager.addProviderOptions(companionOptions, grantConfig, providerManager.getDefaultProviders())
expect(grantConfig.dropbox.key).toBe('dropbox_key')
expect(grantConfig.dropbox.secret).toBe('dropbox_secret')

Expand All @@ -33,7 +33,7 @@ describe('Test Provider options', () => {

test('adds extra provider config', () => {
process.env.COMPANION_INSTAGRAM_KEY = '123456'
providerManager.addProviderOptions(getCompanionOptions(), grantConfig)
providerManager.addProviderOptions(getCompanionOptions(), grantConfig, providerManager.getDefaultProviders())
expect(grantConfig.instagram).toEqual({
transport: 'session',
callback: '/instagram/callback',
Expand Down Expand Up @@ -101,7 +101,7 @@ describe('Test Provider options', () => {

companionOptions = getCompanionOptions()

providerManager.addProviderOptions(companionOptions, grantConfig)
providerManager.addProviderOptions(companionOptions, grantConfig, providerManager.getDefaultProviders())

expect(grantConfig.dropbox.secret).toBe('xobpord')
expect(grantConfig.box.secret).toBe('xwbepqd')
Expand All @@ -115,7 +115,7 @@ describe('Test Provider options', () => {
delete companionOptions.server.host
delete companionOptions.server.protocol

providerManager.addProviderOptions(companionOptions, grantConfig)
providerManager.addProviderOptions(companionOptions, grantConfig, providerManager.getDefaultProviders())
expect(grantConfig.dropbox.key).toBeUndefined()
expect(grantConfig.dropbox.secret).toBeUndefined()

Expand All @@ -134,7 +134,7 @@ describe('Test Provider options', () => {

test('sets a main redirect uri, if oauthDomain is set', () => {
companionOptions.server.oauthDomain = 'domain.com'
providerManager.addProviderOptions(companionOptions, grantConfig)
providerManager.addProviderOptions(companionOptions, grantConfig, providerManager.getDefaultProviders())

expect(grantConfig.dropbox.redirect_uri).toBe('http://domain.com/dropbox/redirect')
expect(grantConfig.box.redirect_uri).toBe('http://domain.com/box/redirect')
Expand Down

0 comments on commit e040c7b

Please sign in to comment.