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

fix: call cryptoService.makeKey directly form computeMasterKey #162

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

Ldoppea
Copy link
Member

@Ldoppea Ldoppea commented Sep 8, 2021

Previous version introduced unnecessary complexity as kdfIterations and kdf were ignored by authService.makePreloginKey()

Also the iterations handling did not work correctly as the ternary operator was reversed

As the only value of authService.makePreloginKey() is to query kdfIterations and kdf and because we never call computeMasterKey() with those values unset, then we call cryptoService.makeKey() directly

Previous version introduced unnecessary complexity as `kdfIterations`
and `kdf` were ignored by `authService.makePreloginKey()`

Also the `iterations` handling did not work correctly as the ternary
operator was reversed

As the only value of `authService.makePreloginKey()` is to query
`kdfIterations` and `kdf` and because we never call
`computeMasterKey()` with those values unset, then we call
`cryptoService.makeKey()` directly
@Ldoppea
Copy link
Member Author

Ldoppea commented Sep 8, 2021

Note that this method is used only from cozy-pass-web and archived cozy-password, so possible side effects are very limited

Therefore I'm tempted to change the method signature as kdf and kdfIterations are not in the same order between
computeMasterKey and cryptoService.makeKey. This would break the API signature, but I don't know if this library is used outside of Cozy.
What do you think about this?

@Crash--
Copy link
Contributor

Crash-- commented Sep 9, 2021

You can create a BC with a message describing how to handle the breaking change. This BC is minor ;)

@Ldoppea
Copy link
Member Author

Ldoppea commented Sep 9, 2021

You can create a BC with a message describing how to handle the breaking change. This BC is minor ;)

Do you mean in a new changelog.md file? or in a comment on the code?

@Crash--
Copy link
Contributor

Crash-- commented Sep 9, 2021

Do you mean in a new changelog.md file? or in a comment on the code?

We're in a cozy-libs, we don't handle changelog.md, github release based on our commit's messages does that for us. https://github.com/cozy/cozy-guidelines#breaking-change is enough ;)

@Ldoppea
Copy link
Member Author

Ldoppea commented Sep 9, 2021

Ok thanks, after some investigation there are more things to do in order to clean the library, I created some issues to do this later : #163 and #164

@Ldoppea Ldoppea merged commit 2dcdf5c into master Sep 9, 2021
@delete-merged-branch delete-merged-branch bot deleted the fix/computeMasterKey branch September 9, 2021 12:49
@cozy-bot
Copy link

cozy-bot commented Sep 9, 2021

🎉 This PR is included in version 3.9.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Ldoppea added a commit to cozy/cozy-pass-web that referenced this pull request Sep 9, 2021
Some old Cozy may not have any `io.cozy.settings.bitwarden` document

This document is generated when the user sets a password, which may not
be the case yet for OIDC users

In this case fetching `io.cozy.settings.bitwarden` would return an
error and it would not be possible to access the application as it is
called as an `isInstalled` guard

This fix allow to access the installation page if no
`io.cozy.settings.bitwarden` exists. Then we excpect the installation
page to force the user to set a password which would generated the
`io.cozy.settings.bitwarden` document

`cozy-keys-lib` has been updated to `3.9.1` to retrieve a fix on
`computeMasterKey` mechanism from cozy/cozy-keys-lib#162
Ldoppea added a commit to cozy/cozy-pass-web that referenced this pull request Sep 9, 2021
Some old Cozy may not have any `io.cozy.settings.bitwarden` document

This document is generated when the user sets a password, which may not
be the case yet for OIDC users

In this case fetching `io.cozy.settings.bitwarden` would return an
error and it would not be possible to access the application as it is
called as an `isInstalled` guard

This fix allow to access the installation page if no
`io.cozy.settings.bitwarden` exists. Then we excpect the installation
page to force the user to set a password which would generated the
`io.cozy.settings.bitwarden` document

`cozy-keys-lib` has been updated to `3.9.1` to retrieve a fix on
`computeMasterKey` mechanism from cozy/cozy-keys-lib#162
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.

3 participants