Added ability to obtain groups from MS Graph when there are too many

[777GE90]

This pull request adds the ability to be able to query the user groups via MS Graph if they do not fit in the initial JWT access token (in the case where the user has too many groups). This only applies for Azure AD logins.

This should resolve #232

[codecov]

Codecov Report

Merging #247 (1852042) into master (5067f8f) will decrease coverage by 0.5%.
The diff coverage is 88.1%.

@@           Coverage Diff            @@
##           master    #247     +/-   ##
========================================
- Coverage    86.0%   85.4%   -0.6%     
========================================
  Files          11      11             
  Lines         515     556     +41     
========================================
+ Hits          443     475     +32     
- Misses         72      81      +9     

Impacted Files	Coverage Δ
django_auth_adfs/backend.py	84.0% <86.7%> (-1.9%)	⬇️
django_auth_adfs/__init__.py	100.0% <100.0%> (ø)
django_auth_adfs/config.py	88.0% <100.0%> (+0.2%)	⬆️

[tim-schilling]

What other permissions / setup / configuration does this change require? Those should be documented. I'm thinking not everyone would want to have this request be made during authentication, so it may be best to put it behind a flag.

[777GE90]

What other permissions / setup / configuration does this change require? Those should be documented. I'm thinking not everyone would want to have this request be made during authentication, so it may be best to put it behind a flag.

Good question, so for starters this extra API call will only run in the following circumstances:

• If you are using the newer Azure AD (as this problem does not occur with the older ADFS).
• Your group claim is not found in the token.
• The _claim_names key exists in the token and your group claim name exists in the _claim_names dict (as this indicates that MS has removed the group claim from the token - i.e. in the event there are too many).

Currently, when this happens in the plugin, it will just assume it can't find the groups claim and errors on that. All this change does is make sure that in such failure cases, one extra check is done to check if the claim can be obtained via an OBO request in these specific circumstances. So it won't alter any existing behaviour, but should only target this very specific failure case.

In terms of what changes the user needs, for this OBO request to succeed they need to make sure that their Azure AD app is configured to have the GroupMember.Read.All permission enabled. Not sure if there is documentation about adding permissions to an AD app? If so, where / how can I update this?

Edit: I also do have a check in the code that will raise a permission error if they do not have this permission enabled in Azure, so when someone is setting this up for the first time, they will be identify the problem from the logs.

            if group_data["displayName"] is None:
                logger.error("The application does not have the required permission to read user groups from MS Graph")
                raise PermissionDenied

[JonasKs]

Thank you so much for this! I won't be able to review before Monday, unfortunately. If you want to bump versions in pyproject.toml and __init__.py that would save some extra time when time comes for a release though 😊

[tim-schilling]

Thank you for the explanations! I think the documentation page that may need to be updated is https://github.com/snok/django-auth-adfs/blob/master/docs/azure_ad_config_guide.rst. Since you have an error that catches this permission error, I'd say that eliminates a fair amount of the documentation work. If you could please change the message to:

The application does not have the required permission to read user groups from MS Graph (GroupMember.Read.All)

Ideally we want the error message to contain all the information the user needs. By explicitly mentioning the permission required, hopefully we'll reduce the number of issues on the repo of people claiming there's a bug.

[777GE90]

Thank you so much for this! I won't be able to review before Monday, unfortunately. If you want to bump versions in pyproject.toml and __init__.py that would save some extra time when time comes for a release though 😊

No problem, done.

[777GE90]

Thank you for the explanations! I think the documentation page that may need to be updated is https://github.com/snok/django-auth-adfs/blob/master/docs/azure_ad_config_guide.rst. Since you have an error that catches this permission error, I'd say that eliminates a fair amount of the documentation work. If you could please change the message to:

The application does not have the required permission to read user groups from MS Graph (GroupMember.Read.All)

Ideally we want the error message to contain all the information the user needs. By explicitly mentioning the permission required, hopefully we'll reduce the number of issues on the repo of people claiming there's a bug.

Thanks for the guidance, updated the message and also updated the docs.

[JonasKs]

Sorry, didn’t have time to look into this today, and I’m out tomorrow. I’ll get back to you ASAP though.

[JonasKs]

Thank you so much. This looks great!

[JonasKs]

I would ideally have test coverage not drop a whole 5%. Could you add a unit test to get_obo_access_token to ensure we don't break the function flow at a later stage?

[777GE90]

I would ideally have test coverage not drop a whole 5%. Could you add a unit test to get_obo_access_token to ensure we don't break the function flow at a later stage?

Agreed, didn't realise we had this test coverage thing running in the pipeline so didn't notice the drop. Will add more tests when I get a chance and report back.

[777GE90]

I would ideally have test coverage not drop a whole 5%. Could you add a unit test to get_obo_access_token to ensure we don't break the function flow at a later stage?

Sorry for the delay, I'm maxed out these past two weeks. Have added some more tests now, coverage seems much better.

[JonasKs]

Thanks a lot! So sorry for the delay.

[JonasKs]

Released here. PyPI should be out as soon as pipelines are done.