Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logout command #23

Merged
merged 3 commits into from
Aug 1, 2016
Merged

Add logout command #23

merged 3 commits into from
Aug 1, 2016

Conversation

waits
Copy link
Contributor

@waits waits commented Jul 3, 2016

The logout command deletes the saved access tokens in auth.json, logging the user out of all accounts (personal and team).

Rationale: This is needed in order to revoke access to your account, which is important for users on a shared compared or with multiple accounts.

I feel like we should also make a call to /auth/token/revoke to permanently revoke the token that was in use, just in case (especially since they never expire). I couldn't find any way of doing that in the SDK, though, so we'd have to wait for that to be added or just make a raw HTTP request.

@lfaraone
Copy link

lfaraone commented Jul 4, 2016

Thanks for the PR @waits!

I'd prefer we actually invalidate the credentials rather than simply remove them locally. But I don't work on the API, so I can't commit to such a SDK call being added.

@waits
Copy link
Contributor Author

waits commented Jul 4, 2016

Should I just make an HTTP request to /auth/token/revoke then?

@diwakergupta
Copy link
Collaborator

Yeah revoke is probably what you want. Though that does complicate the behavior somewhat, because dbxcli actually uses three separate tokens depending on what API is being accessed. So revoke would have to revoke all of those tokens.

@waits
Copy link
Contributor Author

waits commented Jul 6, 2016

Added that in 62490f1. I'm guessing people would expect it to log out of all accounts (if applicable) at once, so it sends one request per saved token.

const revokeURL = "https://api.dropboxapi.com/2/auth/token/revoke"

// Sends a POST request to /auth/token/revoke to revoke the given API token.
func revoke(tok string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SDK already has a auth.TokenRevoke API that you can use. See initDbx in root.go on how to use options.Domain to get a Client object. Once you have that, you can simply call TokenRevoke on the object for each registered domain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diwakergupta Where is this TokenRevoke method defined? I can't find anything like that searching the source or godoc. The auth package is empty, and the Api interface that Client returns doesn't mention it either.

I used raw HTTP requests the first time around since I couldn't find a method for the revoke API, but I'll gladly use an official method if it exits. If it's supposed to exist but hasn't been written yet I can even make a PR for the SDK to add it.

@diwakergupta
Copy link
Collaborator

@waits ah seems like I jumped the gun. The API method is defined here: https://www.dropbox.com/developers/documentation/http/documentation#auth-token-revoke

But it doesn't exist in the public Go SDK yet. I have it in my private branch seems like. Let me try to push an update over the weekend! Sorry for the confusion.

@diwakergupta
Copy link
Collaborator

OK @waits, thanks for your patience. Took more work than I anticipated, but I finally have updated the SDK and updated the vendor-ed dependency as well:

https://github.com/dropbox/dropbox-sdk-go-unofficial/blob/master/auth/interface.go#L30

Can you rebase and re-work this PR? Thanks!

@diwakergupta
Copy link
Collaborator

@waits FYI I pushed some breaking changes to the SDK. The revoke call now lives at https://github.com/dropbox/dropbox-sdk-go-unofficial/blob/master/auth/client.go

Note that you'll have to vendor in the auth package -- I removed it because it wasn't being used anywhere (yet). Sorry for the rebase overhead!

The `logout` command deletes the saved access tokens, logging the user out
of all accounts (personal and team).

Closes dropbox#16.
@waits
Copy link
Contributor Author

waits commented Jul 31, 2016

@diwakergupta Thanks for adding that to the SDK. I switched logout to use your TokenRevoke method in a593310. Is that what you were thinking?

I squashed my original second commit (that manually sent a POST request to /auth/token/revoke) since it was undone by a593310.

@diwakergupta
Copy link
Collaborator

@waits I believe the build is failing because of a missing dependency: the auth sub-package needs to be vendored in first. Just install govendor (https://github.com/kardianos/govendor) and run govendor fetch github.com/dropbox/dropbox-sdk-go-unofficial/dropbox/auth and commit again.

@waits
Copy link
Contributor Author

waits commented Aug 1, 2016

Strange, same problem. I vendored auth. It's saying that dropbox.Config from the vendored folder can't be used as dropbox.Config from the master repo.

@diwakergupta
Copy link
Collaborator

@waits your commit doesn't actually add the auth package -- only updates vendor.json. Did you run govendor add or govendor fetch? (You need the latter). I'll take a stab at this unless you get to it first.

@diwakergupta diwakergupta merged commit ed1180f into dropbox:master Aug 1, 2016
@diwakergupta
Copy link
Collaborator

Alright, I fixed it up. Thanks @waits !

@waits
Copy link
Contributor Author

waits commented Aug 1, 2016

@diwakergupta Thanks! Yes I ran govendor fetch .... My commit didn't add the auth directory itself because it was already there in my tree, which is also why I didn't get the build error travis-ci got. I have no idea why is was already in my tree, or why git didn't pick it up. I never added it; I assumed you had committed it previously. Very strange.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants