GeoNode / geonode

GeoNode is an open source platform that facilitates the creation, sharing, and collaborative use of geospatial data.
https://geonode.org/
Other
1.41k stars 1.11k forks source link

Failing to add Microsoft Azure Account #12317

Open plaf2000 opened 3 weeks ago

plaf2000 commented 3 weeks ago

Expected Behavior

Successfully sign up with Microsoft Azure.

Actual Behavior

Getting the following error:

Traceback (most recent call last):
  File "/usr/local/lib/python3.10/dist-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
  File "/usr/local/lib/python3.10/dist-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/usr/local/lib/python3.10/dist-packages/allauth/socialaccount/providers/oauth2/views.py", line 87, in view
    return self.dispatch(request, *args, **kwargs)
  File "/usr/local/lib/python3.10/dist-packages/allauth/socialaccount/providers/oauth2/views.py", line 160, in dispatch
    login = self.adapter.complete_login(
  File "/usr/src/geonode/./geonode/people/adapters.py", line 343, in complete_login
    login = self.get_provider().sociallogin_from_response(request, extra_data)
  File "/usr/local/lib/python3.10/dist-packages/allauth/socialaccount/providers/base/provider.py", line 70, in sociallogin_from_response
    raise ValueError(f"uid must be a string: {repr(uid)}")

Exception Type: ValueError at /account/geonode_openid_connect/login/callback/
Exception Value: uid must be a string: None

Steps to Reproduce the Problem

  1. Follow the instruction to add a Microsoft Azure login described at https://docs.geonode.org/en/master/advanced/social/.
  2. Try to sign up using the Microsoft Azure option.

Specifications

Where the Problem Lies

The 'unique_name' key specified by the UID_FIELD Azure's provider settings variable (SOCIALACCOUNT_PROVIDERS_DEFS['azure']['UID_FIELD']) is not in the response body from https://graph.microsoft.com/v1.0/me. The attribute that should be considered is 'id' instead: https://learn.microsoft.com/en-us/graph/api/user-get?view=graph-rest-1.0&tabs=http.

Proposed fix(es)

In order to fix the issue, it is enough to change the UID_FIELD to 'id'. This can therefore also be easly done by a settings override, but, probably, this should be the default field defined in settings.py. To keep the backward compatibility with older API versions using the 'unique_name' field (is that so?), the following change could be taken into account as well:

# File geonode/people/socialaccount/providers/geonode_openid_connect/provider.py

def extract_uid(self, data):
        _uid_field = getattr(settings, "SOCIALACCOUNT_PROVIDERS", {}).get(PROVIDER_ID, {}).get("UID_FIELD", None)
        if _uid_field and _uid_field in data: # Checks that data has the defined field
            return data.get(_uid_field)
        else:
            return data.get("uid", data.get("sub", data.get("id")))

In that case it wouldn't be necessary to change the UID_FIELD value.

mattiagiupponi commented 3 weeks ago

Hi @plaf2000 is an open source project, feel free to open a PR with the proposed fixes. Please dont forget to add some test too

plaf2000 commented 3 weeks ago

Hi @mattiagiupponi and thank you for your answer. I will do that, but first I would like to know, if possible, the origin of the attribute 'unique_name': does that come from older versions of the API?

Thank you in advance

plaf2000 commented 3 weeks ago

@mattiagiupponi Some other questions regarding testing. I see that at the moment the only social account tested is Google. Therefore:

  1. Would I need to write tests for Azure account from scratch?
  2. If so, is there a fake/test account to use to call the Microsoft API?
  3. If not (to the 2. question), should I simulate some fake API responses in the tests (e.g. how they do on django-allauth for the microsoft provider)?
mattiagiupponi commented 3 weeks ago
  1. Would I need to write tests for Azure account from scratch?

If are not there, it would be nice

  1. If not (to the 2. question), should I simulate some fake API responses in the tests (e.g. how they do on django-allauth for the microsoft provider)?

Yes i guess mocking the responses is the best approach now