cfsamson / azure-jwt

A library for validating OAuth tokens from Azure AD
MIT License
10 stars 8 forks source link

Remove unnecessary openssl dependency and update jsonwebtoken to allow removing defaults #3

Closed techtunde closed 1 year ago

techtunde commented 1 year ago
cfsamson commented 1 year ago

Thank you for the PR and sorry for the late reply. It will still take som time before I'll have a chance to go through this but I thought I'd let you know and that I intend to merge this as soon as I have the time.

techtunde commented 1 year ago

No problem that sounds great!

jetuk commented 1 year ago

Do these changes pass the tests for you?

cfsamson commented 1 year ago

Do these changes pass the tests for you?

Hi, and sorry for the delay. All the tests fails, but it seems to be due to be something that gives an error on the new version in reqwest:

 Finished test [unoptimized + debuginfo] target(s) in 0.16s
warning: the following packages contain code that will be rejected by a future version of Rust: criterion v0.3.4, ntapi v0.3.6
note: to see what the problems were, use the option `--future-incompat-report`, or run `cargo report future-incompatibilities --id 6`
     Running unittests src\lib.rs (target\debug\deps\azure_jwt-5dd74bc2394a5f16.exe)

running 5 tests
test tests::decode_token ... FAILED
test tests::azure_ad_get_public_keys ... FAILED
test tests::refresh_rwks_uri ... FAILED
test tests::is_not_valid_more_than_24h ... FAILED
test tests::azure_ad_get_refresh_rwks_uri ... FAILED

failures:

---- tests::decode_token stdout ----
thread 'tests::decode_token' panicked at 'assertion failed: verified', src\lib.rs:676:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- tests::azure_ad_get_public_keys stdout ----
thread 'tests::azure_ad_get_public_keys' panicked at 'called `Result::unwrap()` on an `Err` value: ConnectionError(reqwest::Error { kind: Request, url: Url { scheme: "https", username: "", password: None, host: Some(Domain("login.microsoftonline.com")), port: None, path: "/common/.well-known/openid-configuration", query: None, fragment: None }, source: hyper::Error(Connect, "invalid URL, scheme is not http") })', src\lib.rs:712:56

---- tests::refresh_rwks_uri stdout ----
thread 'tests::refresh_rwks_uri' panicked at 'called `Result::unwrap()` on an `Err` value: ConnectionError(reqwest::Error { kind: Request, url: Url { scheme: "https", username: "", password: None, host: Some(Domain("login.microsoftonline.com")), port: None, path: "/common/.well-known/openid-configuration", query: None, fragment: None }, source: hyper::Error(Connect, "invalid URL, scheme is not http") })', src\lib.rs:707:53

---- tests::is_not_valid_more_than_24h stdout ----
thread 'tests::is_not_valid_more_than_24h' panicked at 'called `Result::unwrap()` on an `Err` value: ConnectionError(reqwest::Error { kind: Request, url: Url { scheme: "https", username: "", password: None, host: Some(Domain("login.microsoftonline.com")), port: None, path: "/common/.well-known/openid-configuration", query: None, fragment: None }, source: hyper::Error(Connect, "invalid URL, scheme is not http") })', src\lib.rs:724:56

---- tests::azure_ad_get_refresh_rwks_uri stdout ----
thread 'tests::azure_ad_get_refresh_rwks_uri' panicked at 'called `Result::unwrap()` on an `Err` value: ConnectionError(reqwest::Error { kind: Request, url: Url { scheme: "https", username: "", password: None, host: Some(Domain("login.microsoftonline.com")), port: None, path: "/common/.well-known/openid-configuration", query: None, fragment: None }, source: hyper::Error(Connect, "invalid URL, scheme is not http") })', src\lib.rs:718:56

failures:
    tests::azure_ad_get_public_keys
    tests::azure_ad_get_refresh_rwks_uri
    tests::decode_token
    tests::is_not_valid_more_than_24h
    tests::refresh_rwks_uri

test result: FAILED. 0 passed; 5 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
cfsamson commented 1 year ago

So this seems to be what's causing the failures:

 fn get_jwks_uri() -> Result<String, AuthErr> {
        let resp: Response = reqwest::blocking::get(AZ_OPENID_URL)?;
        let resp: OpenIdResponse = resp.json()?;

        Ok(resp.jwks_uri)
    }

I'm on Windows 11. Have you tried to run cargo test and gotten it to successfully complete? If so, what platform are you running the tests on?

It seems that the newer version of reqwest thinks the url is invalid. I'm not sure exactly what changed in reqwest in this regard between versions but if you get the tests to pass I'll be happy to merge this.

techtunde commented 1 year ago

Good catch sorry about that, I just needed to manually add rustls as a feature since I removed defaults purposefully to remove its use of native-tls. I'l push an update in a minute.

techtunde commented 1 year ago

@cfsamson @jetuk Unit tests fixed. Just needed to revert to the older jsonwebtoken and set rustls as the tls scheme to avoid the openssl dependency.

cfsamson commented 1 year ago

Thanks. Looks good now!

cfsamson commented 1 year ago

Ok, so I have published version 0.2.2 to crates.io with this patch merged. Thanks again for the effort!