Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ interface SSOAuthenticationConfiguration<AP extends SSOAuthenticationProvider<?>
*/
boolean isAutoRedirect();

/**
* Currently SSO-only since SAML is the only production provider that needs the skip re-auth option
*/
boolean isReauthenticationSupported();

@Override
default void handleStartupProperties(Map<String, String> map)
{
Expand Down
45 changes: 35 additions & 10 deletions api/src/org/labkey/api/security/AuthenticationManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ private ModelAndView getAuthView(AuthenticationResponse response, BindException
if (errors.hasErrors() || !response.isAuthenticated())
{
if (!errors.hasErrors())
errors.addError(new LabKeyError("Bad credentials"));
errors.addError(new LabKeyError(getFailureMessage(response)));
}
else
{
Expand Down Expand Up @@ -595,6 +595,18 @@ private ModelAndView getAuthView(AuthenticationResponse response, BindException
return new SimpleErrorView(errors, false);
}

// Most failures can't distinguish a bad password from an unknown user, so they share a deliberately vague
// message. reauthNotConfirmed is different: the user's credentials were never in question, so say what actually
// went wrong and who can fix it.
private String getFailureMessage(AuthenticationResponse response)
{
return FailureReason.reauthNotConfirmed == response.getFailureReason() ?
"Reauthentication failed: your identity provider did not re-verify your credentials. Please contact your administrator. " +
"The identity provider may not support the ForceAuthn option that electronic signatures require; if it does not, " +
"the \"Skip Reauthentication\" option can be enabled in this SSO configuration." :
"Bad credentials";
}

// Reauthentication case for electronic signing and other sensitive operations. Check that reauthentication
// was successful and re-auth user matches session user. Not currently verifying that the same authentication
// configuration was used to reauthenticate.
Expand Down Expand Up @@ -1914,18 +1926,23 @@ private static Map<String, ReauthContext> getTokenMap(HttpServletRequest request
/**
* Retrieves and validates the re-auth context associated with the provided token. If the token has an associated
* context that's not expired and (if requested) the context user matches the provided user, then return the user.
* If there's no token, sessionUser is non-null, and the user's authentication configuration has disabled re-auth,
* return the session user.
* @param request Request from which to retrieve the session
* @param token The reauth token to validate
* @param sessionUser If non-null, causes validation that this user matches the reauth user
* @return The re-auth user, if token is valid and session user check passes. Otherwise, null.
* @param token Re-auth token to validate
* @param sessionUser If non-null, causes validation that this user matches the reauth user. A null value also
* suppresses the skip-reauthentication exemption described above, so callers that require
* proof of an actual reauthentication (e.g. the CAS server's "renew" handling) must pass null.
* @return The re-auth user, if the token is valid and the session user check passes, or the session
* user if reauthentication is disabled for the user's configuration. Otherwise, null.
*/
public static @Nullable User getAndClearReauthUser(HttpServletRequest request, @Nullable String token, @Nullable User sessionUser)
{
if (token != null)
{
HttpSession session = request.getSession(false);
HttpSession session = request.getSession(false);

if (session != null)
if (session != null)
{
if (!StringUtils.isEmpty(token))
{
@SuppressWarnings("unchecked")
Map<String, ReauthContext> tokenMap = (Map<String, ReauthContext>) session.getAttribute(REAUTH_TOKEN_MAP_NAME);
Expand All @@ -1944,6 +1961,14 @@ private static Map<String, ReauthContext> getTokenMap(HttpServletRequest request
}
}
}
else if (sessionUser != null) // Skip the CAS IdP case where sessionUser is null
{
// Potential skip re-auth scenario. If we have a session but no token and the configuration has disabled
// re-auth, just return the session user.
PrimaryAuthenticationConfiguration<?> config = getConfiguration(session);
if (config instanceof AuthenticationConfiguration.SSOAuthenticationConfiguration<?> sso && !sso.isReauthenticationSupported())
return SecurityManager.getSessionUser(request);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we compare sessionUser and SecurityManager.getSessionUser(request) and return null if they don't match?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

To me, "skip reauthentication" means always return the session user. All the callers that pass non-null pass the session user anyway. I could see comparing and throwing IllegalStateException if they don't match, since it'd really be a coding error, not something a user could ever induce (AFAICT).

}
}

return null;
Expand All @@ -1968,7 +1993,7 @@ public void testReauthTokens() throws InterruptedException
User admin = TestContext.get().getUser();
Map<String, ReauthContext> map = getTokenMap(request);
clearExpiredTokens(map);
// Might have some unexpired tokens stashed away. Assume they won't expired during this test run.
// Might have some unexpired tokens stashed away. Assume they won't expire during this test run.
int initialCount = map.size();
ActionURL url = new ActionURL("core", "begin.view", ContainerManager.getRoot());

Expand All @@ -1982,7 +2007,7 @@ public void testReauthTokens() throws InterruptedException
assertEquals(admin, getAndClearReauthUser(request, token, admin));
assertEquals(initialCount, map.size());

// Try same token again
// Try the same token again
assertNull(getAndClearReauthUser(request, token, admin));
// Try a bogus token
assertNull(getAndClearReauthUser(request, "xyz", admin));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,7 @@ enum FailureReason
userDoesNotExist(ReportType.onFailure, "user does not exist", null),
badPassword(ReportType.onFailure, "incorrect password", null),
badCredentials(ReportType.onFailure, "invalid credentials", null), // Use for cases where we can't distinguish between userDoesNotExist and badPassword
reauthNotConfirmed(ReportType.onFailure, "identity provider did not reauthenticate the user", null), // Credentials were fine; the IdP declined to reauthenticate (e.g. ignored SAML ForceAuthn)
complexity(ReportType.onFailure, "password does not meet the complexity requirements", AuthenticationStatus.Complexity),
expired(ReportType.onFailure, "password has expired", AuthenticationStatus.PasswordExpired),
configurationError(ReportType.always, "configuration problem", null),
Expand Down
6 changes: 3 additions & 3 deletions core/src/org/labkey/core/login/LoginController.java
Original file line number Diff line number Diff line change
Expand Up @@ -1657,10 +1657,10 @@ public Object execute(ReturnUrlForm form, BindException errors)
JSONObject resp = new JSONObject();
resp.put("description", configuration.getDescription());
LoginUrls urls = urlProvider(LoginUrls.class);
ActionURL reauthUrl = configuration instanceof SSOAuthenticationConfiguration<?> sso ?
urls.getSSOReauthURL(sso, form.getReturnActionURL()) :
@Nullable ActionURL reauthUrl = configuration instanceof SSOAuthenticationConfiguration<?> sso ?
(sso.isReauthenticationSupported() ? urls.getSSOReauthURL(sso, form.getReturnActionURL()) : null) :
urls.getForceReauthURL(getContainer(), true, form.getReturnActionURL());
resp.put("reauthUrl", reauthUrl.getLocalURIString());
resp.put("reauthUrl", reauthUrl != null ? reauthUrl.getLocalURIString() : null);
return success(resp);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,22 @@
import org.labkey.api.view.ActionURL;
import org.labkey.api.view.ViewContext;

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

public class TestSsoConfiguration extends BaseSSOAuthenticationConfiguration<TestSsoProvider>
{
static final String SKIP_REAUTHENTICATION = "SkipReauthentication";

private final String _domain;
private final boolean _skipReauthentication;
private final LinkFactory _linkFactory = new LinkFactory(this);

protected TestSsoConfiguration(TestSsoProvider provider, Map<String, Object> standardSettings, Map<String, Object> properties)
{
super(provider, standardSettings);
_domain = (String)properties.get("domain");
_skipReauthentication = Boolean.TRUE.equals(properties.get(SKIP_REAUTHENTICATION));
}

@Override
Expand Down Expand Up @@ -64,9 +68,21 @@ public LinkFactory getLinkFactory()
return _linkFactory;
}

@Override
public boolean isReauthenticationSupported()
{
return !_skipReauthentication;
}

@Override
public @NotNull Map<String, Object> getCustomProperties()
{
return null != _domain ? Map.of("domain", _domain) : Collections.emptyMap();
Map<String, Object> map = new HashMap<>();
map.put(SKIP_REAUTHENTICATION, _skipReauthentication);

if (null != _domain)
map.put("domain", _domain);

return map;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.springframework.validation.Errors;
import org.springframework.web.servlet.ModelAndView;

import java.util.HashMap;
import java.util.Map;

public class TestSsoController extends SpringActionController
Expand Down Expand Up @@ -124,16 +125,35 @@ public void validate(TestSsoSaveConfigurationForm form, Errors errors)

public static class TestSsoSaveConfigurationForm extends SsoSaveConfigurationForm
{
private boolean _skipReauthentication = false;

@Override
public String getProvider()
{
return TestSsoProvider.NAME;
}

public boolean isSkipReauthentication()
{
return _skipReauthentication;
}

@SuppressWarnings("unused")
public void setSkipReauthentication(boolean skipReauthentication)
{
_skipReauthentication = skipReauthentication;
}

@Override
public @NotNull Map<String, Object> getPropertyMap()
{
return null != _domain ? Map.of("domain", _domain) : Map.of();
Map<String, Object> map = new HashMap<>();
map.put(TestSsoConfiguration.SKIP_REAUTHENTICATION, _skipReauthentication);

if (null != _domain)
map.put("domain", _domain);

return map;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ public String getDescription()
{
return List.of(
SettingsField.of("autoRedirect", SettingsField.FieldType.checkbox, "Default to this TestSSO configuration", "Redirects the login page directly to the TestSSO page instead of requiring the user to click on a logo.", false, false),
SettingsField.of(TestSsoConfiguration.SKIP_REAUTHENTICATION, SettingsField.FieldType.checkbox, "Skip Reauthentication", "Users who authenticate with this TestSSO configuration will not need to reauthenticate when signing electronically. This is not recommended since reauthentication is a requirement of 21 CFR Part 11.", false, false),
SettingsField.getStandardDomainField()
);
}
Expand Down
38 changes: 28 additions & 10 deletions devtools/src/org/labkey/devtools/view/testReauth.jsp
Original file line number Diff line number Diff line change
Expand Up @@ -34,34 +34,43 @@
success: function(response) {
const needReauth = <%=form.reauthToken() == null%>;
const data = JSON.parse(response.responseText).data;
document.getElementById("description").textContent = data.description;
if (needReauth) {
const skipReauth = (data.reauthUrl == null);
// Setting textContent HTML encodes the value
document.getElementById("description").textContent = data.description + (skipReauth ? ', but that configuration has disabled reauthentication' : '');
if (skipReauth || !needReauth) {
document.getElementById("sign").style.display = "block";
if (skipReauth && needReauth) {
document.getElementById("reauth").style.display = "none";
}
}
else {
document.getElementById("link").href = data.reauthUrl;
}
},
failure: function() {
alert('Failed to retrieve configuration!');
}
failure: LABKEY.Utils.getCallbackWrapper(function(errorInfo) {
document.getElementById("content").innerHTML = '<span>' + LABKEY.Utils.encodeHtml(errorInfo.exception ?? 'Failed to retrieve configuration') + '</span>';
}, this, true)
});
});
</script>

You authenticated with: <span id="description"></span><br/>
<div id="content">

You authenticated with: <span id="description"></span><br/>

<%
if (form.reauthToken() != null)
{
%>
Looks like you successfully re-authenticated and received token: <%=h(form.reauthToken())%><br/>

<labkey:form method="post">
<input type="hidden" name="reauthToken" value="<%=h(form.reauthToken())%>">
<input class="labkey-button primary" type="submit" value="Sign!">
</labkey:form>
<%
}
else
{
%>
<div id="reauth">
<%
if (form.errorMessage() != null)
{
%>
Expand All @@ -76,6 +85,15 @@ You need to re-authenticate.
}
%>
<a id="link" href="">Click here</a>

</div>
<%
}
%>
<div id="sign" style="display: none;">
<labkey:form method="post">
<input type="hidden" name="reauthToken" value="<%=h(form.reauthToken())%>">
<input class="labkey-button primary" type="submit" value="Sign!">
</labkey:form>
</div>
</div>