hgreview.py - Issue 29338096: Issue 3783 - Redirect auth page to review after upload

Delta Between Two Patch Sets: hgreview.py

Issue 29338096: Issue 3783 - Redirect auth page to review after upload (Closed)

Left Patch Set: Don't inherit globals Created March 10, 2016, 2:50 p.m.

Right Patch Set: Try different ports Created March 11, 2016, 10:10 a.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

Left: Side by side diff | Download
Right: Side by side diff | Download

LEFT	RIGHT
1 import BaseHTTPServer	1 import BaseHTTPServer

2 import os	2 import os

3 import random

4 import re	3 import re

	4 import socket

5 import sys	5 import sys

6 import urllib	6 import urllib

7	7

8 from mercurial import cmdutil, error	8 from mercurial import cmdutil, error

9	9

10 SERVER = 'https://codereview.adblockplus.org'	10 SERVER = 'https://codereview.adblockplus.org'

11 UPLOADTOOL_URL = SERVER + '/static/upload.py'	11 UPLOADTOOL_URL = SERVER + '/static/upload.py'

12	12

13 cmdtable = {}	13 cmdtable = {}

14 command = cmdutil.command(cmdtable)	14 command = cmdutil.command(cmdtable)

(...skipping 58 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
73	73

74 args.extend(paths)	74 args.extend(paths)

75	75

76 upload_path = ui.config('review', 'uploadtool_path',	76 upload_path = ui.config('review', 'uploadtool_path',

77 os.path.join('~', '.hgreview_upload.py'))	77 os.path.join('~', '.hgreview_upload.py'))

78 upload_path = os.path.expanduser(upload_path)	78 upload_path = os.path.expanduser(upload_path)

79 if not os.path.exists(upload_path):	79 if not os.path.exists(upload_path):

80 ui.status('Downloading {0} to {1}.\n'.format(UPLOADTOOL_URL, upload_path))	80 ui.status('Downloading {0} to {1}.\n'.format(UPLOADTOOL_URL, upload_path))

81 urllib.urlretrieve(UPLOADTOOL_URL, upload_path)	81 urllib.urlretrieve(UPLOADTOOL_URL, upload_path)

82	82

	83 # Find an available port for our local server

	84 issue = None

	85 class RequestHandler(BaseHTTPServer.BaseHTTPRequestHandler):

	86 def do_GET(self):

	87 self.send_response(200)

	88 self.send_header('Content-type', 'text/javascript')

	89 self.end_headers()

	90 self.wfile.write('location.href = "{0}";'.format(SERVER + '/' + issue))

	91 def log_message(args, *kwargs):

	92 pass

	93 for port in range(54770, 54780):

	94 try:

	95 server = BaseHTTPServer.HTTPServer(('localhost', port), RequestHandler)

	96 break

	97 except socket.error:

	98 pass

	99

83 # Modify upload tool's auth response in order to redirect to the issue	100 # Modify upload tool's auth response in order to redirect to the issue

84 port = random.randrange(2000, 60000)
Sebastian Noack 2016/03/10 15:08:22 I'm not sure whether it's a good idea to use a ran I'm not sure whether it's a good idea to use a random port, at least not without fallback logic if that port is already used. What if a different server, or an instance of this script, is already running on the port we randomly choosed? I think we should rather handle the error when the port is already in use and fall back to another port. Whether the next port is picked incrementally or randomly shouldn't matter much. Wladimir Palant 2016/03/11 10:12:42 Ok, now we will try ten different ports. In the un Show quoted text On 2016/03/10 15:08:22, Sebastian Noack wrote: > I'm not sure whether it's a good idea to use a random port, at least not without > fallback logic if that port is already used. Ok, now we will try ten different ports. In the unlikely event that none of them work we'll simply fall back to the original upload.py logic.
85 scope = {}	101 scope = {}

86 execfile(upload_path, scope)	102 execfile(upload_path, scope)

87 scope['AUTH_HANDLER_RESPONSE'] = '''\	103 if server:

	104 scope['AUTH_HANDLER_RESPONSE'] = '''\

88 <html>	105 <html>

89 <head>	106 <head>

90 <title>Authentication Status</title>	107 <title>Authentication Status</title>

91 <script>	108 <script>

92 window.onload = function()	109 window.onload = function()

93 {	110 {

94 setInterval(function()	111 setInterval(function()

95 {	112 {

96 var script = document.createElement("script");	113 var script = document.createElement("script");

97 script.src = "http://localhost:%s/?" + (new Date().getTime());	114 script.src = "http://localhost:%s/?" + (new Date().getTime());

98 document.body.appendChild(script);	115 document.body.appendChild(script);

99 }, 1000)	116 }, 1000)

100 }	117 }

101 </script>	118 </script>

102 </head>	119 </head>

103 <body>	120 <body>

104 <p>	121 <p>

105 The authentication flow has completed. This page will redirect to your	122 The authentication flow has completed. This page will redirect to your

106 review shortly.	123 review shortly.

107 </p>	124 </p>

108 </body>	125 </body>

109 </html>	126 </html>

110 ''' % port	127 ''' % port

111	128

112 # Run the upload tool	129 # Run the upload tool

113 issue, patchset = scope['RealMain']([upload_path] + args)	130 issue, patchset = scope['RealMain']([upload_path] + args)

114	131

115 # Wait for the page to check in and retrieve issue URL	132 # Wait for the page to check in and retrieve issue URL

116 class RequestHandler(BaseHTTPServer.BaseHTTPRequestHandler):	133 if server:

117 def do_GET(self):	134 server.handle_request()

118 self.send_response(200)

119 self.send_header('Content-type', 'text/javascript')

120 self.end_headers()

121 self.wfile.write('location.href = "{0}";'.format(SERVER + '/' + issue))

122 def log_message(args, *kwargs):

123 pass

124

125 server = BaseHTTPServer.HTTPServer(('localhost', port), RequestHandler)

126 server.handle_request()

LEFT	RIGHT