Closed Bug 737955 Opened 9 years ago Closed 9 years ago

Filelink: deleting a Filelink Dropbox account should delete the corresponding credentials

Categories

(Thunderbird :: Preferences, defect)

13 Branch
defect
Not set
major

Tracking

(thunderbird13 fixed)

RESOLVED FIXED
Thunderbird 14.0
Tracking Status
thunderbird13 --- fixed

People

(Reporter: jb, Assigned: mconley)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → mconley
Component: Account Manager → Preferences
QA Contact: account-manager → preferences
Blocks: BigFiles
Attached patch Patch v1Splinter Review
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?
Attachment #608087 - Flags: feedback?(squibblyflabbetydoo)
Attachment #608087 - Flags: feedback?(dbienvenu)
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.
Attachment #608087 - Flags: feedback?(dbienvenu) → feedback+
David:

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

-Mike
(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.
(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 on attachment 608087 [details] [diff] [review]
Patch v1

How do we feel about this?  Worth landing?
Attachment #608087 - Flags: feedback?(squibblyflabbetydoo) → review?(dbienvenu)
Attachment #608087 - Flags: review?(dbienvenu) → review+
Attachment #608087 - Flags: approval-comm-aurora?
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/46bed17a19a9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
Attachment #608087 - Flags: approval-comm-aurora? → approval-comm-aurora+
Depends on: 739986
You need to log in before you can comment on or make changes to this bug.