-
Notifications
You must be signed in to change notification settings - Fork 17
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
OAuth #26
OAuth #26
Conversation
#[async_trait::async_trait] | ||
pub trait OAuthTokenVerifier { | ||
async fn verify_token(token: &str) -> Option<&str>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should create a clear separation of concern here. I don't think parsing stuff belongs here, so instead I would replace SupportedTokenVerifiers with something like this:
pub enum OAuthToken {
GoogleToken(String),
TestToken(String),
}
#[derive(Debug, PartialEq, Eq)]
struct ParseOAuthError;
impl FromStr for OAuthToken {
type Err = ParseOAuthError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
match s.len() {
0 => Ok(OAuthToken::TestToken(s.into())),
_ => Ok(OAuthToken::GoogleToken(s.into())),
}
}
}
And then OAuthTokenVerifier only works on OAuthToken
and each impl can assert that this specific token corresponds to the expected type (e.g. Universal accepts all, Google only accepts GoogleToken etc). Meaning parsing has already happened by the point of this method's invocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found out that issuer is a required field in ID token. I will refactor it, but probably in some other, more fundamental way and once we have end-to-end Google + some other verifier. Thanks for suggestion, strong Token typing should be usefull.
#[async_trait::async_trait] | ||
pub trait OAuthTokenVerifier { | ||
async fn verify_token(token: &str) -> Option<&str>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal right now, but the right side of Result should ideally be a proper error type, something like
enum OAuthError {
Type1(...),
Type2(...)
}
mpc-recovery/src/oauth.rs
Outdated
} | ||
/// This function validates JWT (OIDC ID token) by checking the signature received | ||
/// from the issuer, issuer, audience, and expiration time. | ||
pub fn validate_jwt( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be inside GoogleTokenVerifier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not, because this function will be used in other verifiers (GitHub, Twitter, etc.). The use the same JWT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to the OAuthTokenVerifier trait.
Info about OIDC, token validation, and how we are planning to handle internal user id added to README.
Some of the TODOs moved to separate issues.
Not sure how to fix audit CI, chrono crate is up tp date. Can we patch
time
version? Is it a good practice?