Last Comment Bug 737955 - Filelink: deleting a Filelink Dropbox account should delete the corresponding credentials
: Filelink: deleting a Filelink Dropbox account should delete the corresponding...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Preferences (show other bugs)
: 13 Branch
: All All
: -- major (vote)
: Thunderbird 14.0
Assigned To: Mike Conley (:mconley) - (Needinfo me!)
:
Mentors:
Depends on: 739986
Blocks: BigFiles
  Show dependency treegraph
 
Reported: 2012-03-21 11:30 PDT by Jb Piacentino
Modified: 2012-03-28 07:21 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Patch v1 (10.35 KB, patch)
2012-03-21 14:11 PDT, Mike Conley (:mconley) - (Needinfo me!)
mozilla: review+
mozilla: feedback+
mozilla: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Jb Piacentino 2012-03-21 11:30:06 PDT
STR 

(with Dropbox)

1 - Set up a Filelink Dropbox account with proper credentials
2 - Remove it (no other LFP registered)
3 - Add a Filelink to an email
4 - Choose Dropbox
5 - File gets uploaded

Expected behaviour
- Credentials should have been deleted in step 2 and Dropbox login/signup page should be launched in step 5

Dropbox username & password remain in the saved password

This problem does not appear with YouSendIt.
Comment 1 Mike Conley (:mconley) - (Needinfo me!) 2012-03-21 14:11:38 PDT
Created attachment 608087 [details] [diff] [review]
Patch v1

So here's what I've done:

Initially I talked to bienvenu about adding a "destroy" method to the provider interface, letting them manage things like cleanup...

Instead, I found that we could simply clear out any secret tokens stored in nsILoginManager from within cloudFileAccounts.jsm.  This seemed a bit simpler - does it seem like the right choice?

So, that clears out the auth tokens when removing an account.  It *still* doesn't fix our Dropbox OAuth cookie persistence problem....

What I did for that, was just fire out an XHRequest to https://www.dropbox.com/logout once the OAuth procedure has successfully completed.  This, essentially, ends the session opened in order to get the OAuth tokens.

I've also added a test for our Dropbox backend to make sure the logout request is sent.

Thoughts?  Is this the right way to go about things?
Comment 2 David :Bienvenu 2012-03-21 14:18:23 PDT
Comment on attachment 608087 [details] [diff] [review]
Patch v1

I definitely prefer this, in that I think this belongs more to the account manager, and the providers don't have to duplicate this. But we should document this a bit, so that providers know to use the login manager, and that if they do, the login info will get cleaned up automatically when accounts are removed. We should probably have a page on devmo about implementing a cloud file provider. I can start one.
Comment 3 Mike Conley (:mconley) - (Needinfo me!) 2012-03-22 07:20:12 PDT
David:

Is the XHRequest to https://www.dropbox.com/logout a reasonable solution?

-Mike
Comment 4 David :Bienvenu 2012-03-22 07:38:42 PDT
(In reply to Mike Conley (:mconley) from comment #3)
> David:
> 
> Is the XHRequest to https://www.dropbox.com/logout a reasonable solution?
> 
> -Mike

I don't know - they don't define that in their REST api, so I really don't know if it will work or if there are downsides.
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2012-03-22 07:55:05 PDT
(In reply to David :Bienvenu from comment #4)
> (In reply to Mike Conley (:mconley) from comment #3)
> > David:
> > 
> > Is the XHRequest to https://www.dropbox.com/logout a reasonable solution?
> > 
> > -Mike
> 
> I don't know - they don't define that in their REST api, so I really don't
> know if it will work or if there are downsides.

From local testing, it does "work", in that the created session for getting the OAuth tokens is destroyed.

Unsure if it's the best way though.  Open to suggestions.
Comment 6 Mike Conley (:mconley) - (Needinfo me!) 2012-03-26 08:11:46 PDT
Comment on attachment 608087 [details] [diff] [review]
Patch v1

How do we feel about this?  Worth landing?
Comment 7 Mike Conley (:mconley) - (Needinfo me!) 2012-03-26 18:39:01 PDT
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/46bed17a19a9
Comment 8 Mike Conley (:mconley) - (Needinfo me!) 2012-03-27 06:55:11 PDT
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/57bfc276a84b

Note You need to log in before you can comment on or make changes to this bug.