Merge pull request #1298 from transloadit/deprecate-companion-auth

companion: return 401 for invalid access token

packages/@uppy/companion-client/src/AuthError.js

@@ -0,0 +1,11 @@
+'use strict'
+
+class AuthError extends Error {
+  constructor () {
+    super('Authorization required')
+    this.name = 'AuthError'
+    this.isAuthError = true
+  }
+}
+
+module.exports = AuthError

packages/@uppy/companion-client/src/Provider.js

@@ -28,6 +28,14 @@ module.exports = class Provider extends RequestClient {
     })
   }
 
+  onReceiveResponse (response) {
+    response = super.onReceiveResponse(response)
+    const authenticated = response.status !== 401
+    this.uppy.getPlugin(this.pluginId).setPluginState({ authenticated })
+    return response
+  }
+
+  // @todo(i.olarewaju) consider whether or not this method should be exposed
   setAuthToken (token) {
     return this.uppy.getPlugin(this.pluginId).storage.setItem(this.tokenKey, token)
   }
@@ -36,13 +44,6 @@ module.exports = class Provider extends RequestClient {
     return this.uppy.getPlugin(this.pluginId).storage.getItem(this.tokenKey)
   }
 
-  checkAuth () {
-    return this.get(`${this.id}/authorized`)
-      .then((payload) => {
-        return payload.authenticated
-      })
-  }
-
   authUrl () {
     return `${this.hostname}/${this.id}/connect`
   }

packages/@uppy/companion-client/src/RequestClient.js

@@ -1,5 +1,7 @@
 'use strict'
 
+const AuthError = require('./AuthError')
+
 // Remove the trailing slash so we can always safely append /xyz.
 function stripSlash (url) {
   return url.replace(/\/$/, '')
@@ -29,6 +31,16 @@ module.exports = class RequestClient {
     return Promise.resolve(Object.assign({}, this.defaultHeaders, this.opts.serverHeaders || {}))
   }
 
+  _getPostResponseFunc (skip) {
+    return (response) => {
+      if (!skip) {
+        return this.onReceiveResponse(response)
+      }
+
+      return response
+    }
+  }
+
   onReceiveResponse (response) {
     const state = this.uppy.getState()
     const companion = state.companion || {}
@@ -52,7 +64,18 @@ module.exports = class RequestClient {
     return `${this.hostname}/${url}`
   }
 
-  get (path) {
+  _json(res) {
+    if (res.status === 401) {
+      throw new AuthError()
+    }
+
+    if (res.status < 200 || res.status > 300) {
+      throw new Error(`Failed request to ${res.url}. ${res.statusText}`)
+    }
+    return res.json()
+  }
+
+  get (path, skipPostResponse) {
     return new Promise((resolve, reject) => {
       this.headers().then((headers) => {
         fetch(this._getUrl(path), {
@@ -60,17 +83,17 @@ module.exports = class RequestClient {
           headers: headers,
           credentials: 'same-origin'
         })
-          // @todo validate response status before calling json
-          .then(this.onReceiveResponse)
-          .then((res) => res.json().then(resolve))
+          .then(this._getPostResponseFunc(skipPostResponse))
+          .then((res) => this._json(res).then(resolve))
           .catch((err) => {
-            reject(new Error(`Could not get ${this._getUrl(path)}. ${err}`))
+            err = err.isAuthError ? err : new Error(`Could not get ${this._getUrl(path)}. ${err}`)
+            reject(err)
           })
       })
     })
   }
 
-  post (path, data) {
+  post (path, data, skipPostResponse) {
     return new Promise((resolve, reject) => {
       this.headers().then((headers) => {
         fetch(this._getUrl(path), {
@@ -79,22 +102,17 @@ module.exports = class RequestClient {
           credentials: 'same-origin',
           body: JSON.stringify(data)
         })
-          .then(this.onReceiveResponse)
-          .then((res) => {
-            if (res.status < 200 || res.status > 300) {
-              reject(new Error(`Could not post ${this._getUrl(path)}. ${res.statusText}`))
-              return
-            }
-            res.json().then(resolve)
-          })
+          .then(this._getPostResponseFunc(skipPostResponse))
+          .then((res) => this._json(res).then(resolve))
           .catch((err) => {
-            reject(new Error(`Could not post ${this._getUrl(path)}. ${err}`))
+            err = err.isAuthError ? err : new Error(`Could not post ${this._getUrl(path)}. ${err}`)
+            reject(err)
           })
       })
     })
   }
 
-  delete (path, data) {
+  delete (path, data, skipPostResponse) {
     return new Promise((resolve, reject) => {
       this.headers().then((headers) => {
         fetch(`${this.hostname}/${path}`, {
@@ -103,11 +121,11 @@ module.exports = class RequestClient {
           credentials: 'same-origin',
           body: data ? JSON.stringify(data) : null
         })
-          .then(this.onReceiveResponse)
-          // @todo validate response status before calling json
-          .then((res) => res.json().then(resolve))
+          .then(this._getPostResponseFunc(skipPostResponse))
+          .then((res) => this._json(res).then(resolve))
           .catch((err) => {
-            reject(new Error(`Could not delete ${this._getUrl(path)}. ${err}`))
+            err = err.isAuthError ? err : new Error(`Could not delete ${this._getUrl(path)}. ${err}`)
+            reject(err)
           })
       })
     })

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

@@ -2,7 +2,7 @@ const Uploader = require('../Uploader')
 const redis = require('../redis')
 const logger = require('../logger')
 
-function get (req, res) {
+function get (req, res, next) {
   const providerName = req.params.providerName
   const id = req.params.id
   const body = req.body
@@ -11,7 +11,11 @@ function get (req, res) {
   const { providerOptions } = req.uppy.options
 
   // get the file size before proceeding
-  provider.size({ id, token }, (size) => {
+  provider.size({ id, token }, (err, size) => {
+    if (err) {
+      return err.isAuthError ? res.sendStatus(401) : next(err)
+    }
+
     if (!size) {
       logger.error('unable to determine file size', 'controller.get.provider.size')
       return res.status(400).json({error: 'unable to determine file size'})

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

@@ -3,7 +3,10 @@ function list ({ query, params, uppy }, res, next) {
   const token = uppy.providerTokens[providerName]
 
   uppy.provider.list({ uppy, token, directory: params.id, query }, (err, data) => {
-    return err ? next(err) : res.json(data)
+    if (err) {
+      return err.isAuthError ? res.sendStatus(401) : next(err)
+    }
+    return res.json(data)
   })
 }
 

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

@@ -3,13 +3,18 @@
  * @param {object} req
  * @param {object} res
  */
-function thumbnail (req, res) {
+function thumbnail (req, res, next) {
   const providerName = req.params.providerName
   const id = req.params.id
   const token = req.uppy.providerTokens[providerName]
   const provider = req.uppy.provider
 
-  provider.thumbnail({ id, token }, (response) => response ? response.pipe(res) : res.sendStatus(404))
+  provider.thumbnail({ id, token }, (err, response) => {
+    if (err) {
+      err.isAuthError ? res.sendStatus(401) : next(err)
+    }
+    response ? response.pipe(res) : res.sendStatus(404)
+  })
 }
 
 module.exports = thumbnail

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

@@ -45,5 +45,7 @@ exports.debug = (msg, tag) => {
 const log = (msg, tag, level) => {
   // @TODO add some colors based on log level
   const time = new Date().toISOString()
-  console.log(`uppy: ${time} [${level}] ${tag || ''} ${msg}`)
+  // exclude msg from template string so values such as error objects
+  // can be well formatted
+  console.log(`uppy: ${time} [${level}] ${tag || ''}`, msg)
 }

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

@@ -3,6 +3,7 @@ const request = require('request')
 const purest = require('purest')({ request })
 const logger = require('../../logger')
 const adapter = require('./adapter')
+const AuthError = require('../error')
 const DRIVE_FILE_FIELDS = 'kind,id,name,mimeType,ownedByMe,permissions(role,emailAddress),size,modifiedTime,iconLink,thumbnailLink,teamDriveId'
 const DRIVE_FILES_FIELDS = `kind,nextPageToken,incompleteSearch,files(${DRIVE_FILE_FIELDS})`
 const TEAM_DRIVE_FIELDS = 'teamDrives(kind,id,name,backgroundImageLink)'
@@ -30,10 +31,10 @@ class Drive {
     let listResponse
     let reqErr
     const finishReq = () => {
-      if (reqErr) {
-        done(reqErr)
-      } else if (listResponse.statusCode !== 200) {
-        done(new Error(`request to ${this.authProvider} returned ${listResponse.statusCode}`))
+      if (reqErr || listResponse.statusCode !== 200) {
+        const err = this._error(reqErr, listResponse)
+        logger.error(err, 'provider.drive.list.error')
+        done(err)
       } else {
         done(null, this.adaptData(listResponse.body, teamDrives ? teamDrives.body : null, options.uppy))
       }
@@ -110,21 +111,24 @@ class Drive {
 
   thumbnail ({id, token}, done) {
     return this.stats({id, token}, (err, resp, body) => {
-      if (err) {
+      if (err || resp.statusCode !== 200) {
+        err = this._error(err, resp)
         logger.error(err, 'provider.drive.thumbnail.error')
-        return done(null)
+        return done(err)
       }
-      done(body.thumbnailLink ? request(body.thumbnailLink) : null)
+
+      done(null, body.thumbnailLink ? request(body.thumbnailLink) : null)
     })
   }
 
   size ({id, token}, done) {
     return this.stats({ id, token }, (err, resp, body) => {
-      if (err) {
+      if (err || resp.statusCode !== 200) {
+        err = this._error(err, resp)
         logger.error(err, 'provider.drive.size.error')
-        return done(null)
+        return done(err)
       }
-      done(parseInt(body.size))
+      done(null, parseInt(body.size))
     })
   }
 
@@ -152,6 +156,14 @@ class Drive {
 
     return data
   }
+
+  _error (err, resp) {
+    if (resp) {
+      const errMsg = `request to ${this.authProvider} returned ${resp.statusCode}`
+      return resp.statusCode === 401 ? new AuthError() : new Error(errMsg)
+    }
+    return err
+  }
 }
 
 module.exports = Drive

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

@@ -2,6 +2,7 @@ const request = require('request')
 const purest = require('purest')({ request })
 const logger = require('../../logger')
 const adapter = require('./adapter')
+const AuthError = require('../error')
 
 class DropBox {
   constructor (options) {
@@ -35,10 +36,10 @@ class DropBox {
     let stats
     let reqErr
     const finishReq = () => {
-      if (reqErr) {
-        done(reqErr)
-      } else if (stats.statusCode !== 200) {
-        done(new Error(`request to ${this.authProvider} returned ${stats.statusCode}`))
+      if (reqErr || stats.statusCode !== 200) {
+        const err = this._error(reqErr, stats)
+        logger.error(err, 'provider.dropbox.list.error')
+        done(err)
       } else {
         stats.body.user_email = userInfo.body.email
         done(null, this.adaptData(stats.body, options.uppy))
@@ -106,7 +107,14 @@ class DropBox {
       })
       .auth(token)
       .request()
-      .on('response', done)
+      .on('response', (resp) => {
+        if (resp.statusCode !== 200) {
+          const err = this._error(null, resp)
+          logger.error(err, 'provider.dropbox.thumbnail.error')
+          return done(err)
+        }
+        done(null, resp)
+      })
       .on('error', (err) => {
         logger.error(err, 'provider.dropbox.thumbnail.error')
       })
@@ -122,12 +130,12 @@ class DropBox {
         include_media_info: true
       })
       .request((err, resp, body) => {
-        if (err) {
+        if (err || resp.statusCode !== 200) {
+          err = this._error(err, resp)
           logger.error(err, 'provider.dropbox.size.error')
-          return done(null)
+          return done(err)
         }
-
-        done(body.size)
+        done(null, parseInt(body.size))
       })
   }
 
@@ -151,6 +159,15 @@ class DropBox {
 
     return data
   }
+
+  _error (err, resp) {
+    if (resp) {
+      const errMsg = `request to ${this.authProvider} returned ${resp.statusCode}`
+      return resp.statusCode === 401 ? new AuthError() : new Error(errMsg)
+    }
+
+    return err
+  }
 }
 
 module.exports = DropBox

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

@@ -0,0 +1,13 @@
+/**
+ * AuthError is error returned when an adapter encounters
+ * an authorization error while communication with its corresponding provider
+ */
+class AuthError extends Error {
+  constructor () {
+    super('invalid access token detected by Provider')
+    this.name = 'AuthError'
+    this.isAuthError = true
+  }
+}
+
+module.exports = AuthError

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

@@ -3,6 +3,7 @@ const purest = require('purest')({ request })
 const utils = require('../../helpers/utils')
 const logger = require('../../logger')
 const adapter = require('./adapter')
+const AuthError = require('../error')
 
 class Instagram {
   constructor (options) {
@@ -21,10 +22,10 @@ class Instagram {
       .qs(qs)
       .auth(token)
       .request((err, resp, body) => {
-        if (err) {
-          done(err)
-        } else if (resp.statusCode !== 200) {
-          done(new Error(`request to ${this.authProvider} returned ${resp.statusCode}`))
+        if (err || resp.statusCode !== 200) {
+          err = this._error(err, resp)
+          logger.error(err, 'provider.instagram.list.error')
+          return done(err)
         } else {
           done(null, this.adaptData(body))
         }
@@ -67,10 +68,21 @@ class Instagram {
       .get(`media/${id}`)
       .auth(token)
       .request((err, resp, body) => {
-        if (err) return logger.error(err, 'provider.instagram.thumbnail.error')
+        if (err) {
+          err = this._error(err, resp)
+          logger.error(err, 'provider.instagram.thumbnail.error')
+          return done(err)
+        }
 
         request(body.data.images.thumbnail.url)
-          .on('response', done)
+          .on('response', (resp) => {
+            if (resp.statusCode !== 200) {
+              err = this._error(null, resp)
+              logger.error(err, 'provider.instagram.thumbnail.error')
+              return done(err)
+            }
+            done(null, resp)
+          })
           .on('error', (err) => {
             logger.error(err, 'provider.instagram.thumbnail.error')
           })
@@ -82,13 +94,14 @@ class Instagram {
       .get(`media/${id}`)
       .auth(token)
       .request((err, resp, body) => {
-        if (err) {
+        if (err || resp.statusCode !== 200) {
+          err = this._error(err, resp)
           logger.error(err, 'provider.instagram.size.error')
-          return done()
+          return done(err)
         }
 
         utils.getURLMeta(this._getMediaUrl(body, query.carousel_id))
-          .then(({ size }) => done(size))
+          .then(({ size }) => done(null, size))
           .catch((err) => {
             logger.error(err, 'provider.instagram.size.error')
             done()
@@ -115,6 +128,18 @@ class Instagram {
     data.nextPagePath = adapter.getNextPagePath(items)
     return data
   }
+
+  _error (err, resp) {
+    if (resp) {
+      if (resp.statusCode === 400 && resp.body && resp.body.meta.error_type === 'OAuthAccessTokenException') {
+        return new AuthError()
+      }
+
+      return new Error(`request to ${this.authProvider} returned ${resp.statusCode}`)
+    }
+
+    return err
+  }
 }
 
 module.exports = Instagram

packages/@uppy/core/src/index.js

@@ -36,6 +36,7 @@ class Uppy {
         exceedsSize: 'This file exceeds maximum allowed size of',
         youCanOnlyUploadFileTypes: 'You can only upload: %{types}',
         companionError: 'Connection with Companion failed',
+        companionAuthError: 'Authorization required',
         failedToUpload: 'Failed to upload %{file}',
         noInternetConnection: 'No Internet connection',
         connectedToInternet: 'Connected to the Internet',

packages/@uppy/dropbox/src/index.js

@@ -25,7 +25,7 @@ module.exports = class Dropbox extends Plugin {
       pluginId: this.id
     })
 
-    this.onAuth = this.onAuth.bind(this)
+    this.onFirstRender = this.onFirstRender.bind(this)
     this.render = this.render.bind(this)
   }
 
@@ -55,11 +55,8 @@ module.exports = class Dropbox extends Plugin {
     this.unmount()
   }
 
-  onAuth (authenticated) {
-    this.setPluginState({ authenticated })
-    if (authenticated) {
-      this.view.getFolder()
-    }
+  onFirstRender () {
+    return this.view.getFolder()
   }
 
   render (state) {

packages/@uppy/google-drive/src/index.js

@@ -29,7 +29,7 @@ module.exports = class GoogleDrive extends Plugin {
       pluginId: this.id
     })
 
-    this.onAuth = this.onAuth.bind(this)
+    this.onFirstRender = this.onFirstRender.bind(this)
     this.render = this.render.bind(this)
   }
 
@@ -62,11 +62,8 @@ module.exports = class GoogleDrive extends Plugin {
     this.unmount()
   }
 
-  onAuth (authenticated) {
-    this.setPluginState({ authenticated })
-    if (authenticated) {
-      this.view.getFolder('root', '/')
-    }
+  onFirstRender () {
+    return this.view.getFolder('root', '/')
   }
 
   render (state) {

packages/@uppy/instagram/src/index.js

@@ -26,7 +26,7 @@ module.exports = class Instagram extends Plugin {
       pluginId: this.id
     })
 
-    this.onAuth = this.onAuth.bind(this)
+    this.onFirstRender = this.onFirstRender.bind(this)
     this.render = this.render.bind(this)
   }
 
@@ -60,11 +60,8 @@ module.exports = class Instagram extends Plugin {
     this.unmount()
   }
 
-  onAuth (authenticated) {
-    this.setPluginState({ authenticated })
-    if (authenticated) {
-      this.view.getFolder('recent')
-    }
+  onFirstRender () {
+    this.view.getFolder('recent')
   }
 
   render (state) {

packages/@uppy/provider-views/src/AuthView.js

@@ -1,4 +1,3 @@
-const LoaderView = require('./Loader')
 const { h, Component } = require('preact')
 
 class AuthBlock extends Component {
@@ -29,14 +28,7 @@ class AuthBlock extends Component {
 }
 
 class AuthView extends Component {
-  componentDidMount () {
-    this.props.checkAuth()
-  }
-
   render () {
-    if (this.props.checkAuthInProgress) {
-      return <LoaderView />
-    }
     return <AuthBlock {...this.props} />
   }
 }

packages/@uppy/provider-views/src/index.js

@@ -56,7 +56,7 @@ module.exports = class ProviderView {
     this.getFolder = this.getFolder.bind(this)
     this.getNextFolder = this.getNextFolder.bind(this)
     this.logout = this.logout.bind(this)
-    this.checkAuth = this.checkAuth.bind(this)
+    this.preFirstRender = this.preFirstRender.bind(this)
     this.handleAuth = this.handleAuth.bind(this)
     this.handleDemoAuth = this.handleDemoAuth.bind(this)
     this.sortByTitle = this.sortByTitle.bind(this)
@@ -93,17 +93,13 @@ module.exports = class ProviderView {
     this.plugin.setPluginState({ folders, files })
   }
 
-  checkAuth () {
-    this.plugin.setPluginState({ checkAuthInProgress: true })
-    this.provider.checkAuth()
-      .then((authenticated) => {
-        this.plugin.setPluginState({ checkAuthInProgress: false })
-        this.plugin.onAuth(authenticated)
-      })
-      .catch((err) => {
-        this.plugin.setPluginState({ checkAuthInProgress: false })
-        this.handleError(err)
-      })
+  /**
+   * Called only the first time the provider view is rendered.
+   * Kind of like an init function.
+   */
+  preFirstRender () {
+    this.plugin.setPluginState({ didFirstRender: true })
+    this.plugin.onFirstRender()
   }
 
   /**
@@ -434,7 +430,7 @@ module.exports = class ProviderView {
       authWindow.close()
       window.removeEventListener('message', handleToken)
       this.provider.setAuthToken(e.data.token)
-      this._loaderWrapper(this.provider.checkAuth(), this.plugin.onAuth, this.handleError)
+      this.preFirstRender()
     }
     window.addEventListener('message', handleToken)
   }
@@ -456,8 +452,8 @@ module.exports = class ProviderView {
 
   handleError (error) {
     const uppy = this.plugin.uppy
-    const message = uppy.i18n('companionError')
     uppy.log(error.toString())
+    const message = uppy.i18n(error.isAuthError ? 'companionAuthError' : 'companionError')
     uppy.info({message: message, details: error.toString()}, 'error', 5000)
   }
 
@@ -517,9 +513,14 @@ module.exports = class ProviderView {
   }
 
   render (state) {
-    const { authenticated, checkAuthInProgress, loading } = this.plugin.getPluginState()
+    const { authenticated, didFirstRender } = this.plugin.getPluginState()
+    if (!didFirstRender) {
+      this.preFirstRender()
+    }
 
-    if (loading) {
+    // reload pluginState for "loading" attribute because it might
+    // have changed above.
+    if (this.plugin.getPluginState().loading) {
       return (
         <CloseWrapper onUnmount={this.clearSelection}>
           <LoaderView />
@@ -534,10 +535,8 @@ module.exports = class ProviderView {
             pluginName={this.plugin.title}
             pluginIcon={this.plugin.icon}
             demo={this.plugin.opts.demo}
-            checkAuth={this.checkAuth}
             handleAuth={this.handleAuth}
-            handleDemoAuth={this.handleDemoAuth}
-            checkAuthInProgress={checkAuthInProgress} />
+            handleDemoAuth={this.handleDemoAuth} />
         </CloseWrapper>
       )
     }