Skip to content

Commit

Permalink
fix passcode prompt for json responses
Browse files Browse the repository at this point in the history 
See issue #3321
  • Loading branch information
@strehle
strehle committed Mar 5, 2025
1 parent 0b59fbe commit 7fc4568
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 28 deletions.
56 changes: 46 additions & 10 deletions server/src/main/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpoint.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@
import java.security.Principal;
import java.sql.Timestamp;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Comparator;
import java.util.Date;
Expand All @@ -85,7 +84,6 @@
import java.util.Properties;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Stream;

import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Base64.getDecoder;
Expand Down Expand Up @@ -289,8 +287,8 @@ private String login(Model model, Principal principal, List<String> excludedProm
String loginHintParam = extractLoginHintParam(session, request);
UaaLoginHint uaaLoginHint = UaaLoginHint.parseRequestParameter(loginHintParam);

Map<String, SamlIdentityProviderDefinition> samlIdentityProviders;
Map<String, AbstractExternalOAuthIdentityProviderDefinition> oauthIdentityProviders;
Map<String, SamlIdentityProviderDefinition> samlIdentityProviders = null;
Map<String, AbstractExternalOAuthIdentityProviderDefinition> oauthIdentityProviders = null;
Map<String, AbstractIdentityProviderDefinition> allIdentityProviders = Map.of();
Map.Entry<String, AbstractIdentityProviderDefinition> loginHintProvider = null;

Expand All @@ -304,32 +302,34 @@ private String login(Model model, Principal principal, List<String> excludedProm
);
if (idp != null) {
loginHintProvider = Map.entry(idp.getOriginKey(), idp.getConfig());
oauthIdentityProviders = new HashMap<>();
oauthIdentityProviders.put(idp.getOriginKey(), (AbstractExternalOAuthIdentityProviderDefinition) idp.getConfig());
}
} catch (EmptyResultDataAccessException ignored) {
// ignore
}
}
if (loginHintProvider != null) {
oauthIdentityProviders = Map.of();
samlIdentityProviders = Map.of();
oauthIdentityProviders = addDefaultOauthMap(oauthIdentityProviders, allowedIdentityProviderKeys, defaultIdentityProviderName);
samlIdentityProviders = addDefaultSamlMap(samlIdentityProviders, allowedIdentityProviderKeys, defaultIdentityProviderName);
} else {
accountChooserNeeded = false;
samlIdentityProviders = getSamlIdentityProviderDefinitions(allowedIdentityProviderKeys);
oauthIdentityProviders = getOauthIdentityProviderDefinitions(allowedIdentityProviderKeys);
allIdentityProviders = concatenateMaps(samlIdentityProviders, oauthIdentityProviders);
}
} else if (!jsonResponse && (accountChooserNeeded || (accountChooserEnabled && !discoveryEnabled && !discoveryPerformed))) {
} else if (!jsonResponse && (accountChooserNeeded || (accountChooserEnabled && !discoveryEnabled && !discoveryPerformed))) {
// when `/login` is requested to return html response (as opposed to json response)
//Account and origin chooser do not need idp information
oauthIdentityProviders = Map.of();
samlIdentityProviders = Map.of();
oauthIdentityProviders = addDefaultOauthMap(oauthIdentityProviders, allowedIdentityProviderKeys, defaultIdentityProviderName);
samlIdentityProviders = addDefaultSamlMap(samlIdentityProviders, allowedIdentityProviderKeys, defaultIdentityProviderName);
} else {
samlIdentityProviders = getSamlIdentityProviderDefinitions(allowedIdentityProviderKeys);

if (jsonResponse) {
/* the OAuth IdPs and all IdPs are used for determining the redirect; if jsonResponse is true, the
* redirect is ignored anyway */
oauthIdentityProviders = Map.of();
oauthIdentityProviders = addDefaultOauthMap(oauthIdentityProviders, allowedIdentityProviderKeys, defaultIdentityProviderName);
} else {
oauthIdentityProviders = getOauthIdentityProviderDefinitions(allowedIdentityProviderKeys);
}
Expand Down Expand Up @@ -652,6 +652,36 @@ private Map<String, SamlIdentityProviderDefinition> getSamlIdentityProviderDefin
return filteredIdps.stream().collect(new MapCollector<>(SamlIdentityProviderDefinition::getIdpEntityAlias, idp -> idp));
}

private Map<String, SamlIdentityProviderDefinition> addDefaultSamlMap(Map<String, SamlIdentityProviderDefinition> list, List<String> allowedIdps, String defaultIdp) {
Map<String, SamlIdentityProviderDefinition> defaultList = list == null ? new HashMap<>() : list;
IdentityProvider samlIdP = getIdentityProvider(allowedIdps, defaultIdp);
if (samlIdP != null && samlIdP.getConfig() instanceof SamlIdentityProviderDefinition samlDefinition) {
defaultList.putIfAbsent(samlDefinition.getIdpEntityAlias(), samlDefinition);
}
return defaultList;
}

private Map<String, AbstractExternalOAuthIdentityProviderDefinition> addDefaultOauthMap(Map<String, AbstractExternalOAuthIdentityProviderDefinition> list, List<String> allowedIdps, String defaultIdp) {
Map<String, AbstractExternalOAuthIdentityProviderDefinition> defaultList = list == null ? new HashMap<>() : list;
IdentityProvider oauthIdP = getIdentityProvider(allowedIdps, defaultIdp);
if (oauthIdP != null && oauthIdP.getConfig() instanceof AbstractExternalOAuthIdentityProviderDefinition oDefinition) {
defaultList.putIfAbsent(oauthIdP.getOriginKey(), oDefinition);
}
return defaultList;
}

private IdentityProvider getIdentityProvider(List<String> allowedIdps, String defaultIdp) {
IdentityProvider samlIdP = null;
try {
if (defaultIdp != null && (allowedIdps == null || allowedIdps.contains(defaultIdp))) {
samlIdP = providerProvisioning.retrieveByOrigin(defaultIdp, IdentityZoneHolder.get().getId());
}
} catch (EmptyResultDataAccessException ignored) {
// ignore
}
return samlIdP;
}

protected Map<String, AbstractExternalOAuthIdentityProviderDefinition> getOauthIdentityProviderDefinitions(List<String> allowedIdps) {
List<IdentityProvider> identityProviders = externalOAuthProviderConfigurator.retrieveActiveByTypes(
IdentityZoneHolder.get().getId(),
Expand Down Expand Up @@ -704,22 +734,28 @@ private void populatePrompts(
Map<String, AbstractExternalOAuthIdentityProviderDefinition> oauthIdentityProviders,
boolean returnLoginPrompts
) {
boolean noIdpsPresent = true;
for (SamlIdentityProviderDefinition idp : samlIdentityProviders.values()) {
if (idp.isShowSamlLink()) {
model.addAttribute(SHOW_LOGIN_LINKS, true);
noIdpsPresent = false;
break;
}
}
for (AbstractExternalOAuthIdentityProviderDefinition oauthIdp : oauthIdentityProviders.values()) {
if (oauthIdp.isShowLinkText()) {
model.addAttribute(SHOW_LOGIN_LINKS, true);
noIdpsPresent = false;
break;
}
}

//make the list writeable
final List<String> excludedPrompts = new LinkedList<>(exclude);

if (noIdpsPresent) {
excludedPrompts.add(PASSCODE);
}
if (!returnLoginPrompts) {
excludedPrompts.add(USERNAME_PARAMETER);
excludedPrompts.add("password");
Expand Down
37 changes: 19 additions & 18 deletions server/src/test/java/org/cloudfoundry/identity/uaa/login/LoginInfoEndpointTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -580,10 +580,10 @@ void promptLogic() throws Exception {
assertThat(extendedModelMap).as("prompts attribute should be present").containsKey("prompts");
assertThat(extendedModelMap.get("prompts")).as("prompts should be a Map for JSON content").isInstanceOf(Map.class);
mapPrompts = (Map<String, Object>) extendedModelMap.get("prompts");
assertThat(mapPrompts).as("there should be three prompts for html").hasSize(3)
assertThat(mapPrompts).as("there should be three prompts for html").hasSize(2)
.containsKey("username")
.containsKey("password")
.containsKey("passcode");
.doesNotContainKey("passcode");

//add a SAML IDP
extendedModelMap.clear();
Expand Down Expand Up @@ -832,7 +832,7 @@ void oauth_provider_links_shown() throws Exception {
}

@Test
void passcode_prompt_present_whenThereIsAtleastOneActiveOauthProvider() throws Exception {
void no_passcode_prompt_present_whenThereIsAtleastOneActiveOauthProvider() throws Exception {
LoginInfoEndpoint endpoint = getEndpoint(IdentityZoneHolder.get());

RawExternalOAuthIdentityProviderDefinition definition = new RawExternalOAuthIdentityProviderDefinition()
Expand All @@ -847,11 +847,11 @@ void passcode_prompt_present_whenThereIsAtleastOneActiveOauthProvider() throws E
endpoint.infoForLoginJson(extendedModelMap, null, new MockHttpServletRequest("GET", "http://someurl"));

Map<String, Object> mapPrompts = (Map<String, Object>) extendedModelMap.get("prompts");
assertThat(mapPrompts).containsKey("passcode");
assertThat(mapPrompts).doesNotContainKey("passcode");
}

@Test
void passcode_prompt_present_whenThereIsAtleastOneActiveOauthProvider_stillWorksWithAccountChooser() throws Exception {
void no_passcode_prompt_present_whenThereIsAtleastOneActiveOauthProvider_stillWorksWithAccountChooser() throws Exception {
IdentityZoneHolder.get().getConfig().setAccountChooserEnabled(true);
LoginInfoEndpoint endpoint = getEndpoint(IdentityZoneHolder.get());

Expand All @@ -867,11 +867,11 @@ void passcode_prompt_present_whenThereIsAtleastOneActiveOauthProvider_stillWorks
endpoint.infoForLoginJson(extendedModelMap, null, new MockHttpServletRequest("GET", "http://someurl"));

Map<String, Object> mapPrompts = (Map<String, Object>) extendedModelMap.get("prompts");
assertThat(mapPrompts).containsKey("passcode");
assertThat(mapPrompts).doesNotContainKey("passcode");
}

@Test
void passcode_prompt_present_whenThereIsAtleastOneActiveOauthProvider_stillWorksWithDiscovery() throws Exception {
void no_passcode_prompt_present_whenThereIsAtleastOneActiveOauthProvider_stillWorksWithDiscovery() throws Exception {
IdentityZoneHolder.get().getConfig().setIdpDiscoveryEnabled(true);
LoginInfoEndpoint endpoint = getEndpoint(IdentityZoneHolder.get());

Expand All @@ -886,7 +886,7 @@ void passcode_prompt_present_whenThereIsAtleastOneActiveOauthProvider_stillWorks
endpoint.infoForLoginJson(extendedModelMap, null, new MockHttpServletRequest("GET", "http://someurl"));

Map<String, Object> mapPrompts = (Map<String, Object>) extendedModelMap.get("prompts");
assertThat(mapPrompts).containsKey("passcode");
assertThat(mapPrompts).doesNotContainKey("passcode");
}

@Test
Expand Down Expand Up @@ -1267,13 +1267,13 @@ void getPromptsFromOIDCProvider() {
assertThat(extendedModelMap).containsKey("prompts");
assertThat(extendedModelMap.get("prompts")).isInstanceOf(Map.class);
Map<String, String[]> returnedPrompts = (Map<String, String[]>) extendedModelMap.get("prompts");
assertThat(returnedPrompts).hasSize(3)
assertThat(returnedPrompts).hasSize(2)
.containsKey("username")
.containsKey("password")
.containsKey("passcode");
.doesNotContainKey("passcode");
assertThat(returnedPrompts.get("username")[1]).isEqualTo("MyEmail");
assertThat(returnedPrompts.get("password")[1]).isEqualTo("MyPassword");
assertThat(returnedPrompts.get("passcode")[1]).isEqualTo(passcodePromptText);
assertThat(returnedPrompts.get("passcode")).isNull();
}

@Test
Expand All @@ -1292,7 +1292,7 @@ void getPromptsFromNonOIDCProvider() {
assertThat(extendedModelMap).containsKey("prompts");
assertThat(extendedModelMap.get("prompts")).isInstanceOf(Map.class);
Map<String, String[]> returnedPrompts = (Map<String, String[]>) extendedModelMap.get("prompts");
assertUsernamePasswordAndPasscodePromptsAreReturned(returnedPrompts);
assertUsernamePasswordButNoPasscodePromptsAreReturned(returnedPrompts);
}

@Test
Expand All @@ -1308,7 +1308,7 @@ void getPromptsFromNonExistentProvider() {
assertThat(extendedModelMap).containsKey("prompts");
assertThat(extendedModelMap.get("prompts")).isInstanceOf(Map.class);
Map<String, String[]> returnedPrompts = (Map<String, String[]>) extendedModelMap.get("prompts");
assertUsernamePasswordAndPasscodePromptsAreReturned(returnedPrompts);
assertUsernamePasswordButNoPasscodePromptsAreReturned(returnedPrompts);
}

@Test
Expand All @@ -1330,7 +1330,7 @@ void getPromptsFromOIDCProviderWithoutPrompts() {
assertThat(extendedModelMap).containsKey("prompts");
assertThat(extendedModelMap.get("prompts")).isInstanceOf(Map.class);
Map<String, String[]> returnedPrompts = (Map<String, String[]>) extendedModelMap.get("prompts");
assertUsernamePasswordAndPasscodePromptsAreReturned(returnedPrompts);
assertUsernamePasswordButNoPasscodePromptsAreReturned(returnedPrompts);
}

@Test
Expand Down Expand Up @@ -1733,6 +1733,7 @@ private static void mockOidcProvider(IdentityProviderProvisioning mockIdentityPr
when(mockProvider.getConfig()).thenReturn(mockOidcConfig);
when(mockOidcConfig.isShowLinkText()).thenReturn(true);
when(mockIdentityProviderProvisioning.retrieveActiveByTypes(anyString(), any())).thenReturn(singletonList(mockProvider));
when(mockIdentityProviderProvisioning.retrieveByOrigin(eq("my-OIDC-idp1"), any())).thenReturn(mockProvider);
}

private static void mockLoginHintProvider(ExternalOAuthProviderConfigurator mockIdentityProviderProvisioning)
Expand All @@ -1750,15 +1751,15 @@ private static void mockLoginHintProvider(ExternalOAuthProviderConfigurator mock
when(mockIdentityProviderProvisioning.retrieveByOrigin(eq("my-OIDC-idp1"), any())).thenReturn(mockProvider);
}

private static void assertUsernamePasswordAndPasscodePromptsAreReturned(
private static void assertUsernamePasswordButNoPasscodePromptsAreReturned(
final Map<String, String[]> returnedPrompts
) {
assertThat(returnedPrompts).hasSize(3)
assertThat(returnedPrompts).hasSize(2)
.containsKey("username")
.containsKey("password")
.containsKey("passcode");
.doesNotContainKey("passcode");
assertThat(returnedPrompts.get("username")[1]).isEqualTo("Email");
assertThat(returnedPrompts.get("password")[1]).isEqualTo("Password");
assertThat(returnedPrompts.get("passcode")[1]).startsWith("Temporary Authentication Code ( Get one at");
assertThat(returnedPrompts.get("passcode")).isNull();
}
}

0 comments on commit 7fc4568

Please sign in to comment.