Skip to content

Commit

Permalink
Fix code styles and review suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
@lasanthaS
lasanthaS committed Apr 16, 2024
1 parent 1e9d22f commit 8e23d68
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 41 deletions.
4 changes: 2 additions & 2 deletions ...a/org/wso2/carbon/identity/application/authenticator/oidc/OIDCAuthenticatorConstants.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ private OIDCAuthenticatorConstants() {
* This class holds the constants related to authenticator configuration parameters.
*/

public static final String OAUTH_FEDERATED_PKCE_CODE_VERIFIER = "OAUTH_PKCE_CODE_VERIFIER";
public static final String ENABLE_FEDERATED_PKCE = "IsPKCEEnabled";
public static final String PKCE_CODE_VERIFIER = "PKCE_CODE_VERIFIER";
public static final String IS_PKCE_ENABLED = "IsPKCEEnabled";

public class AuthenticatorConfParams {

Expand Down
67 changes: 35 additions & 32 deletions ...a/org/wso2/carbon/identity/application/authenticator/oidc/OpenIDConnectAuthenticator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ public class OpenIDConnectAuthenticator extends AbstractApplicationAuthenticator

private static final Log LOG = LogFactory.getLog(OpenIDConnectAuthenticator.class);
private static final String OIDC_DIALECT = "http://wso2.org/oidc/claim";
private static final String PKCE_CODE_CHALLENGE_METHOD = "S256";

private static final String DYNAMIC_PARAMETER_LOOKUP_REGEX = "\\$\\{(\\w+)\\}";
private static final String IS_API_BASED = "IS_API_BASED";
Expand All @@ -153,6 +154,11 @@ public class OpenIDConnectAuthenticator extends AbstractApplicationAuthenticator
private static final String[] NON_USER_ATTRIBUTES = new String[]{"at_hash", "iss", "iat", "exp", "aud", "azp"};
private static final String AUTHENTICATOR_MESSAGE = "authenticatorMessage";

private static final String IS_PKCE_ENABLED_NAME = "isPKCEEnabled";
private static final String IS_PKCE_ENABLED_DISPLAY_NAME = "Enable PKCE";
private static final String IS_PKCE_ENABLED_DESCRIPTION = "Specifies that PKCE should be used for client authentication";
private static final String TYPE_BOOLEAN = "boolean";

@Override
public AuthenticatorFlowStatus process(HttpServletRequest request, HttpServletResponse response,
AuthenticationContext context)
Expand Down Expand Up @@ -518,7 +524,7 @@ protected String prepareLoginPage(HttpServletRequest request, AuthenticationCont
String nonce = UUID.randomUUID().toString();
context.setProperty(OIDC_FEDERATION_NONCE, nonce);
boolean isPKCEEnabled = Boolean.parseBoolean(
authenticatorProperties.get(OIDCAuthenticatorConstants.ENABLE_FEDERATED_PKCE));
authenticatorProperties.get(OIDCAuthenticatorConstants.IS_PKCE_ENABLED));

OAuthClientRequest authzRequest;

Expand Down Expand Up @@ -593,13 +599,10 @@ protected String prepareLoginPage(HttpServletRequest request, AuthenticationCont
// If PKCE is enabled, add code_challenge and code_challenge_method to the request.
if (isPKCEEnabled) {
String codeVerifier = generateCodeVerifier();
context.setProperty(OIDCAuthenticatorConstants.OAUTH_FEDERATED_PKCE_CODE_VERIFIER, codeVerifier);
try {
String codeChallenge = generateCodeChallenge(codeVerifier);
loginPage += "&code_challenge=" + codeChallenge + "&code_challenge_method=S256";
} catch (NoSuchAlgorithmException e) {
LOG.error("Error while generating the code challenge", e);
}
context.setProperty(OIDCAuthenticatorConstants.PKCE_CODE_VERIFIER, codeVerifier);
String codeChallenge = generateCodeChallenge(codeVerifier);
loginPage += "&code_challenge=" + codeChallenge + "&code_challenge_method="
+ PKCE_CODE_CHALLENGE_METHOD;
}

if (StringUtils.isNotBlank(queryString)) {
Expand Down Expand Up @@ -1485,8 +1488,8 @@ protected OAuthClientRequest getAccessTokenRequest(AuthenticationContext context
String clientSecret = authenticatorProperties.get(OIDCAuthenticatorConstants.CLIENT_SECRET);
String tokenEndPoint = getTokenEndpoint(authenticatorProperties);
boolean isPKCEEnabled = Boolean.parseBoolean(
authenticatorProperties.get(OIDCAuthenticatorConstants.ENABLE_FEDERATED_PKCE));
Object codeVerifier = context.getProperty(OIDCAuthenticatorConstants.OAUTH_FEDERATED_PKCE_CODE_VERIFIER);
authenticatorProperties.get(OIDCAuthenticatorConstants.IS_PKCE_ENABLED));
String codeVerifier = (String) context.getProperty(OIDCAuthenticatorConstants.PKCE_CODE_VERIFIER);

String callbackUrl = getCallbackUrlFromInitialRequestParamMap(context);
if (StringUtils.isBlank(callbackUrl)) {
Expand Down Expand Up @@ -1516,11 +1519,10 @@ protected OAuthClientRequest getAccessTokenRequest(AuthenticationContext context
.setCode(authzResponse.getCode());

if (isPKCEEnabled) {
if (codeVerifier != null) {
tokenRequestBuilder.setParameter("code_verifier", codeVerifier.toString());
} else {
LOG.warn("PKCE is enabled, but the code verifier is not found.");
if (StringUtils.isEmpty(codeVerifier)) {
throw new AuthenticationFailedException("PKCE is enabled, but the code verifier is not found.");
}
tokenRequestBuilder.setParameter("code_verifier", codeVerifier);
}

accessTokenRequest = tokenRequestBuilder.buildBodyMessage();
Expand All @@ -1541,15 +1543,14 @@ protected OAuthClientRequest getAccessTokenRequest(AuthenticationContext context
.setRedirectURI(callbackUrl)
.setCode(authzResponse.getCode());
if (isPKCEEnabled) {
if (codeVerifier != null) {
tokenRequestBuilder.setParameter("code_verifier", codeVerifier.toString());
} else {
LOG.warn("PKCE is enabled, but the code verifier is not found.");
if (StringUtils.isEmpty(codeVerifier)) {
throw new AuthenticationFailedException("PKCE is enabled, but the code verifier is not found.");
}
tokenRequestBuilder.setParameter("code_verifier", codeVerifier);
}
accessTokenRequest = tokenRequestBuilder.buildBodyMessage();
}
context.removeProperty(OIDCAuthenticatorConstants.OAUTH_FEDERATED_PKCE_CODE_VERIFIER);
context.removeProperty(OIDCAuthenticatorConstants.PKCE_CODE_VERIFIER);
// set 'Origin' header to access token request.
if (accessTokenRequest != null) {
// fetch the 'Hostname' configured in carbon.xml
Expand Down Expand Up @@ -1737,11 +1738,11 @@ public List<Property> getConfigurationProperties() {
configProperties.add(enableBasicAuth);

Property enablePKCE = new Property();
enablePKCE.setName("isPKCEEnabled");
enablePKCE.setDisplayName("Enable PKCE");
enablePKCE.setName(IS_PKCE_ENABLED_NAME);
enablePKCE.setDisplayName(IS_PKCE_ENABLED_DISPLAY_NAME);
enablePKCE.setRequired(false);
enablePKCE.setDescription("Specifies that PKCE should be used for client authentication");
enablePKCE.setType("boolean");
enablePKCE.setDescription(IS_PKCE_ENABLED_DESCRIPTION);
enablePKCE.setType(TYPE_BOOLEAN);
enablePKCE.setDisplayOrder(10);
configProperties.add(enablePKCE);

Expand Down Expand Up @@ -2218,15 +2219,17 @@ private String generateCodeVerifier() {
*
* @param codeVerifier code verifier
* @return code challenge
* @throws UnsupportedEncodingException
* @throws NoSuchAlgorithmException
* @throws AuthenticationFailedException
*/
private String generateCodeChallenge(String codeVerifier)
throws UnsupportedEncodingException, NoSuchAlgorithmException {
byte[] bytes = codeVerifier.getBytes("US-ASCII");
MessageDigest messageDigest = MessageDigest.getInstance("SHA-256");
messageDigest.update(bytes, 0, bytes.length);
byte[] digest = messageDigest.digest();
return java.util.Base64.getUrlEncoder().withoutPadding().encodeToString(digest);
private String generateCodeChallenge(String codeVerifier) throws AuthenticationFailedException {
try {
byte[] bytes = codeVerifier.getBytes("US-ASCII");
MessageDigest messageDigest = MessageDigest.getInstance("SHA-256");
messageDigest.update(bytes, 0, bytes.length);
byte[] digest = messageDigest.digest();
return java.util.Base64.getUrlEncoder().withoutPadding().encodeToString(digest);
} catch (UnsupportedEncodingException | NoSuchAlgorithmException e) {
throw new AuthenticationFailedException("Error while generating code challenge", e);
}
}
}
15 changes: 8 additions & 7 deletions ...g/wso2/carbon/identity/application/authenticator/oidc/OpenIDConnectAuthenticatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ public void testInitiateAuthenticationRequestNullProperties() throws OAuthSystem
public void testPassProcessAuthenticationResponse() throws Exception {

setupTest();

authenticatorProperties.put(OIDCAuthenticatorConstants.IS_PKCE_ENABLED, "false");
IdentityProviderProperty property = new IdentityProviderProperty();
property.setName(IdPManagementConstants.IS_TRUSTED_TOKEN_ISSUER);
property.setValue("false");
Expand Down Expand Up @@ -523,6 +523,7 @@ public void testPassProcessAuthenticationResponseWithNonce() throws Exception {
when(mockAuthenticationContext.getExternalIdP()).thenReturn(externalIdPConfig);
when(externalIdPConfig.getIdentityProvider()).thenReturn(identityProvider);
when(identityProvider.getIdpProperties()).thenReturn(identityProviderProperties);
authenticatorProperties.put(OIDCAuthenticatorConstants.IS_PKCE_ENABLED, "false");
when(openIDConnectAuthenticatorDataHolder.getClaimMetadataManagementService()).thenReturn
(claimMetadataManagementService);
when(mockAuthenticationContext.getExternalIdP()).thenReturn(externalIdPConfig);
Expand Down Expand Up @@ -550,19 +551,17 @@ public void testPassProcessAuthenticationResponseWithNonce() throws Exception {
@Test()
public void testGetAccessTokenRequestWithPKCE() throws URLBuilderException, AuthenticationFailedException {
mockAuthenticationRequestContext(mockAuthenticationContext);
mockAuthenticationContext.getAuthenticatorProperties()
.put(OIDCAuthenticatorConstants.ENABLE_FEDERATED_PKCE, "true");
when(mockAuthenticationContext.getProperty(OIDCAuthenticatorConstants.OAUTH_FEDERATED_PKCE_CODE_VERIFIER))
authenticatorProperties.put(OIDCAuthenticatorConstants.IS_PKCE_ENABLED, "true");
when(mockAuthenticationContext.getProperty(OIDCAuthenticatorConstants.PKCE_CODE_VERIFIER))
.thenReturn("sample_code_verifier");
OAuthAuthzResponse oAuthAuthzResponse = mock(OAuthAuthzResponse.class);
when(oAuthAuthzResponse.getCode()).thenReturn("abc");
when(mockOAuthzResponse.getCode()).thenReturn("abc");
mockStatic(ServiceURLBuilder.class);
ServiceURLBuilder serviceURLBuilder = mock(ServiceURLBuilder.class);
when(ServiceURLBuilder.create()).thenReturn(serviceURLBuilder);
when(serviceURLBuilder.build()).thenReturn(serviceURL);
when(serviceURL.getAbsolutePublicURL()).thenReturn("http://localhost:9443");
OAuthClientRequest request = openIDConnectAuthenticator
.getAccessTokenRequest(mockAuthenticationContext, oAuthAuthzResponse);
.getAccessTokenRequest(mockAuthenticationContext, mockOAuthzResponse);
assertTrue(request.getBody().contains("code_verifier=sample_code_verifier"));
}

Expand All @@ -585,6 +584,7 @@ public void testPassProcessAuthenticationWithBlankCallBack() throws Exception {

setupTest();
authenticatorProperties.put("callbackUrl", " ");
authenticatorProperties.put(OIDCAuthenticatorConstants.IS_PKCE_ENABLED, "false");
mockStatic(IdentityUtil.class);
when(IdentityUtil.getServerURL(FrameworkConstants.COMMONAUTH, true, true))
.thenReturn("http:/localhost:9443/oauth2/callback");
Expand Down Expand Up @@ -645,6 +645,7 @@ public void testPassProcessAuthenticationWithParamValue() throws Exception {
setupTest();
when(LoggerUtils.isDiagnosticLogsEnabled()).thenReturn(true);
authenticatorProperties.put("callbackUrl", "http://localhost:8080/playground2/oauth2client");
authenticatorProperties.put(OIDCAuthenticatorConstants.IS_PKCE_ENABLED, "false");
Map<String, String> paramMap = new HashMap<>();
paramMap.put("redirect_uri", "http:/localhost:9443/oauth2/redirect");
when(mockAuthenticationContext.getProperty(OIDC_PARAM_MAP_STRING)).thenReturn(paramMap);
Expand Down

0 comments on commit 8e23d68

Please sign in to comment.