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

ownCloud 2.5: Config stored in %APPDATA%, but password is still stored in %LOCALAPPDATA% #6729

Closed
klada opened this issue Aug 17, 2018 · 17 comments
Assignees
Labels
p2-high Escalation, on top of current planning, release blocker ReadyToTest QA, please validate the fix/enhancement type:bug Upstream issue
Milestone

Comments

@klada
Copy link

klada commented Aug 17, 2018

In ownCloud 2.5 the location of the config folder in Windows has changed from %LOCALAPPDATA% to %APPDATA%, which is a good for roaming profiles (see #684) 👍 .

This leads to a new issue though, because the password which is stored through QtKeychain is still stored in the local Windows credential store, which resides in %LOCALAPPDATA%.

Expected behaviour

The password should be stored in the Windows Credential Manager with "enterprise persistence" instead of "local machine persistence". Otherwise the user has to enter the sync password on every machine on the network or - depending on the roaming profile policy - on each logon.

Actual behaviour

The user has to enter the sync password on every logon.

Steps to reproduce

  1. Configure ownCloud client (2.5 or newer) on Windows
  2. Enter password
  3. Log off from machine
  4. Log in on different machine
  5. Password needs to be re-entered

This may also happen on the same machine, as "machine local" profiles are often not persisted after logging out.

Client configuration

Client version: 2.5.0 beta1

Operating system: Windows 7, Windows 10

@ogoffart ogoffart self-assigned this Aug 17, 2018
@klada
Copy link
Author

klada commented Aug 17, 2018

This probably can be fixed by building QtKeychain with -DUSE_CREDENTIAL_STORE=OFF. The password will still be encrypted with the user's logon credentials, but it will be stored through QSettings in a config file in the profile (hopefully %APPDATA%). I haven't tested this though.

@ogoffart
Copy link
Contributor

Qt Keychain issue: frankosterfeld/qtkeychain#121

@guruz guruz added this to the 2.5.0 milestone Aug 17, 2018
@guruz
Copy link
Contributor

guruz commented Aug 17, 2018

@jnweiger @michaelstingl If we want this in 2.5.0 it means we need a newer QtKeychain for Windows.

@michaelstingl
Copy link
Contributor

How can we get "a newer QtKeychain for Windows"?

@guruz
Copy link
Contributor

guruz commented Aug 17, 2018

We ask @frankosterfeld to make a new release/tag in his git repo (Sorry Frank..). (We can avoid this by just taking the sources directly from a fork).
Then we ask @jnweiger and @dschmidt to get it into mingw and MSVC builds.

@michaelstingl
Copy link
Contributor

If it's not that much effort, @dschmidt could give it a try next week…

@ogoffart ogoffart modified the milestones: 2.5.0, 2.5.1 Aug 18, 2018
@jnweiger
Copy link
Contributor

The mingw toolchain should be in maintenance mode now. 2.5.0 development should be MSVC toolchain only.

frankosterfeld pushed a commit to frankosterfeld/qtkeychain that referenced this issue Aug 20, 2018
Otherwise the credentials are not preserved accross conneciton in a roaming setting

Reported in owncloud/client#6729
@frankosterfeld
Copy link

@guruz 0.9.1 has been released

@guruz
Copy link
Contributor

guruz commented Aug 20, 2018

FYI, according to @jakobvogel in atom/node-keytar#123 (Which is like QtKeyChain for nodejs) the CRED_PERSIST_ENTERPRISE will still read the old CRED_PERSIST_LOCAL_MACHINE too if it already existed 👍

@guruz guruz added type:bug p2-high Escalation, on top of current planning, release blocker labels Aug 20, 2018
@jnweiger
Copy link
Contributor

Updated https://build.opensuse.org/package/show/isv:ownCloud:Qt5101/ocqt5101-qt5keychain
to 0.9.1 -- although that update is probably irrelevant for Linux.

@jakobvogel
Copy link

@guruz ... a little caveat, though: While the old CRED_PERSIST_LOCAL_MACHINE entries are in fact still available, it seems not to be possible to upgrade them to CRED_PERSIST_ENTERPRISE. Even when changing the contents, the persistence level will remain the same. (Credentials reside as hidden files in %LOCALAPPDATA%\Microsoft\Credentials and %APPDATA%\Microsoft\Credentials, respectively. The system seems reluctant to move these files around.)

In order to upgrade legacy credentials to roam, it is probably necessary to delete the CRED_PERSIST_LOCAL_MACHINE key and to write a new one with CRED_PERSIST_ENTERPRISE. At least, that is what I will do in our own software to transparently enable roaming for our legacy users.

By the way, owncloud is a great piece of software. Love to use it myself. Thanks for your work!

@guruz guruz mentioned this issue Aug 20, 2018
@dschmidt dschmidt added the ReadyToTest QA, please validate the fix/enhancement label Aug 29, 2018
@dschmidt
Copy link
Member

Latest MSI builds contain the new QtKeychain - please test

@klada
Copy link
Author

klada commented Sep 3, 2018

Latest MSI builds contain the new QtKeychain - please test

Which builds do you mean? I have tested the following builds:

  • ownCloud-2.5.0.10539.-daily20180824.10521.msi
  • ownCloud-2.6.0.10541.-daily20180824.10523.msi
  • ownCloud-2.5.0.10306-daily20180902-setup.exe
  • ownCloud-2.5.0.10516.-beta2.10506.msi

All of them still contain the old QtKeychain, where this bug is still present. Where can we get the latest MSI builds you are referring to?

@dschmidt
Copy link
Member

dschmidt commented Sep 4, 2018

@guruz
Copy link
Contributor

guruz commented Sep 5, 2018

@klada Did you manage to test one of the builds that @dschmidt linked?

@klada
Copy link
Author

klada commented Sep 5, 2018

Yes, I just tested ownCloud-2.5.0.10597.-daily20180904.10541.msi and it is working properly - the password is now stored with "enterprise" scope, which means it'll be persisted in %APPDATA% . Very cool 👍

@guruz
Copy link
Contributor

guruz commented Sep 5, 2018

Thank you everyone...

@guruz guruz closed this as completed Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-high Escalation, on top of current planning, release blocker ReadyToTest QA, please validate the fix/enhancement type:bug Upstream issue
Projects
None yet
Development

No branches or pull requests

8 participants