Skip to content

deserialization support for some missing algorithms #324

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

Merged
merged 6 commits into from
Oct 21, 2023

Conversation

nick9822
Copy link
Contributor

Fix towards #252
Added deserialization support for some missing algorithms in order to avoid panic while deserialization.

@nick9822
Copy link
Contributor Author

@Keats suspicious_doc_comments

error: this is an outer doc comment and does not apply to the parent module or crate
 --> src/jwk.rs:2:1

Let me know if you want me to remove ! from the comments.

@nick9822 nick9822 requested a review from Keats August 18, 2023 19:15
@@ -80,6 +83,11 @@ impl Algorithm {
Algorithm::EdDSA => AlgorithmFamily::Ed,
}
}

/// Converting Key Algorithm to Algorithm
pub fn from_key_algorithm(s: &KeyAlgorithm) -> Result<Self> {
Copy link
Owner

Choose a reason for hiding this comment

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

I think it makes more sense to do KeyAlgorithm.to_algorithm since this is not really meant to be public in practice and this way all the logic is contained in jwk.rs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I previously thought so but in the example auth0.rs it needed Algorithm from KeyAlgorithm at Validation::new. I think we don't need public method just for that, we can do it via Algorithm::from_str

@nick9822 nick9822 requested a review from Keats August 23, 2023 15:08
== KeyAlgorithm::from_str(s).unwrap() as isize
);
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

let's remove that test, I don't want tests to rely on enum ordering

@nick9822 nick9822 requested a review from Keats August 25, 2023 12:37
@byte-sized-emi
Copy link

Any update on this? It's been approved for some time now, but still hasn't gotten merged, would be nice to atleast know the reason why

@Keats Keats merged commit b32e0a3 into Keats:master Oct 21, 2023
@Keats
Copy link
Owner

Keats commented Oct 21, 2023

I forgot about it

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.

3 participants