Skip to content

Commit a329a15

Browse files
mifiaduh95
andauthored
@uppy/companion: fix authProvider property inconsistency (#4672)
* remove useless line * fix broken cookie removal logic related #4426 * fix mime type of thumbnails not critical but some browsers might have problems * simplify/speedup token generation so we don't have to decode/decrypt/encode/encrypt so many times * use instanceof instead of prop check * Implement alternative provider auth New concept "simple auth" - authentication that happens immediately (in one http request) without redirecting to any third party. uppyAuthToken initially used to simply contain an encrypted & json encoded OAuth2 access_token for a specific provider. Then we added refresh tokens as well inside uppyAuthToken #4448. Now we also allow storing other state or parameters needed for that specific provider, like username, password, host name, webdav URL etc... This is needed for providers like webdav, ftp etc, where the user needs to give some more input data while authenticating Companion: - `providerTokens` has been renamed to `providerUserSession` because it now includes not only tokens, but a user's session with a provider. Companion `Provider` class: - New `hasSimpleAuth` static boolean property - whether this provider uses simple auth - uppyAuthToken expiry default 24hr again for providers that don't support refresh tokens - make uppyAuthToken expiry configurable per provider - new `authStateExpiry` static property (defaults to 24hr) - new static property `grantDynamicToUserSession`, allows providers to specify which state from Grant `dynamic` to include into the provider's `providerUserSession`. * refactor * use respondWithError also for thumbnails for consistency * fix prepareStream it wasn't returning the status code (like `got` does on error) it's needed to respond properly with a http error * don't throw when missing i18n key instead log error and show the key this in on par with other i18n frameworks * fix bugged try/catch * allow aborting login too and don't replace the whole view with a loader when plugin state loading it will cause auth views to lose state an inter-view loading text looks much more graceful and is how SearchProviderView works too * add json http error support add support for passing objects and messages from companion to uppy this allows companion to for example give a more detailed error when authenticating * don't tightly couple auth form with html form don't force the user to use html form and use preact for it, for flexibility * fix i18n * make contentType parameterized * allow sending certain errors to the user this is useful because: // onedrive gives some errors here that the user might want to know about // e.g. these happen if you try to login to a users in an organization, // without an Office365 licence or OneDrive account setup completed // 400: Tenant does not have a SPO license // 403: You do not have access to create this personal site or you do not have a valid license * make `authProvider` consistent always use the static property ignoring the instance propety fixes #4460 * fix bug * fix test also * don't have default content-type * make a loginSimpleAuth api too * make removeAuthToken protected (cherry picked from commit 4be2b6f) * fix lint * run yarn format * Apply suggestions from code review Co-authored-by: Antoine du Hamel <antoine@transloadit.com> * fix broken merge conflict * improve inheritance * fix bug * fix bug with dynamic grant config * use duck typing for error checks see discussion here: #4619 (comment) * Apply suggestions from code review Co-authored-by: Antoine du Hamel <antoine@transloadit.com> * fix broken lint fix script * fix broken merge code * try to fix flakey tets * fix lint * fix merge issue --------- Co-authored-by: Antoine du Hamel <antoine@transloadit.com>
1 parent 1b6c260 commit a329a15

File tree

16 files changed

+53
-80
lines changed

16 files changed

+53
-80
lines changed

packages/@uppy/companion/src/companion.js

+15-6
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,10 @@ const { ProviderApiError, ProviderUserError, ProviderAuthError } = require('./se
2020
const { getCredentialsOverrideMiddleware } = require('./server/provider/credentials')
2121
// @ts-ignore
2222
const { version } = require('../package.json')
23+
const { isOAuthProvider } = require('./server/provider/Provider')
2324

24-
function setLoggerProcessName ({ loggerProcessName }) {
25+
26+
function setLoggerProcessName({ loggerProcessName }) {
2527
if (loggerProcessName != null) logger.setProcessName(loggerProcessName)
2628
}
2729

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

7375
const providers = providerManager.getDefaultProviders()
7476

75-
providerManager.addProviderOptions(options, grantConfig)
76-
7777
const { customProviders } = options
7878
if (customProviders) {
7979
providerManager.addCustomProviders(customProviders, providers, grantConfig)
8080
}
8181

82+
const getAuthProvider = (providerName) => providers[providerName]?.authProvider
83+
84+
providerManager.addProviderOptions(options, grantConfig, getAuthProvider)
85+
8286
// mask provider secrets from log messages
8387
logger.setMaskables(getMaskableSecrets(options))
8488

@@ -147,13 +151,18 @@ module.exports.app = (optionsArg = {}) => {
147151
// for simplicity, we just return the normal credentials for the provider, but in a real-world scenario,
148152
// we would query based on parameters
149153
const { key, secret } = options.providerOptions[providerName]
154+
155+
function getRedirectUri() {
156+
const authProvider = getAuthProvider(providerName)
157+
if (!isOAuthProvider(authProvider)) return undefined
158+
return grantConfig[authProvider]?.redirect_uri
159+
}
160+
150161
res.send({
151162
credentials: {
152163
key,
153164
secret,
154-
redirect_uri: providerManager.getGrantConfigForProvider({
155-
providerName, companionOptions: options, grantConfig,
156-
})?.redirect_uri,
165+
redirect_uri: getRedirectUri(),
157166
},
158167
})
159168
})

packages/@uppy/companion/src/server/controllers/connect.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ module.exports = function connect(req, res) {
3030
}
3131

3232
const state = oAuthState.encodeState(stateObj, secret)
33-
const { provider, providerGrantConfig } = req.companion
33+
const { providerClass, providerGrantConfig } = req.companion
3434

3535
// pass along grant's dynamic config (if specified for the provider in its grant config `dynamic` section)
3636
// this is needed for things like custom oauth domain (e.g. webdav)
@@ -45,12 +45,12 @@ module.exports = function connect(req, res) {
4545
]]
4646
}) || [])
4747

48-
const providerName = provider.authProvider
48+
const { authProvider } = providerClass
4949
const qs = queryString({
5050
...grantDynamicConfig,
5151
state,
5252
})
5353

5454
// Now we redirect to grant's /connect endpoint, see `app.use(Grant(grantConfig))`
55-
res.redirect(req.companion.buildURL(`/connect/${providerName}${qs}`, true))
55+
res.redirect(req.companion.buildURL(`/connect/${authProvider}${qs}`, true))
5656
}

packages/@uppy/companion/src/server/controllers/logout.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ async function logout (req, res, next) {
2626
const { accessToken } = providerUserSession
2727
const data = await companion.provider.logout({ token: accessToken, providerUserSession, companion })
2828
delete companion.providerUserSession
29-
tokenService.removeFromCookies(res, companion.options, companion.provider.authProvider)
29+
tokenService.removeFromCookies(res, companion.options, companion.providerClass.authProvider)
3030
cleanSession()
3131
res.json({ ok: true, ...data })
3232
} catch (err) {

packages/@uppy/companion/src/server/controllers/oauth-redirect.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ const oAuthState = require('../helpers/oauth-state')
1010
*/
1111
module.exports = function oauthRedirect (req, res) {
1212
const params = qs.stringify(req.query)
13-
const { authProvider } = req.companion.provider
13+
const { authProvider } = req.companion.providerClass
1414
if (!req.companion.options.server.oauthDomain) {
1515
res.redirect(req.companion.buildURL(`/connect/${authProvider}/callback?${params}`, true))
1616
return

packages/@uppy/companion/src/server/helpers/jwt.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ module.exports.addToCookiesIfNeeded = (req, res, uppyAuthToken, maxAge) => {
125125
res,
126126
token: uppyAuthToken,
127127
companionOptions: req.companion.options,
128-
authProvider: req.companion.provider.authProvider,
128+
authProvider: req.companion.providerClass.authProvider,
129129
maxAge,
130130
})
131131
}

packages/@uppy/companion/src/server/middlewares.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ exports.gentleVerifyToken = (req, res, next) => {
122122
}
123123

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

packages/@uppy/companion/src/server/provider/Provider.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -122,5 +122,5 @@ class Provider {
122122
}
123123

124124
module.exports = Provider
125-
// OAuth providers are those that have a `static authProvider` set. It means they require OAuth authentication to work
125+
// OAuth providers are those that have an `authProvider` set. It means they require OAuth authentication to work
126126
module.exports.isOAuthProvider = (authProvider) => typeof authProvider === 'string' && authProvider.length > 0

packages/@uppy/companion/src/server/provider/box/index.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ async function list ({ directory, query, token }) {
3131
class Box extends Provider {
3232
constructor (options) {
3333
super(options)
34-
this.authProvider = Box.authProvider
3534
// needed for the thumbnails fetched via companion
3635
this.needsCookieAuth = true
3736
}
@@ -116,11 +115,12 @@ class Box extends Provider {
116115
})
117116
}
118117

118+
// eslint-disable-next-line class-methods-use-this
119119
async #withErrorHandling (tag, fn) {
120120
return withProviderErrorHandling({
121121
fn,
122122
tag,
123-
providerName: this.authProvider,
123+
providerName: Box.authProvider,
124124
isAuthError: (response) => response.statusCode === 401,
125125
getJsonErrorMessage: (body) => body?.message,
126126
})

packages/@uppy/companion/src/server/provider/drive/index.js

+2-6
Original file line numberDiff line numberDiff line change
@@ -51,11 +51,6 @@ async function getStats ({ id, token }) {
5151
* Adapter for API https://developers.google.com/drive/api/v3/
5252
*/
5353
class Drive extends Provider {
54-
constructor (options) {
55-
super(options)
56-
this.authProvider = Drive.authProvider
57-
}
58-
5954
static get authProvider () {
6055
return 'google'
6156
}
@@ -200,11 +195,12 @@ class Drive extends Provider {
200195
})
201196
}
202197

198+
// eslint-disable-next-line class-methods-use-this
203199
async #withErrorHandling (tag, fn) {
204200
return withProviderErrorHandling({
205201
fn,
206202
tag,
207-
providerName: this.authProvider,
203+
providerName: Drive.authProvider,
208204
isAuthError: (response) => (
209205
response.statusCode === 401
210206
|| (response.statusCode === 400 && response.body?.error === 'invalid_grant') // Refresh token has expired or been revoked

packages/@uppy/companion/src/server/provider/dropbox/index.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@ async function userInfo ({ token }) {
5656
class DropBox extends Provider {
5757
constructor (options) {
5858
super(options)
59-
this.authProvider = DropBox.authProvider
6059
this.needsCookieAuth = true
6160
}
6261

@@ -136,11 +135,12 @@ class DropBox extends Provider {
136135
})
137136
}
138137

138+
// eslint-disable-next-line class-methods-use-this
139139
async #withErrorHandling (tag, fn) {
140140
return withProviderErrorHandling({
141141
fn,
142142
tag,
143-
providerName: this.authProvider,
143+
providerName: DropBox.authProvider,
144144
isAuthError: (response) => response.statusCode === 401,
145145
getJsonErrorMessage: (body) => body?.error_summary,
146146
})

packages/@uppy/companion/src/server/provider/facebook/index.js

+2-6
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,6 @@ async function getMediaUrl ({ token, id }) {
2424
* Adapter for API https://developers.facebook.com/docs/graph-api/using-graph-api/
2525
*/
2626
class Facebook extends Provider {
27-
constructor (options) {
28-
super(options)
29-
this.authProvider = Facebook.authProvider
30-
}
31-
3227
static get authProvider () {
3328
return 'facebook'
3429
}
@@ -86,11 +81,12 @@ class Facebook extends Provider {
8681
})
8782
}
8883

84+
// eslint-disable-next-line class-methods-use-this
8985
async #withErrorHandling (tag, fn) {
9086
return withProviderErrorHandling({
9187
fn,
9288
tag,
93-
providerName: this.authProvider,
89+
providerName: Facebook.authProvider,
9490
isAuthError: (response) => typeof response.body === 'object' && response.body?.error?.code === 190, // Invalid OAuth 2.0 Access Token
9591
getJsonErrorMessage: (body) => body?.error?.message,
9692
})

packages/@uppy/companion/src/server/provider/index.js

+7-25
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ const zoom = require('./zoom')
1212
const { getURLBuilder } = require('../helpers/utils')
1313
const logger = require('../logger')
1414
const { getCredentialsResolver } = require('./credentials')
15-
// eslint-disable-next-line
1615
const Provider = require('./Provider')
1716

1817
const { isOAuthProvider } = Provider
@@ -25,26 +24,6 @@ const validOptions = (options) => {
2524
return options.server.host && options.server.protocol
2625
}
2726

28-
/**
29-
*
30-
* @param {string} name of the provider
31-
* @param {{server: object, providerOptions: object}} options
32-
* @returns {string} the authProvider for this provider
33-
*/
34-
const providerNameToAuthName = (name, options) => { // eslint-disable-line no-unused-vars
35-
const providers = exports.getDefaultProviders()
36-
return (providers[name] || {}).authProvider
37-
}
38-
39-
function getGrantConfigForProvider({ providerName, companionOptions, grantConfig }) {
40-
const authProvider = providerNameToAuthName(providerName, companionOptions)
41-
42-
if (!isOAuthProvider(authProvider)) return undefined
43-
return grantConfig[authProvider]
44-
}
45-
46-
module.exports.getGrantConfigForProvider = getGrantConfigForProvider
47-
4827
/**
4928
* adds the desired provider module to the request object,
5029
* based on the providerName parameter specified
@@ -106,10 +85,11 @@ module.exports.addCustomProviders = (customProviders, providers, grantConfig) =>
10685

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

110-
if (isOAuthProvider(customProvider.module.authProvider)) {
90+
if (isOAuthProvider(authProvider)) {
11191
// eslint-disable-next-line no-param-reassign
112-
grantConfig[providerName] = {
92+
grantConfig[authProvider] = {
11393
...customProvider.config,
11494
// todo: consider setting these options from a universal point also used
11595
// by official providers. It'll prevent these from getting left out if the
@@ -125,8 +105,9 @@ module.exports.addCustomProviders = (customProviders, providers, grantConfig) =>
125105
*
126106
* @param {{server: object, providerOptions: object}} companionOptions
127107
* @param {object} grantConfig
108+
* @param {(a: string) => string} getAuthProvider
128109
*/
129-
module.exports.addProviderOptions = (companionOptions, grantConfig) => {
110+
module.exports.addProviderOptions = (companionOptions, grantConfig, getAuthProvider) => {
130111
const { server, providerOptions } = companionOptions
131112
if (!validOptions({ server })) {
132113
logger.warn('invalid provider options detected. Providers will not be loaded', 'provider.options.invalid')
@@ -143,7 +124,8 @@ module.exports.addProviderOptions = (companionOptions, grantConfig) => {
143124
const { oauthDomain } = server
144125
const keys = Object.keys(providerOptions).filter((key) => key !== 'server')
145126
keys.forEach((providerName) => {
146-
const authProvider = providerNameToAuthName(providerName, companionOptions)
127+
const authProvider = getAuthProvider?.(providerName)
128+
147129
if (isOAuthProvider(authProvider) && grantConfig[authProvider]) {
148130
// explicitly add providerOptions so users don't override other providerOptions.
149131
// eslint-disable-next-line no-param-reassign

packages/@uppy/companion/src/server/provider/instagram/graph/index.js

+2-6
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,6 @@ async function getMediaUrl ({ token, id }) {
2323
* Adapter for API https://developers.facebook.com/docs/instagram-api/overview
2424
*/
2525
class Instagram extends Provider {
26-
constructor (options) {
27-
super(options)
28-
this.authProvider = Instagram.authProvider
29-
}
30-
3126
// for "grant"
3227
static getExtraConfig () {
3328
return {
@@ -86,11 +81,12 @@ class Instagram extends Provider {
8681
return { revoked: false, manual_revoke_url: 'https://www.instagram.com/accounts/manage_access/' }
8782
}
8883

84+
// eslint-disable-next-line class-methods-use-this
8985
async #withErrorHandling (tag, fn) {
9086
return withProviderErrorHandling({
9187
fn,
9288
tag,
93-
providerName: this.authProvider,
89+
providerName: Instagram.authProvider,
9490
isAuthError: (response) => typeof response.body === 'object' && response.body?.error?.code === 190, // Invalid OAuth 2.0 Access Token
9591
getJsonErrorMessage: (body) => body?.error?.message,
9692
})

packages/@uppy/companion/src/server/provider/onedrive/index.js

+2-6
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,6 @@ const getRootPath = (query) => (query.driveId ? `drives/${query.driveId}` : 'me/
2323
* Adapter for API https://docs.microsoft.com/en-us/onedrive/developer/rest-api/
2424
*/
2525
class OneDrive extends Provider {
26-
constructor (options) {
27-
super(options)
28-
this.authProvider = OneDrive.authProvider
29-
}
30-
3126
static get authProvider () {
3227
return 'microsoft'
3328
}
@@ -98,11 +93,12 @@ class OneDrive extends Provider {
9893
})
9994
}
10095

96+
// eslint-disable-next-line class-methods-use-this
10197
async #withErrorHandling (tag, fn) {
10298
return withProviderErrorHandling({
10399
fn,
104100
tag,
105-
providerName: this.authProvider,
101+
providerName: OneDrive.authProvider,
106102
isAuthError: (response) => response.statusCode === 401,
107103
isUserFacingError: (response) => [400, 403].includes(response.statusCode),
108104
// onedrive gives some errors here that the user might want to know about

packages/@uppy/companion/src/server/provider/zoom/index.js

+2-6
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,6 @@ async function findFile ({ client, meetingId, fileId, recordingStart }) {
2929
* Adapter for API https://marketplace.zoom.us/docs/api-reference/zoom-api
3030
*/
3131
class Zoom extends Provider {
32-
constructor (options) {
33-
super(options)
34-
this.authProvider = Zoom.authProvider
35-
}
36-
3732
static get authProvider () {
3833
return 'zoom'
3934
}
@@ -157,6 +152,7 @@ class Zoom extends Provider {
157152
})
158153
}
159154

155+
// eslint-disable-next-line class-methods-use-this
160156
async #withErrorHandling (tag, fn) {
161157
const authErrorCodes = [
162158
124, // expired token
@@ -166,7 +162,7 @@ class Zoom extends Provider {
166162
return withProviderErrorHandling({
167163
fn,
168164
tag,
169-
providerName: this.authProvider,
165+
providerName: Zoom.authProvider,
170166
isAuthError: (response) => authErrorCodes.includes(response.statusCode),
171167
getJsonErrorMessage: (body) => body?.message,
172168
})

0 commit comments

Comments
 (0)