[Merge] lp:~abreu-alexandre/webbrowser-app/saml-url-persistence into lp:webbrowser-app
Alberto Mardegan
alberto.mardegan at canonical.com
Tue Jun 2 15:14:14 UTC 2015
Review: Needs Fixing
A few minor comments, mostly personal preferences.
Diff comments:
> === modified file 'src/app/webcontainer/WebApp.qml'
> --- src/app/webcontainer/WebApp.qml 2015-05-20 14:14:18 +0000
> +++ src/app/webcontainer/WebApp.qml 2015-05-27 16:25:53 +0000
> @@ -21,6 +21,7 @@
> import Ubuntu.Components 1.1
> import Ubuntu.Unity.Action 1.1 as UnityActions
> import Ubuntu.UnityWebApps 0.1 as UnityWebApps
> +import Qt.labs.settings 1.0
> import "../actions" as Actions
> import ".."
>
> @@ -35,7 +36,7 @@
> property string webappModelSearchPath: ""
>
> property alias webappName: webview.webappName
> - property alias webappUrlPatterns: webview.webappUrlPatterns
> + property var webappUrlPatterns
> property alias popupRedirectionUrlPrefixPattern: webview.popupRedirectionUrlPrefixPattern
> property alias webviewOverrideFile: webview.webviewOverrideFile
> property alias blockOpenExternalUrls: webview.blockOpenExternalUrls
> @@ -47,6 +48,8 @@
> property bool chromeVisible: false
> readonly property bool chromeless: !chromeVisible && !backForwardButtonsVisible
>
> + signal generatedUrlPatternsFileUpdated(string patterns)
I'd prefer not to ha public signals that are only used in unit tests. I suggest removing this signal altogether, since it seems to me that it's not really essential for the unit tests either.
> +
> actions: [
> Actions.Back {
> enabled: webapp.backForwardButtonsVisible && webview.currentWebview && webview.currentWebview.canGoBack
> @@ -61,6 +64,56 @@
> }
> ]
>
> + Settings {
> + id: generatedUrlPatternSettings
> + property string generatedUrlPatterns
> + }
> +
> + function getGeneratedUrlPatterns() {
I would do without this function, and maybe shorten the name (id) of the Settings element (just "settings" could do).
If you prefer to keep this, then please change line 60 so that this function is being used, instead of directly accessing the property.
> + return generatedUrlPatternSettings.generatedUrlPatterns
> + }
> +
> + function addGeneratedUrlPattern(urlPattern) {
> + var patterns;
Remove semicolon :-)
> + try {
> + patterns = JSON.parse(generatedUrlPatternSettings.generatedUrlPatterns)
> + } catch(e) {
> + console.error("Invalid JSON content found in url patterns file")
> + }
> + if (! (patterns instanceof Array)) {
> + console.error("Invalid JSON content type found in url patterns file (not an array)")
> + patterns = []
> + }
> + if (patterns.indexOf(urlPattern) < 0) {
> + patterns.push(urlPattern)
> + }
> + generatedUrlPatternSettings.generatedUrlPatterns = JSON.stringify(patterns)
I would move this inside the above "if", to avoid writing to the file if the list hasn't changed.
> +
> + generatedUrlPatternsFileUpdated(
> + generatedUrlPatternSettings.generatedUrlPatterns)
> + }
> +
> + function mergeUrlPatternSets(p1, p2) {
> + if ( ! (p1 instanceof Array)) {
> + return (p2 instanceof Array) ? p2 : []
> + }
> + if ( ! (p2 instanceof Array)) {
> + return (p1 instanceof Array) ? p1 : []
> + }
> + var p1hash = {}
> + var result = []
> + for (var i1 in p1) {
> + p1hash[p1[i1]] = 1
> + result.push(p1[i1])
> + }
> + for (var i2 in p2) {
> + if (! (p2[i2] in p1hash)) {
> + result.push(p2[i2])
> + }
> + }
> + return result
> + }
> +
> Item {
> id: webviewContainer
> anchors.fill: parent
> @@ -76,6 +129,11 @@
> }
> height: parent.height - osk.height
> developerExtrasEnabled: webapp.developerExtrasEnabled
> + onSamlRequestUrlPatternReceived: {
> + addGeneratedUrlPattern(urlPattern)
> + }
> + webappUrlPatterns: mergeUrlPatternSets(getGeneratedUrlPatterns(),
> + webapp.webappUrlPatterns)
> }
>
> Loader {
>
> === modified file 'src/app/webcontainer/WebViewImplOxide.qml'
> --- src/app/webcontainer/WebViewImplOxide.qml 2015-04-29 21:32:06 +0000
> +++ src/app/webcontainer/WebViewImplOxide.qml 2015-05-27 16:25:53 +0000
> @@ -44,6 +44,8 @@
> // (if any) or navigations resulting in new windows being created.
> property bool blockOpenExternalUrls: false
>
> + signal samlRequestUrlPatternReceived(string urlPattern)
> +
> // Those signals are used for testing purposes to externally
> // track down the various internal logic & steps of a popup lifecycle.
> signal openExternalUrlTriggered(string url)
> @@ -78,6 +80,12 @@
> StateSaver.properties: "url"
> StateSaver.enabled: !runningLocalApplication
>
> + function handleSAMLRequestPattern(urlPattern) {
> + webappUrlPatterns.push(urlPattern)
> +
> + samlRequestUrlPatternReceived(urlPattern)
> + }
> +
> function shouldOpenPopupsInDefaultBrowser() {
> return formFactor !== "desktop";
> }
> @@ -163,9 +171,12 @@
> var match = urlRegExp.exec(url)
> var host = match[1]
> var escapeDotsRegExp = new RegExp("\\.", "g")
> - var hostPattern = "https?://" + host.replace(escapeDotsRegExp, "\\.") + "/"
> + var hostPattern = "https?://" + host.replace(escapeDotsRegExp, "\\.") + "/*"
> +
> console.log("SAML request detected. Adding host pattern: " + hostPattern)
> - webappUrlPatterns.push(hostPattern)
> +
> + handleSAMLRequestPattern(hostPattern)
> +
> request.action = Oxide.NavigationRequest.ActionAccept
> }
>
>
> === modified file 'src/app/webcontainer/WebappContainerWebview.qml'
> --- src/app/webcontainer/WebappContainerWebview.qml 2015-04-21 14:01:13 +0000
> +++ src/app/webcontainer/WebappContainerWebview.qml 2015-05-27 16:25:53 +0000
> @@ -40,6 +40,8 @@
> property bool blockOpenExternalUrls: false
> property bool runningLocalApplication: false
>
> + signal samlRequestUrlPatternReceived(string urlPattern)
> +
> PopupWindowController {
> id: popupController
> objectName: "popupController"
> @@ -52,6 +54,14 @@
> id: webappContainerWebViewLoader
> objectName: "containerWebviewLoader"
> anchors.fill: parent
> + onLoaded: {
I'd use a Connections element here.
> + if (webappContainerWebViewLoader.status === Loader.Ready) {
> + webappContainerWebViewLoader.item
> + .samlRequestUrlPatternReceived.connect(function(urlPattern) {
> + samlRequestUrlPatternReceived(urlPattern)
> + })
> + }
> + }
> }
>
> onUrlChanged: if (webappContainerWebViewLoader.item) webappContainerWebViewLoader.item.url = url
>
> === modified file 'tests/autopilot/webapp_container/tests/fake_servers.py'
> --- tests/autopilot/webapp_container/tests/fake_servers.py 2015-03-23 19:58:08 +0000
> +++ tests/autopilot/webapp_container/tests/fake_servers.py 2015-05-27 16:25:53 +0000
> @@ -99,6 +99,24 @@
> </html>
> """.format("'"+self.headers['user-agent']+"'")
>
> + def saml(self, loopcount):
> + return """
> + <html>
> + <head>
> + <title>open-close</title>
> + <script>
> + </script>
> + </head>
> + <body>
> + <a href="/redirect-to-saml/?loopcount={}&SAMLRequest=1">
> + <div style="height: 100%; width: 100%; background-color: red">
> + target blank link
> + </div>
> + </a>
> + </body>
> + </html>
> + """.format(loopcount)
> +
> def open_close_content(self):
> return """
> <html>
> @@ -142,6 +160,27 @@
> elif self.path == '/open-close-content':
> self.send_response(200)
> self.serve_content(self.open_close_content())
> + elif self.path.startswith('/saml/'):
> + args = self.path[len('/saml/'):]
> + loopCount = 0
> + if args.startswith('?loopcount='):
> + loopCount = int(args[len('?loopcount='):].split(';')[0])
> + self.send_response(200)
> + self.serve_content(self.saml(loopCount))
> + elif self.path.startswith('/redirect-to-saml/'):
> + locationTarget = '/'
> + args = self.path[len('/redirect-to-saml/'):]
> + if args.startswith('?loopcount='):
> + header_size = len('?loopcount=')
> + loopCount = int(
> + args[header_size:args.index('&')].split(';')[0])
> + if loopCount > 0:
> + loopCount = loopCount - 1
> + locationTarget += 'redirect-to-saml\
> +/?loopcount=' + str(loopCount) + '&SAMLRequest=1'
> + self.send_response(302)
> + self.send_header("Location", locationTarget)
> + self.end_headers()
> else:
> self.send_error(404)
>
>
> === added file 'tests/autopilot/webapp_container/tests/test_saml_url_patterns.py'
> --- tests/autopilot/webapp_container/tests/test_saml_url_patterns.py 1970-01-01 00:00:00 +0000
> +++ tests/autopilot/webapp_container/tests/test_saml_url_patterns.py 2015-05-27 16:25:53 +0000
> @@ -0,0 +1,65 @@
> +# -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*-
> +# Copyright 2014 Canonical
> +#
> +# This program is free software: you can redistribute it and/or modify it
> +# under the terms of the GNU General Public License version 3, as published
> +# by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +from testtools.matchers import Equals, Contains
> +from autopilot.matchers import Eventually
> +
> +from webapp_container.tests import WebappContainerTestCaseWithLocalContentBase
> +
> +
> +class WebappContainerSAMLUrlPatternsTestCase(
> + WebappContainerTestCaseWithLocalContentBase):
> + def test_saml_urls_added(self):
> + rule = 'MAP *.test.com:80 ' + self.get_base_url_hostname()
> + args = ["--webappUrlPatterns=\
> +http://www.test.com/saml/*,{}/saml/*".format(self.base_url)]
> +
> + samlRequestRedirectsCount = 1
> + target_path = '/saml/?\
> +loopcount={}'.format(str(samlRequestRedirectsCount))
> +
> + self.launch_webcontainer_app_with_local_http_server(
> + args,
> + target_path,
> + {'WEBAPP_CONTAINER_BLOCK_OPEN_URL_EXTERNALLY': '1',
> + 'UBUNTU_WEBVIEW_HOST_MAPPING_RULES': rule})
> + self.get_webcontainer_window().visible.wait_for(True)
> + self.assert_page_eventually_loaded(self.base_url+target_path)
> +
> + container_webview = self.get_webcontainer_webview()
> + url_patterns_file_updated_watcher = container_webview.watch_signal(
> + 'generatedUrlPatternsFileUpdated(QString)')
> +
> + webview = self.get_oxide_webview()
> +
> + gr = webview.globalRect
> + self.pointing_device.move(
> + gr.x + webview.width*0.5,
> + gr.y + webview.height*0.5)
> + self.pointing_device.click()
> +
> + self.assertThat(
> + lambda: url_patterns_file_updated_watcher.was_emitted,
> + Eventually(Equals(True)))
> + self.assertThat(
> + lambda: url_patterns_file_updated_watcher.num_emissions,
> + Eventually(Equals(samlRequestRedirectsCount)))
> +
> + saved_patterns = container_webview.get_signal_emissions(
> + 'generatedUrlPatternsFileUpdated(QString)')[0][0]
> +
> + self.assertThat(
> + saved_patterns,
> + Contains("\"https?://{}/*\"".format(self.get_base_url_hostname())))
>
--
https://code.launchpad.net/~abreu-alexandre/webbrowser-app/saml-url-persistence/+merge/260248
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.
More information about the Ubuntu-reviews
mailing list