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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

freyp577
Copy link

@freyp577 freyp577 commented Feb 9, 2023

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
Copy link
Author

freyp577 commented Feb 9, 2023

Fix for #272

@codecov
Copy link

codecov bot commented Feb 9, 2023

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%) ⬇️
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
Copy link
Member

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
Copy link
Author

freyp577 commented Feb 9, 2023

@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
Copy link
Member

JonasKs commented Mar 30, 2023

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.

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

3 participants