Change cred.Persist to CRED_PERSIST_ENTERPRISE

[bkostjens]

Hello,

This has been requested before, but the pull request was never merged.

See: #123

node-keytar/src/keytar_win.cc

Line 136 in 99d3058

cred.Persist = CRED_PERSIST_LOCAL_MACHINE;

Would it be possible to change this (or make it configurable) to CRED_PERSIST_ENTERPRISE, so passwords don't get lost when logging out on a windows computer that is joined in a domain.

[shiftkey]

I'm tentatively 👍 on this idea, but we'd also need to be mindful of existing users of this library - what does the migration story look like for those users of the library if we change this?

[jakobvogel]

I'm tentatively 👍 on this idea, but we'd also need to be mindful of existing users of this library - what does the migration story look like for those users of the library if we change this?

The migration should be transparent in most cases. Windows has a pool of credentials, where some such key/value pairs have "local machine" persistence, while others have "enterprise" persistence. The difference is just whether the respective credentials are roamed in an enterprise network or not. For computers outside of such a network (i.e. most home users), there is no difference as there is no network to roam across. Both datasets stay local in that case.

The "worst" case is, therefore, that credentials stored on an enterprise machine are distributed to other machines of the same network. However, this is precisely what our customers expect: They want to find the credentials independently of which machine of an enterprise network they use. Their entire user profiles, apps etc. are roamed, and they expect stored passwords to follow along. Electron apps are also roamed by default if I recall correctly.

When querying credentials, there should not be a difference with respect to the persistence settings.

[bkostjens]

Any update on this issue? If migration is still a worry, maybe make it a configurable option and leave it set to CRED_PERSIST_LOCAL_MACHINE by default?

[shiftkey]

Any update on this issue?

Not really. Given the details above I think a PR to make the change and some testing to confirm the change won't affect existing apps is all that's needed to move this along.

[jakobvogel]

Any update on this issue?

Not really. Given the details above I think a PR to make the change and some testing to confirm the change won't affect existing apps is all that's needed to move this along.

PR done, see #191.