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

MFA AADSTS50076 error handling #101

Merged
merged 8 commits into from
Jan 16, 2021
Merged

MFA AADSTS50076 error handling #101

merged 8 commits into from
Jan 16, 2021

Conversation

JonasKs
Copy link
Member

@JonasKs JonasKs commented May 18, 2020

My collegue encountered the issue explained in #67 today so I offered to look into the problem. This is my proposed solution.

This is what happens:

  1. User logs in from a trusted location (or is already logged in through another site using AD) and is for that reason not required to enter MFA.
  2. The server sends a POST request (in backend.py) with authorization_code towards Azure AD.
  3. The server is not in a trusted location (and for various reasons this may not be applicable), and therefor Azure AD will reject the authorization code, as it was not acquired with MFA.

MFA can be forced by setting amr_values to ngcmfa. Docs on this can be found here.

In other words, this PoC contains:

  • A view that forces MFA.
  • backend.py will raise a MFARequired if the error starts with the code AADSTS50076 which again is handled in the view, forcing a new MFA log in if that happens. In other words, a second log in is required, this time with MFA.

If this is approved, let me know and I'll expand the tests and docs. I've confirmed it works on our set up.
Manual testing (for those with the issue) can be done by installing my branch: pip install git+https://github.com/JonasKs/django-auth-adfs.git@mfa_poc.

@codecov
Copy link

codecov bot commented May 18, 2020

Codecov Report

Merging #101 (44f20de) into master (93bcb3f) will decrease coverage by 0.64%.
The diff coverage is 61.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #101      +/-   ##
==========================================
- Coverage   82.69%   82.05%   -0.65%     
==========================================
  Files           7        8       +1     
  Lines         416      429      +13     
==========================================
+ Hits          344      352       +8     
- Misses         72       77       +5     
Impacted Files Coverage Δ
django_auth_adfs/backend.py 79.87% <33.33%> (-0.65%) ⬇️
django_auth_adfs/middleware.py 91.30% <60.00%> (-8.70%) ⬇️
django_auth_adfs/config.py 84.43% <66.66%> (-0.33%) ⬇️
django_auth_adfs/exceptions.py 100.00% <100.00%> (ø)
django_auth_adfs/signals.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68745fa...1096833. Read the comment docs.

@jobec
Copy link
Collaborator

jobec commented Sep 13, 2020

I'm back in business.

Thanks for figuring this out.

About this piece of code

query["amr_values"] = "ngcmfa"

What happens if you always send it? E.g. instead of adding a view it would remove complexity if including those query param fixes it in all situations.

@JonasKs
Copy link
Member Author

JonasKs commented Sep 13, 2020

Welcome back!

I’m not 100% sure, I think it would force login to MFA even if the server is in a trusted location. Maybe it could be a setting, but with some good docs.
It’s been a while since I looked at this, so I’ll see if I can set up a demo project and test out the different scenarios. Unfortunately I’ll be on vacation for a few weeks now, so it won’t be for a while. I’ll keep you posted.

@peterfarrell
Copy link
Contributor

I'm interested in this PR. Anything I can help with?

@JonasKs
Copy link
Member Author

JonasKs commented Oct 26, 2020

@peterfarrell I don’t have an azure environment set up at the moment, so if you’re able to check what @jobec is asking, that would be nice. I’m currently coding on a few other projects on my spare time, so haven’t had time to look into this one in a while, unfortunately.

Tests also need to be written if you want to help out with that.

I’ll also try to prioritize this a bit more in the weekend.

@peterfarrell
Copy link
Contributor

@JonasKs I don't have an Azure setup but onsite ADFS. When I add amr_values=ngcmfa to the querystring for the authroize, I get an ADFS error.

@JonasKs
Copy link
Member Author

JonasKs commented Oct 27, 2020

Ok. Did you ever experience this error with your setup in ADFS? Which ADFS version in that case?

@peterfarrell
Copy link
Contributor

ADFS 4.0, but I haven't seen this error directly, but surmise it might come up.

@JonasKs
Copy link
Member Author

JonasKs commented Oct 27, 2020

Hmm, maybe. Are you able to reproduce the steps above to check? I feel like the error you get from ADFS when appending the parameters either means you don’t have MFA on your account, or that ADFS don’t work the same way as Azure AD.

@peterfarrell
Copy link
Contributor

@JonasKs The error I found in the ADFS backend was:

The requested authentication method 'ngcmfa' is not valid for relying party trust

So I guess adding that option to all request will fail if that authentication method is not listed for the configured relying party trust.

@JonasKs
Copy link
Member Author

JonasKs commented Oct 27, 2020

Alright, yeah, that makes sense. You will never get AADSTS50076 without a MFA enabled. So I guess this solution would work fine as it is.

@JonasKs JonasKs linked an issue Jan 16, 2021 that may be closed by this pull request
@JonasKs JonasKs merged commit 82f0731 into snok:master Jan 16, 2021
@JonasKs JonasKs mentioned this pull request Jan 19, 2021
5 tasks
mmcclelland1002 pushed a commit to mmcclelland1002/django-auth-adfs that referenced this pull request Nov 12, 2021
MFA AADSTS50076 error handling
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.

Azure AD authentication issue with trusted locations
3 participants