Fix ClientRegistrationResponse failing to deserialize optional fiel…

[sunli829]

This PR fixes the following two issues:

Fix ClientRegistrationResponse failing to deserialize optional fields client_id_issued_at and client_secret_expires_at.

By default, serde accepts a missing field if the type of the field is Option<_>. If you add a with or deserialize_with attribute serde will fail with the "missing field" error message, as you noticed. This is on purpose. You can change what happens on missing fields with the default attribute, either applied to the field in question or the whole struct.

Reference: serde-rs/serde#1951

Made XXXUrl's equal behavior consistent with url::Url.

The following two Urls are now equal.

assert_eq!(
    IssueUrl::new("http://example.com").unwrap(),
    IssueUrl::new("http://example.com/").unwrap(),
);

openidconnect-rs/src/discovery.rs

Line 404 in d5cf0ae

if provider_metadata.issuer() != issuer_url {

[codecov]

Codecov Report

Merging #66 (efd006c) into main (d5cf0ae) will decrease coverage by 0.51%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #66      +/-   ##
==========================================
- Coverage   76.93%   76.42%   -0.52%     
==========================================
  Files          16       16              
  Lines        3482     3482              
==========================================
- Hits         2679     2661      -18     
- Misses        803      821      +18     

Impacted Files	Coverage Δ
src/registration.rs	66.88% <ø> (ø)
src/discovery.rs	88.76% <0.00%> (-2.06%)	⬇️
src/core/crypto.rs	81.53% <0.00%> (-1.54%)	⬇️
src/types.rs	72.24% <0.00%> (-1.15%)	⬇️
src/jwt.rs	80.86% <0.00%> (-0.48%)	⬇️
src/lib.rs	71.60% <0.00%> (-0.41%)	⬇️
src/verification.rs	84.87% <0.00%> (-0.17%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5cf0ae...efd006c. Read the comment docs.

[ramosbugs]

Thanks for the PR!

The URL exact string comparison is intended behavior, as documented here:

openidconnect-rs/src/macros.rs

Lines 258 to 261 in d5cf0ae

	/// syntactically valid URL. However, comparisons and hashes of these types are based on the string
	/// representation given during construction, disregarding any canonicalization performed by the
	/// underlying `Url` struct. OpenID Connect requires certain URLs (e.g., ID token issuers) to be
	/// compared exactly, without canonicalization.

From the spec:

2. The Issuer Identifier for the OpenID Provider (which is typically obtained during Discovery) MUST exactly match the value of the iss (issuer) Claim.

I don't see any mention of URL canonicalization in the spec, and various forms of canonicalization have caused security vulnerabilities in other authentication protocols like SAML. Consequently, I think it makes sense to stick to exact URL string comparisons throughout this crate. Client code is free to do a looser comparison if desired by first calling the .url() method and then comparing the returned &Url.

I'm happy to merge the deserialization fix though. Nice catch 👍

[sunli829]

Thanks for you quick response! 😁

Using https://broker.pod.inrupt.com to call the CoreProviderMetadata::discover_async function will trigger an error, the error output is:

unexpected issuer URI `https://broker.pod.inrupt.com/` (expected `https://broker.pod.inrupt.com/`)

Need to use https://broker.pod.inrupt.com/ to make sure the url is the same as defined by openid-configuration, which is inconvenient, don't you think? 😁

[ramosbugs]

Using https://broker.pod.inrupt.com to call the CoreProviderMetadata::discover_async function will trigger an error, the error output is:

unexpected issuer URI `https://broker.pod.inrupt.com/` (expected `https://broker.pod.inrupt.com/`)

Ah, I see! I think we can improve the error output here to print the .as_str() output instead, so that at least the error message matches the semantics of the comparison:

openidconnect-rs/src/discovery.rs

Lines 407 to 408 in cfa5af5

	provider_metadata.issuer().url(),
	issuer_url.url()

(wonder if this issue exists elsewhere in the crate...)

Need to use https://broker.pod.inrupt.com/ to make sure the url is the same as defined by openid-configuration, which is inconvenient, don't you think? 😁

It doesn't involve any extra code, so I don't think it's inconvenient; just how OpenID Connect works. OIDC providers should consider this when picking their issuer URL.

[sunli829]

Thanks a lot, I updated the PR. 😁