@@ -1078,7 +1079,7 @@ def pickme_conflict_detected(cls, updated_request, send_notifications):
%(user)s - %(title)s
%(repo)s/%(branch)s
- https://%(pushmanager_servername)s%(pushmanager_port)s/request?id=%(id)s
+ %(pushmanager_url)s/request?id=%(id)s
Review # (if specified): %(reviewid)s
@@ -1105,14 +1106,13 @@ def pickme_conflict_detected(cls, updated_request, send_notifications):
else "another pickme"
),
'conflicts': updated_request['conflicts'].replace('\n', '
'),
- 'pushmanager_servername': Settings['main_app']['servername'],
+ 'reviewboard_servername': Settings['reviewboard']['servername'],
+ 'pushmanager_url' : pushmanager_url,
'pushmanager_port': (
(':%d' % Settings['main_app']['port'])
if Settings['main_app']['port'] != 443
else ''
- ),
- 'reviewboard_servername': Settings['reviewboard']['servername']
-
+ )
}
)
escaped_request = EscapedDict(updated_request)
@@ -1126,7 +1126,7 @@ def pickme_conflict_detected(cls, updated_request, send_notifications):
MailQueue.enqueue_user_email([user_to_notify], msg, subject)
msg = """PushManager has detected that your pickme for %(pickme_name)s contains conflicts with %(conflicts_with)s
- https://%(pushmanager_servername)s%(pushmanager_port)s/request?id=%(pickme_id)s""" % {
+ %(pushmanager_url)s/request?id=%(pickme_id)s""" % {
'conflicts_with': (
"master"
if 'conflict-master' in updated_request['tags']
@@ -1134,17 +1134,12 @@ def pickme_conflict_detected(cls, updated_request, send_notifications):
),
'pickme_name': updated_request['branch'],
'pickme_id': updated_request['id'],
- 'pushmanager_servername': Settings['main_app']['servername'],
- 'pushmanager_port': (
- (':%d' % Settings['main_app']['port'])
- if Settings['main_app']['port'] != 443
- else ''
- )
+ 'pushmanager_url' : pushmanager_url
}
XMPPQueue.enqueue_user_xmpp([user_to_notify], msg)
@classmethod
- def verify_branch(cls, request_id):
+ def verify_branch(cls, request_id, pushmanager_url):
req = cls._get_request(request_id)
if not req:
# Just log this and return. We won't be able to get more
@@ -1158,12 +1153,12 @@ def verify_branch(cls, request_id):
if not req['branch']:
error_msg = "Git queue worker received a job for request with no branch (id %s)" % request_id
- return cls.verify_branch_failure(req, error_msg)
+ return cls.verify_branch_failure(req, error_msg, pushmanager_url)
sha = cls._get_branch_sha_from_repo(req)
if sha is None:
error_msg = "Git queue worker could not get the revision from request branch (id %s)" % request_id
- return cls.verify_branch_failure(req, error_msg)
+ return cls.verify_branch_failure(req, error_msg, pushmanager_url)
duplicate_req = cls._get_request_with_sha(sha)
if (
@@ -1175,7 +1170,7 @@ def verify_branch(cls, request_id):
duplicate_req['id'],
request_id
)
- return cls.verify_branch_failure(req, error_msg)
+ return cls.verify_branch_failure(req, error_msg, pushmanager_url)
updated_tags = add_to_tags_str(req['tags'], 'git-ok')
updated_tags = del_from_tags_str(updated_tags, 'git-error')
@@ -1183,10 +1178,10 @@ def verify_branch(cls, request_id):
updated_request = cls._update_request(req, updated_values)
if updated_request:
- cls.verify_branch_successful(updated_request)
+ cls.verify_branch_successful(updated_request, pushmanager_url)
@classmethod
- def verify_branch_successful(cls, updated_request):
+ def verify_branch_successful(cls, updated_request, pushmanager_url):
msg = (
"""
@@ -1195,7 +1190,7 @@ def verify_branch_successful(cls, updated_request):
%(user)s - %(title)s
%(repo)s/%(branch)s
- https://%(pushmanager_servername)s%(pushmanager_port)s/request?id=%(id)s
+ %(pushmanager_url)s/request?id=%(id)s
Review # (if specified): %(reviewid)s @@ -1212,7 +1207,7 @@ def verify_branch_successful(cls, updated_request): """ ) updated_request.update({ - 'pushmanager_servername': Settings['main_app']['servername'], + 'pushmanager_url' : pushmanager_url, 'pushmanager_port': ( (':%d' % Settings['main_app']['port']) if Settings['main_app']['port'] != 443 @@ -1251,7 +1246,7 @@ def verify_branch_successful(cls, updated_request): ) @classmethod - def verify_branch_failure(cls, request, failure_msg): + def verify_branch_failure(cls, request, failure_msg, pushmanager_url): logging.error(failure_msg) updated_tags = add_to_tags_str(request['tags'], 'git-error') updated_tags = del_from_tags_str(updated_tags, 'git-ok') @@ -1267,7 +1262,7 @@ def verify_branch_failure(cls, request, failure_msg):
%(user)s - %(title)s
%(repo)s/%(branch)s
- https://%(pushmanager_servername)s/request?id=%(id)s
+ %(pushmanager_url)s/request?id=%(id)s
Error message:
@@ -1289,7 +1284,7 @@ def verify_branch_failure(cls, request, failure_msg):
)
request.update({
'failure_msg': failure_msg,
- 'pushmanager_servername': Settings['main_app']['servername'],
+ 'pushmanager_url' : pushmanager_url,
'reviewboard_servername': Settings['reviewboard']['servername']
})
msg %= EscapedDict(request)
@@ -1332,7 +1327,7 @@ def process_sha_queue(cls):
try:
if task.task_type is GitTaskAction.VERIFY_BRANCH:
- cls.verify_branch(task.request_id)
+ cls.verify_branch(task.request_id, task.kwargs['pushmanager_url'])
else:
logging.error(
"GitSHAQueue encountered unknown task type %d",
diff --git a/pushmanager/core/requesthandler.py b/pushmanager/core/requesthandler.py
index 05a4ec3..66d2dbe 100644
--- a/pushmanager/core/requesthandler.py
+++ b/pushmanager/core/requesthandler.py
@@ -6,6 +6,7 @@
import tornado.httpclient
import tornado.stack_context
import tornado.web
+
from pushmanager.core.settings import JSSettings
from pushmanager.core.settings import Settings
@@ -23,6 +24,21 @@ def async_api_call_error():
else:
raise
+def get_base_url(request):
+
+ default_ports = { 'https' : ':443', 'http' : ':80' }
+ protocol = request.headers.get('X-Forwarded-Proto', request.protocol).lower()
+ pushmanager_port = ':%s' % request.headers.get('X-Forwarded-Port', Settings['main_app']['port'])
+ if default_ports[protocol] == pushmanager_port:
+ pushmanager_port = ''
+
+ pushmanager_base_url = '%(protocol)s://%(pushmanager_servername)s%(pushmanager_port)s' % {
+ 'protocol' : protocol,
+ 'pushmanager_servername' : Settings['main_app']['servername'],
+ 'pushmanager_port' : pushmanager_port
+ }
+ return pushmanager_base_url
+
class RequestHandler(tornado.web.RequestHandler):
def get_current_user(self):
@@ -53,6 +69,9 @@ def async_api_call(self, method, arguments, callback):
body=urllib.urlencode(arguments)
)
+ def get_base_url(self):
+ return get_base_url(self.request)
+
def get_api_results(self, response):
if response.error:
return self.send_error()
diff --git a/pushmanager/handlers.py b/pushmanager/handlers.py
index 4fb4c74..22715de 100644
--- a/pushmanager/handlers.py
+++ b/pushmanager/handlers.py
@@ -3,11 +3,8 @@
import urlparse
-import tornado.httpserver
-import tornado.web
from pushmanager.core.auth import authenticate
from pushmanager.core.requesthandler import RequestHandler
-from pushmanager.core.settings import Settings
class NullRequestHandler(RequestHandler):
@@ -66,13 +63,7 @@ def get(self):
post = get
-class RedirHandler(tornado.web.RequestHandler):
+class RedirHandler(RequestHandler):
def get(self, path):
- pushmanager_servername = Settings['main_app']['servername']
- pushmanager_servername = pushmanager_servername.rstrip('/')
- pushmanager_port = ':%d' % Settings['main_app']['port'] if Settings['main_app']['port'] != 443 else ''
-
- pushmanager_url = "https://%s/" % pushmanager_servername + pushmanager_port
-
- self.redirect(urlparse.urljoin(pushmanager_url, path), permanent=True)
+ self.redirect(urlparse.urljoin(self.get_base_url(), path), permanent=True)
post = get
diff --git a/pushmanager/servlets/addrequest.py b/pushmanager/servlets/addrequest.py
index 92170cd..13fca02 100644
--- a/pushmanager/servlets/addrequest.py
+++ b/pushmanager/servlets/addrequest.py
@@ -3,7 +3,6 @@
from pushmanager.core.db import InsertIgnore
from pushmanager.core.mail import MailQueue
from pushmanager.core.requesthandler import RequestHandler
-from pushmanager.core.settings import Settings
from pushmanager.core.xmppclient import XMPPQueue
@@ -61,9 +60,8 @@ def on_db_complete(self, success, db_results):
})
subject = "[push] %s - %s" % (user_string, req['title'])
MailQueue.enqueue_user_email(users, msg, subject)
- msg = '%(pushmaster)s has accepted request "%(title)s" for %(user)s into a push:\nhttps://%(pushmanager_servername)s%(pushmanager_port)s/push?id=%(pushid)s' % {
- 'pushmanager_servername': Settings['main_app']['servername'],
- 'pushmanager_port': ':%d' % Settings['main_app']['port'] if Settings['main_app']['port'] != 443 else '',
+ msg = '%(pushmaster)s has accepted request "%(title)s" for %(user)s into a push:\n%(pushmanager_base_url)s/push?id=%(pushid)s' % {
+ 'pushmanager_base_url' : self.get_base_url(),
'pushmaster': self.current_user,
'title': req['title'],
'pushid': self.pushid,
diff --git a/pushmanager/servlets/deploypush.py b/pushmanager/servlets/deploypush.py
index 5812c75..7b1d7cd 100644
--- a/pushmanager/servlets/deploypush.py
+++ b/pushmanager/servlets/deploypush.py
@@ -4,7 +4,6 @@
import pushmanager.core.util
from pushmanager.core.mail import MailQueue
from pushmanager.core.requesthandler import RequestHandler
-from pushmanager.core.settings import Settings
from pushmanager.core.xmppclient import XMPPQueue
@@ -62,8 +61,8 @@ def on_db_complete(self, success, db_results):
Once you've checked that it works, mark it as verified here: - - https://%(pushmanager_servername)s%(pushmanager_port)s/push?id=%(pushid)s + + %(pushmanager_base_url)s/push?id=%(pushid)s
@@ -72,8 +71,7 @@ def on_db_complete(self, success, db_results):
""" ) % pushmanager.core.util.EscapedDict({ 'pushmaster': self.current_user, - 'pushmanager_servername': Settings['main_app']['servername'], - 'pushmanager_port' : ':%d' % Settings['main_app']['port'] if Settings['main_app']['port'] != 443 else '', + 'pushmanager_base_url': self.get_base_url(), 'user': user_string, 'title': req['title'], 'repo': req['repo'], @@ -84,10 +82,9 @@ def on_db_complete(self, success, db_results): subject = "[push] %s - %s" % (user_string, req['title']) MailQueue.enqueue_user_email(users, msg, subject) - msg = '%(pushmaster)s has deployed request "%(title)s" for %(user)s to %(pushstage)s.\nPlease verify it at https://%(pushmanager_servername)s%(pushmanager_port)s/push?id=%(pushid)s' % { + msg = '%(pushmaster)s has deployed request "%(title)s" for %(user)s to %(pushstage)s.\nPlease verify it at %(pushmanager_base_url)s/push?id=%(pushid)s' % { 'pushmaster': self.current_user, - 'pushmanager_servername': Settings['main_app']['servername'], - 'pushmanager_port': ':%d' % Settings['main_app']['port'] if Settings['main_app']['port'] != 443 else '', + 'pushmanager_base_url': self.get_base_url(), 'title': req['title'], 'pushid': self.pushid, 'user': user_string, diff --git a/pushmanager/servlets/newpush.py b/pushmanager/servlets/newpush.py index 47549a1..4cbc1de 100644 --- a/pushmanager/servlets/newpush.py +++ b/pushmanager/servlets/newpush.py @@ -10,13 +10,8 @@ from pushmanager.core.util import send_people_msg_in_groups -def send_notifications(people, pushtype, pushurl): +def send_notifications(people, pushtype, pushmanager_url): pushmanager_servername = Settings['main_app']['servername'] - pushmanager_servername = pushmanager_servername.rstrip('/') - pushmanager_port = ':%d' % Settings['main_app']['port'] if Settings['main_app']['port'] != 443 else '' - - pushurl = pushurl.lstrip('/') - pushmanager_url = "https://%s/%s" % (pushmanager_servername + pushmanager_port, pushurl) if people: msg = '%s: %s push starting! %s' % (', '.join(people), pushtype, pushmanager_url) @@ -75,6 +70,7 @@ def on_db_complete(self, success, db_results): insert_results, select_results = db_results pushurl = '/push?id=%s' % insert_results.lastrowid + pushmanager_url = self.get_base_url() + pushurl def users_involved(request): if request['watchers']: @@ -88,6 +84,6 @@ def users_involved(request): else: people = set(user for x in select_results for user in users_involved(x)) - send_notifications(people, self.pushtype, pushurl) + send_notifications(people, self.pushtype, pushmanager_url) return self.redirect(pushurl) diff --git a/pushmanager/servlets/newrequest.py b/pushmanager/servlets/newrequest.py index 0d986b6..54184ef 100644 --- a/pushmanager/servlets/newrequest.py +++ b/pushmanager/servlets/newrequest.py @@ -141,12 +141,12 @@ def on_checklist_upsert_complete(self, success, db_results): return self.send_error(500) if self.requestid: - GitQueue.enqueue_request(GitTaskAction.VERIFY_BRANCH, self.requestid) + GitQueue.enqueue_request(GitTaskAction.VERIFY_BRANCH, self.requestid, pushmanager_url = self.get_base_url()) # Check if the request is already pickme'd for a push, and if # so also enqueue it to be checked for conflicts. request_push_id = GitQueue._get_push_for_request(self.requestid) if request_push_id: - GitQueue.enqueue_request(GitTaskAction.TEST_PICKME_CONFLICT, self.requestid) + GitQueue.enqueue_request(GitTaskAction.TEST_PICKME_CONFLICT, self.requestid, pushmanager_url = self.get_base_url()) return self.redirect("/requests?user=%s" % self.request_user) diff --git a/pushmanager/servlets/pickmerequest.py b/pushmanager/servlets/pickmerequest.py index a151264..fef2738 100644 --- a/pushmanager/servlets/pickmerequest.py +++ b/pushmanager/servlets/pickmerequest.py @@ -61,7 +61,7 @@ def condition_fn(db_results): def on_db_complete(self, success, db_results): self.check_db_results(success, db_results) for request_id in self.request_ids: - GitQueue.enqueue_request(GitTaskAction.TEST_PICKME_CONFLICT, request_id) + GitQueue.enqueue_request(GitTaskAction.TEST_PICKME_CONFLICT, request_id, pushmanager_url = self.get_base_url()) class UnpickMeRequestServlet(RequestHandler): diff --git a/pushmanager/servlets/verifyrequest.py b/pushmanager/servlets/verifyrequest.py index 8e1348c..bce02f7 100644 --- a/pushmanager/servlets/verifyrequest.py +++ b/pushmanager/servlets/verifyrequest.py @@ -3,7 +3,6 @@ import pushmanager.core.db as db import pushmanager.core.util from pushmanager.core.requesthandler import RequestHandler -from pushmanager.core.settings import Settings from pushmanager.core.xmppclient import XMPPQueue @@ -46,9 +45,8 @@ def on_db_complete(self, success, db_results): push = db_results[0].first() unfinished_requests = db_results[2].first() - pushmanager_servername = Settings['main_app']['servername'] - pushmanager_port = ':%d' % Settings['main_app']['port'] if Settings['main_app']['port'] != 443 else '' + pushmanager_base_url = self.get_base_url() if not unfinished_requests: - msg = "All currently staged requests in https://%s/push?id=%s have been marked as verified." % \ - (pushmanager_servername + pushmanager_port, self.pushid) + msg = "All currently staged requests in %s/push?id=%s have been marked as verified." % \ + (pushmanager_base_url, self.pushid) XMPPQueue.enqueue_user_xmpp([push['user']], msg) diff --git a/pushmanager/tests/test_core_git.py b/pushmanager/tests/test_core_git.py index 56c6738..bbe62df 100644 --- a/pushmanager/tests/test_core_git.py +++ b/pushmanager/tests/test_core_git.py @@ -83,7 +83,7 @@ def mocked_update_request(self, req, duplicate_req=None): return_value=duplicate_req ), ): - pushmanager.core.git.GitQueue.verify_branch(req['id']) + pushmanager.core.git.GitQueue.verify_branch(req['id'], 'http://example.com') yield def test_get_repository_uri_basic(self): @@ -172,7 +172,7 @@ def test_process_queue_duplicate(self): # (from mock library). T.assert_in( "another request with the same revision sha", - pushmanager.core.git.GitQueue.verify_branch_failure.call_args_list[0][0][-1] + pushmanager.core.git.GitQueue.verify_branch_failure.call_args_list[0][0][1] ) def test_update_duplicate_request_discarded(self): @@ -190,7 +190,7 @@ def test_verify_branch(self): with mock.patch('pushmanager.core.git.GitCommand') as GC: GC.return_value = GC GC.run.return_value = (0, "hashashash", "") - pushmanager.core.git.GitQueue.verify_branch(1) + pushmanager.core.git.GitQueue.verify_branch(1, 'http://example.com') calls = [ mock.call('ls-remote', '-h', u'git://git.example.com/devs/bmetin', u'bmetin_fix_stuff'), mock.call.run() @@ -202,7 +202,7 @@ def test_verify_branch_successful(self): mock.patch("%s.pushmanager.core.git.MailQueue.enqueue_user_email" % __name__), mock.patch("%s.pushmanager.core.git.webhook_req" % __name__) ): - pushmanager.core.git.GitQueue.verify_branch_successful(self.fake_request) + pushmanager.core.git.GitQueue.verify_branch_successful(self.fake_request, 'http://example.com') T.assert_equal(pushmanager.core.git.MailQueue.enqueue_user_email.call_count, 1) T.assert_equal(pushmanager.core.git.webhook_req.call_count, 3) @@ -212,7 +212,7 @@ def test_verify_branch_failure(self): mock.patch("%s.pushmanager.core.git.webhook_req" % __name__), mock.patch("%s.pushmanager.core.git.logging.error" % __name__), ): - pushmanager.core.git.GitQueue.verify_branch_failure(self.fake_request, "fake failure") + pushmanager.core.git.GitQueue.verify_branch_failure(self.fake_request, "fake failure", 'http://example.com') T.assert_equal(pushmanager.core.git.MailQueue.enqueue_user_email.call_count, 1) def test_verify_branch_excluded_from_git_verification(self): diff --git a/pushmanager/tests/test_core_requesthandler.py b/pushmanager/tests/test_core_requesthandler.py index e29e4d3..e833681 100644 --- a/pushmanager/tests/test_core_requesthandler.py +++ b/pushmanager/tests/test_core_requesthandler.py @@ -2,11 +2,14 @@ import mock import testify as T + +import tornado.httpserver + from pushmanager.core.requesthandler import RequestHandler +from pushmanager.core.requesthandler import get_base_url from pushmanager.core.settings import Settings from pushmanager.testing.mocksettings import MockedSettings - class RequestHandlerTest(T.TestCase): def test_get_api_page(self): @@ -16,3 +19,74 @@ def test_get_api_page(self): RequestHandler.get_api_page("pushes"), "http://push.test.com:8043/api/pushes" ) + + def test_get_base_url_empty_headers(self): + MockedSettings['main_app'] = {'port': 1111, 'servername': 'example.com'} + request = tornado.httpserver.HTTPRequest('GET', '') + request.protocol = 'https' + + with mock.patch.dict(Settings, MockedSettings): + T.assert_equal( + get_base_url(request), + 'https://example.com:1111' + ) + + Settings['main_app']['port'] = 443 + T.assert_equal( + get_base_url(request), + 'https://example.com' + ) + + def test_get_base_url_proto_header(self): + MockedSettings['main_app'] = {'port': 1111, 'servername': 'example.com'} + request = tornado.httpserver.HTTPRequest('GET', '') + request.protocol = 'https' + request.headers['X-Forwarded-Proto'] = 'http' + + with mock.patch.dict(Settings, MockedSettings): + T.assert_equal( + get_base_url(request), + 'http://example.com:1111' + ) + + Settings['main_app']['port'] = 80 + T.assert_equal( + get_base_url(request), + 'http://example.com' + ) + + def test_get_base_url_port_header(self): + MockedSettings['main_app'] = {'port': 1111, 'servername': 'example.com'} + request = tornado.httpserver.HTTPRequest('GET', '') + request.protocol = 'https' + request.headers['X-Forwarded-Port'] = '4321' + + with mock.patch.dict(Settings, MockedSettings): + T.assert_equal( + get_base_url(request), + 'https://example.com:4321' + ) + + request.headers['X-Forwarded-Port'] = 443 + T.assert_equal( + get_base_url(request), + 'https://example.com' + ) + + + def test_RequestHandler_get_base_url(self): + MockedSettings['main_app'] = {'port': 1111, 'servername': 'example.com'} + request = tornado.httpserver.HTTPRequest('GET', '') + request.protocol = 'https' + class FakeRequest(object): + def __init__(self): + self.request = request + + with mock.patch.dict(Settings, MockedSettings): + fake_requesthandler = FakeRequest() + T.assert_equal( + #Accessing raw, unbound function, so that type of self is not checked + #http://stackoverflow.com/a/12935356 + RequestHandler.get_base_url.__func__(fake_requesthandler), + 'https://example.com:1111' + ) diff --git a/pushmanager/tests/test_servlet_newpush.py b/pushmanager/tests/test_servlet_newpush.py index 53cc38d..b87dd58 100644 --- a/pushmanager/tests/test_servlet_newpush.py +++ b/pushmanager/tests/test_servlet_newpush.py @@ -77,6 +77,7 @@ def call_on_db_complete(self, urgent=False): mocked_self.check_db_results = mock.Mock(return_value=None) mocked_self.redirect = mock.Mock(return_value=None) mocked_self.pushtype = 'normal' + mocked_self.get_base_url = mock.Mock(return_value="http://example.com") mocked_self.on_db_complete = types.MethodType(NewPushServlet.on_db_complete.im_func, mocked_self) @@ -123,14 +124,13 @@ def mocked_notifications(self): def test_send_notifications(self): """New push sends notifications via IRC, XMPP and emails.""" self.people = ["fake_user1", "fake_user2"] - self.pushurl = "/fake_push_url?id=123" + self.pushmanager_url = "https://example.com/fake_push_url?id=123" self.pushtype = "fake_puth_type" with self.mocked_notifications() as (mocked_call, mocked_mail, mocked_xmpp): - send_notifications(self.people, self.pushtype, self.pushurl) + send_notifications(self.people, self.pushtype, self.pushmanager_url) - url = "https://%s%s" % (Settings['main_app']['servername'], self.pushurl) - msg = "%s: %s push starting! %s" % (', '.join(self.people), self.pushtype, url) + msg = "%s: %s push starting! %s" % (', '.join(self.people), self.pushtype, self.pushmanager_url) mocked_call.assert_called_once_with([ '/nail/sys/bin/nodebot', '-i', @@ -145,18 +145,18 @@ def test_send_notifications(self): ) mocked_xmpp.assert_called_once_with( self.people, - "Push starting! %s" % url + "Push starting! %s" % self.pushmanager_url ) def test_send_notifications_empty_user_list(self): """If there is no pending push request we'll only send IRC and email notifications, but not XMPP messages.""" self.people = [] - self.pushurl = "fake_push_url" + self.pushmanager_url = "fake_push_url" self.pushtype = "fake_puth_type" with self.mocked_notifications() as (mocked_call, mocked_mail, mocked_xmpp): - send_notifications(self.people, self.pushtype, self.pushurl) + send_notifications(self.people, self.pushtype, self.pushmanager_url) mocked_call.assert_called_once_with([ '/nail/sys/bin/nodebot', '-i',