WIP: Introduce add_claims_by_scope per client configuration

[ctriant]

This MR enables per client configuration for add_claims_by_scope parameter. If configuration for specific client exists, it overrides the global configuration.

For example consider the following list of clients and that the default oidc-op configuration sets add_claims_by_scope = True:

oidc_clients:
  client1:
    # client secret is "password"
    client_secret: "Namnam"
    redirect_uris:
      - ['https://openidconnect.net/callback', '']
    response_types:
      - code
    add_claims_by_scope: False

  client2:
    client_secret: "spraket"
    redirect_uris:
      - ['https://app1.example.net/foo', '']
      - ['https://app2.example.net/bar', '']
    response_types:
      - code

Also consider the two clients form the following authorization request where CLIENT_ID is "client1" and "client2" respectively:

AREQS = AuthorizationRequest(
    response_type="code",
    client_id=CLIENT_ID,
    redirect_uri="http://example.com/authz",
    scope=["openid", "address", "email"],
    state="state000",
    nonce="nonce",
)

As long as for "client1" add_claims_by_scope = False, the returned claims will be the union of the base claims and the explicit claims requested for this user given enable_claims_per_client = True.

On the other hand, for "client2" the returned claims will be the same as for "client1" plus the the claims that correspond to the requested scopes after oidcop.scopes.convert_scopes2claims() is applied.

[peppelinux]

Thank you @ctriant can you rebase this PR to develop branch? we cannot merge contributions directly on master branch

[peppelinux]

unit tests fails, some changes are required

[ctriant]

unit tests fails, some changes are required

On it

[peppelinux]

Ok I'd merge It now but I'm wondering that It needs to be documented, otherwise only the code would show this feature.

@ctriant can you put a brief note on this feature, how It works with a short example?

[ctriant]

Ok I'd merge It now but I'm wondering that It needs to be documented, otherwise only the code would show this feature.

@ctriant can you put a brief note on this feature, how It works with a short example?

Hey @peppelinux, please see if the example on the MR description helps. Thanks!

[peppelinux]

@criant yes it helps Would you like to put it also in the documentation, where I found that we still don't have the "client" section documented!

https://github.com/IdentityPython/oidc-op/blob/master/docs/source/contents/conf.rst

@rohe ^

[nsklikas]

Shouldn't this be per client? E.g introduce the options userinfo_add_claims_by_scope, idtoken_add_claims_by_scope, etc

[rohe]

@nsklikas Since we have the client parameters id_token_claims and userinfo_claims it sounds reasonable to have userinfo_add_claims_by_scope and idtoken_add_claims_by_scope. Even though the long names makes me cringe.

Also, we might want to think about structuring this is some way. That is instead of having:

oidc_clients = {
    "client_1": {
        "client_secret": "Namnam",
        "redirect_uris": [('https://openidconnect.net/callback', '')],
        "response_types": ["code"],
        "userinfo_add_claims_by_scope": False,
        "id_token_add_claims_by_scope": False,
        "userinfo_claims": ["email", "email_verified"],
        "id_token_claims": ["email", "phone"]
    }
}

we could have (to improve readability)

oidc_clients_s = {
    "client_1": {
        "client_secret": "Namnam",
        "redirect_uris": [('https://openidconnect.net/callback', '')],
        "response_types": ["code"],
        "add_claims": {
            "by_scope": {
                "userinfo": False,
                "id_token": False
            },
            "always": {
                "userinfo": ["email", "email_verified"],
                "id_token": ["email", "phone"]
            }
        }
    }
}

Just a thought.

[nsklikas]

I prefer the flat model. I think it's easier to handle and it makes the type of our fields simpler (if we want to add pydantic or something similar in the future it will probably be easier to describe primitive types rather than complex dicts)

Having said that I have no hard objections against the nested model roland suggested and I agree that it also has certain advantages (e.g. all the information is gathered in one dict so it's easier to access)

[ctriant]

@peppelinux i think we better revert this MR because it was a WIP. Some tests are missing and also we have to make a decision regarding the configuration pattern before we are ready to merge ti.

[peppelinux]

I should have known during the last developer meeting and this PR should have been a draft. How long does it take to complete this feature?

we will make a release solely to cover this issue, I beg you to give priority to this

[ctriant]

The MR was marked as WIP so I thought this was enough to be considered as draft. Sorry for the inconvenience @peppelinux .

[peppelinux]

don't worry, for the next time create drafts and non-PRs and switch to PR when ready for final revisions.

what time do you have to finish this WiP?

[ctriant]

The implementation follows the proposed scheme by @rohe . If we decide not to adopt this proposal I have to revert some changes. Then add some tests. I think there is no much work needed to close it.

[peppelinux]

yes I saw unit tests and I pushed an example in the documentation, it works as it is, please add your minor changes and unit test for the next, imminent, release and thank you for all of this