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

Improve checkApiUrlValidity() method #251

Merged
merged 1 commit into from
Mar 12, 2016
Merged

Improve checkApiUrlValidity() method #251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 29 additions & 1 deletion src/main/java/org/kohsuke/github/GitHub.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.Reader;
import java.net.HttpURLConnection;
import java.net.MalformedURLException;
import java.net.URL;
import java.text.ParseException;
Expand Down Expand Up @@ -482,7 +483,34 @@ void check(String apiUrl) throws IOException {
* Otherwise this method throws {@link IOException} to indicate the problem.
*/
public void checkApiUrlValidity() throws IOException {
retrieve().to("/", GHApiInfo.class).check(apiUrl);
try {
retrieve().to("/", GHApiInfo.class).check(apiUrl);
} catch (IOException ioe) {
if (isPrivateModeEnabled()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not convinced this check is enough. There may be other errors (e.g. network failure), but this check suppresses all other issues. Maybe you should throw a new exception with the cause and a message, which says that it maybe related to the private mode

Copy link
Contributor

Choose a reason for hiding this comment

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

Same feeling here, I would prefer a logic based on the HTTP error code (401 for private mode, http proxy auth...) and on the Network exception (connection refused, ssl handshake rejected for negotiation failure...). I am investigating to make proposals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The method basically is the same but now we have included a new case.

I tested with:

  1. https://api.github.com/
  2. http://github.mycompany.com/api/v3/
  3. https://github.mycompany.com/api/v3/ (self-signed certificate)
  4. https://github.mycompany.com/api/v3/ (certificate)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A Network exception throws a IOException and turns out an negative validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oleg-nenashev

but this check suppresses all other issues

Why? How? I'm only handling a special case. If private mode is not configured and there is an problem, an exception is thrown.

Maybe you should throw a new exception with the cause and a message, which says that it maybe related to the private mode

The exception is thrown with a customized message (IMO, very clear). The doubt can be about the way used to verify the privade mode.

throw new IOException("GitHub Enterprise server (" + apiUrl + ") with private mode enabled");
}
throw ioe;
}
}

/**
* Ensures if a GitHub Enterprise server is configured in private mode.
*
* @return {@code true} if private mode is enabled. If it tries to use this method with GitHub, returns {@code
* false}.
*/
private boolean isPrivateModeEnabled() {
try {
HttpURLConnection connect = getConnector().connect(getApiURL("/"));
if (connect.getResponseCode() == HttpURLConnection.HTTP_UNAUTHORIZED
&& connect.getHeaderField("Server") != null
&& connect.getHeaderField("Server").equals("GitHub.com")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This header is suspicious for GitHub Enterprise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oleg-nenashev No, they use this header for both.

github.com

pegaso:~ recena$ curl -i https://api.github.com/
HTTP/1.1 200 OK
Server: GitHub.com
Date: Sat, 05 Mar 2016 19:43:33 GMT
Content-Type: application/json; charset=utf-8
Content-Length: 2064
Status: 200 OK
X-RateLimit-Limit: 60
X-RateLimit-Remaining: 59
X-RateLimit-Reset: 1457210613
Cache-Control: public, max-age=60, s-maxage=60
Vary: Accept
ETag: "d251d84fc3f78921c16c7f9c99d74eae"
X-GitHub-Media-Type: github.v3
Access-Control-Expose-Headers: ETag, Link, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval
Access-Control-Allow-Origin: *
Content-Security-Policy: default-src 'none'
Strict-Transport-Security: max-age=31536000; includeSubdomains; preload
X-Content-Type-Options: nosniff
X-Frame-Options: deny
X-XSS-Protection: 1; mode=block
Vary: Accept-Encoding
X-Served-By: 01d096e6cfe28f8aea352e988c332cd3
X-GitHub-Request-Id: 5B7E4817:13E78:5AE0363:56DB36E5

My GitHub Enterprise server

pegaso:~ recena$ curl -i https://github.mycompany.com/api/v3/
HTTP/1.1 401 Unauthorized
Server: GitHub.com
Date: Sat, 05 Mar 2016 19:45:01 GMT
Content-Type: application/json; charset=utf-8
Content-Length: 130
Status: 401 Unauthorized
X-GitHub-Media-Type: github.v3
X-XSS-Protection: 1; mode=block
X-Frame-Options: deny
Content-Security-Policy: default-src 'none'
Access-Control-Allow-Credentials: true
Access-Control-Expose-Headers: ETag, Link, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval
Access-Control-Allow-Origin: *
X-GitHub-Request-Id: dbc70361-b11d-4131-9a7f-674b8edd0411
Strict-Transport-Security: max-age=31536000; includeSubdomains; preload
X-Content-Type-Options: nosniff

Copy link
Contributor

Choose a reason for hiding this comment

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

@recena sorry but I am not comfortable with this "Server" thing as reverse proxies could remove this header.
I would be happy with a simple exception message saying "401 Unauthorized".
I proposed #254 to surface the http error code, we could catch this 401 and say that there is an authentication exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cyrille-leclerc The goal of this PR was to provide a method to verify if a URL represents a GitHub endpoint, only that. A HttpException 401 Unauthorized does not involve that we have a valid GitHub endpoint.

return true;
}
return false;
} catch (IOException e) {
return false;
}
}

/**
Expand Down
7 changes: 6 additions & 1 deletion src/test/java/org/kohsuke/github/GitHubTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ public void testGitHubEnterpriseDoesNotHaveRateLimit() throws IOException {
@Test
public void testGitHubIsApiUrlValid() throws IOException {
GitHub github = GitHub.connectAnonymously();
github.checkApiUrlValidity();
//GitHub github = GitHub.connectToEnterpriseAnonymously("https://github.mycompany.com/api/v3/");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cyrille-leclerc You can use this test to play with the method and different scenarios.

try {
github.checkApiUrlValidity();
} catch (IOException ioe) {
assertTrue(ioe.getMessage().contains("private mode enabled"));
}
}
}