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

HTTPCreds: remove all cookies when logging out #5383

Merged
merged 1 commit into from
Jan 3, 2017
Merged

Conversation

ogoffart
Copy link
Contributor

Some custom server use persistent cookies with the auth token. So we should
clear all the cookies when disconnecting.
Account::clearCookieJar is only called from the HTTPCredentials. This funciton
is not used for shibboleth.
There is probably no reasons to keep the HTTP cookie anyway.

Issue #5370

Some custom server use persistent cookies with the auth token. So we should
clear all the cookies when disconnecting.
Account::clearCookieJar is only called from the HTTPCredentials. This funciton
is not used for shibboleth.
There is probably no reasons to keep the HTTP cookie anyway.

Issue #5370
@mention-bot
Copy link

@ogoffart, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jturcotte, @danimo and @ckamm to be potential reviewers.

@guruz guruz added this to the 2.3.0 milestone Dec 14, 2016
@guruz guruz added the type:bug label Dec 14, 2016
@SamuAlfageme
Copy link
Contributor

SamuAlfageme commented Dec 15, 2016

HTTP basic auth. working like a charm now. Have to try a couple more scenarios, but seems to be all right.

Once it's done, should we cherrypick this into https://github.com/owncloud/client/releases/tag/v2.2.4 ?.

cc/ @michaelstingl

@SamuAlfageme
Copy link
Contributor

Leaving here a corner case due to the same account being able to log in multiple times in the same client (#5305)

Actual behavior:

Both cookies are wiped as they refer to the same username@domain combination.

Expected behavior:

The cookies could be wiped based on which was logged out and which wasn't

Steps to reproduce:

  1. Log in to the same account with the same sync client
  2. Log out of one of the accounts - the other account is fully usable
  3. Close the client and reopen
  4. Both accounts ask for password.
  5. Enter one of the passwords, leave the other logged out.
  6. Repeat 3.
  7. Both accounts are logged in.

As said, this is a corner case, don't know how relevant.

@michaelstingl
Copy link
Contributor

michaelstingl commented Dec 19, 2016

@SamuAlfageme From my perspective, same account logged in multiple times, should be out of scope. (but there are other opinions)

@SamuAlfageme
Copy link
Contributor

@owncloud/desktop-developers this is ready to be merged & cherrypicked to v2.2.4 tag, anyone?

@guruz
Copy link
Contributor

guruz commented Jan 2, 2017

@SamuAlfageme 2.2.4 is already released and tags can't be changed. Was there a decision to do a 2.2.5 for this?

@michaelstingl
Copy link
Contributor

@guruz I need a patch that would work on the 2.2.4 tag. No 2.2.5 required.

@guruz
Copy link
Contributor

guruz commented Jan 2, 2017

@michaelstingl https://gist.github.com/guruz/f81cbc4cbcc499f9a87ee74f1fc9e86b (haven't tested it on 2.2.4 though but it should work)

@michaelstingl
Copy link
Contributor

@SamuAlfageme Could you test-drive https://gist.github.com/guruz/f81cbc4cbcc499f9a87ee74f1fc9e86b and give a 👍 if it could be applied to 2.2.x?

@guruz guruz merged commit d433f0e into master Jan 3, 2017
@guruz guruz deleted the clear_cookies branch January 3, 2017 10:39
@SamuAlfageme
Copy link
Contributor

After struggling a bit because of some unrelated issues to test the patch over 2.2.4, finally did in both Win7 and OS X, and it's working as expected 🎉

Couple of concerns though:

  1. If we don't make a specific tag containing the patch, building a client with it it's going to be hard from ownbrander.
  2. If someone uses a v2.2.4 client with the patch applied, the update notification is triggered; even if the patched version is built on top of 2.2.4, updater will try to install the stable:

updater_win

screenshot 2017-01-05 14 56 37

cc/ @michaelstingl

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

Successfully merging this pull request may close these issues.

5 participants