Skip to content

[JENKINS-48625] Restore binding of doCheckUrl methods and add some initial checks #841

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

Merged
MarkEWaite merged 20 commits into from
Mar 1, 2020
Merged

[JENKINS-48625] Restore binding of doCheckUrl methods and add some initial checks #841

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
2510bd3
=[JENKINS-56063] added expansion of env variables in refspec in case …
rishabhBudhouliya Jan 30, 2020
7d7f670
=removed the check flag and changed the implementation
rishabhBudhouliya Jan 31, 2020
58320c4
Changed minor indentation issues
rishabhBudhouliya Feb 1, 2020
503e0a3
[JENKINS-48625] Restore binding of doCheckUrl methods and add some in…
rishabhBudhouliya Feb 19, 2020
f4b0710
Merge branch 'master' into JENKINS-48625-Fix
MarkEWaite Feb 20, 2020
f99b832
Proposed AssemblaWebTest changes
MarkEWaite Feb 20, 2020
a062e0b
Clarify comments for my benefit
MarkEWaite Feb 20, 2020
c04b4a7
Extend null project test name to refer to null project
MarkEWaite Feb 20, 2020
3a8d549
Test with a random URL that meets criteria
MarkEWaite Feb 20, 2020
f298249
Fix compile error on use of Random
MarkEWaite Feb 20, 2020
7293a3f
initial checks and utility function is shifted to GitRepositoryBrowse…
rishabhBudhouliya Feb 20, 2020
141c79f
Changed access modifier for utilities in GitRepositoryBrowser
rishabhBudhouliya Feb 20, 2020
454397e
change in checkURIFormat
rishabhBudhouliya Feb 21, 2020
6532b34
Revert "change in checkURIFormat"
rishabhBudhouliya Feb 21, 2020
4a0915a
Change in checkURIFormat and addition of checkURIFormatAndHostname in…
rishabhBudhouliya Feb 21, 2020
5f48308
Removal of fixes of whitespace and temporary variable response
rishabhBudhouliya Feb 21, 2020
ac1beeb
Merge branch 'master' into JENKINS-48625-Fix
MarkEWaite Feb 28, 2020
8b1433c
Revert CloneOptions change from a different branch
MarkEWaite Feb 29, 2020
acebb46
Reduce whitespace differences from master branch
MarkEWaite Feb 29, 2020
d9fe71b
Better variable name in Assembla checker
MarkEWaite Mar 1, 2020
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
11 changes: 11 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,17 @@
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>commons-validator</groupId>
<artifactId>commons-validator</artifactId>
<version>1.6</version>
<exclusions>
<exclusion>
<groupId>commons-digester</groupId>
<artifactId>commons-digester</artifactId>
</exclusion>
</exclusions>
</dependency>
</dependencies>

<dependencyManagement>
Expand Down
54 changes: 31 additions & 23 deletions src/main/java/hudson/plugins/git/browser/AssemblaWeb.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
package hudson.plugins.git.browser;

import hudson.Extension;
import hudson.Util;
import hudson.model.Descriptor;
import hudson.model.Item;
import hudson.plugins.git.GitChangeSet;
import hudson.plugins.git.GitChangeSet.Path;
import hudson.plugins.git.Messages;
import hudson.scm.EditType;
import hudson.scm.RepositoryBrowser;
import hudson.util.FormValidation;
import hudson.util.FormValidation.URLCheck;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
import org.kohsuke.stapler.AncestorInPath;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.interceptor.RequirePOST;
import org.kohsuke.stapler.QueryParameter;
Expand All @@ -18,6 +21,7 @@
import javax.annotation.Nonnull;
import javax.servlet.ServletException;
import java.io.IOException;
import java.net.URISyntaxException;
import java.net.URL;

/**
Expand Down Expand Up @@ -94,33 +98,37 @@ public AssemblaWeb newInstance(StaplerRequest req, @Nonnull JSONObject jsonObjec
}

@RequirePOST
public FormValidation doCheckUrl(@QueryParameter(fixEmpty = true) final String url)
throws IOException, ServletException {
if (url == null) // nothing entered yet
{
public FormValidation doCheckRepoUrl(@AncestorInPath Item project, @QueryParameter(fixEmpty = true) final String repoUrl)
throws IOException, ServletException, URISyntaxException {

String cleanUrl = Util.fixEmptyAndTrim(repoUrl);
if (initialChecksAndReturnOk(project, cleanUrl)) {
return FormValidation.ok();
}
// Connect to URL and check content only if we have admin permission
if (!Jenkins.get().hasPermission(Jenkins.ADMINISTER))
return FormValidation.ok();
return new URLCheck() {
protected FormValidation check() throws IOException, ServletException {
String v = url;
if (!v.endsWith("/")) {
v += '/';
}
FormValidation response;
if (checkURIFormat(cleanUrl, "assembla")) {
return new URLCheck() {
protected FormValidation check() throws IOException, ServletException {
String v = cleanUrl;
if (!v.endsWith("/")) {
v += '/';
}

try {
if (findText(open(new URL(v)), "Assembla")) {
return FormValidation.ok();
} else {
return FormValidation.error("This is a valid URL but it doesn't look like Assembla");
try {
if (findText(open(new URL(v)), "Assembla")) {
return FormValidation.ok();
} else {
return FormValidation.error("This is a valid URL but it does not look like Assembla");
}
} catch (IOException e) {
return handleIOException(v, e);
}
} catch (IOException e) {
return handleIOException(v, e);
}
}
}.check();
}.check();
} else {
response = FormValidation.error(Messages.invalidUrl());
}
return response;
}
}
}
65 changes: 42 additions & 23 deletions src/main/java/hudson/plugins/git/browser/GitBlitRepositoryBrowser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
package hudson.plugins.git.browser;

import hudson.Extension;
import hudson.Util;
import hudson.model.Descriptor;
import hudson.model.Item;
import hudson.plugins.git.GitChangeSet;
import hudson.plugins.git.GitChangeSet.Path;
import hudson.plugins.git.Messages;
import hudson.scm.EditType;
import hudson.scm.RepositoryBrowser;
import hudson.util.FormValidation;
import hudson.util.FormValidation.URLCheck;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
import org.kohsuke.stapler.AncestorInPath;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.interceptor.RequirePOST;
import org.kohsuke.stapler.QueryParameter;
Expand All @@ -19,6 +22,7 @@
import javax.servlet.ServletException;
import java.io.IOException;
import java.io.UnsupportedEncodingException;
import java.net.URISyntaxException;
import java.net.URL;
import java.net.URLEncoder;

Expand Down Expand Up @@ -63,9 +67,10 @@ public String getProjectName() {
return projectName;
}

private String encodeString(final String s) throws UnsupportedEncodingException {
private String encodeString(final String s) throws UnsupportedEncodingException {
return URLEncoder.encode(s, "UTF-8").replaceAll("\\+", "%20");
}

@Extension
public static class ViewGitWebDescriptor extends Descriptor<RepositoryBrowser<?>> {
@Nonnull
Expand All @@ -80,33 +85,47 @@ public GitBlitRepositoryBrowser newInstance(StaplerRequest req, @Nonnull JSONObj
}

@RequirePOST
public FormValidation doCheckUrl(@QueryParameter(fixEmpty = true) final String url)
throws IOException, ServletException {
if (url == null) // nothing entered yet
{
public FormValidation doCheckRepoUrl(@AncestorInPath Item project, @QueryParameter(fixEmpty = true) final String repoUrl)
throws IOException, ServletException, URISyntaxException {

String cleanUrl = Util.fixEmptyAndTrim(repoUrl);

if (cleanUrl == null) {
return FormValidation.ok();
}
// Connect to URL and check content only if we have admin permission
if (!Jenkins.get().hasPermission(Jenkins.ADMINISTER))

if (project == null || !project.hasPermission(Item.CONFIGURE)) {
return FormValidation.ok();
return new URLCheck() {
protected FormValidation check() throws IOException, ServletException {
String v = url;
if (!v.endsWith("/")) {
v += '/';
}
}

try {
if (findText(open(new URL(v)), "Gitblit")) {
return FormValidation.ok();
} else {
return FormValidation.error("This is a valid URL but it doesn't look like Gitblit");
if (cleanUrl.contains("$")) {
// set by variable, can't validate
return FormValidation.ok();
}
FormValidation response;
if (checkURIFormat(cleanUrl, "gitblit")) {
Copy link
Contributor

@MarkEWaite MarkEWaite Feb 20, 2020

Choose a reason for hiding this comment

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

I don't think you can reasonably assume that a gitblit server must include gitblit in the hostname. Assembla only provides a hosted solution, so it is safe to check their specific hostname. Gitblit provides a solution that is either hosted or on premise.

Copy link
Contributor Author

@rishabhBudhouliya rishabhBudhouliya Feb 20, 2020

Choose a reason for hiding this comment

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

Okay, I never anticipated this obvious case. My implementation was based on this assumption that the hostname of the URL will contain the browser name (for ex: http://gitblit.example.com/).

As far as my understanding goes, either I can:

  • remove this assumption, but then would have to provide a custom implementation for Assembla, i.e making checkURIFormat an abstract method.
  • branch the logic into two cases, hosted check and on-premise check.

What do you think?

Copy link
Contributor

@MarkEWaite MarkEWaite Feb 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think that Assembla is the exception in this case. Most git hosting providers allow a hosted implementation and an implementation that is installed and maintained inside the user network.

I think it would be better to place checkURIFormat (or some more specific implementation of it) inside the Assembla specific class and then place something more general in the GitRepositoryBrowser class. The part of checkURIFormat that confirms the URL is an http or https URL is useful in all cases. The part that checks for a specific substring in the hostname is likely limited to Assembla and possibly a few others.

Copy link
Contributor Author

@rishabhBudhouliya rishabhBudhouliya Feb 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Most git hosting providers allow a hosted implementation and an implementation that is installed and maintained inside the user network.

This means that creating a simple host name check is not feasible, hence I have created a separate checkURIFormatAndHostName for AssemblaWeb since Assembla provides a hosted solution only. For others, it's just a plain UrlValidatior check. For reference, the commit is 4a0915a .

return new URLCheck() {
protected FormValidation check() throws IOException, ServletException {
String v = cleanUrl;
if (!v.endsWith("/")) {
v += '/';
}

try {
if (findText(open(new URL(v)), "Gitblit")) {
return FormValidation.ok();
} else {
return FormValidation.error("This is a valid URL but it doesn't look like Gitblit");
}
} catch (IOException e) {
return handleIOException(v, e);
}
} catch (IOException e) {
return handleIOException(v, e);
}
}
}.check();
}.check();
} else {
response = FormValidation.error(Messages.invalidUrl());
}
return response;
}
}
}
24 changes: 24 additions & 0 deletions src/main/java/hudson/plugins/git/browser/GitRepositoryBrowser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
package hudson.plugins.git.browser;

import hudson.EnvVars;
import hudson.model.Item;
import hudson.model.Job;
import hudson.model.TaskListener;
import hudson.plugins.git.GitChangeSet;
import hudson.plugins.git.GitChangeSet.Path;
import hudson.scm.RepositoryBrowser;

import org.apache.commons.validator.routines.UrlValidator;
import org.kohsuke.stapler.Stapler;
import org.kohsuke.stapler.StaplerRequest;

Expand Down Expand Up @@ -117,5 +119,27 @@ public static URL encodeURL(URL url) throws IOException {
}
}

public static boolean initialChecksAndReturnOk(Item project, String cleanUrl){
if (cleanUrl == null) {
return true;
}
if (project == null || !project.hasPermission(Item.CONFIGURE)) {
return true;
}
if (cleanUrl.contains("$")) {
// set by variable, can't validate
return true;
}
return false;
}

public static boolean checkURIFormat(String url, String browserName) throws URISyntaxException {
URI uri = new URI(url);
String[] schemes = {"http", "https"};
UrlValidator urlValidator = new UrlValidator(schemes);
browserName = browserName + ".";
return urlValidator.isValid(uri.toString()) && uri.getHost().contains(browserName);
}

private static final long serialVersionUID = 1L;
}
56 changes: 33 additions & 23 deletions src/main/java/hudson/plugins/git/browser/Gitiles.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,26 @@
package hudson.plugins.git.browser;

import hudson.Extension;
import hudson.Util;
import hudson.model.Descriptor;
import hudson.model.Item;
import hudson.plugins.git.GitChangeSet;
import hudson.plugins.git.GitChangeSet.Path;
import hudson.plugins.git.Messages;
import hudson.scm.RepositoryBrowser;
import hudson.util.FormValidation;
import hudson.util.FormValidation.URLCheck;

import jenkins.model.Jenkins;

import java.io.IOException;
import java.net.URISyntaxException;
import java.net.URL;

import javax.annotation.Nonnull;
import javax.servlet.ServletException;

import net.sf.json.JSONObject;

import org.kohsuke.stapler.AncestorInPath;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.interceptor.RequirePOST;
import org.kohsuke.stapler.QueryParameter;
Expand Down Expand Up @@ -70,30 +73,37 @@ public Gitiles newInstance(StaplerRequest req, @Nonnull JSONObject jsonObject) t
}

@RequirePOST
public FormValidation doCheckUrl(@QueryParameter(fixEmpty = true) final String url) throws IOException, ServletException {
if (url == null) // nothing entered yet
return FormValidation.ok();
// Connect to URL and check content only if we have admin permission
if (!Jenkins.get().hasPermission(Jenkins.ADMINISTER))
public FormValidation doCheckRepoUrl(@AncestorInPath Item project, @QueryParameter(fixEmpty = true) final String repoUrl)
throws IOException, ServletException, URISyntaxException {

String cleanUrl = Util.fixEmptyAndTrim(repoUrl);
if(initialChecksAndReturnOk(project, cleanUrl)){
return FormValidation.ok();
return new URLCheck() {
protected FormValidation check() throws IOException, ServletException {
String v = url;
if (!v.endsWith("/"))
v += '/';

try {
// gitiles has a line in main page indicating how to clone the project
if (findText(open(new URL(v)), "git clone")) {
return FormValidation.ok();
} else {
return FormValidation.error("This is a valid URL but it doesn't look like Gitiles");
}
FormValidation response;
if (checkURIFormat(cleanUrl, "gerrit")) {
Copy link
Contributor

@MarkEWaite MarkEWaite Feb 20, 2020

Choose a reason for hiding this comment

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

Can't assume that the Gerrit server includes "gerrit" in its hostname.

Copy link
Contributor Author

@rishabhBudhouliya rishabhBudhouliya Feb 21, 2020

Choose a reason for hiding this comment

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

Agreed, implementation has been changed.

return new URLCheck() {
protected FormValidation check() throws IOException, ServletException {
String v = cleanUrl;
if (!v.endsWith("/")) {
v += '/';
}

try {
if (findText(open(new URL(v)), "git clone")) {
return FormValidation.ok();
} else {
return FormValidation.error("This is a valid URL but it doesn't look like Gitiles");
}
} catch (IOException e) {
return handleIOException(v, e);
}
} catch (IOException e) {
return handleIOException(v, e);
}
}
}.check();
}.check();
} else {
response = FormValidation.error(Messages.invalidUrl());
}
return response;
}
}
}
Loading