Weird-Sheep-Labs / django-azure-auth

A simple Django app for user authentication with Azure Active Directory/Entra ID.
MIT License
17 stars 10 forks source link

Auth handler assumes `first_name` and `last_name` fields exist on user model #23

Closed regoawt closed 4 months ago

regoawt commented 6 months ago

AuthHandler.authenticate() currently expects the user model to have first_name and last_name fields, and will error if not:

user.first_name = attr if (attr := azure_user["givenName"]) else ""
user.last_name = attr if (attr := azure_user["surname"]) else ""
user.save()

Provisional fix

sebastian-muthwill commented 5 months ago

Thanks for creating the issue @regoawt

May I suggest to also take the django username as well into account. This would allow to map the username to the userPrincipalName instead having it fixed with the email

By the way, I saw you marked #11 as not planned. Would the request not also fit the same logic of having the fields configurable from the settings providing more flexibility what can be passed from Azure to the application?

regoawt commented 5 months ago

@sebastian-muthwill thanks for the suggestions.

Despite what I've written in the issue description, I'm currently leaning away from using a mapping structure to assign the AAD attributes on the Django user model, and instead towards making the user define a custom UserManager.create_user() method in which one can do the assignment in a fully customisable way.

One of the main advantages of this approach is to avoid the 1 to 1 mapping between the Azure attributes and Django fields. An example would be if a User model includes a full_name field which should be set to azure_user['givenName'] + azure['surname'].

I marked #11 as unplanned for this reason but I will reopen it as I am planning on incorporating some elements from that issue i.e pulling out the extra fields from the Microsoft Graph endpoint and passing them as kwargs to the UserManager.create_user() method.

regoawt commented 4 months ago

I've made a v2.0.0 prerelease that people can check out.

In the end I didn't pass the attributes directly into UserManager.create_user() as:

  1. This would force users into creating a custom UserManager, which would be a problem if you're using the default User model
  2. The create_user() method assumes that all kwargs passed in correspond to attributes/fields on the user model, so passing in raw AAD attributes without handling them would result in AttributeErrors during authentication.

The intermediate mapping function strategy should hopefully mitigate those issues while still allowing full customisation of how AAD attributes can be translated into Django user model attributes/fields.