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

@uppy/companion: fix authProvider property inconsistency #4672

Merged
merged 46 commits into from
Dec 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
df98cca
remove useless line
mifi Aug 9, 2023
9ccf2ae
fix broken cookie removal logic
mifi Aug 9, 2023
04128f9
fix mime type of thumbnails
mifi Aug 9, 2023
9ceac7c
simplify/speedup token generation
mifi Aug 9, 2023
600b2d0
use instanceof instead of prop check
mifi Aug 9, 2023
da67ac9
Implement alternative provider auth
mifi Aug 9, 2023
7d59489
refactor
mifi Aug 13, 2023
ab06da6
use respondWithError
mifi Aug 13, 2023
16d4e3f
fix prepareStream
mifi Aug 13, 2023
2092387
don't throw when missing i18n key
mifi Aug 13, 2023
f575dbc
fix bugged try/catch
mifi Aug 13, 2023
754e2e0
allow aborting login too
mifi Aug 13, 2023
6441123
add json http error support
mifi Aug 13, 2023
f503d1f
don't tightly couple auth form with html form
mifi Aug 13, 2023
272e3d1
fix i18n
mifi Aug 14, 2023
f2e5aa4
make contentType parameterized
mifi Aug 14, 2023
8016a5d
merge main
mifi Sep 6, 2023
9f160f4
Merge branch 'main' into provider-user-sessions
mifi Sep 6, 2023
a28fcec
allow sending certain errors to the user
mifi Sep 6, 2023
e040c7b
make `authProvider` consistent
mifi Sep 6, 2023
408ac3d
fix bug
mifi Sep 6, 2023
e98af01
fix test also
mifi Sep 6, 2023
3166db9
don't have default content-type
mifi Sep 6, 2023
5c20186
make a loginSimpleAuth api too
mifi Sep 7, 2023
b33f5b1
Merge branch 'main' into provider-user-sessions
mifi Oct 2, 2023
7549da4
make removeAuthToken protected
mifi Oct 2, 2023
8794f63
Merge branch 'main' into provider-user-sessions
mifi Nov 27, 2023
42f74b2
fix lint
mifi Nov 27, 2023
7a84c8d
run yarn format
mifi Nov 27, 2023
38eee72
Apply suggestions from code review
mifi Nov 29, 2023
70a7a48
fix broken merge conflict
mifi Nov 29, 2023
0d81a9b
improve inheritance
mifi Dec 1, 2023
96eb565
fix bug
mifi Dec 1, 2023
67d8595
fix bug with dynamic grant config
mifi Dec 2, 2023
5025a73
use duck typing for error checks
mifi Dec 5, 2023
761dcdc
Apply suggestions from code review
mifi Dec 5, 2023
95c8e38
Merge branch 'main' into provider-user-sessions
mifi Dec 5, 2023
64ce471
fix broken lint fix script
mifi Dec 5, 2023
654da1a
fix broken merge code
mifi Dec 5, 2023
a69a1fe
try to fix flakey tets
mifi Dec 5, 2023
5c508d3
Merge branch 'provider-user-sessions' into fix-authprovider-inconsist…
mifi Dec 5, 2023
6d766cb
fix lint
mifi Dec 5, 2023
7a920f9
fix merge issue
mifi Dec 5, 2023
117644f
Merge branch 'provider-user-sessions' into fix-authprovider-inconsist…
mifi Dec 7, 2023
2af7b24
Merge branch 'main' into provider-user-sessions
mifi Dec 7, 2023
854093e
Merge branch 'provider-user-sessions' into fix-authprovider-inconsist…
mifi Dec 7, 2023
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
21 changes: 15 additions & 6 deletions packages/@uppy/companion/src/companion.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ const { ProviderApiError, ProviderUserError, ProviderAuthError } = require('./se
const { getCredentialsOverrideMiddleware } = require('./server/provider/credentials')
// @ts-ignore
const { version } = require('../package.json')
const { isOAuthProvider } = require('./server/provider/Provider')

function setLoggerProcessName ({ loggerProcessName }) {

function setLoggerProcessName({ loggerProcessName }) {
if (loggerProcessName != null) logger.setProcessName(loggerProcessName)
}

Expand Down Expand Up @@ -72,13 +74,15 @@ module.exports.app = (optionsArg = {}) => {

const providers = providerManager.getDefaultProviders()

providerManager.addProviderOptions(options, grantConfig)

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

const getAuthProvider = (providerName) => providers[providerName]?.authProvider

providerManager.addProviderOptions(options, grantConfig, getAuthProvider)

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

Expand Down Expand Up @@ -147,13 +151,18 @@ module.exports.app = (optionsArg = {}) => {
// for simplicity, we just return the normal credentials for the provider, but in a real-world scenario,
// we would query based on parameters
const { key, secret } = options.providerOptions[providerName]

function getRedirectUri() {
const authProvider = getAuthProvider(providerName)
if (!isOAuthProvider(authProvider)) return undefined
return grantConfig[authProvider]?.redirect_uri
}

res.send({
credentials: {
key,
secret,
redirect_uri: providerManager.getGrantConfigForProvider({
providerName, companionOptions: options, grantConfig,
})?.redirect_uri,
redirect_uri: getRedirectUri(),
},
})
})
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 @@ -30,7 +30,7 @@ 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)
// this is needed for things like custom oauth domain (e.g. webdav)
Expand All @@ -45,12 +45,12 @@ module.exports = function connect(req, res) {
]]
}) || [])

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 @@ -51,11 +51,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 @@ -200,11 +195,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) => typeof response.body === 'object' && response.body?.error?.code === 190, // Invalid OAuth 2.0 Access Token
getJsonErrorMessage: (body) => body?.error?.message,
})
Expand Down
32 changes: 7 additions & 25 deletions packages/@uppy/companion/src/server/provider/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ const zoom = require('./zoom')
const { getURLBuilder } = require('../helpers/utils')
const logger = require('../logger')
const { getCredentialsResolver } = require('./credentials')
// eslint-disable-next-line
const Provider = require('./Provider')

const { isOAuthProvider } = Provider
Expand All @@ -25,26 +24,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
}

function getGrantConfigForProvider({ providerName, companionOptions, grantConfig }) {
const authProvider = providerNameToAuthName(providerName, companionOptions)

if (!isOAuthProvider(authProvider)) return undefined
return grantConfig[authProvider]
}

module.exports.getGrantConfigForProvider = getGrantConfigForProvider

/**
* adds the desired provider module to the request object,
* based on the providerName parameter specified
Expand Down Expand Up @@ -106,10 +85,11 @@ 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] = {
grantConfig[authProvider] = {
...customProvider.config,
// todo: consider setting these options from a universal point also used
// by official providers. It'll prevent these from getting left out if the
Expand All @@ -125,8 +105,9 @@ module.exports.addCustomProviders = (customProviders, providers, grantConfig) =>
*
* @param {{server: object, providerOptions: object}} companionOptions
* @param {object} grantConfig
* @param {(a: string) => string} getAuthProvider
*/
module.exports.addProviderOptions = (companionOptions, grantConfig) => {
module.exports.addProviderOptions = (companionOptions, grantConfig, getAuthProvider) => {
const { server, providerOptions } = companionOptions
if (!validOptions({ server })) {
logger.warn('invalid provider options detected. Providers will not be loaded', 'provider.options.invalid')
Expand All @@ -143,7 +124,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 = getAuthProvider?.(providerName)

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) => typeof response.body === 'object' && response.body?.error?.code === 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 @@ -98,11 +93,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
Copy link
Contributor

Choose a reason for hiding this comment

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

This indicates that we could move it out of the class (i.e. as a standalone function). Can we do that in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, but this has to be done in all providers so I didn't feel it was worth the effort (and breakage of git blame/history). maybe a todo will do?

Copy link
Contributor Author

@mifi mifi Dec 8, 2023

Choose a reason for hiding this comment

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

I just tried to refactor one of the provider classes by moving it out of the class, but then we have to add 6 new // eslint-disable-next-line class-methods-use-this because each instance method gets the same error (due to no longer using this.#withErrorHandling). so i'm guessing this is why i didn't do it in the first place 😅

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
Loading