From 648590c35652df3096cd066486d7a3e348b10a60 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 22 May 2018 13:09:48 -0400 Subject: [PATCH] Change from manual URL construction to using a lib Makes the code cleaner to read and more resilient to changes Fixes https://jira.coreos.com/browse/QUAY-940 --- oauth/base.py | 9 +------ oauth/services/github.py | 2 +- oauth/services/gitlab.py | 2 +- oauth/services/google.py | 2 +- oauth/services/test/test_github.py | 1 - package.json | 1 + static/js/directives/ui/entity-search.js | 17 ++++++++----- static/js/directives/ui/logs-view.js | 16 ++++++------ static/js/services/api-service.js | 18 +++++++------ static/js/services/roles-service.js | 2 +- static/js/services/trigger-service.js | 16 ++++++++---- static/js/services/util-service.js | 32 +++++++++++++++++++++--- yarn.lock | 23 +++++++++-------- 13 files changed, 85 insertions(+), 56 deletions(-) diff --git a/oauth/base.py b/oauth/base.py index 1e3d451d6..12ca4dfef 100644 --- a/oauth/base.py +++ b/oauth/base.py @@ -25,14 +25,7 @@ class OAuthEndpoint(object): params_copy = copy.copy(self.params) params_copy.update(parameters) return OAuthEndpoint(self.base_url, params_copy) - - def to_url_prefix(self): - prefix = self.to_url() - if self.params: - return prefix + '&' - else: - return prefix + '?' - + def to_url(self): (scheme, netloc, path, _, fragment) = urlparse.urlsplit(self.base_url) updated_query = urllib.urlencode(self.params) diff --git a/oauth/services/github.py b/oauth/services/github.py index 3923b6c95..6bc9350ff 100644 --- a/oauth/services/github.py +++ b/oauth/services/github.py @@ -113,7 +113,7 @@ class GithubOAuthService(OAuthLoginService): def get_public_config(self): return { 'CLIENT_ID': self.client_id(), - 'AUTHORIZE_ENDPOINT': self.authorize_endpoint().to_url_prefix(), + 'AUTHORIZE_ENDPOINT': self.authorize_endpoint().to_url(), 'GITHUB_ENDPOINT': self._endpoint(), 'ORG_RESTRICT': self.config.get('ORG_RESTRICT', False) } diff --git a/oauth/services/gitlab.py b/oauth/services/gitlab.py index 4ac0dda22..1ee2f90ed 100644 --- a/oauth/services/gitlab.py +++ b/oauth/services/gitlab.py @@ -55,6 +55,6 @@ class GitLabOAuthService(OAuthService): def get_public_config(self): return { 'CLIENT_ID': self.client_id(), - 'AUTHORIZE_ENDPOINT': self.authorize_endpoint().to_url_prefix(), + 'AUTHORIZE_ENDPOINT': self.authorize_endpoint().to_url(), 'GITLAB_ENDPOINT': self._endpoint(), } diff --git a/oauth/services/google.py b/oauth/services/google.py index ede5203dd..a22964bb6 100644 --- a/oauth/services/google.py +++ b/oauth/services/google.py @@ -61,7 +61,7 @@ class GoogleOAuthService(OAuthLoginService): def get_public_config(self): return { 'CLIENT_ID': self.client_id(), - 'AUTHORIZE_ENDPOINT': self.authorize_endpoint().to_url_prefix() + 'AUTHORIZE_ENDPOINT': self.authorize_endpoint().to_url() } def get_login_service_id(self, user_info): diff --git a/oauth/services/test/test_github.py b/oauth/services/test/test_github.py index c19ec3f42..b14ac4952 100644 --- a/oauth/services/test/test_github.py +++ b/oauth/services/test/test_github.py @@ -29,7 +29,6 @@ def test_basic_enterprise_config(trigger_config, domain, api_endpoint, is_enterp assert github_trigger.is_enterprise() == is_enterprise assert github_trigger.authorize_endpoint().to_url() == '%s/login/oauth/authorize' % domain - assert github_trigger.authorize_endpoint().to_url_prefix() == '%s/login/oauth/authorize?' % domain assert github_trigger.token_endpoint().to_url() == '%s/login/oauth/access_token' % domain diff --git a/package.json b/package.json index 828195b23..5ed245cb3 100644 --- a/package.json +++ b/package.json @@ -43,6 +43,7 @@ "showdown": "^1.6.4", "underscore": "^1.5.2", "urijs": "^1.18.10", + "url-parse": "^1.4.0", "zeroclipboard": "^2.3.0" }, "devDependencies": { diff --git a/static/js/directives/ui/entity-search.js b/static/js/directives/ui/entity-search.js index c95b7ca76..9a3f868b8 100644 --- a/static/js/directives/ui/entity-search.js +++ b/static/js/directives/ui/entity-search.js @@ -84,7 +84,7 @@ angular.module('quay').directive('entitySearch', function () { $scope.checkLazyLoad = function() { if (!$scope.namespace || !$scope.thisUser || !$scope.requiresLazyLoading || $scope.isLazyLoading || !$scope.userRequestedLazyLoading) { - return; + return; } $scope.isLazyLoading = true; @@ -229,15 +229,20 @@ angular.module('quay').directive('entitySearch', function () { name: 'entities' + $rootScope.__entity_search_counter, remote: { url: '/api/v1/entities/%QUERY', - replace: function (url, uriEncodedQuery) { + replace: function (query_url, uriEncodedQuery) { + $scope.lazyLoad(); + var namespace = $scope.namespace || ''; - url = url.replace('%QUERY', uriEncodedQuery); - url += '?namespace=' + encodeURIComponent(namespace); + + var url = UtilService.getRestUrl(query_url.replace('%QUERY', uriEncodedQuery)); + url.setQueryParameter('namespace', namespace); + if ($scope.isOrganization && isSupported('team')) { - url += '&includeTeams=true' + url.setQueryParameter('includeTeams', true); } + if (isSupported('org')) { - url += '&includeOrgs=true' + url.setQueryParameter('includeOrgs', true); } return url; }, diff --git a/static/js/directives/ui/logs-view.js b/static/js/directives/ui/logs-view.js index 544263a1e..877863b78 100644 --- a/static/js/directives/ui/logs-view.js +++ b/static/js/directives/ui/logs-view.js @@ -1,5 +1,5 @@ import { LogUsageChart } from '../../graphing'; - +import { parse } from 'path'; /** * Element which displays usage logs for the given entity. @@ -383,8 +383,8 @@ angular.module('quay').directive('logsView', function () { url = UtilService.getRestUrl('superuser', suffix) } - url += '?starttime=' + encodeURIComponent(getDateString($scope.options.logStartDate)); - url += '&endtime=' + encodeURIComponent(getDateString($scope.options.logEndDate)); + url.setQueryParameter('starttime', getDateString($scope.options.logStartDate)); + url.setQueryParameter('endtime', getDateString($scope.options.logEndDate)); return url; }; @@ -405,7 +405,7 @@ angular.module('quay').directive('logsView', function () { $scope.chartLoading = true; - var aggregateUrl = getUrl('aggregatelogs') + var aggregateUrl = getUrl('aggregatelogs').toString(); var loadAggregate = Restangular.one(aggregateUrl); loadAggregate.customGET().then(function(resp) { $scope.chart = new LogUsageChart(logKinds); @@ -430,12 +430,10 @@ angular.module('quay').directive('logsView', function () { $scope.loading = true; - var logsUrl = getUrl('logs'); - if ($scope.nextPageToken) { - logsUrl = logsUrl + '&next_page=' + encodeURIComponent($scope.nextPageToken); - } + var url = getUrl('logs'); + url.setQueryParameter('next_page', $scope.nextPageToken); - var loadLogs = Restangular.one(logsUrl); + var loadLogs = Restangular.one(url.toString()); loadLogs.customGET().then(function(resp) { resp.logs.forEach(function(log) { $scope.logs.push(log); diff --git a/static/js/services/api-service.js b/static/js/services/api-service.js index 9031e558f..581239d75 100644 --- a/static/js/services/api-service.js +++ b/static/js/services/api-service.js @@ -1,3 +1,5 @@ +var urlParseURL = require('url-parse'); + /** * Service which exposes the server-defined API as a nice set of helper methods and automatic * callbacks. Any method defined on the server is exposed here as an equivalent method. Also @@ -44,7 +46,7 @@ angular.module('quay').factory('ApiService', ['Restangular', '$q', 'UtilService' // Build the path, adjusted with the inline parameters. var used = {}; - var url = ''; + var urlPath = ''; for (var i = 0; i < path.length; ++i) { var c = path[i]; if (c == '{') { @@ -56,29 +58,29 @@ angular.module('quay').factory('ApiService', ['Restangular', '$q', 'UtilService' } used[varName] = true; - url += parameters[varName]; + urlPath += encodeURI(parameters[varName]); i = end; continue; } - url += c; + urlPath += c; } // Append any query parameters. - var isFirst = true; + var url = new urlParseURL(urlPath, '/'); + url.query = {}; + for (var paramName in parameters) { if (!parameters.hasOwnProperty(paramName)) { continue; } if (used[paramName]) { continue; } var value = parameters[paramName]; if (value != null) { - url += isFirst ? '?' : '&'; - url += paramName + '=' + encodeURIComponent(value) - isFirst = false; + url.query[paramName] = value } } - return url; + return url.toString(); }; var getGenericOperationName = function(userOperationName) { diff --git a/static/js/services/roles-service.js b/static/js/services/roles-service.js index b192e715d..9011d7425 100644 --- a/static/js/services/roles-service.js +++ b/static/js/services/roles-service.js @@ -29,7 +29,7 @@ angular.module('quay').factory('RolesService', ['UtilService', 'Restangular', 'A var namespace = repository.namespace; var name = repository.name; var url = UtilService.getRestUrl('repository', namespace, name, 'permissions', entityKind, entityName); - return Restangular.one(url); + return Restangular.one(url.toString()); }; roleService.deleteRepositoryRole = function(repository, entityKind, entityName, callback) { diff --git a/static/js/services/trigger-service.js b/static/js/services/trigger-service.js index 7d6a1d9b4..d05f5d46c 100644 --- a/static/js/services/trigger-service.js +++ b/static/js/services/trigger-service.js @@ -23,11 +23,13 @@ angular.module('quay').factory('TriggerService', ['UtilService', '$sanitize', 'K var redirect_uri = KeyService['githubRedirectUri'] + '/trigger/' + namespace + '/' + repository; - var authorize_url = KeyService['githubTriggerAuthorizeUrl']; var client_id = KeyService['githubTriggerClientId']; - return authorize_url + 'client_id=' + client_id + - '&scope=repo,user:email&redirect_uri=' + redirect_uri; + var authorize_url = new UtilService.UrlBuilder(KeyService['githubTriggerAuthorizeUrl']); + authorize_url.setQueryParameter('client_id', client_id); + authorize_url.setQueryParameter('scope', 'repo,user:email'); + authorize_url.setQueryParameter('redirect_uri', redirect_uri); + return authorize_url.toString(); }, 'is_external': true, 'is_enabled': function() { @@ -81,10 +83,14 @@ angular.module('quay').factory('TriggerService', ['UtilService', '$sanitize', 'K 'run_parameters': [branch_tag], 'get_redirect_url': function(namespace, repository) { var redirect_uri = KeyService['gitlabRedirectUri'] + '/trigger'; - var authorize_url = KeyService['gitlabTriggerAuthorizeUrl']; var client_id = KeyService['gitlabTriggerClientId']; - return authorize_url + 'client_id=' + client_id + '&redirect_uri=' + redirect_uri + '&response_type=code&state=repo:' + namespace + '/' + repository; + var authorize_url = new UtilService.UrlBuilder(KeyService['gitlabTriggerAuthorizeUrl']); + authorize_url.setQueryParameter('client_id', client_id); + authorize_url.setQueryParameter('state', 'repo:' + namespace + '/' + repository); + authorize_url.setQueryParameter('redirect_uri', redirect_uri); + authorize_url.setQueryParameter('response_type', 'code'); + return authorize_url.toString(); }, 'is_external': false, 'is_enabled': function() { diff --git a/static/js/services/util-service.js b/static/js/services/util-service.js index 8f3f3ea25..d6f08f2c3 100644 --- a/static/js/services/util-service.js +++ b/static/js/services/util-service.js @@ -1,3 +1,23 @@ +var urlParseURL = require('url-parse'); + +var UrlBuilder = function(initial_url) { + this.url = urlParseURL(initial_url || '', '/'); +}; + +UrlBuilder.prototype.setQueryParameter = function(paramName, paramValue) { + if (paramValue == null) { + return; + } + + this.url.query = this.url.query || {}; + this.url.query[paramName] = paramValue; +}; + +UrlBuilder.prototype.toString = function() { + return this.url.toString(); +}; + + /** * Service which exposes various utility methods. */ @@ -98,19 +118,23 @@ angular.module('quay').factory('UtilService', ['$sanitize', 'markdownConverter', }; utilService.getRestUrl = function(args) { - var url = ''; + var path = ''; + for (var i = 0; i < arguments.length; ++i) { if (i > 0) { - url += '/'; + path += '/'; } - url += encodeURI(arguments[i]) + path += encodeURI(arguments[i]) } - return url; + + return new UrlBuilder(path); }; utilService.textToSafeHtml = function(text) { return $sanitize(utilService.escapeHtmlString(text)); }; + utilService.UrlBuilder = UrlBuilder; + return utilService; }]); diff --git a/yarn.lock b/yarn.lock index f307b5717..6ebb33dc1 100644 --- a/yarn.lock +++ b/yarn.lock @@ -48,16 +48,6 @@ version "0.0.32" resolved "https://registry.yarnpkg.com/@types/q/-/q-0.0.32.tgz#bd284e57c84f1325da702babfc82a5328190c0c5" -"@types/react-dom@0.14.17": - version "0.14.17" - resolved "https://registry.yarnpkg.com/@types/react-dom/-/react-dom-0.14.17.tgz#d8b0dec27e873c218d9075856c6ca1c5db956d5d" - dependencies: - "@types/react" "*" - -"@types/react@*", "@types/react@0.14.39": - version "0.14.39" - resolved "https://registry.yarnpkg.com/@types/react/-/react-0.14.39.tgz#11cb715768da5f7605aa2030a5dc63e77a137eb5" - "@types/selenium-webdriver@^2.53.35", "@types/selenium-webdriver@~2.53.39": version "2.53.42" resolved "https://registry.yarnpkg.com/@types/selenium-webdriver/-/selenium-webdriver-2.53.42.tgz#74cb77fb6052edaff2a8984ddafd88d419f25cac" @@ -3044,6 +3034,10 @@ querystring@0.2.0: version "0.2.0" resolved "https://registry.yarnpkg.com/querystring/-/querystring-0.2.0.tgz#b209849203bb25df820da756e747005878521620" +querystringify@^2.0.0: + version "2.0.0" + resolved "https://registry.yarnpkg.com/querystringify/-/querystringify-2.0.0.tgz#fa3ed6e68eb15159457c89b37bc6472833195755" + randomatic@^1.1.3: version "1.1.6" resolved "https://registry.yarnpkg.com/randomatic/-/randomatic-1.1.6.tgz#110dcabff397e9dcff7c0789ccc0a49adf1ec5bb" @@ -3257,7 +3251,7 @@ require-main-filename@^1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/require-main-filename/-/require-main-filename-1.0.1.tgz#97f717b69d48784f5f526a6c5aa8ffdda055a4d1" -requires-port@1.x.x: +requires-port@1.x.x, requires-port@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/requires-port/-/requires-port-1.0.0.tgz#925d2601d39ac485e091cf0da5c6e694dc3dcaff" @@ -3909,6 +3903,13 @@ urijs@^1.18.10: version "1.18.10" resolved "https://registry.yarnpkg.com/urijs/-/urijs-1.18.10.tgz#b94463eaba59a1a796036a467bb633c667f221ab" +url-parse@^1.4.0: + version "1.4.0" + resolved "https://registry.yarnpkg.com/url-parse/-/url-parse-1.4.0.tgz#6bfdaad60098c7fe06f623e42b22de62de0d3d75" + dependencies: + querystringify "^2.0.0" + requires-port "^1.0.0" + url@^0.11.0: version "0.11.0" resolved "https://registry.yarnpkg.com/url/-/url-0.11.0.tgz#3838e97cfc60521eb73c525a8e55bfdd9e2e28f1"