django-auth-adfs icon indicating copy to clipboard operation
django-auth-adfs copied to clipboard

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

Open freyp577 opened this issue 2 years ago • 6 comments

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 avatar Feb 09 '23 16:02 freyp577

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

freyp577 avatar Feb 09 '23 16:02 freyp577

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%> (ø)

codecov[bot] avatar Feb 09 '23 16:02 codecov[bot]

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?

tim-schilling avatar Feb 09 '23 16:02 tim-schilling

@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.

freyp577 avatar Feb 09 '23 17:02 freyp577

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.

JonasKs avatar Mar 30 '23 14:03 JonasKs