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.

Sign up for GitHub

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

Jump to bottom

Remove Storage::set_password #733

Merged
merged 8 commits into from
Mar 22, 2022
Merged

Remove Storage::set_password #733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8b4a701
remove `set-password` from `Storage`
Mar 18, 2022
2c81a7a
cargo fmt
Mar 18, 2022
b22b6cb
remove `setPassword` from napi
Mar 18, 2022
3cf6087
restore `set_password` to Stronghold
Mar 18, 2022
135164e
restore `setPassword` in stronghold.ts
Mar 18, 2022
5865978
re-remove `set_password` from napi
Mar 18, 2022
c0d163e
remove set_password from stronghold.rs
Mar 21, 2022
7aa1a47
remove unused import
Mar 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions bindings/stronghold-nodejs/js/stronghold.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,6 @@ export class Stronghold implements Storage {
return stronghold
}

public async setPassword(encryptionKey: Uint8Array) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we still need this to set the stronghold password, although it is not part of the interface anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, thanks Eike.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not required because we set the password once during construction. This does not really suffice in the current stronghold, because the password expires after some time. This is also the case in Rust, though. Once we have migrated to the refactored stronghold, it will be fixed. I still think we can remove set_password in the NAPI bindings already.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, because we set it in the constructor initially, which is enough for the new stronghold version. How do we coordinate this?

Copy link
Contributor

@PhilippGackstatter PhilippGackstatter Mar 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coordinate the upgrade? Personally, I'd like to do it as soon as possible once the stronghold team gives the green light.

Edit: Should await the KeyLocation refactor, though.

Copy link
Collaborator

@eike-hass eike-hass Mar 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe worded differently: what is the effect if we merge it like this and do we need to wait for a new stronghold version for a potential (dev) release?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the effect if we merge it like this

Sorry for the delay, had to re-understand how password expiration works. Actually, the default is that the password does not expire at all. Merging it like this is therefore fine, as we set the password during construction, and then we don't need to set it again.

do we need for a new stronghold version for a potential (dev) release?

I don't think we need it. However, I can't guarantee that there is not going to be a breaking change during the migration to the new version. That said, from doing the KeyLocation refactor and from what I know about the new version, at least I cannot say that there will definitely be anything broken after the migration.

Copy link
Collaborator

@eike-hass eike-hass Mar 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for clarifying. I was worried it would expire. So we can totally merge and do a dev release.
Let's discuss potential future breaking changes before 0.5.0 proper release.

return this.napiStronghold.setPassword(Array.from(encryptionKey))
}

public async flushChanges() {
return this.napiStronghold.flushChanges()
}
Expand Down
10 changes: 0 additions & 10 deletions bindings/stronghold-nodejs/src/account/storage/stronghold.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,16 +54,6 @@ impl NapiStronghold {
self.0.set_dropsave(dropsave);
}

/// Sets the account password.
#[napi]
pub async fn set_password(&self, password: Vec<u32>) -> Result<()> {
let password: [u8; 32] = password
.try_into_bytes()?
.try_into()
.map_err(|_| Error::from_reason("Invalid password type. Expected [u8; 32]".to_owned()))?;
self.0.set_password(password).await.napi_result()
}

/// Write any unsaved changes to disk.
#[napi]
pub async fn flush_changes(&self) -> Result<()> {
Expand Down
2 changes: 0 additions & 2 deletions bindings/wasm/examples-account/src/memory_storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ class MemStore implements Storage {
this._vaults = new Map();
}

public async setPassword(_encryptionKey: Uint8Array): Promise<void> {}

public async flushChanges(): Promise<void> {}

public async keyNew(did: DID, keyLocation: KeyLocation): Promise<string> {
Expand Down
13 changes: 0 additions & 13 deletions bindings/wasm/src/account/storage/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use core::fmt::Debug;
use core::fmt::Formatter;

use identity::account_storage::ChainState;
use identity::account_storage::EncryptionKey;
use identity::account_storage::Error as AccountStorageError;
use identity::account_storage::IdentityState;
use identity::account_storage::KeyLocation;
Expand Down Expand Up @@ -47,8 +46,6 @@ extern "C" {
extern "C" {
pub type WasmStorage;

#[wasm_bindgen(method, js_name = setPassword)]
pub fn set_password(this: &WasmStorage, password: Vec<u8>) -> PromiseUnit;
#[wasm_bindgen(method, js_name = flushChanges)]
pub fn flush_changes(this: &WasmStorage) -> PromiseUnit;
#[wasm_bindgen(method, js_name = keyNew)]
Expand Down Expand Up @@ -88,13 +85,6 @@ impl Debug for WasmStorage {

#[async_trait::async_trait(?Send)]
impl Storage for WasmStorage {
/// Sets the account password.
async fn set_password(&self, password: EncryptionKey) -> AccountStorageResult<()> {
let promise: Promise = Promise::resolve(&self.set_password(password.to_vec()));
let result: JsValueResult = JsFuture::from(promise).await.into();
result.into()
}

/// Write any unsaved changes to disk.
async fn flush_changes(&self) -> AccountStorageResult<()> {
let promise: Promise = Promise::resolve(&self.flush_changes());
Expand Down Expand Up @@ -226,9 +216,6 @@ impl Storage for WasmStorage {
const STORAGE: &'static str = r#"
/** All methods an object must implement to be used as an account storage. */
interface Storage {
/** Sets the account password.*/
setPassword: (encryptionKey: Uint8Array) => Promise<void>;

/** Write any unsaved changes to disk.*/
flushChanges: () => Promise<void>;

Expand Down
5 changes: 0 additions & 5 deletions identity-account-storage/src/storage/memstore.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ use crate::identity::IdentityState;
use crate::storage::Storage;
use crate::types::KeyLocation;
use crate::types::Signature;
use crate::utils::EncryptionKey;
use crate::utils::Shared;

type MemVault = HashMap<KeyLocation, KeyPair>;
Expand Down Expand Up @@ -65,10 +64,6 @@ impl MemStore {
#[cfg_attr(not(feature = "send-sync-storage"), async_trait(?Send))]
#[cfg_attr(feature = "send-sync-storage", async_trait)]
impl Storage for MemStore {
async fn set_password(&self, _password: EncryptionKey) -> Result<()> {
Ok(())
}

async fn flush_changes(&self) -> Result<()> {
Ok(())
}
Expand Down
6 changes: 0 additions & 6 deletions identity-account-storage/src/storage/stronghold.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ use crate::stronghold::Vault;
use crate::types::KeyLocation;
use crate::types::Signature;
use crate::utils::derive_encryption_key;
use crate::utils::EncryptionKey;

#[derive(Debug)]
pub struct Stronghold {
Expand Down Expand Up @@ -80,11 +79,6 @@ impl Stronghold {
#[cfg_attr(not(feature = "send-sync-storage"), async_trait(?Send))]
#[cfg_attr(feature = "send-sync-storage", async_trait)]
impl Storage for Stronghold {
async fn set_password(&self, password: EncryptionKey) -> Result<()> {
self.snapshot.set_password(password).await?;
Ok(())
}

async fn flush_changes(&self) -> Result<()> {
self.snapshot.save().await?;
Ok(())
Expand Down
4 changes: 0 additions & 4 deletions identity-account-storage/src/storage/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ use crate::identity::ChainState;
use crate::identity::IdentityState;
use crate::types::KeyLocation;
use crate::types::Signature;
use crate::utils::EncryptionKey;

#[cfg(not(feature = "send-sync-storage"))]
mod storage_sub_trait {
Expand All @@ -34,9 +33,6 @@ mod storage_sub_trait {
#[cfg_attr(not(feature = "send-sync-storage"), async_trait(?Send))]
#[cfg_attr(feature = "send-sync-storage", async_trait)]
pub trait Storage: storage_sub_trait::StorageSendSyncMaybe + Debug {
/// Sets the account password.
async fn set_password(&self, password: EncryptionKey) -> Result<()>;

/// Write any unsaved changes to disk.
async fn flush_changes(&self) -> Result<()>;

Expand Down