-
Notifications
You must be signed in to change notification settings - Fork 259
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
Support email domain based organization discovery during self-registration. #7083
base: master
Are you sure you want to change the base?
Conversation
d6ed343
to
cb488c4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7083 +/- ##
=======================================
Coverage 32.02% 32.02%
=======================================
Files 41 41
Lines 893 893
Branches 216 220 +4
=======================================
Hits 286 286
Misses 557 557
Partials 50 50
Flags with carried forward coverage won't be shown. Click here to find out more. |
} else if (isErrorFallbackLocale) { | ||
errorMessage = AuthenticationEndpointUtil.i18n(resourceBundle,"error.retry"); | ||
} | ||
} else if (isSelfRegistration && errorMessage.equalsIgnoreCase("Organization is not associated with this application.")) { |
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.
use a key instead of this string
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.
This is the error message currently returned from the organization SSO authenticator. Since this is the existing behaviour will track this improvement as a separate effort.
|
||
<% if (Boolean.parseBoolean(request.getParameter("isSelfRegistration"))) { %> | ||
$(".ui.segment").hide(); | ||
window.location = "<%=getRegistrationUrl(accountRegistrationEndpointContextURL, srURLEncodedURL, (String) request.getAttribute(JAVAX_SERVLET_FORWARD_QUERY_STRING))%>"; |
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.
check if similar logic as below works here
request.getRequestDispatcher("error.jsp").forward(request, response);
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.
check if similar logic as below works here request.getRequestDispatcher("error.jsp").forward(request, response);
This is not possible since the registration page resides in a seperate web app (recovery portal)
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.
Have your tried using response.sendRedirect
from Java context?
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.
Have your tried using
response.sendRedirect
from Java context?
Yes, since the login page url is generated and added to the auth response as a 302 location header, using sendRedirect in this flow will introduce some inconsistencies and unwanted complexity.
invalid.organization.discovery.type=Given organization discovery type is invalid or not enabled. | ||
discovery.input.cannot.be.empty=Email cannot be empty. | ||
organization.name.cannot.be.empty=Organization name cannot be empty. | ||
provide.organization.name=Provide organization name | ||
provide.email.address=Provide email address | ||
self.registration.application.not.shared=The application is not shared with the organization associated with the provided email domain. |
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.
Do we really need to show this error message to an end user? an average user will not have an idea about what sharing an application means
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.
we can remove and show the general message. updated
identity-apps-core/apps/recovery-portal/src/main/webapp/self-registration-username-request.jsp
Show resolved
Hide resolved
} else { | ||
isSSOLoginAuthenticatorConfigured = true; | ||
} | ||
} |
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.
Is it possible to break the for loop once the sso authenticator is found?
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.
yes, updated
03b1051
to
f77acad
Compare
🦋 Changeset detectedThe changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. |
Purpose
With this pr, the recovery portal will be improved to handle email domain based orgnization discovery for self-registration. Changes are made to the login.jsp, org_discovery.jsp and self-registration-username-request.jsp pages to handle new parameters
Related issue