-
-
Notifications
You must be signed in to change notification settings - Fork 209
Issues/523 saml auth create email address objects #544
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
Issues/523 saml auth create email address objects #544
Conversation
nemesifier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove (it's redundant, create() already peforms the save operation):
| email_address.save() |
openwisp_radius/saml/views.py
Outdated
| try: | ||
| user.registered_user | ||
| except ObjectDoesNotExist: | ||
| email = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of retrieving the email address from the SAML request, can we use the user.email field to get the email address? The field should get automatically populated by djangosaml2 using attribute mapping. Then, we can create the EmailAddress object with that value.
78031d2 to
b1b6fb6
Compare
- 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
-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
b1b6fb6 to
4054392
Compare
Checklist
Reference to Existing Issue
Closes #523 .
Description of Changes
Screenshot