IdentityPython / SATOSA

Proxy translating between different authentication protocols (SAML2, OpenID Connect and OAuth2)
https://idpy.org
Apache License 2.0
197 stars 121 forks source link

fix AppleBackend #427

Closed melanger closed 9 months ago

melanger commented 1 year ago

common parts are not duplicated

All Submissions:

melanger commented 1 year ago

@c00kiemon5ter Hi, I have a problem and I am not sure how to solve it so that it fits SATOSA logic.

You wrote that a backend should not transform attributes. The problem here (apart from many others) is that "Sign in with Apple" is similar to OIDC, but not fully compliant. Instead of a name claim in id_token or userinfo they supply a user dictionary like '{"name":{"firstName":"Pavel","lastName":"Břoušek"},"email":"anonymousmail@apple.com"}'.

In the current state, this Apple backend puts the name sub-dictionary into data.attributes, so I have to have this in internal_attributes.yml (because this backend uses the openid mappings, not its own):

# ...
name:
    openid: [name]
givenname:
    openid: [given_name, name.firstName]
surname:
    openid: [family_name, name.lastName]
# ...

This works for Apple backend but fails for standard OIDC providers (e.g. Google), there is an uncaught exception in attribute mapping when it is trying to use the standard name claim (string) as a dictionary (because of name.firstName in config):

[2022-11-29 11:11:21] [ERROR]: [urn:uuid:0eb72947-5f36-421b-a45e-bd515e6050e1] Uncaught exception
Traceback (most recent call last):
  File "/usr/local/lib/python3.11/site-packages/satosa/base.py", line 240, in run
    resp = self._run_bound_endpoint(context, spec)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/satosa/base.py", line 180, in _run_bound_endpoint
    return spec(context)
           ^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/satosa/backends/openid_connect.py", line 221, in response_endpoint
    internal_resp = self._translate_response(all_user_claims, self.client.authorization_endpoint)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/satosa/backends/openid_connect.py", line 240, in _translate_response
    internal_resp.attributes = self.converter.to_internal("openid", response)
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/satosa/attribute_mapping.py", line 100, in to_internal
    attribute_values = self._collate_attribute_values_by_priority_order(external_attribute_name,
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/satosa/attribute_mapping.py", line 119, in _collate_attribute_values_by_priority_order
    attr_val = self._get_nested_attribute_value(attr_name, data)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/satosa/attribute_mapping.py", line 160, in _get_nested_attribute_value
    d = d.get(key)
        ^^^^^
AttributeError: 'str' object has no attribute 'get'

I can imagine at least two possible solutions (both of them may be implemented):

  1. Change Apple backend so that it does not use openid attribute mappings but its own apple mappings. Most of them will be duplicated from openid, but the name claims will be different and there will be no problem. This just means changing self.converter.to_internal("openid", response) in Apple backend to self.converter.to_internal("apple", response).
  2. Change attribute mapping so that it does not fail when trying nested attribute mapping on a non-nested attribute (string).

What do you suggest?

melanger commented 1 year ago

@c00kiemon5ter any thoughts on this?

melanger commented 1 year ago

@c00kiemon5ter Hi, can you please merge this?

melanger commented 9 months ago

@c00kiemon5ter Hi, is there anything blocking this PR? I am using it in production, it works.

c00kiemon5ter commented 9 months ago

re https://github.com/IdentityPython/SATOSA/pull/427#issuecomment-1330474635

How did you end up with d (data) being a string? It should have been a dict.

c00kiemon5ter commented 9 months ago

OK, I have figured it out; thanks for the comment above.

I have rebased on top of current master and updated the code with 2e3a7823, adding new test cases and fixing the traversal of the internal data.