Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

SecretStore: return error 404 when there's no key shares for given key on all nodes #7331

Merged
merged 1 commit into from
Dec 29, 2017

Conversation

svyatonik
Copy link
Collaborator

The same behavior was there before KeyVersionNegotiationSession was introduced: return HTTP 404 error when trying to restore the key before it was generated.
By @grbIzl requrest

@svyatonik svyatonik added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Dec 19, 2017
@@ -227,7 +227,7 @@ impl<T> SessionImpl<T> where T: SessionTransport {
// try to complete session
Self::try_complete(&self.core, &mut *data);
if no_confirmations_required && data.state != SessionState::Finished {
return Err(Error::ConsensusUnreachable);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason for this change? Can you elaborate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean this particular line? It's initialize method which is called on the 'session master' node. no_confirmations_required means that there are no other nodes in the SecretStore (i.e. SS consists of the single KeyServer && we are on this KeyServer). Since we haven't found key share for given key on this key server (state != SessionState::Finished) => this means that key haven't been generated at all => we need to return MissingKeyShare.

MissingKeyShare in turn is converted to the 404 HTTP error later

@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 21, 2017
@svyatonik svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Dec 21, 2017
@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 21, 2017
@debris debris merged commit 81d4187 into master Dec 29, 2017
@debris debris deleted the secretstore_resurrect_error404 branch December 29, 2017 09:33
@5chdn 5chdn added this to the 1.9 milestone Jan 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants