Closed 777GE90 closed 1 year ago
Merging #247 (1852042) into master (5067f8f) will decrease coverage by
0.5%
. The diff coverage is88.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%) |
:arrow_down: |
django_auth_adfs/__init__.py | 100.0% <100.0%> (ø) |
|
django_auth_adfs/config.py | 88.0% <100.0%> (+0.2%) |
:arrow_up: |
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.
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:
_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
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 😊
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.
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.
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.
Sorry, didn’t have time to look into this today, and I’m out tomorrow. I’ll get back to you ASAP though.
Thank you so much. This looks great!
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?
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.
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.
Thanks a lot! So sorry for the delay.
Released here. PyPI should be out as soon as pipelines are done.
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 https://github.com/snok/django-auth-adfs/issues/232