-
Notifications
You must be signed in to change notification settings - Fork 6
jp-fake-auth-fix #115
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.
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
Open
jpallas
wants to merge
3
commits into
master
Choose a base branch
from
jp-fake-auth-fix
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+252
−2
Open
jp-fake-auth-fix #115
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
234 changes: 234 additions & 0 deletions
234
src/test/java/com/github/susom/vertx/base/test/FakeAuthenticatorHttpClientTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,234 @@ | ||
| /* | ||
| * Copyright 2016 The Board of Trustees of The Leland Stanford Junior University. | ||
| * All Rights Reserved. | ||
| * | ||
| * See the NOTICE and LICENSE files distributed with this work for information | ||
| * regarding copyright ownership and licensing. You may not use this file except | ||
| * in compliance with a written license agreement with Stanford University. | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
| * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See your | ||
| * License for the specific language governing permissions and limitations under | ||
| * the License. | ||
| */ | ||
| package com.github.susom.vertx.base.test; | ||
|
|
||
| import com.github.susom.vertx.base.FakeAuthenticator; | ||
| import io.vertx.core.Vertx; | ||
| import io.vertx.core.http.HttpClient; | ||
| import io.vertx.core.http.HttpMethod; | ||
| import io.vertx.core.http.RequestOptions; | ||
| import io.vertx.ext.unit.Async; | ||
| import io.vertx.ext.unit.TestContext; | ||
| import io.vertx.ext.unit.junit.VertxUnitRunner; | ||
| import io.vertx.ext.web.Router; | ||
| import org.junit.After; | ||
| import org.junit.Before; | ||
| import org.junit.Test; | ||
| import org.junit.runner.RunWith; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import java.net.URI; | ||
| import java.security.SecureRandom; | ||
| import java.util.function.Function; | ||
|
|
||
| /** | ||
| * Test that FakeAuthenticator properly handles HTTP client requests with non-standard ports. | ||
| * This test verifies that the token endpoint URL is correctly parsed and the port is preserved. | ||
| * | ||
| * @author jpallas | ||
| */ | ||
| @RunWith(VertxUnitRunner.class) | ||
| public class FakeAuthenticatorHttpClientTest { | ||
| private static final Logger log = LoggerFactory.getLogger(FakeAuthenticatorHttpClientTest.class); | ||
| private Vertx vertx; | ||
| private HttpClient client; | ||
| private int port = 8899; // Non-standard port to test port handling | ||
|
|
||
| @Before | ||
| public void setUp(TestContext context) { | ||
| vertx = Vertx.vertx(); | ||
| client = vertx.createHttpClient(); | ||
| } | ||
|
|
||
| @After | ||
| public void tearDown(TestContext context) { | ||
| if (client != null) { | ||
| client.close(); | ||
| } | ||
| if (vertx != null) { | ||
| vertx.close(context.asyncAssertSuccess()); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Test that FakeAuthenticator's HTTP client properly connects to the token endpoint | ||
| * when a non-standard port is used. In Vert.x 4, the HttpClient.request() method | ||
| * requires proper URI parsing to extract host and port. | ||
| */ | ||
| @Test | ||
| public void testTokenEndpointWithNonStandardPort(TestContext context) throws Exception { | ||
| Async async = context.async(); | ||
|
|
||
| // Create a simple HTTP server on a non-standard port to act as the token endpoint | ||
| Router tokenServer = Router.router(vertx); | ||
|
|
||
| tokenServer.post("/fake-authentication/token").handler(rc -> { | ||
| log.info("Token endpoint received request on port {}", port); | ||
| rc.response() | ||
| .putHeader("content-type", "application/json") | ||
| .end("{\"sub\":\"testuser\",\"name\":\"Test User\",\"authority\":[\"test:read\"]}"); | ||
| }); | ||
|
|
||
| // Start the token server on the non-standard port | ||
| vertx.createHttpServer() | ||
| .requestHandler(tokenServer) | ||
| .listen(port, context.asyncAssertSuccess(server -> { | ||
| log.info("Token server started on port {}", port); | ||
|
|
||
| // Configure FakeAuthenticator to use our non-standard port | ||
| Function<String, String> config = key -> { | ||
| switch (key) { | ||
| case "root.url": | ||
| return "http://localhost:" + port; | ||
| case "public.url": | ||
| return "http://localhost:" + port; | ||
| case "context.path": | ||
| return ""; | ||
| case "insecure.log.full.requests": | ||
| return "false"; | ||
| default: | ||
| return null; | ||
| } | ||
| }; | ||
|
|
||
| Router appRouter = Router.router(vertx); | ||
| SecureRandom secureRandom = new SecureRandom(); | ||
|
|
||
| try { | ||
| FakeAuthenticator authenticator = new FakeAuthenticator(vertx, appRouter, secureRandom, config); | ||
| Router protectedRouter = authenticator.authenticatedRouter("/app"); | ||
|
|
||
| protectedRouter.get("/test").handler(rc -> { | ||
| rc.response().end("Protected resource"); | ||
| }); | ||
|
|
||
| // Start the application server on a different port | ||
| int appPort = port + 1; | ||
| vertx.createHttpServer() | ||
| .requestHandler(appRouter) | ||
| .listen(appPort, context.asyncAssertSuccess(appServer -> { | ||
| log.info("App server started on port {}", appPort); | ||
|
|
||
| // Simulate the OAuth callback flow | ||
| // First, get the auth page to establish a session | ||
| client.request(HttpMethod.GET, appPort, "localhost", "/app/test") | ||
| .compose(req -> req.send()) | ||
| .onSuccess(response1 -> { | ||
| context.assertEquals(401, response1.statusCode(), | ||
| "Expected 401 for unauthenticated request"); | ||
| log.info("Initial request returned 401 as expected"); | ||
|
|
||
| // Now simulate the callback with an auth code | ||
| // This will trigger the HTTP client to connect to the token endpoint | ||
| client.request(HttpMethod.GET, appPort, "localhost", | ||
| "/app/callback?code=testcode123&state=teststate") | ||
| .compose(req -> { | ||
| // Add the state cookie that would have been set by the auth flow | ||
| req.putHeader("Cookie", "state=teststate"); | ||
| return req.send(); | ||
| }) | ||
| .onSuccess(response2 -> { | ||
| log.info("Callback response status: {}", response2.statusCode()); | ||
| response2.bodyHandler(body -> { | ||
| log.info("Callback response body: {}", body.toString()); | ||
|
|
||
| // The test passes if we get a response (even if it's an error) | ||
| // as long as the server does not return a 5xx error. A 5xx | ||
| // status indicates the token endpoint could not be reached | ||
| // or processed correctly (including potential port issues). | ||
| if (response2.statusCode() >= 500 && response2.statusCode() < 600) { | ||
| context.fail("FakeAuthenticator failed while calling token endpoint on port " + port + | ||
| ". The HttpClient may not be properly parsing the port from the tokenUrl; " + | ||
| "status=" + response2.statusCode()); | ||
| } else if (response2.statusCode() == 302 || response2.statusCode() == 200) { | ||
| log.info("Test passed: Token endpoint was successfully reached"); | ||
| async.complete(); | ||
| } else { | ||
| // Some other non-5xx status code, but not a clear port issue | ||
| log.info("Got response code {} which indicates the port was reached", | ||
| response2.statusCode()); | ||
| async.complete(); | ||
| } | ||
| }); | ||
| }) | ||
| .onFailure(err -> { | ||
| log.error("Callback request failed", err); | ||
| context.fail("Failed to make callback request: " + err.getMessage()); | ||
| }); | ||
| }) | ||
| .onFailure(err -> { | ||
| log.error("Initial request failed", err); | ||
| context.fail("Failed to make initial request: " + err.getMessage()); | ||
| }); | ||
| })); | ||
| } catch (Exception e) { | ||
| log.error("Failed to create FakeAuthenticator", e); | ||
| context.fail(e); | ||
| } | ||
| })); | ||
| } | ||
|
|
||
| /** | ||
| * Test that demonstrates the fix for HttpClient port parsing. | ||
| * In Vert.x 4, passing a full URL string to HttpClient.request(method, url) | ||
| * doesn't properly parse the port. The solution is to use RequestOptions | ||
| * with explicit host, port, and URI. | ||
| */ | ||
| @Test | ||
| public void testHttpClientWithPortInUrl(TestContext context) { | ||
| Async async = context.async(); | ||
|
|
||
| // Create a simple server on a non-standard port | ||
| Router router = Router.router(vertx); | ||
| router.post("/endpoint").handler(rc -> { | ||
| log.info("Endpoint hit successfully"); | ||
| rc.response().end("Success"); | ||
| }); | ||
|
|
||
| vertx.createHttpServer() | ||
| .requestHandler(router) | ||
| .listen(port, context.asyncAssertSuccess(server -> { | ||
| log.info("Test server started on port {}", port); | ||
|
|
||
| // This is the FIX: Parse the URL and use RequestOptions with explicit host and port | ||
| String fullUrl = "http://localhost:" + port + "/endpoint"; | ||
| URI uri = URI.create(fullUrl); | ||
| RequestOptions requestOptions = new RequestOptions() | ||
| .setMethod(HttpMethod.POST) | ||
| .setHost(uri.getHost()) | ||
| .setPort(uri.getPort() != -1 ? uri.getPort() : (uri.getScheme().equals("https") ? 443 : 80)) | ||
| .setURI(uri.getPath() + (uri.getQuery() != null ? "?" + uri.getQuery() : "")) | ||
| .setSsl(uri.getScheme().equals("https")); | ||
|
|
||
| client.request(requestOptions) | ||
| .compose(req -> req.send("test")) | ||
| .onSuccess(response -> { | ||
| response.bodyHandler(body -> { | ||
| log.info("Response received: status={}, body={}", response.statusCode(), body.toString()); | ||
| context.assertEquals(200, response.statusCode(), | ||
| "HttpClient should successfully connect to the correct port when using RequestOptions"); | ||
| context.assertEquals("Success", body.toString()); | ||
| async.complete(); | ||
| }); | ||
| }) | ||
| .onFailure(err -> { | ||
| log.error("Request failed", err); | ||
| context.fail("HttpClient failed to connect: " + err.getMessage()); | ||
| }); | ||
| })); | ||
| } | ||
| } | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded port number may cause test failures if port 8899 is already in use. Consider using a dynamic port assignment (port 0) and retrieving the actual port from the server after it starts, which is a common pattern in Vert.x testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback