Warn when Git URL is not an absolute URL

Warn about some Git URLs that the API server will allow, but won't
actually work for builds.

https://bugzilla.redhat.com/show_bug.cgi?id=1225420

Author: spadgett

Diff for: app/scripts/app.js

@@ -419,7 +419,11 @@ angular
     screenLgMin:  1200,  // screen-lg
     screenXlgMin: 1600   // screen-xlg
   })
-  .constant('SOURCE_URL_PATTERN', /^((ftp|http|https|git|ssh):\/\/(\w+:{0,1}[^\s@]*@)|git@)?([^\s@]+)(:[0-9]+)?(\/|\/([\w#!:.?+=&%@!\-\/]))?$/ )
+  // A (very) basic regex to determine if a URL is an absolute URL, enough to
+  // warn the user the Git URL probably won't work. This should only be used
+  // as a sanity test and shouldn't block submitting the form. Rely on the API
+  // server for any additional validation.
+  .constant('SOURCE_URL_PATTERN', /^[a-z][a-z0-9+.-@]*:(\/\/)?[0-9a-z_-]+/i)
   // http://stackoverflow.com/questions/9038625/detect-if-device-is-ios
   .constant('IS_IOS', /iPad|iPhone|iPod/.test(navigator.userAgent) && !window.MSStream)
   .constant('amTimeAgoConfig', {

Diff for: app/scripts/controllers/edit/deploymentConfig.js

@@ -20,7 +20,6 @@ angular.module('openshiftConsole')
                        Navigate,
                        ProjectsService,
                        SecretsService,
-                       SOURCE_URL_PATTERN,
                        keyValueEditorUtils) {
     $scope.projectName = $routeParams.project;
     $scope.deploymentConfig = null;

Diff for: app/views/create/fromimage.html

@@ -75,7 +75,7 @@
                               'col-lg-12': !advancedOptions}">
                               <div class="form-group">
                                 <label for="sourceUrl" class="required">Git Repository URL</label>
-                                <div ng-class="{'has-warning': form.sourceUrl.$dirty && !sourceURLPattern.test(buildConfig.sourceUrl), 'has-error': (form.sourceUrl.$error.required && form.sourceUrl.$dirty)}">
+                                <div ng-class="{'has-warning': form.sourceUrl.$touched && !sourceURLPattern.test(buildConfig.sourceUrl), 'has-error': (form.sourceUrl.$error.required && form.sourceUrl.$dirty)}">
                                   <!-- Unfortunately, we can't set type="url" because some valid values don't pass browser validation. -->
                                   <input class="form-control"
                                     id="sourceUrl"
@@ -98,8 +98,8 @@
                                 <div class="has-error" ng-show="form.sourceUrl.$error.required && form.sourceUrl.$dirty">
                                   <span class="help-block">A Git repository URL is required.</span>
                                 </div>
-                                <div>
-                                  <span class="text-warning" ng-if="form.sourceUrl.$dirty && !sourceURLPattern.test(buildConfig.sourceUrl)">Git repository should be a URL.</span>
+                                <div class="has-warning" ng-if="buildConfig.sourceUrl && form.sourceUrl.$touched && !sourceURLPattern.test(buildConfig.sourceUrl)">
+                                  <span class="help-block">This might not be a valid Git URL. Check that it is the correct URL to a remote Git repository.</span>
                                 </div>
                               </div>
                             </div>

Diff for: app/views/edit/build-config.html

@@ -43,13 +43,13 @@ <h3>Source Configuration</h3>
                                     aria-describedby="source-url-help"
                                     required>
                                 </div>
-                                <div>
-                                  <span class="text-warning" ng-if="form.sourceUrl.$dirty && !sourceURLPattern.test(updatedBuildConfig.spec.source.git.uri)">Git repository should be a URL.</span>
-                                </div>
                                 <div class="help-block" id="source-url-help">
                                   Git URL of the source code to build.
                                   <span ng-if="!view.advancedOptions">If your Git repository is private, view the <a href="" ng-click="view.advancedOptions = true">advanced options</a> to set up authentication.</span>
                                 </div>
+                                <div class="has-warning" ng-if="updatedBuildConfig.spec.source.git.uri && form.sourceUrl.$touched && !sourceURLPattern.test(buildConfig.sourceUrl)">
+                                  <span class="help-block">This might not be a valid Git URL. Check that it is the correct URL to a remote Git repository.</span>
+                                </div>
                               </div>
 
                             </div>

Diff for: dist/scripts/scripts.js

@@ -530,7 +530,7 @@ screenSmMin:768,
 screenMdMin:992,
 screenLgMin:1200,
 screenXlgMin:1600
-}).constant("SOURCE_URL_PATTERN", /^((ftp|http|https|git|ssh):\/\/(\w+:{0,1}[^\s@]*@)|git@)?([^\s@]+)(:[0-9]+)?(\/|\/([\w#!:.?+=&%@!\-\/]))?$/).constant("IS_IOS", /iPad|iPhone|iPod/.test(navigator.userAgent) && !window.MSStream).constant("amTimeAgoConfig", {
+}).constant("SOURCE_URL_PATTERN", /^[a-z][a-z0-9+.-@]*:(\/\/)?[0-9a-z_-]+/i).constant("IS_IOS", /iPad|iPhone|iPod/.test(navigator.userAgent) && !window.MSStream).constant("amTimeAgoConfig", {
 titleFormat:"LLL"
 }).config([ "$httpProvider", "AuthServiceProvider", "RedirectLoginServiceProvider", "AUTH_CFG", "API_CFG", "kubernetesContainerSocketProvider", function(a, b, c, d, e, f) {
 a.interceptors.push("AuthInterceptor"), b.LoginService("RedirectLoginService"), b.LogoutService("DeleteTokenLogoutService"), b.UserStore("LocalStorageUserStore"), c.OAuthClientID(d.oauth_client_id), c.OAuthAuthorizeURI(d.oauth_authorize_uri), c.OAuthRedirectURI(URI(d.oauth_redirect_base).segment("oauth").toString()), f.WebSocketFactory = "ContainerWebSocket";
@@ -6943,7 +6943,7 @@ details:a("getErrorDetails")(b)
 e.unwatchAll(i);
 });
 }));
-} ]), angular.module("openshiftConsole").controller("EditDeploymentConfigController", [ "$scope", "$filter", "$location", "$routeParams", "$uibModal", "AlertMessageService", "AuthorizationService", "BreadcrumbsService", "DataService", "Navigate", "ProjectsService", "SecretsService", "SOURCE_URL_PATTERN", "keyValueEditorUtils", function(a, b, c, d, e, f, g, h, i, j, k, l, m, n) {
+} ]), angular.module("openshiftConsole").controller("EditDeploymentConfigController", [ "$scope", "$filter", "$location", "$routeParams", "$uibModal", "AlertMessageService", "AuthorizationService", "BreadcrumbsService", "DataService", "Navigate", "ProjectsService", "SecretsService", "keyValueEditorUtils", function(a, b, c, d, e, f, g, h, i, j, k, l, m) {
 a.projectName = d.project, a.deploymentConfig = null, a.alerts = {}, a.view = {
 advancedStrategyOptions:!1,
 advancedImageOptions:!1
@@ -6956,7 +6956,7 @@ includeProject:!0
 }), a.deploymentConfigStrategyTypes = [ "Recreate", "Rolling", "Custom" ], f.getAlerts().forEach(function(b) {
 a.alerts[b.name] = b.data;
 }), f.clearAlerts();
-var o = [], p = function(a) {
+var n = [], o = function(a) {
 switch (a) {
 case "Recreate":
 return "recreateParams";
@@ -7018,7 +7018,7 @@ a.updatedDeploymentConfig = angular.copy(a.deploymentConfig), a.containerNames =
 pullSecrets:angular.copy(a.deploymentConfig.spec.template.spec.imagePullSecrets) || [ {
 name:""
 } ]
-}, a.volumeNames = _.map(a.deploymentConfig.spec.template.spec.volumes, "name"), a.strategyData = angular.copy(a.deploymentConfig.spec.strategy), a.originalStrategy = a.strategyData.type, a.strategyParamsPropertyName = p(a.strategyData.type), a.triggers.hasConfigTrigger = _.some(a.updatedDeploymentConfig.spec.triggers, {
+}, a.volumeNames = _.map(a.deploymentConfig.spec.template.spec.volumes, "name"), a.strategyData = angular.copy(a.deploymentConfig.spec.strategy), a.originalStrategy = a.strategyData.type, a.strategyParamsPropertyName = o(a.strategyData.type), a.triggers.hasConfigTrigger = _.some(a.updatedDeploymentConfig.spec.triggers, {
 type:"ConfigChange"
 }), "Custom" !== a.strategyData.type || _.has(a.strategyData, "customParams.environment") || (a.strategyData.customParams.environment = []), i.list("secrets", e, function(b) {
 var c = l.groupSecretsByType(b), d = _.mapValues(c, function(a) {
@@ -7027,7 +7027,7 @@ return _.map(a, "metadata.name");
 a.secretsByType = _.each(d, function(a) {
 a.unshift("");
 });
-}), o.push(i.watchObject("deploymentconfigs", d.deploymentconfig, e, function(b, c) {
+}), n.push(i.watchObject("deploymentconfigs", d.deploymentconfig, e, function(b, c) {
 "MODIFIED" === c && (a.alerts["updated/deleted"] = {
 type:"warning",
 message:"This deployment configuration has changed since you started editing it. You'll need to copy any changes you've made and edit again."
@@ -7044,9 +7044,9 @@ details:b("getErrorDetails")(c)
 };
 }) :void j.toErrorPage("You do not have authority to update deployment config " + d.deploymentconfig + ".", "access_denied");
 }));
-var q = function() {
+var p = function() {
 return "Custom" !== a.strategyData.type && "Custom" !== a.originalStrategy && a.strategyData.type !== a.originalStrategy;
-}, r = function(b) {
+}, q = function(b) {
 if (!_.has(a.strategyData, b)) {
 var c = e.open({
 animation:!0,
@@ -7066,21 +7066,21 @@ cancelButtonText:"No"
 }
 });
 c.result.then(function() {
-a.strategyData[b] = angular.copy(a.strategyData[p(a.originalStrategy)]);
+a.strategyData[b] = angular.copy(a.strategyData[o(a.originalStrategy)]);
 }, function() {
 a.strategyData[b] = {};
 });
 }
 };
 a.strategyChanged = function() {
-var b = p(a.strategyData.type);
-q() ? r(b) :_.has(a.strategyData, b) || ("Custom" !== a.strategyData.type ? a.strategyData[b] = {} :a.strategyData[b] = {
+var b = o(a.strategyData.type);
+p() ? q(b) :_.has(a.strategyData, b) || ("Custom" !== a.strategyData.type ? a.strategyData[b] = {} :a.strategyData[b] = {
 image:"",
 command:[],
 environment:[]
 }), a.strategyParamsPropertyName = b;
 };
-var s = function(a, b, c, d) {
+var r = function(a, b, c, d) {
 var e = {
 kind:"ImageStreamTag",
 namespace:b.namespace,
@@ -7094,12 +7094,12 @@ containerNames:[ a ],
 from:e
 }
 }, c;
-}, t = function() {
+}, s = function() {
 var b = _.reject(a.updatedDeploymentConfig.spec.triggers, function(a) {
 return "ImageChange" === a.type || "ConfigChange" === a.type;
 });
 return _.each(a.containerConfigByName, function(c, d) {
-if (c.hasDeploymentTrigger) b.push(s(d, c.triggerData.istag, c.triggerData.data, c.triggerData.automatic)); else {
+if (c.hasDeploymentTrigger) b.push(r(d, c.triggerData.istag, c.triggerData.data, c.triggerData.automatic)); else {
 var e = _.find(a.updatedDeploymentConfig.spec.template.spec.containers, {
 name:d
 });
@@ -7114,10 +7114,10 @@ a.disableInputs = !0, _.each(a.containerConfigByName, function(b, c) {
 var d = _.find(a.updatedDeploymentConfig.spec.template.spec.containers, {
 name:c
 });
-d.env = n.compactEntries(b.env);
-}), q() && delete a.strategyData[p(a.originalStrategy)], "Custom" !== a.strategyData.type ? _.each([ "pre", "mid", "post" ], function(b) {
-_.has(a.strategyData, [ a.strategyParamsPropertyName, b, "execNewPod", "env" ]) && (a.strategyData[a.strategyParamsPropertyName][b].execNewPod.env = n.compactEntries(a.strategyData[a.strategyParamsPropertyName][b].execNewPod.env));
-}) :_.has(a, "strategyData.customParams.environment") && (a.strategyData.customParams.environment = n.compactEntries(a.strategyData.customParams.environment)), a.updatedDeploymentConfig.spec.template.spec.imagePullSecrets = _.filter(a.secrets.pullSecrets, "name"), a.updatedDeploymentConfig.spec.strategy = a.strategyData, a.updatedDeploymentConfig.spec.triggers = t(), i.update("deploymentconfigs", a.updatedDeploymentConfig.metadata.name, a.updatedDeploymentConfig, a.context).then(function() {
+d.env = m.compactEntries(b.env);
+}), p() && delete a.strategyData[o(a.originalStrategy)], "Custom" !== a.strategyData.type ? _.each([ "pre", "mid", "post" ], function(b) {
+_.has(a.strategyData, [ a.strategyParamsPropertyName, b, "execNewPod", "env" ]) && (a.strategyData[a.strategyParamsPropertyName][b].execNewPod.env = m.compactEntries(a.strategyData[a.strategyParamsPropertyName][b].execNewPod.env));
+}) :_.has(a, "strategyData.customParams.environment") && (a.strategyData.customParams.environment = m.compactEntries(a.strategyData.customParams.environment)), a.updatedDeploymentConfig.spec.template.spec.imagePullSecrets = _.filter(a.secrets.pullSecrets, "name"), a.updatedDeploymentConfig.spec.strategy = a.strategyData, a.updatedDeploymentConfig.spec.triggers = s(), i.update("deploymentconfigs", a.updatedDeploymentConfig.metadata.name, a.updatedDeploymentConfig, a.context).then(function() {
 f.addAlert({
 name:a.updatedDeploymentConfig.metadata.name,
 data:{
@@ -7135,7 +7135,7 @@ details:b("getErrorDetails")(c)
 };
 });
 }, a.$on("$destroy", function() {
-i.unwatchAll(o);
+i.unwatchAll(n);
 });
 } ]), angular.module("openshiftConsole").controller("EditAutoscalerController", [ "$scope", "$filter", "$routeParams", "$window", "APIService", "AuthorizationService", "BreadcrumbsService", "DataService", "HPAService", "MetricsService", "Navigate", "ProjectsService", "keyValueEditorUtils", function(a, b, c, d, e, f, g, h, i, j, k, l, m) {
 if (!c.kind || !c.name) return void k.toErrorPage("Kind or name parameter missing.");

Diff for: dist/scripts/templates.js

@@ -4656,7 +4656,7 @@ angular.module('openshiftConsoleTemplates', []).run(['$templateCache', function(
     "                              'col-lg-12': !advancedOptions}\">\n" +
     "<div class=\"form-group\">\n" +
     "<label for=\"sourceUrl\" class=\"required\">Git Repository URL</label>\n" +
-    "<div ng-class=\"{'has-warning': form.sourceUrl.$dirty && !sourceURLPattern.test(buildConfig.sourceUrl), 'has-error': (form.sourceUrl.$error.required && form.sourceUrl.$dirty)}\">\n" +
+    "<div ng-class=\"{'has-warning': form.sourceUrl.$touched && !sourceURLPattern.test(buildConfig.sourceUrl), 'has-error': (form.sourceUrl.$error.required && form.sourceUrl.$dirty)}\">\n" +
     "\n" +
     "<input class=\"form-control\" id=\"sourceUrl\" name=\"sourceUrl\" type=\"text\" required aria-describedby=\"from_source_help\" ng-model=\"buildConfig.sourceUrl\" autocorrect=\"off\" autocapitalize=\"off\" spellcheck=\"false\">\n" +
     "</div>\n" +
@@ -4667,8 +4667,8 @@ angular.module('openshiftConsoleTemplates', []).run(['$templateCache', function(
     "<div class=\"has-error\" ng-show=\"form.sourceUrl.$error.required && form.sourceUrl.$dirty\">\n" +
     "<span class=\"help-block\">A Git repository URL is required.</span>\n" +
     "</div>\n" +
-    "<div>\n" +
-    "<span class=\"text-warning\" ng-if=\"form.sourceUrl.$dirty && !sourceURLPattern.test(buildConfig.sourceUrl)\">Git repository should be a URL.</span>\n" +
+    "<div class=\"has-warning\" ng-if=\"buildConfig.sourceUrl && form.sourceUrl.$touched && !sourceURLPattern.test(buildConfig.sourceUrl)\">\n" +
+    "<span class=\"help-block\">This might not be a valid Git URL. Check that it is the correct URL to a remote Git repository.</span>\n" +
     "</div>\n" +
     "</div>\n" +
     "</div>\n" +
@@ -8038,13 +8038,13 @@ angular.module('openshiftConsoleTemplates', []).run(['$templateCache', function(
     "\n" +
     "<input class=\"form-control\" id=\"sourceUrl\" name=\"sourceUrl\" ng-model=\"updatedBuildConfig.spec.source.git.uri\" type=\"text\" autocorrect=\"off\" autocapitalize=\"off\" spellcheck=\"false\" aria-describedby=\"source-url-help\" required>\n" +
     "</div>\n" +
-    "<div>\n" +
-    "<span class=\"text-warning\" ng-if=\"form.sourceUrl.$dirty && !sourceURLPattern.test(updatedBuildConfig.spec.source.git.uri)\">Git repository should be a URL.</span>\n" +
-    "</div>\n" +
     "<div class=\"help-block\" id=\"source-url-help\">\n" +
     "Git URL of the source code to build.\n" +
     "<span ng-if=\"!view.advancedOptions\">If your Git repository is private, view the <a href=\"\" ng-click=\"view.advancedOptions = true\">advanced options</a> to set up authentication.</span>\n" +
     "</div>\n" +
+    "<div class=\"has-warning\" ng-if=\"updatedBuildConfig.spec.source.git.uri && form.sourceUrl.$touched && !sourceURLPattern.test(buildConfig.sourceUrl)\">\n" +
+    "<span class=\"help-block\">This might not be a valid Git URL. Check that it is the correct URL to a remote Git repository.</span>\n" +
+    "</div>\n" +
     "</div>\n" +
     "</div>\n" +
     "<div class=\"col-lg-4\" ng-if=\"view.advancedOptions\">\n" +

Diff for: test/spec/controllers/create/createFromImageSpec.js

@@ -69,22 +69,16 @@ describe("CreateFromImageController", function(){
     expect(match).not.toBeNull();
   });
 
-  it("valid http URL, without http part", function(){
+  it("invalid http URL, without http part", function(){
     var match = 'example.com/dir1/dir2'.match($scope.sourceURLPattern);
-    expect(match).not.toBeNull();
+    expect(match).toBeNull();
   });
 
-
   it("valid http URL with user and password", function(){
     var match = 'http://user:[email protected]/dir1/dir2'.match($scope.sourceURLPattern);
     expect(match).not.toBeNull();
   });
 
-  it("invalid http URL with user and password containing space", function(){
-    var match = 'http://user:pa [email protected]/dir1/dir2'.match($scope.sourceURLPattern);
-    expect(match).toBeNull();
-  });
-
   it("valid http URL with user and password with special characters", function(){
     var match = 'https://user:[email protected]/gerrit/p/myrepo.git'.match($scope.sourceURLPattern);
     expect(match).not.toBeNull();
@@ -115,11 +109,6 @@ describe("CreateFromImageController", function(){
     expect(match).not.toBeNull();
   });
 
-  it("invalid git+ssh URL (double @@)", function(){
-    var match = 'git@@example.com:dir1/dir2'.match($scope.sourceURLPattern);
-    expect(match).toBeNull();
-  });
-
   it("valid ssh URL", function(){
     var match = 'ssh://[email protected]/dir1/dir2'.match($scope.sourceURLPattern);
     expect(match).not.toBeNull();
@@ -129,5 +118,5 @@ describe("CreateFromImageController", function(){
     var match = 'ssh://[email protected]:8080/dir1/dir2'.match($scope.sourceURLPattern);
     expect(match).not.toBeNull();
   });
-  
+
 });