Skip to content
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

feat: add claims and scope support #18

Merged
merged 29 commits into from
Jun 2, 2023
Merged

feat: add claims and scope support #18

merged 29 commits into from
Jun 2, 2023

Conversation

nanderstabel
Copy link
Collaborator

@nanderstabel nanderstabel commented Apr 19, 2023

Description of change

This PR adds the following:

  • Support for the claims parameter as described in the OIDC core specification: https://openid.net/specs/openid-connect-core-1_0.html#Claims.
  • Support for additional scope values: profile, phone, address and email as described here: https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims
  • Proper (de)serialization to both json and form-urlencoded. This was one by removing the custom serde for most of the structs and fixing the conversion from url string to RequestUrl using the FromStr trait and the std::fmt::Display trait for RequestUrl. This is basically cleaning up a previous incorrect implementation which over-complicated the (de)serialization of nested objects in the SiopRequest.
  • Change to Provider::generate_response method's prototype to pub async fn generate_response(&self, request: SiopRequest, user_claims: StandardClaims) -> Result<SiopResponse> by adding user_claims: StandardClaims. By using the StandardClaims, the subject requested claims can be inclued to the SiopResponse.
  • The MemoryStorage struct to the test_utils module to improve unit testing.

The "Steps involved" part of this repo's README is a pretty good overview of all the steps in a SIOP authentication flow: https://github.com/Sphereon-Opensource/SIOP-OID4VP#steps-involved. Important to note though is that for this PR it is not necessary to know the flow in detail. The example provides a good overview of what siop request and id tokens (can look like).

What is not present in the "Steps involved" part is that in the case when an authentication request is presented as a QR-Code, the Relying Party can choose to host the request object at an endpoint, and instead encode the endpoints uri using the request-uri parameter, example:

openid://?request-uri=https://example.com/request-uri

When the Provider receives this request uri, it can retrieve the actual request body by sending an GET request to the request uri.

This ensures that the QR Code remains simple and easy to decode by a qr-scanner.

I would also advise to take a look at our own example unit test which illustrates how the authentication flow is currently implemented.

More information on how the claims are requested and how they are returned in the authentication response:
For example this is the "structure" in which claims can be specified inside an authentication request:
AUTHENTICATION REQUEST

{
   ... other request parameters
   "claims":     {
        "id_token": {
          "auth_time": {
            "essential": true
          }
        }
}

The actual auth_time claim is nested inside >claims>id_token. However, the requested claims are returned in the root level of the id token:
ID TOKEN

{
   "iss": "did:example:123",
   ... other subject claims,
   "auth_time": 12345678910

}

Links to any relevant issues

#4

How the change has been tested

Multiple unit-tests for both scope and the claims related structs. They mainly resolve around their respective (de)serialization. Their functionality is also tested within the test_relying_party test.

Definition of Done checklist

Add an x to the boxes that are relevant to your changes.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@nanderstabel nanderstabel marked this pull request as draft April 20, 2023 07:37
@nanderstabel nanderstabel changed the title feat: add claims and scope support, add storage trait feat: add claims and scope support Apr 24, 2023
@nanderstabel nanderstabel added the Added A new feature that requires a minor release. label Apr 24, 2023
@nanderstabel nanderstabel force-pushed the feat/claims branch 2 times, most recently from 1e0ae55 to 2142cd8 Compare April 25, 2023 12:39
@nanderstabel nanderstabel marked this pull request as ready for review April 25, 2023 12:40
Copy link

@DeppLearning DeppLearning left a comment

Choose a reason for hiding this comment

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

As discussed I focused on the rustyness of the code and not too much on the correctness w.r.t. the SIOPv2 implementation. Looks good to us, very clean and elegant implementation. Especially liked the part where you merge multiple StandardClaims into one struct.

src/claims.rs Outdated
/// An individual claim request as defined in [OpenID Connect Core 1.0, section 5.5.1](https://openid.net/specs/openid-connect-core-1_0.html#IndividualClaimsRequests).
#[skip_serializing_none]
#[derive(Default, Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
pub struct IndividualClaimRequest<T> {

Choose a reason for hiding this comment

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

According to the spec this probably needs a deny_unknown_fields?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually the spec mentions this:

"Other members MAY be defined to provide additional information about the requested Claims. Any members used that are not understood MUST be ignored."

So other members are allowed in the IndividualClaimRequest, which is way I ended up adding the other field. This allows end-users to have the flexibility to provide extra (business) logic in their requests for claims. I added a doctest in which I try to illustrate this: https://github.com/impierce/siopv2/blob/feat/claims/src/claims.rs#L35-L62

Specifically in this part:

///     "birthdate": {
///         "essential": true,
///         "between": [ // <- end-users are free to add extra 'requirements' to a claim request
///             "1970-01-01",
///             "2000-01-01"
///         ]
/// }

src/claims.rs Outdated
#[derive(Default, Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
pub struct IndividualClaimRequest<T> {
pub essential: Option<bool>,
pub value: Option<T>,

Choose a reason for hiding this comment

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

Are value and values ever populated at the same time? If not, maybe it's a good idea to enforce this using the type system (i.e. enum or so)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The specification does not mention whether these are mutually exclusive or not so. Even though I agree that it would make sense to me that they would be mutually exclusive I rather adhere to the spec and try not to add any opinionated implementations. So it is up to the end-user to decide how this description fits their (business) logic.

src/id_token.rs Outdated
pub sub: String,
#[getset(set = "pub")]

Choose a reason for hiding this comment

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

why do you need this? the field is already public.

src/scope.rs Outdated

/// Set of scope values as specified in the
/// [OpenID Connect specification](https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims).
#[derive(PartialEq, Debug, Clone, Default, Deref)]

Choose a reason for hiding this comment

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

Deref is only needed for using the .iter method in claims.rs, I think I'd prefer either pub field for now, or adding a thin wrapper method:

    pub fn iter(&self) -> Iter<'_, ScopeValue> {
        self.0.iter()
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, I removed the Deref derive 👍

src/scope.rs Outdated
Phone,
}

impl std::fmt::Display for ScopeValue {

Choose a reason for hiding this comment

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

this could be removed in favor of derive_more::Display and then annotation enum variants with e.g.:

#[display(fmt="openid")]

Alternatively, you might be interested in strum

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for providing both these options and pointing me to strum, that crate certainly looks useful. Just for the sake of not adding another dependency I'm going for derive_more for now 👍

src/provider.rs Outdated
let subject_did = self.subject.did()?;
let id_token = IdToken::new(
let mut id_token = IdToken::new(

Choose a reason for hiding this comment

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

very nit picky, you might want to shadow id_token unmutably after initialization or do the initialization within a scope:

        let id_token = {
            let mut id_token = IdToken::new(
                subject_did.to_string(),
                subject_did.to_string(),
                request.client_id().clone(),
                request.nonce().clone(),
                (Utc::now() + Duration::minutes(10)).timestamp(),
            )
            .state(request.state().clone());
            id_token.set_standard_claims(user_claims);
            id_token
        };

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good suggestion, I adjusted it in the code. In general I am planning to add a builder pattern for IdToken, which will make the creation of id tokens more idiomatic. That will also resolve your other comment.

assert_eq!(
id_token.standard_claims,
StandardClaims {
name: Some(Claim::Value("Jane Doe".to_string())),

Choose a reason for hiding this comment

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

throughout at least this test .to_string and to_owned seem to be used interchangeably, maybe consistently use either one, I prefer .to_string

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, it's better to be consistent and to_string is indeed more explicit 👍

@nanderstabel
Copy link
Collaborator Author

nanderstabel commented May 12, 2023

As discussed I focused on the rustyness of the code and not too much on the correctness w.r.t. the SIOPv2 implementation. Looks good to us, very clean and elegant implementation. Especially liked the part where you merge multiple StandardClaims into one struct.

@DeppLearning Thank you for your useful review! Your two comments regarding IndividualClaimRequest actually triggered me to refactor claims.rs quite a bit, I admit I got a little bit carried away but I think my updated solution is a lot better and semantically closer to de OpenID specifications.

  • The main change I made in this regard is that I got rid of the Claims enum and introduced a new Claims trait instead. This enforces that all fields in the StandardClaims struct need to be either IndividualClaimRequest (StandardClaims<IndividualClaimRequest> that can be used in an authentication request) OR ClaimValue (StandardClaims<ClaimValue> that can be used in the IdToken that's returned in the response. Previously, when Claim was still an enum, it was possible to have a combination Claim::Value and Claim::IndividualClaimRequest fields in StandardClaims which logically does not make sense.

  • In the process I also got rid of the merge dependency and implemented a merge for StandardClaims method myself.

  • I added a bunch more comments.

nanderstabel and others added 18 commits May 24, 2023 10:41
Add tests for RequestUrl

Add missing request parameters

Add sphereon demo website test

Update documentation with new RequestUrl

Remove sphereon demo example

Add validate_request method to Provider struct

Add preoper Ser and De for SiopRequest and RequestBuilder

Add skeptic for Markdown code testing

Add support for Request by reference

fix: fix rebase conflicts

Add comments and fix some tests

fix: Move `derivative` to dev-dependencies

Refactor Provider and Subject

improve tests and example using wiremock

Improve struct field serde

fix: remove claims from lib.rs

style: fix arguments order

Add did:key DID method

Add support for Request by reference

fix: Remove lifetime annotations

Add preoper Ser and De for SiopRequest and RequestBuilder

Add Scope and Claim

fix: fix rebase conflicts
fix: disable default features for reqwest

fix: add dependency feature

fix: add dependency feature
@nanderstabel nanderstabel force-pushed the feat/claims branch 2 times, most recently from c9d783e to 1e8e818 Compare May 24, 2023 19:36
src/claims.rs Outdated
Comment on lines 13 to 18
pub trait Claim: Default + Clone + DeserializeOwned {
type ValueType;
type ClaimType<S>: Claim<ValueType = S> + Serialize
where
S: Serialize + Default + Clone + DeserializeOwned;
}

Choose a reason for hiding this comment

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

Reordering the bounds a little bit makes it more easy to understand for me and seems equivalent.

Suggested change
pub trait Claim: Default + Clone + DeserializeOwned {
type ValueType;
type ClaimType<S>: Claim<ValueType = S> + Serialize
where
S: Serialize + Default + Clone + DeserializeOwned;
}
pub trait Claim: Default + Clone + DeserializeOwned + Serialize {
type ValueType;
type ClaimType<S>: Claim<ValueType = S>
where
S: Default + Clone + DeserializeOwned + Serialize;
}

The more I think about it, the less I think it's possible to remove one associated type, since the code needs to say something (i.e. it's serializable) about the wrapper (self or the type implementing claim), and the inside what it's wrapping (i.e. also serializable). That's basically all the trait is saying right? That's not so much, makes me wonder even more if the trait should be there in the first place, or if a more explicit enum based variant would uphold all guarantees while being less elegant but also way simpler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After much trial and error I managed to get it down to one single associative type and reduce the amount of trait bounds. In the core it looks like this:

mod sealed {
    /// [`Claim`] trait that is implemented by both [`ClaimValue`] and [`ClaimRequest`].
    pub trait Claim {
        type Container<T>;
    }
}

#[derive(Default, Debug, Clone, PartialEq, Deserialize, Serialize)]
pub struct ClaimValue<T = ()>(pub T);

impl<T> sealed::Claim for ClaimValue<T> {
    type Container<U> = U;
}

#[derive(Default, Debug, Clone, PartialEq, Deserialize, Serialize)]
pub struct ClaimRequest<T = ()>(IndividualClaimRequest<T>);

impl<T> sealed::Claim for ClaimRequest<T> {
    type Container<U> = IndividualClaimRequest<U>;
}

#[derive(Default, Debug, Clone, PartialEq, Deserialize, Serialize)]
#[serde(untagged)]
pub enum IndividualClaimRequest<T> {
    #[default]
    Null,
    Object {
        #[serde(skip_serializing_if = "Option::is_none")]
        essential: Option<bool>,
        #[serde(skip_serializing_if = "Option::is_none")]
        value: Option<T>,
        #[serde(skip_serializing_if = "Option::is_none")]
        values: Option<Vec<T>>,
        #[serde(flatten, deserialize_with = "parse_other")]
        other: Option<serde_json::Value>,
    },
}

#[skip_serializing_none]
#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)]
#[serde(default, deny_unknown_fields)]
pub struct StandardClaims<C: sealed::Claim> {
    #[serde(bound(serialize = "C::Container<String>: Serialize"))]
    #[serde(bound(deserialize = "C::Container<String>: Deserialize<'de> + Default"))]
    #[serde(deserialize_with = "parse_optional_claim")]
    pub name: Option<C::Container<String>>,
    #[serde(bound(serialize = "C::Container<String>: Serialize"))]
    #[serde(bound(deserialize = "C::Container<String>: Deserialize<'de> + Default"))]
    #[serde(deserialize_with = "parse_optional_claim")]
    pub family_name: Option<C::Container<String>>,
}

So imo pretty clean and to the point. This approach guarantees the typesafety of the StandardClaims struct for both variations.

Copy link
Collaborator Author

@nanderstabel nanderstabel May 26, 2023

Choose a reason for hiding this comment

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

That's basically all the trait is saying right? That's not so much, makes me wonder even more if the trait should be there in the first place, or if a more explicit enum based variant would uphold all guarantees while being less elegant but also way simpler.

The main purpose of the trait is to guarantee object type safety of the StandardClaims struct. The Default + Clone + DeserializeOwned + Serialize trait bounds admittedly was more a result of me fighting with the compiler. In this new implementation the functionality is reduced to exactly that what it is supposed to do, i.e. guaranteeing object type safety for StandardClaims.

The added benefit of this is that wrong usage of the standard claims can be caught at compile time instead of runtime.

@nanderstabel nanderstabel merged commit 198e111 into dev Jun 2, 2023
@nanderstabel nanderstabel deleted the feat/claims branch December 14, 2023 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added A new feature that requires a minor release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants