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

Credentials: Namespace windows cred keys #6126

Merged
merged 1 commit into from
Nov 1, 2017

Conversation

ckamm
Copy link
Contributor

@ckamm ckamm commented Oct 26, 2017

The application name is prepended to the key. QtKeychain doesn't
do that automatically on the platform.

See #6125

@SamuAlfageme I'd like to ask you to redo the 2.3 -> 2.4 upgrade tests after this patch hits. It's expected that 2.4alpha credentials won't be found or deleted.

The application name is prepended to the key. QtKeychain doesn't
do that automatically on the platform.
@ckamm ckamm added this to the 2.4.0 milestone Oct 26, 2017
@ckamm ckamm self-assigned this Oct 26, 2017
@ckamm ckamm requested review from guruz and SamuAlfageme October 26, 2017 11:40
@guruz
Copy link
Contributor

guruz commented Oct 30, 2017

It looks good. We could merge it but let's think about this:

Should we have QtKeychain as git submodule #1416 and fix it directly in the QtKeychain fork and then attempt to upstream this to QtKeychain?

Hm.. @ikushn-s mentioned though that this sucks a bit for QtKeychain users updating to a newer QtKeychain with suddenly different keynames..
CC @frankosterfeld
Opinions?

@ckamm
Copy link
Contributor Author

ckamm commented Nov 1, 2017

Fixing it upstream would be just as well. I agree that there'd need to be a flag to toggle between old and new behavior to make migration possible. We'd still need the migration code - we'd just save on composing the key ourselves. The main advantage would be alerting other users of the library and, possibly, changing the default behavior.

@guruz I'd merge this as-is. If there's an update to QtKeychain we can update to using that with minor changes.

@ckamm ckamm merged commit 6ac44f0 into 2.4 Nov 1, 2017
@ckamm ckamm deleted the windows-credential-key-namespace-6125 branch November 1, 2017 16:03
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.

2 participants