Client Side Http Authentication Mechanism #1881
Conversation
There was a problem hiding this comment.
Hi @keshav-725 Thank you for PR! Looks great, I just added some ideas and feedback
| * Get the Http Mechanism Type to use for the given configuration. | ||
| * | ||
| * @param configuration the configuration (must not be {@code null}) | ||
| * @return the WS-Security type to use |
There was a problem hiding this comment.
@keshav-725 Just a minor, the return parameter is wrong here
| import java.util.ServiceLoader; | ||
|
|
||
| /** | ||
| * Http Client side Authentication using java 11 HttpClient |
There was a problem hiding this comment.
The HttpClient implementation might have been introduced in java 11 but it is available in later java as well so we should not name it java 11 HttpClient here. Something like Elytron client for HTTP authentication might describe this class better.
| return "Basic " + Base64.getEncoder().encodeToString((username + ":" + password).getBytes()); | ||
| } | ||
| public HttpResponse<String> connect(String uri) throws Exception{ | ||
| HttpClient client = HttpClient.newHttpClient(); |
There was a problem hiding this comment.
@keshav-725 We should not create a new instance in the connect method as it will be inefficient to create new instances with every use of connect. We should rather instantiate it in the constructor of ElytronHttpClient
| return response; | ||
| } | ||
| public HttpRequest getRequest(String uri) throws Exception{ | ||
| Iterator<ClientConfigProvider> serviceLoaderIterator = ServiceLoader.load(ClientConfigProvider.class).iterator(); |
There was a problem hiding this comment.
@keshav-725 the ServiceLoader is used to load implementations of the interface form classpath. This can be handy if we want to provide a possibility to extend / change the implementation of interface, we do not know who will provide the implementation or if we do not want to introduce a dependency on each provider. You can read about SPIs and ServiceLoader here.
For our elytron HTTP client, only we will provide an implementation of the methods getUsername, getPassword and getHttpAuthenticationType. While it might be useful for someone to plug in their own ways to obtain credentials and autentication type, for now we plan for users to use Elytron's client configuration with ElytronHttpClient. So the credentias are planned to always be obtained from current AuthenticationContext or wildfly-config.xml file. So there is no need to introduce service loading
| * | ||
| * @author <a href="mailto:kekumar@redhat.com">Keshav Kumar</a> | ||
| */ | ||
| @MetaInfServices(value = ClientConfigProvider.class) |
There was a problem hiding this comment.
this annotation can be removed when we remove the service loading and instead we call the methods directly
|
Hi @Skyllarr ,Thank you for reviewing the PR.I have updated the required changes,please have a look. |
| package org.wildfly.security.http.client; | ||
|
|
||
| import org.wildfly.security.http.client.hpi.ClientConfigProvider; | ||
| import org.wildfly.security.http.client.hpi.HttpMechClientConfigProviderImpl; |
There was a problem hiding this comment.
@keshav-725 just a minor, what does hpi stand for?
There was a problem hiding this comment.
I have used short of http-client provider implementation for hpi.As we are not using it as provider so I will update it in the next commit.
| * Interface to enable configuration of authentication properties on the client side. | ||
| * @author kekumar@redhat.com | ||
| */ | ||
| public interface ClientConfigProvider { |
There was a problem hiding this comment.
@keshav-725 Just a minor, since this is not a java provider, we should rename this class and HttpMechClientConfigProviderImpl class. We can also make these private
| @@ -0,0 +1,35 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
Just a minor, we should name this file wildfly-config-http-client-basic since we will add more files like this with different auth mechanisms to the tests later
| </xsd:documentation> | ||
| </xsd:annotation> | ||
| </xsd:element> | ||
| <xsd:element name="http-mechanism-selector" type="name-type" minOccurs="0"> |
There was a problem hiding this comment.
@keshav-725 Just a minor, user might want to allow both DIGEST and BEARER for example. So we might want to configure multiple HTTP mechanisms here, so the name-type might not be the best here, maybe selector-type is better.
Then the mechanism chosen by the client will be the mechanism that is both in the selector and that is also specified in the server's challenge
There was a problem hiding this comment.
Just to make the new changes to the Elytron Client schema more clear, it's good practice to have one commit that just adds a new schema file for 1.8 that is identical to the previous version but just has the version changed. That commit should reference ELY-2575.
Then a subsequent commit can include the new elements/attributes your changes are adding. That can reference [ELY-201].
keshav-725
force-pushed
the
http-client
branch
2 times, most recently
from
93754d9
to
045b597
Compare
| HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString()); | ||
|
|
||
| if (response.statusCode() == 401) { | ||
| String authHeader = response.headers().allValues("www-authenticate").get(0); |
There was a problem hiding this comment.
@keshav-725 Just a minor, we should create a constant out of the www-authenticate header and out of response codes like 401, 403.
Is the response.headers().allValues case insensitive? The response "WWW-Authenticate" is a common capitalisation so we could iterate over header values and find those headers taht equalsIgnoreCase www-authenticate
There was a problem hiding this comment.
@keshav-725 This was not addressed yet
| resp = computeDigestWithQop(path, nonce, "0a4f113b", "00000001", userName, password, algorithm, realm, qop, "GET"); | ||
| } | ||
|
|
||
| HttpRequest request2 = HttpRequest |
There was a problem hiding this comment.
@keshav-725 Just a minor, can this be renamed to response?
| return finalHash; | ||
| } | ||
|
|
||
| private static String calculateMD5(String value) { |
There was a problem hiding this comment.
@keshav-725 Just a minor comment, we could create new class like DigestHttpMechanism that will encompass all this util methods that are used specifically with digest mechanism
| "algorithm=" + algorithm) | ||
| .build(); | ||
| int updateNonceCount = Integer.parseInt(authParams.getOrDefault("ncount", "0")) + 1; | ||
| authParams.put("ncount", String.valueOf(updateNonceCount)); |
There was a problem hiding this comment.
@keshav-725 Just a minor, does "ncount" parameter exists or is this a typo and it should be named "nc" instead?
There was a problem hiding this comment.
@Skyllarr ,This is not a type.I have used this variable to store in hashmap only.while adding to request header I put the varable name as "nc" only.
| HttpRequest request = HttpRequest | ||
| .newBuilder() | ||
| .uri(new URI(uri)) | ||
| .header("Authorization", AuthHeader) |
There was a problem hiding this comment.
Just a minor, make a constant out of Authorization header
| callbackHandler.handle(new Callback[]{nameCallback}); | ||
| return nameCallback.getName(); | ||
| } catch (IOException | UnsupportedCallbackException e) { | ||
| throw new ElytronHttpClientException("Name call back handle unsucessfull"); |
There was a problem hiding this comment.
@keshav-725 Just a minor, we can create ElytronMessages class and put the specific messages there. So this would result in something like new ElytronHttpClientException(ElytronMessages.log.abcReason())
| } | ||
|
|
||
| @Test | ||
| public void testRequest2() throws Exception{ |
There was a problem hiding this comment.
Just a total minor, we can rename these tests so the name expresses what is being tested
| mechanism.evaluateRequest(request1); | ||
| TestingHttpServerResponse response = request1.getResponse(); | ||
| HttpRequest request2 = elytronHttpClient.getResponseHeader(response.getAuthenticateHeader()); | ||
| System.out.println(request2.headers()); |
There was a problem hiding this comment.
Just a minor, we should not use system.out.println statements in the resulting code
| throw new RuntimeException(e); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
@keshav-725 Just a minor, we should add more tests here that tests the same URL twice to make sure nc works well, and test responses that contain a status of 403, 404 and other edge cases
There was a problem hiding this comment.
Hi @Skyllarr,I have added these kind of testcases in wildfly repo.
There was a problem hiding this comment.
@keshav-725 Similar tests will need to be added here as well, so that in the future the failures in these tests can be discovered early
| String qop = authParams.get("qop"); | ||
|
|
||
| String path = "/test"; | ||
| String uri = "http://localhost:8080" + path; |
There was a problem hiding this comment.
@keshav-725 Just a minor, the address should not be hard coded here. It is better to send the URL into this method
|
@keshav-725 Just a minor, there is an unrelated commit in this PR for ELY-2487 |
There was a problem hiding this comment.
Hi @keshav-725 , thank you for the updates! I have added some more feedback
| HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString()); | ||
|
|
||
| if (response.statusCode() == 401) { | ||
| String authHeader = response.headers().allValues("www-authenticate").get(0); |
There was a problem hiding this comment.
@keshav-725 This was not addressed yet
| } | ||
|
|
||
| //To test header values from ElytronHttpClientTest | ||
| public HttpRequest getResponseHeader(String responseHeader, String uri) throws Exception { |
There was a problem hiding this comment.
@keshav-725 This method can be made package private.
There was a problem hiding this comment.
@Skyllarr ,We cannot make this method private,as we are calling this metod from our test class
There was a problem hiding this comment.
@keshav-725 this method is in the same package as the test method so it can be made package private
| return response; | ||
| } | ||
|
|
||
| //To test header values from ElytronHttpClientTest |
There was a problem hiding this comment.
@keshav-725 This comment should be removed and the javadoc describing the method should be added.
| } | ||
|
|
||
| public HttpResponse evaluateBasicMechanism(String uri) throws Exception { | ||
| HttpRequest request = HttpRequest |
There was a problem hiding this comment.
@keshav-725 Just a minor, if we leave each mechanism in this class this class will be too long. Please make a new class for each mechanism.
| response = evaluateBasicMechanism(uri); | ||
| break; | ||
| case "digest": | ||
| response = evaluateDigestMechanism(uri); |
There was a problem hiding this comment.
@keshav-725 Just a minor, it is better to create a new package and a class for each mechanism that will have evaluateMessage method for example. Take a look at the SaslClient classes for inspiration.
| return response; | ||
| } | ||
|
|
||
| public HttpResponse connect(String uri) throws Exception { |
There was a problem hiding this comment.
@keshav-725 Just a minor, add javadoc for each public method
There was a problem hiding this comment.
Hi @keshav-725 , thank you for updates! Looks good! I added some initial comments and will continue my review later today or tomorrow
| public ElytronHttpClient() { | ||
| this.httpClient = HttpClient.newHttpClient(); | ||
| this.httpMechClientConfigUtil = new HttpMechClientConfigUtil(); | ||
| digestHttpMechanismUtil = new DigestHttpMechanismUtil(); |
There was a problem hiding this comment.
@keshav-725 Just a minor, the digestHttpMechanismUtil and httpMechClientConfigUtil are not used anywhere
| request = elytronHttpClientAuthMechanism.evaluateMechanism(uriPath); | ||
| } else if (authHeader.toLowerCase().startsWith("digest")) { | ||
| elytronHttpClientAuthMechanism = new ElytronHttpClientDigestAuthMechanism(authHeader); | ||
| request = elytronHttpClientAuthMechanism.evaluateMechanism(uriPath); |
There was a problem hiding this comment.
@keshav-725 Just a minor, you can take the line request = elytronHttpClientAuthMechanism.evaluateMechanism(uriPath); here and above and put if after the if-else
| return response; | ||
| } | ||
|
|
||
| String authHeader = null; |
There was a problem hiding this comment.
@keshav-725 Just a minor, we can extract some method like getAuthHeader(response); here
| /** | ||
| * Used to connect to the secured uri and return the response based on that. | ||
| */ | ||
| public HttpResponse connect(String uri) throws Exception { |
There was a problem hiding this comment.
@keshav-725 just a minor, can we replace the Exception with more specific exception types?
| /** | ||
| * Add required header to request object based on authentication type and return it. | ||
| */ | ||
| HttpRequest evaluateMechanism(URI uri) throws Exception; |
There was a problem hiding this comment.
@keshav-725 just a minor, you should make the exception more specific
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.wildfly.security.http.client.utils; |
There was a problem hiding this comment.
@keshav-725 just a minor, you can move this class to the org.wildfly.security.http.client.mechanism.digest.util subpackage
|
|
||
| public ElytronHttpClientDigestAuthMechanism(String authHeader){ | ||
| this.authHeader = authHeader; | ||
| this.digestHttpMechanismUtil = new DigestHttpMechanismUtil(); |
There was a problem hiding this comment.
Just a minor, can DigestHttpMechanismUtil class be made into a static class so we don't have to instantiate it?
There was a problem hiding this comment.
@keshav-725 was this addressed?
| * | ||
| * @author <a href="mailto:kekumar@redhat.com">Keshav Kumar</a> | ||
| */ | ||
| public class ElytronHttpClientCredentialProvider { |
There was a problem hiding this comment.
@keshav-725 Just a minor, usually the name Provider is used to mean java security provider. This is not a java security provider nor a java service provider, so I would rename it to eg. ElytronHttpClientCredentialUtils
| throw new ElytronHttpClientException(ElytronMessages.log.responseHeaderExtractionFailed()); | ||
| } | ||
|
|
||
| if (authHeader.toLowerCase().startsWith("basic")) { |
There was a problem hiding this comment.
@keshav-725 just a minor, for readability it is better to parse the string before the first space and then use switch here instead of if-elseif
| if (authHeader.toLowerCase().startsWith("basic")) { | ||
| elytronHttpClientAuthMechanism = new ElytronHttpClientBasicAuthMechanism(); | ||
| } else if (authHeader.toLowerCase().startsWith("digest")) { | ||
| elytronHttpClientAuthMechanism = new ElytronHttpClientDigestAuthMechanism(authHeader); |
There was a problem hiding this comment.
it does not seem like a good design that for each different authHeader we would have to create a new instance of ElytronHttpClientDigestAuthMechanism.
One better solution that comes to mind is for evaluatioNemchanism to require HttpResponse as a parameter instead of uri. The HttpResponse will contain all needed info.
|
|
||
| public ElytronHttpClientDigestAuthMechanism(String authHeader){ | ||
| this.authHeader = authHeader; | ||
| this.digestHttpMechanismUtil = new DigestHttpMechanismUtil(); |
There was a problem hiding this comment.
@keshav-725 was this addressed?
| private static final String AUTHORIZATION = "Authorization"; | ||
| private static final String CHALLENGE_PREFIX = "Digest "; | ||
|
|
||
| public static HttpRequest createDigestRequest(URI uri, String userName, String password,String authHeader) throws AuthenticationMechanismException { |
There was a problem hiding this comment.
Just a minor, missing space before String authHeader
| public HttpRequest evaluateMechanism(URI uri) throws AuthenticationMechanismException { | ||
| String userName = elytronHttpClientCredentialProvider.getUserName(uri); | ||
| String password = elytronHttpClientCredentialProvider.getPassword(uri); | ||
| return DigestHttpMechanismUtil.createDigestRequest(uri,userName,password,authHeader); |
There was a problem hiding this comment.
Just a minor, missing spaces after ,
| requestAuthHeader.append("realm=\"").append(realm).append("\", "); | ||
| requestAuthHeader.append("nonce=\"").append(nonce).append("\", "); | ||
| requestAuthHeader.append("uri=\"").append(uriPath).append("\", "); | ||
| requestAuthHeader.append("qop=\"").append(qop).append("\", "); |
There was a problem hiding this comment.
@keshav-725 What if some of these values are null?
You can take a look at DigestSaslClient to see how null values are taken care of. For example here it uses a default value "auth" for qop
| String qop = authParams.getOrDefault("qop", null); | ||
| String uriPath = getUriPath(uri); | ||
| String cnonce = generateCNonce(); | ||
| String nc = String.format("%08x", Integer.parseInt(authParams.getOrDefault("nc", String.valueOf(0)))); |
There was a problem hiding this comment.
We already use org.wildfly.security.mechanism.digest.DigestUtil class , so we can take its utilities like convertToHexBytesWithLeftPadding for nc here to be consistent
| private static String computeDigestWithQop(String uri, String nonce, String cnonce, String nc, String | ||
| username, String password, String algorithm, String realm, String qop, String method) { | ||
|
|
||
| System.out.println("uri : " + uri + " nonce " + nonce + " cnonce " + cnonce + " nc " + nc + " username " + username + " password " + password + " realm " + realm + " qop " + qop + " method " + method); |
There was a problem hiding this comment.
Remove the System.out.println from the classes
| return finalHash; | ||
| } | ||
|
|
||
| private static String computeDigestWithQop(String uri, String nonce, String cnonce, String nc, String |
There was a problem hiding this comment.
@keshav-725 Can DigestUtil class be used here as well? We already have sasl client computing this? If so, can these methods be reused for http as well?
Or can we refactor the classes so that both sasldigestclient and httpdigestclient use the same methods?
There was a problem hiding this comment.
Maybe moving the methods that both the SaslClient and HttpClient need to org.wildfly.security.mechanism.digest.DigestUtil makes sense? If this can be done, it shoul be done in a separate commit
http/client/pom.xml
Outdated
| </dependency> | ||
| <dependency> | ||
| <groupId>org.wildfly.security</groupId> | ||
| <artifactId>wildfly-elytron-digest</artifactId> |
There was a problem hiding this comment.
I think this is being used only in tests. If so, add a scope test here and to the rest of the modules that are used only in tests
http/client/pom.xml
Outdated
| </dependency> | ||
| <dependency> | ||
| <groupId>org.wildfly.security</groupId> | ||
| <artifactId>wildfly-elytron-http-basic</artifactId> |
There was a problem hiding this comment.
probably scope test can be added here
http/client/pom.xml
Outdated
| </dependency> | ||
| <dependency> | ||
| <groupId>org.wildfly.security</groupId> | ||
| <artifactId>wildfly-elytron-sasl-digest</artifactId> |
There was a problem hiding this comment.
@keshav-725 We should not use sasl digest dependency in HTTP client
There was a problem hiding this comment.
@keshav-725 this comment was not addressed
There was a problem hiding this comment.
Can we refactor what we need from the sasl digest into another module, wildfly-elytron-mechanism-digest or wildfly-elytron-digest ?
| @@ -1243,6 +1244,13 @@ static void parseAuthenticationConfigurationType(ConfigurationXMLStreamReader re | |||
| configuration = andThenOp(configuration, parentConfig -> parentConfig.useWebServices(webServices)); | |||
| break; | |||
| } | |||
| case "http-mechanism-selector": { | |||
| if (isSet(foundBits, 15) || !xmlVersion.isAtLeast(Version.VERSION_1_8)) throw reader.unexpectedElement(); | |||
There was a problem hiding this comment.
@keshav-725 Since we are not adding an HTTP selector at this time, please remove the selector related code from this PR for now
| return httpClient.send(request, HttpResponse.BodyHandlers.ofString()); | ||
| } | ||
|
|
||
| private void addSSLContextToHttpCient(URI uri){ |
There was a problem hiding this comment.
typo, should be addSSLContextToHttpClient
| httpClient = HttpClient.newBuilder().sslContext(sslContext).build(); | ||
| } | ||
| private HttpRequest evaluateMechanism(URI uri) { | ||
| HttpRequest request = HttpRequest |
There was a problem hiding this comment.
@keshav-725 This method should be renamed as it does not evaluate mechanism, it only builds a request
| public HttpResponse connect(String uri) throws IOException, InterruptedException, URISyntaxException { | ||
|
|
||
| URI uriPath = new URI(uri); | ||
|
|
There was a problem hiding this comment.
just a minor, superfluous new line
| */ | ||
| public class ElytronHttpClientCredentialUtils { | ||
|
|
||
| private HttpMechClientConfigUtil httpMechClientConfigUtil = new HttpMechClientConfigUtil(); |
There was a problem hiding this comment.
@keshav-725 Can the methods of HttpMechClientConfigUtil be made static so we dont need the new instance here?
tests/base/pom.xml
Outdated
| @@ -380,6 +384,10 @@ | |||
| <groupId>org.wildfly.security</groupId> | |||
| <artifactId>wildfly-elytron-credential-store</artifactId> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.wildfly.security</groupId> | |||
| <artifactId>wildfly-elytron-asn1</artifactId> | |||
There was a problem hiding this comment.
@keshav-725 Just wondering why the asn1 was added here?
There was a problem hiding this comment.
Hi @Skyllarr , we are using convertToHexBytesWithLeftPadding method so we need this dependency
.../org/wildfly/security/http/client/mechanism/bearer/ElytronHttpClientBearerAuthMechanism.java
Outdated
Show resolved
Hide resolved
|
|
||
| public class ElytronHttpClientBearerAuthMechanism { | ||
|
|
||
| private static final String AUTHORIZATION = "Authorization"; |
There was a problem hiding this comment.
@keshav-725 We use the AUTHORIZATION constant in all mechanisms. You should you create some constant that will be shared between all
| public class ElytronHttpClientBearerAuthMechanism { | ||
|
|
||
| private static final String AUTHORIZATION = "Authorization"; | ||
| private static ElytronHttpClientCredentialUtils elytronHttpClientCredentialProvider = new ElytronHttpClientCredentialUtils(); |
There was a problem hiding this comment.
I think we can make the methods in ElytronHttpClientCredentialUtils static and do not have to create new instance here and in other mechanisms
| } | ||
| private HttpRequest evaluateMechanism(URI uri) { | ||
| HttpRequest request = HttpRequest | ||
| .newBuilder() |
There was a problem hiding this comment.
@keshav-725 The request we are sending is always a GET request ( https://docs.oracle.com/en/java/javase/11/docs/api/java.net.http/java/net/http/HttpRequest.Builder.html ) . We have to think about how a user of this API will be able to send a POST or PUT requests with some body as well, and add a needed methods for that.
To achieve this, we will probably have to add a new method, for example send(HttpRequest request) where we allow a user of this API to configure a POST request with a body and some headers.
Then the ElytronHttpClient will add a needed authentication information but the rest of the request (request body etc) will be in the hands of the user
...ain/java/org/wildfly/security/http/client/mechanism/digest/util/DigestHttpMechanismUtil.java
Outdated
Show resolved
Hide resolved
http/client/src/main/java/org/wildfly/security/http/client/ElytronHttpClient.java
Outdated
Show resolved
Hide resolved
http/client/src/main/java/org/wildfly/security/http/client/ElytronHttpClient.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| public static String getHttpAuthenticationType(URI uri) throws ElytronHttpClientException { |
...se/src/test/resources/org/wildfly/security/http/client/wildfly-config-http-client-bearer.xml
Outdated
Show resolved
Hide resolved
| /** | ||
| * Used to connect to the secured uri and return the response based on that. | ||
| */ | ||
| public HttpResponse connect(String uri, String method, String body, Map<String, String> headers) throws IOException, InterruptedException, URISyntaxException { |
There was a problem hiding this comment.
@keshav-725 Just a minor, I commented this on the proposal but will add a comment here as well. Adding a builder is more convenient than using so many parameters, and for parameters that are not provided a default value should be used.
http/client/src/main/java/org/wildfly/security/http/client/ElytronHttpClient.java
Outdated
Show resolved
Hide resolved
http/client/src/main/java/org/wildfly/security/http/client/ElytronHttpClient.java
Outdated
Show resolved
Hide resolved
http/client/src/main/java/org/wildfly/security/http/client/ElytronHttpClient.java
Outdated
Show resolved
Hide resolved
|
Hi @keshav-725 , looks good, I added some comments. Another thing you can do is squashing some of the commits together if they are related and it makes sense to do it. For example the "added bearer token for elytron http client" and "added bearer authentication mechanism" should probably be squashed together. Also you can squash the test commits to the commits that added the functionality that the test commits are testing. Also make sure to add an ELY issue number on the beginning of all commits. Thank you! |
...ain/java/org/wildfly/security/http/client/mechanism/digest/util/DigestHttpMechanismUtil.java
Outdated
Show resolved
Hide resolved
...ain/java/org/wildfly/security/http/client/mechanism/digest/util/DigestHttpMechanismUtil.java
Outdated
Show resolved
Hide resolved
| --> | ||
|
|
||
| <xsd:schema xmlns:xsd="http://www.w3.org/2001/XMLSchema" | ||
| targetNamespace="urn:elytron:client:1.8" |
There was a problem hiding this comment.
I've created https://issues.redhat.com/browse/ELY-2575 to track adding version 1.8 of the Elytron Client schema. Please update this commit to reference [ELY-2575]. Thanks!
| </xsd:documentation> | ||
| </xsd:annotation> | ||
| </xsd:element> | ||
| <xsd:element name="http-mechanism-selector" type="name-type" minOccurs="0"> |
There was a problem hiding this comment.
Just to make the new changes to the Elytron Client schema more clear, it's good practice to have one commit that just adds a new schema file for 1.8 that is identical to the previous version but just has the version changed. That commit should reference ELY-2575.
Then a subsequent commit can include the new elements/attributes your changes are adding. That can reference [ELY-201].
http/client/pom.xml
Outdated
| <parent> | ||
| <groupId>org.wildfly.security</groupId> | ||
| <artifactId>wildfly-elytron-parent</artifactId> | ||
| <version>1.20.4.CR1-SNAPSHOT</version> |
There was a problem hiding this comment.
Note that since the base branch was updated to 2.x, the version here will need to be updated too.
| @@ -169,8 +170,7 @@ public void evaluateRequest(final HttpServerRequest request) throws HttpAuthenti | |||
| } else { | |||
| httpBasic.debugf("User %s authorization failed.", username); | |||
| fail(); | |||
|
|
|||
| request.authenticationFailed(httpBasic.authorizationFailed(username), response -> prepareResponse(request, displayRealmName, response)); | |||
| request.authenticationFailed(httpBasic.authorizationFailed(username), response -> response.setStatusCode(HttpConstants.FORBIDDEN)); | |||
There was a problem hiding this comment.
@keshav-725 Just a minor, the commit that adds this change should be the same (have a same commit hash) like the affiliated PR's commit - #1899 . You can do that by rebasing this on that PR's branch or by cherry-picking that commit in this branch.
| try { | ||
| String uri = "https://localhost:10001"; | ||
| ElytronHttpClient.ElytronHttpHeaderBuilder builder = new ElytronHttpClient.ElytronHttpHeaderBuilder(uri); | ||
| HttpResponse httpResponse = builder.connect(); |
There was a problem hiding this comment.
@keshav-725 Just some thought on this. Previously, I was thinking of this:
ElytronHttpClientBuilder clientBuilder = new ElytronHttpClientBuilder();
ElytronHttpClient client = clientBuilder.url(myUrl)
.addHeader(myHeader)
.build();
client.connect();
But seeing this written out it is not a good idea to have request details configured for the client instance. It is better if the request details are detached from the client configuration. So the client can make requests to different endpoints without reconfiguration.
So I propose this usage:
ElytronHttpClient client = new ElytronHttpClient();
HttpRequest httpRequest = HttpRequestBuilder.url(myUrl)
.addHeader(myHeader)
.addMehtod(POST)
.build();
client.connect(httpRequest);
But since HttpRequestBuilder already exists in java https://docs.oracle.com/en/java/javase/11/docs/api/java.net.http/java/net/http/HttpRequest.Builder.html we do not have to introduce a new builder but use the existing one and so allow for the connect method to accept HttpRequest parameter as I mentioned on the proposal before:
public HttpResponse connect(HttpRequest httpRequest) {
...
}
It can then be used like this:
ElytronHttpClient client = new ElytronHttpClient();
HttpRequest request = HttpRequest.newBuilder()
.uri(new URI("myURI"))
.POST()
.header(myKey,myValue)
.build();
client.connect(request)
Or we can later introduce our new wrapper for the HttpRequest, like ElytronHttpRequest that can be used with clients that do not use Java's HttpRequest if this will be needed. But this can be introduced later if needed along with the changes that should be added to make the ElytronHttpClient more generic and possible to use with other clients than just java's HttpClient.
Let me know if this makes sense and what you think? Thanks!
There was a problem hiding this comment.
@keshav-725 Hi, took a quick look and added minor comments, looks good, and the connect method looks good to me now, thank you for updates! Don't worry about addressing all these comments now, it is better to clean up the commits of this PR first
|
|
||
| switch (mechanismType) { | ||
| case "basic": | ||
| authRequest = ElytronHttpClientBasicAuthMechanism.evaluateMechanism(httpRequest); |
There was a problem hiding this comment.
@keshav-725 Just a minor, we can create an interface for all client HTTP mechanisms that will contain an evaluateMechanism method
|
|
||
| public static HttpRequest evaluateMechanism(HttpRequest httpRequest) { | ||
| String userName = elytronHttpClientCredentialProvider.getUserName(httpRequest.uri()); | ||
| String password = elytronHttpClientCredentialProvider.getPassword(httpRequest.uri()); |
There was a problem hiding this comment.
@keshav-725 Just a minor, getUsername and getPassword are static so they don't require an instance to access these methods,you can remove the elytronHttpClientCredentialProvider from this class
| private static String generateHash(String value, String algorithm) { | ||
| try { | ||
| MessageDigest messageDigest; | ||
| if (algorithm.equals("SHA-512-256")) { |
There was a problem hiding this comment.
@keshav-725 Just aminor, why is the SHA-512-256 treated differently here than other algorithms? can we used the WildFlyElytronDigestProvider for all algorithms instead?
| public class ElytronHttpClientRequestBuilder { | ||
| public static HttpRequest buildRequest(HttpRequest httpRequest, Map<String, String> headers){ | ||
| String body = null; | ||
| if(httpRequest.bodyPublisher() != null){ |
There was a problem hiding this comment.
@keshav-725 just a minor, you can use httpRequest.bodyPublisher().isPresent() instead
| } | ||
| HttpRequest authRequest = null; | ||
|
|
||
| String challenge = getFirstChallenge(authenticateHeader); |
There was a problem hiding this comment.
just a total minor, you can remove the challenge variable to make this method a bit simpler and use
mechanismType = getMechanismType(getFirstChallenge(authenticateHeader))
| } | ||
| } | ||
|
|
||
| if (headers != null) { |
There was a problem hiding this comment.
@keshav-725 just a minor, should this be if (httpHeaders != null) ?
|
@keshav-725 just a minor, there is a conflicting file so this needs a rebase, also changing of the pom.xml's version like Farah mentioned in the previous comment |
|
I'm going to close this one for now until we are ready to pick up working on it again. |
No description provided.