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

[feature] Added view to collect email in SAML registration #557

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pandafy
Copy link
Member

@pandafy pandafy commented Oct 25, 2024

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

Reference to Existing Issue

Description of Changes

If the IdP does not include email address in SAML response, then the user will be asked for email during sign-up.

@pandafy pandafy force-pushed the saml-intermediate-view branch 4 times, most recently from d5426db to cbaef31 Compare October 25, 2024 16:57
@coveralls
Copy link

coveralls commented Oct 25, 2024

Coverage Status

coverage: 98.645% (-0.01%) from 98.657%
when pulling f2ed3ea on saml-intermediate-view
into 23331a4 on master.

@pandafy pandafy force-pushed the saml-intermediate-view branch 4 times, most recently from 4a7897f to 6f84147 Compare November 4, 2024 15:03
If the IdP does not include email address in SAML response,
then the user will be asked for email during sign-up.
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good except the naming, see my comment below and let me know what you think.

@@ -26,6 +26,11 @@ def get_saml_urls(saml_views=None):
saml_views.AssertionConsumerServiceView.as_view(),
name='saml2_acs',
),
path(
'additional-info/',
saml_views.LoginAdditionalInfoView.as_view(),
Copy link
Member

@nemesifier nemesifier Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love the naming here, if we are asking only for the email here, why are we calling everything "additional info"?
I think we should call this view LoginCollectEmailView, the URL collect-email/ and the template collect-email.html.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used a generic name in-case we used this view to ask for additional information. E.g., we could ask the user to verify the name received from the IdP.

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.

3 participants