snok / django-auth-adfs

A Django authentication backend for Microsoft ADFS and AzureAD
http://django-auth-adfs.readthedocs.io/
BSD 2-Clause "Simplified" License
270 stars 98 forks source link

Allow more than 100 group items to be fetched from MS Graph API #274

Open freyp577 opened 1 year ago

freyp577 commented 1 year ago

Use of MS Graph API to fetch group information for the user imposes a limitation of max 100 items returned per API request so when there are more than 100 groups, the request needs to be repeated with the API call using the odata.nextLink value returned in the current result

see Microsoft Graph API Result Size Limit for details

freyp577 commented 1 year ago

Fix for https://github.com/snok/django-auth-adfs/issues/272

codecov[bot] commented 1 year ago

Codecov Report

Merging #274 (0d6d9ef) into master (896d65b) will decrease coverage by 0.2%. The diff coverage is 73.6%.

@@           Coverage Diff            @@
##           master    #274     +/-   ##
========================================
- Coverage    86.3%   86.1%   -0.2%     
========================================
  Files           8      11      +3     
  Lines         497     557     +60     
========================================
+ Hits          429     480     +51     
- Misses         68      77      +9     
Impacted Files Coverage Δ
django_auth_adfs/backend.py 85.9% <73.6%> (-0.5%) :arrow_down:
django_auth_adfs/views.py 85.1% <0.0%> (ø)
django_auth_adfs/__init__.py 100.0% <0.0%> (ø)
django_auth_adfs/urls.py 100.0% <0.0%> (ø)
tim-schilling commented 1 year ago

My concern once is that we're now at 1 to N requests to authenticate a user. If we're building something for folks who have hundreds of groups per user, do we take it to the next step and give them the ability to search and filter?

freyp577 commented 1 year ago

@tim-schilling good point, actually we need only a couple of groups from the > 200 groups we get from our AD onpremise / azureAD combination - they all have a certain prefix (_EDB) so if there were filtering it would be useful here

note also that we have solved this issue picking our groups by subclassing some functionality django-auth-adfs provides as we not only need to filter but also normalize the group names was they do not match 1:1 between our AD/azureAD and Django worlds

So it is your decision if you accept this as improvement, or replace it by a better approach that is more flexible.

JonasKs commented 1 year ago

I'm so sorry for the late review. I hope this haven't been a blocker for you - it's allowed to ping me when I forget 😁

I really don't think there's that many users with hundreds of roles, but I agree with Tim, this would potentially add a lot of overhead to e.g. DRF APIs where this would be fetched on every single request. I'd love if you could try to implement the search and filter, so we keep it to a minimum.