Issues/523 saml auth create email address objects #544
Conversation
- Check if the NameID is an email and use it as the user's email. - If NameID is not an email, check for the 'email' attribute in the SAML response. - Create an EmailAddress object using the retrieved email. Fixes openwisp#523
There was a problem hiding this comment.
Thank you very much for your patience @kaushikaryan04.
I've been super busy publishing the new unified documentation website of OpenWISP, now I am back reviewing PRs.
This looks almost ready to merge, but there's a bit of validation we have to do before saving data got from external sources to the DB.
Please see my comments below.
openwisp_radius/saml/views.py
Outdated
| email = session_info['ava'].get('email', [None])[0] | ||
| if email: | ||
| user.email = email | ||
| user.save() |
There was a problem hiding this comment.
Saving this information without validating it is dangerous.
Here we need to run full_clean() and catch any possible exception in case the email we get from the identity provider is invalid for some reason. If the email is invalid wont save/create, log a warning and skip, eg:
try:
user.full_clean()
except ValidationError:
logger.exception(f'Failed email validation for {user} during SAML user creation')
else:
user.save()
email_address = EmailAddress.objects.create(
user=user, email=email, verified=True, primary=True
)Please add a new test for this error case, you can use mock or any other way to pass an invalid email and catch the validation error, there's similar tests in this and other modules you can base your test on, look for ValidationError in the test suites of the codebase.
There was a problem hiding this comment.
No problem.The new unified documentation looks great. I will make the suggested changes.
openwisp_radius/saml/views.py
Outdated
| email_address = EmailAddress.objects.create( | ||
| user=user, email=email, verified=True, primary=True | ||
| ) | ||
| email_address.save() |
There was a problem hiding this comment.
Remove (it's redundant, create() already peforms the save operation):
| email_address.save() |
-Added Validation for email -If validation is failed we try to get email from attributes -Added tests to see if Exception is raised when invalid mail is provided Fixes openwisp#523
Checklist
Reference to Existing Issue
Closes #523 .
Description of Changes
Screenshot