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.

Sign up for GitHub

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

Jump to bottom

Refacftored OAuth2 #1324

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

Refacftored OAuth2 #1324

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
891eeef
refactored OAuth2
ritikk112 Oct 13, 2024
b2872ae
refactored oauth
ritikk112 Oct 13, 2024
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
87 changes: 40 additions & 47 deletions native/src/main/java/io/ballerina/stdlib/oauth2/OAuth2Client.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import io.ballerina.runtime.api.values.BString;
import io.ballerina.stdlib.crypto.nativeimpl.Decode;

import java.util.Optional;
import java.io.FileInputStream;
import java.io.IOException;
import java.net.URI;
Expand Down Expand Up @@ -70,16 +71,16 @@ public static Object doHttpRequest(BString url, BMap<BString, Object> clientConf
headersList.add(entry.getValue().getValue());
}

BMap<BString, ?> customHeaders = getBMapValueIfPresent(clientConfig, OAuth2Constants.CUSTOM_HEADERS);
if (customHeaders != null) {
Optional<BMap<BString, ?>> customHeaders = getBMapValueIfPresent(clientConfig, OAuth2Constants.CUSTOM_HEADERS);
Copy link
Member

Choose a reason for hiding this comment

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

Replacing null checks with Optional types doesn't add any value here, as we are following an imperative style of coding. The previous code block looks fine, so let's keep the original code.

Copy link
Author

Choose a reason for hiding this comment

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

I introduced Optional checks to improve code readability as there were a lot of if else statements prior to changes and made the code less streamlined, should I revert back to if checks for null values?

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove.

customHeaders.ifPresent( customHeaders -> {
for (Map.Entry<BString, ?> entry : customHeaders.entrySet()) {
headersList.add(entry.getKey().getValue());
headersList.add(((BString) entry.getValue()).getValue());
}
}
});

String httpVersion = getBStringValueIfPresent(clientConfig, OAuth2Constants.HTTP_VERSION).getValue();
BMap<BString, ?> secureSocket = getBMapValueIfPresent(clientConfig, OAuth2Constants.SECURE_SOCKET);
Optional<BMap<BString, ?>> secureSocket = getBMapValueIfPresent(clientConfig, OAuth2Constants.SECURE_SOCKET);
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Member

Choose a reason for hiding this comment

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

check other places.


HttpRequest request;
URI uri;
Expand All @@ -95,29 +96,26 @@ public static Object doHttpRequest(BString url, BMap<BString, Object> clientConf
String[] flatHeaders = headersList.toArray(String[]::new);
request = buildHttpRequest(uri, flatHeaders, textPayload);
}

if (secureSocket != null) {
if (secureSocket.isPresent()) {
try {
SSLContext sslContext = getSslContext(secureSocket);
SSLContext sslContext = getSslContext(secureSocket.get());
HttpClient client = buildHttpClient(httpVersion, sslContext);
return callEndpoint(client, request);
} catch (Exception e) {
return createError("Failed to init SSL context. " + e.getMessage());
}
}
}
HttpClient client = buildHttpClient(httpVersion);
return callEndpoint(client, request);
}

private static URI buildUri(String url, BMap<BString, ?> secureSocket) throws IllegalArgumentException {
private static URI buildUri(String url, Optional<BMap<BString, ?>> secureSocket) throws IllegalArgumentException {
String[] urlParts = url.split(OAuth2Constants.SCHEME_SEPARATOR, 2);
if (urlParts.length == 1) {
urlParts = secureSocket != null ? new String[]{OAuth2Constants.HTTPS_SCHEME, urlParts[0]} :
urlParts = secureSocket.isPresent() ? new String[]{OAuth2Constants.HTTPS_SCHEME, urlParts[0]} :
new String[]{OAuth2Constants.HTTP_SCHEME, urlParts[0]};
} else {
if (urlParts[0].equals(OAuth2Constants.HTTP_SCHEME) && secureSocket != null) {
err.println(OAuth2Constants.RUNTIME_WARNING_PREFIX + OAuth2Constants.HTTPS_RECOMMENDATION_ERROR);
}
} else if (urlParts[0].equals(OAuth2Constants.HTTP_SCHEME) && secureSocket.isPresent()){
err.println(OAuth2Constants.RUNTIME_WARNING_PREFIX + OAuth2Constants.HTTPS_RECOMMENDATION_ERROR);
}
urlParts[1] = urlParts[1].replaceAll(OAuth2Constants.DOUBLE_SLASH, OAuth2Constants.SINGLE_SLASH);
url = urlParts[0] + OAuth2Constants.SCHEME_SEPARATOR + urlParts[1];
Expand All @@ -129,49 +127,46 @@ private static SSLContext getSslContext(BMap<BString, ?> secureSocket) throws Ex
if (disable) {
return initSslContext();
}
BMap<BString, BString> key = (BMap<BString, BString>) getBMapValueIfPresent(secureSocket, OAuth2Constants.KEY);
Optional<BMap<BString, BString>> key = Optional.ofNullable(getBMapValueIfPresent(secureSocket, OAuth2Constants.KEY));
Object cert = secureSocket.get(OAuth2Constants.CERT);
if (cert == null) {
throw new Exception("Need to configure 'crypto:TrustStore' or 'cert' with client SSL certificates file.");
}
KeyManagerFactory kmf;
TrustManagerFactory tmf;
KeyManagerFactory kmf = null;
TrustManagerFactory tmf = null;
if (cert instanceof BString) {
if (key != null) {
if (key.containsKey(OAuth2Constants.CERT_FILE)) {
BString certFile = key.get(OAuth2Constants.CERT_FILE);
BString keyFile = key.get(OAuth2Constants.KEY_FILE);
BString keyPassword = getBStringValueIfPresent(key, OAuth2Constants.KEY_PASSWORD);
if (key.isPresent()) {
tmf = getTrustManagerFactory((BString) cert);
if (key.get().containsKey(OAuth2Constants.CERT_FILE)) {
BString certFile = key.get().get(OAuth2Constants.CERT_FILE);
BString keyFile = key.get().get(OAuth2Constants.KEY_FILE);
BString keyPassword = getBStringValueIfPresent(key.get(), OAuth2Constants.KEY_PASSWORD);
kmf = getKeyManagerFactory(certFile, keyFile, keyPassword);
} else {
kmf = getKeyManagerFactory(key);
return buildSslContext(kmf.getKeyManagers(), tmf.getTrustManagers());
}
tmf = getTrustManagerFactory((BString) cert);
kmf = getKeyManagerFactory(key.get());
return buildSslContext(kmf.getKeyManagers(), tmf.getTrustManagers());
} else {
tmf = getTrustManagerFactory((BString) cert);
return buildSslContext(null, tmf.getTrustManagers());
}
}
tmf = getTrustManagerFactory((BString) cert);
return buildSslContext(null, tmf.getTrustManagers());
}
if (cert instanceof BMap) {
BMap<BString, BString> trustStore = (BMap<BString, BString>) cert;
if (key != null) {
if (key.containsKey(OAuth2Constants.CERT_FILE)) {
if(key.isPresent()){
tmf = getTrustManagerFactory(trustStore);
if (key.get().containsKey(OAuth2Constants.CERT_FILE)) {
BString certFile = key.get(OAuth2Constants.CERT_FILE);
BString keyFile = key.get(OAuth2Constants.KEY_FILE);
BString keyPassword = getBStringValueIfPresent(key, OAuth2Constants.KEY_PASSWORD);
kmf = getKeyManagerFactory(certFile, keyFile, keyPassword);
} else {
kmf = getKeyManagerFactory(key);
return buildSslContext(kmf.getKeyManagers(), tmf.getTrustManagers());
}
tmf = getTrustManagerFactory(trustStore);
kmf = getKeyManagerFactory(key.get());
return buildSslContext(kmf.getKeyManagers(), tmf.getTrustManagers());
} else {
tmf = getTrustManagerFactory(trustStore);
return buildSslContext(null, tmf.getTrustManagers());
}
return buildSslContext(null, tmf.getTrustManagers());
}
return null;
throw new Exception("Failed to initialize SSLContext.");
}

private static HttpClient.Version getHttpVersion(String httpVersion) {
Expand Down Expand Up @@ -209,10 +204,9 @@ private static TrustManagerFactory getTrustManagerFactory(BString cert) throws E
TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
tmf.init(ts);
return tmf;
} else {
throw new Exception("Failed to get the public key from Crypto API. " +
((BError) publicKeyMap).getErrorMessage().getValue());
}
throw new Exception("Failed to get the public key from Crypto API. " +
((BError) publicKeyMap).getErrorMessage().getValue());
}

private static TrustManagerFactory getTrustManagerFactory(BMap<BString, BString> trustStore) throws Exception {
Expand Down Expand Up @@ -250,14 +244,10 @@ private static KeyManagerFactory getKeyManagerFactory(BString certFile, BString
KeyManagerFactory kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
kmf.init(ks, "".toCharArray());
return kmf;
} else {
throw new Exception("Failed to get the private key from Crypto API. " +
((BError) privateKeyMap).getErrorMessage().getValue());
}
} else {
throw new Exception("Failed to get the public key from Crypto API. " +
((BError) publicKey).getErrorMessage().getValue());
}
throw new Exception("Failed to get the public key from Crypto API. " +
((BError) publicKey).getErrorMessage().getValue());
}

private static KeyStore getKeyStore(BString path, BString password) throws Exception {
Expand Down Expand Up @@ -309,6 +299,9 @@ private static Object callEndpoint(HttpClient client, HttpRequest request) {
return createError("Failed to get a success response from the endpoint. Response code: '" +
response.statusCode() + "'. Response body: '" + response.body() + "'");
} catch (IOException | InterruptedException e) {
if (e instanceof InterruptedException) {
Thread.currentThread().interrupt(); // Restore interrupted status
}
Comment on lines +302 to +304
Copy link
Member

Choose a reason for hiding this comment

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

Here we have already handled the InterruptedException by throwing an error to the Ballerina side via the createError method. It's not necessary to restore the status here. Is there a any particular reason to introduce this change ?

Copy link
Author

Choose a reason for hiding this comment

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

I added this just for additional security measures and to handle errors more gracefully

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this as there's no need to propagate this interruption.

return createError("Failed to send the request to the endpoint. " + e.getMessage());
}
}
Expand Down