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

fix AppleBackend #427

Merged
merged 5 commits into from
Nov 13, 2023
Merged

fix AppleBackend #427

merged 5 commits into from
Nov 13, 2023

Conversation

melanger
Copy link
Contributor

@melanger melanger commented Nov 28, 2022

common parts are not duplicated

All Submissions:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what problem you are trying to solve with this PR?
  • Have you added information on what your changes do and why you chose this as your solution?
  • Have you written new tests for your changes?
  • Does your submission pass tests?
  • This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

@melanger melanger changed the title refactor: base AppleBackend on OpenIDConnectBackend fix AppleBackend Nov 29, 2022
@melanger
Copy link
Contributor Author

@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 melanger marked this pull request as ready for review November 30, 2022 20:06
@melanger
Copy link
Contributor Author

@c00kiemon5ter any thoughts on this?

@melanger
Copy link
Contributor Author

melanger commented Jul 7, 2023

@c00kiemon5ter Hi, can you please merge this?

@c00kiemon5ter c00kiemon5ter self-assigned this Sep 26, 2023
@melanger
Copy link
Contributor Author

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

@c00kiemon5ter
Copy link
Member

re #427 (comment)

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

melanger and others added 4 commits November 13, 2023 21:37
incorrect function was used for parsing json (load is for files, loads for strings)
and the error was masked because of too broad except clause
when internal_attributes contain nested attribute but the actual value is not nested
@c00kiemon5ter
Copy link
Member

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

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

Co-authored-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
@c00kiemon5ter c00kiemon5ter merged commit a6e1f09 into IdentityPython:master Nov 13, 2023
@melanger melanger deleted the patch-6 branch November 13, 2023 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants