ramosbugs / openidconnect-rs

OpenID Connect Library for Rust
MIT License
426 stars 103 forks source link

Error occurred when fetching user info under the Logto endpoint #184

Closed AH-dark closed 2 months ago

AH-dark commented 2 months ago

I use Logto's Traditional Web application. The front end provides Access Token to the back end and then performs back end user operations.

pub async fn get_user_by_token(&self, token: String) -> Result<user::Model, Error> {
    let access_token = AccessToken::new(token);
    let user_info_claims: CoreUserInfoClaims = self.client.user_info(access_token, None)?
        .request_async(async_http_client)
        .await?;

    let sub = user_info_claims.subject().as_str();
    let user = self.get_or_create_user(sub).await?;

    Ok(user)
}

In this code, the asynchronous method request_async returns the error Failed to parse server response. The specific response is obtained through tracing:

{
  "sub": "ghmyn2r614pl",
  "name": "Example",
  "picture": null,
  "updated_at": 1726466609578,
  "username": "example",
  "created_at": 1726394844098,
  "email": "example@example.com",
  "email_verified": true,
  "phone_number": "85212345678",
  "phone_number_verified": false
}

I don't know if there is a problem with my thinking or with the implementation. I used the standard CoreClient and CoreUserInfoClaims.

ramosbugs commented 2 months ago

Hey @AH-dark,

This crate follows the standard Rust error chaining approach using the Error trait's source method. The UserInfoError::Parse variant includes the underlying serde error as its source: https://github.com/ramosbugs/openidconnect-rs/blob/052f4a7234f9df9e1510cf6e49c8a45b0bf0d941/src/user_info.rs#L526

See some example code here for logging errors including all of the underlying causes.

At first glance, those timestamps look like milliseconds, while the standard defines them as seconds. That might prevent them from being parsed successfully as chrono::DateTime<Utc> types.

My usual suggestion for dealing with spec-noncompliant identity providers is to define a custom HTTP client to rewrite the response as a compliant one before returning it to this crate for further processing. This is a bit easier to do in the 4.x alpha release of this crate, which should be stabilized soon.

AH-dark commented 2 months ago

Hey @AH-dark,

This crate follows the standard Rust error chaining approach using the Error trait's source method. The UserInfoError::Parse variant includes the underlying serde error as its source:

https://github.com/ramosbugs/openidconnect-rs/blob/052f4a7234f9df9e1510cf6e49c8a45b0bf0d941/src/user_info.rs#L526

See some example code here for logging errors including all of the underlying causes.

At first glance, those timestamps look like milliseconds, while the standard defines them as seconds. That might prevent them from being parsed successfully as chrono::DateTime<Utc> types.

My usual suggestion for dealing with spec-noncompliant identity providers is to define a custom HTTP client to rewrite the response as a compliant one before returning it to this crate for further processing. This is a bit easier to do in the 4.x alpha release of this crate, which should be stabilized soon.

I tracked down the error to serde: invalid type: null, expected a string. Is there a better way, such as adding Additional Claims, to solve this problem?

AH-dark commented 2 months ago

I pinpointed the problem to the picture field. Maybe there is something wrong with your deserializer for this field 🤔.

ramosbugs commented 2 months ago

I pinpointed the problem to the picture field. Maybe there is something wrong with your deserializer for this field 🤔.

Ah yes, this looks like a similar issue to ramosbugs/oauth2-rs#278. The spec defines many optional fields, and serde_json's default handling of Option fields treats omitted fields and null as equivalent. However, using a custom deserializer breaks this default behavior unless the custom deserializer explicitly handles null.

Note that the spec explicitly discourages null values for optional claims:

If a Claim is not returned, that Claim Name SHOULD be omitted from the JSON object representing the Claims; it SHOULD NOT be present with a null or empty string value.

That said, since it's not a "MUST NOT" statement, it probably makes sense to gracefully handle JSON null values as if they're omitted. I'll work on a bug fix.