RISCfuture / dropbox

An easy-to-use client library for the official Dropbox API.
MIT License
519 stars 47 forks source link

Bad error handling if user de-authorized app #15

Closed igorw closed 13 years ago

igorw commented 13 years ago

The dropbox gem only checks for an existing @access_token. If the app has a valid access token but the user de-authorized the app, the library will throw an UnsuccessfulResponseError. It would be nice if it could detect this already when authorizing and simply set @access_token to nil.

http://github.com/RISCfuture/dropbox/blob/master/lib/dropbox/api.rb#L477

RISCfuture commented 13 years ago

The problem is that invalid access tokens are definitely unexpected behavior. Theoretically a Dropbox access token should be good for a "long" period of time. Realistically, they go bad because of things like server hiccups or Memcached issues or whatever. When this happens, I think raising an exception is the appropriate thing to do, as it is an error state.

Authorization only happens once during the OAuth process. Once Dropbox::Session has the access token it does no more authorization. To check for a valid auth token would require the overhead of an additional API call, which is not something I'd like to do "automatically." My recommendation, if you want to write robust code that can handle invalid auth tokens, is to make a quick call to API#list (or some equally harmless method) when your app starts (or otherwise periodically) and just check for a 401.

I'm still ears though if you feel strongly about this and would like to make a further case.

igorw commented 13 years ago

I see your point. However it's still a very common use case. By treating the state as unauthorized instead of an error, you can prevent every application from having to handle this case individually. Which is quite a gain, in my opinion.

Possible compromises would be a dedicated method to check for this case, or an option to disable the check (and thus the overhead).

For now, I'm using this:

# user de-authorized account - request re-authorization
begin
  @dropbox.account
rescue Dropbox::UnsuccessfulResponseError
  redirect_to oauth_authorize_path
  return false
end
RISCfuture commented 13 years ago

Closing old tickets.