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

Shibboleth accounts don't need the credentials to log in after wiping its keychain entry #5469

Closed
SamuAlfageme opened this issue Jan 17, 2017 · 10 comments
Assignees
Labels
ReadyToTest QA, please validate the fix/enhancement sev2-high type:bug
Milestone

Comments

@SamuAlfageme
Copy link
Contributor

SamuAlfageme commented Jan 17, 2017

Found while testing #5408

Steps to reproduce:

  1. Login with a valid shibb account and sync your files
  2. Remove keychain entry for the account
  3. Restart the client, it asks for shibb credentials
  4. Input a different but valid account
  5. An error is displayed (see the one for the http://testshib.org down). Close it.
  6. Click Log in in the account view
  7. Account is logged back without entering the credentials again

error_shib

I assume wiping the keychain entry is not enough to force log out and the client is using some session token to login back again, but then, why it asks for the credentials, if they're not needed?


Edit: Error in step 5 is indeed only displayed when both accounts were previously set and you input the credentials for the wrong one, otherwise you get the usual:

screenshot 2017-01-13 11 02 52

Made a screencast explaining this particular issue: https://webmshare.com/play/dEwbv


Also, I'm only seeing one entry in the keychain for all the shibboleth accounts. When a shibb account is removed, is this entry updated stripping the right _shibsession_?

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/41048267-shibboleth-accounts-don-t-need-the-credentials-to-log-in-after-wiping-its-keychain-entry?utm_campaign=plugin&utm_content=tracker%2F216457&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F216457&utm_medium=issues&utm_source=github).
@guruz
Copy link
Contributor

guruz commented Jan 17, 2017

Which version?

@SamuAlfageme
Copy link
Contributor Author

@guruz Version 2.3.0-nightly20170116 (build 3970)

@ckamm ckamm added this to the 2.3.0 milestone Jan 24, 2017
@ckamm ckamm self-assigned this Jan 24, 2017
@ckamm
Copy link
Contributor

ckamm commented Jan 24, 2017

Putting into 2.3 since it's a surprising auth-related issue.

@ckamm
Copy link
Contributor

ckamm commented Jan 24, 2017

Can reproduce:

  1. For the same shib server, configure two account.
  2. Log out of one, log in, enter credentials for the other, wrong one.
  3. Close the error that pops up.
  4. Click "Log in" again. Now the account is logged in with the wrong credentials, syncing to the wrong account.

This potentially messes up user data.

@ckamm
Copy link
Contributor

ckamm commented Jan 24, 2017

The problem is that both shib accounts use the exact same keychain entry for their cookies.

ckamm added a commit to ckamm/owncloud-client that referenced this issue Jan 24, 2017
Previously shib multiaccount didn't work at all because the
session cookie was stored in the same keychain entry.
@ckamm
Copy link
Contributor

ckamm commented Jan 24, 2017

Pull request: #5486

ckamm added a commit that referenced this issue Jan 24, 2017
Previously shib multiaccount didn't work at all because the
session cookie was stored in the same keychain entry.
@ckamm ckamm added the ReadyToTest QA, please validate the fix/enhancement label Jan 24, 2017
@SamuAlfageme
Copy link
Contributor Author

@ckamm yes, that was my final concern in the original issue:

Also, I'm only seeing one entry in the keychain for all the shibboleth accounts. When a shibb account is removed, is this entry updated stripping the right shibsession?

@SamuAlfageme
Copy link
Contributor Author

Looking good 😎, have to run some more tests but the whole behavior seems fixed.

guruz added a commit that referenced this issue Jan 25, 2017
guruz added a commit that referenced this issue Jan 26, 2017
@guruz
Copy link
Contributor

guruz commented Jan 26, 2017

The code looks good, however I believe it would still clash if you sync the SAME CREDENTIALS with TWO OWNCLOUD CLIENT ACCOUNTS right?

@guruz
Copy link
Contributor

guruz commented Feb 8, 2017

Still fine IMHO.

@guruz guruz closed this as completed Feb 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ReadyToTest QA, please validate the fix/enhancement sev2-high type:bug
Projects
None yet
Development

No branches or pull requests

3 participants