Deprecate server_port from request data dictionary

`server_port` is unnecessary, since the HTTP Host header sent by the client
already includes any non-standard port.  In addition, when the Python
application server is sitting behind a reverse proxy/TLS terminator,
SERVER_PORT is likely to be wrong anyway (since it would be the server port
of the non-reverse-proxied server).

See SAML-Toolkits#273 (comment)

README.md

@@ -573,7 +573,6 @@ This parameter has the following scheme:
 req = {
     "http_host": "",
     "script_name": "",
-    "server_port": "",
     "get_data": "",
     "post_data": "",
 
@@ -594,16 +593,14 @@ def prepare_from_django_request(request):
     return {
         'http_host': request.META['HTTP_HOST'],
         'script_name': request.META['PATH_INFO'],
-        'server_port': request.META['SERVER_PORT'],
         'get_data': request.GET.copy(),
         'post_data': request.POST.copy()
     }
 
 def prepare_from_flask_request(request):
     url_data = urlparse(request.url)
     return {
-        'http_host': request.host,
-        'server_port': url_data.port,
+        'http_host': request.netloc,
         'script_name': request.path,
         'get_data': request.args.copy(),
         'post_data': request.form.copy()

demo-django/demo/views.py

@@ -20,7 +20,6 @@ def prepare_django_request(request):
         'https': 'on' if request.is_secure() else 'off',
         'http_host': request.META['HTTP_HOST'],
         'script_name': request.META['PATH_INFO'],
-        'server_port': request.META['SERVER_PORT'],
         'get_data': request.GET.copy(),
         # Uncomment if using ADFS as IdP, https://github.com/onelogin/python-saml/pull/144
         # 'lowercase_urlencoding': True,

demo-flask/index.py

@@ -21,11 +21,9 @@ def init_saml_auth(req):
 
 def prepare_flask_request(request):
     # If server is behind proxys or balancers use the HTTP_X_FORWARDED fields
-    url_data = urlparse(request.url)
     return {
         'https': 'on' if request.scheme == 'https' else 'off',
         'http_host': request.host,
-        'server_port': url_data.port,
         'script_name': request.path,
         'get_data': request.args.copy(),
         # Uncomment if using ADFS as IdP, https://github.com/onelogin/python-saml/pull/144

demo-tornado/views.py

@@ -157,9 +157,8 @@ def prepare_tornado_request(request):
 
     result = {
         'https': 'on' if request == 'https' else 'off',
-        'http_host': tornado.httputil.split_host_and_port(request.host)[0],
+        'http_host': request.host,
         'script_name': request.path,
-        'server_port': tornado.httputil.split_host_and_port(request.host)[1],
         'get_data': dataDict,
         'post_data': dataDict,
         'query_string': request.query

demo_pyramid/demo_pyramid/views.py

@@ -15,19 +15,16 @@ def init_saml_auth(req):
 
 
 def prepare_pyramid_request(request):
-    # Uncomment this portion to set the request.scheme and request.server_port
+    # Uncomment this portion to set the request.scheme
     # based on the supplied `X-Forwarded` headers.
     # Useful for running behind reverse proxies or balancers.
     #
     # if 'X-Forwarded-Proto' in request.headers:
     #    request.scheme = request.headers['X-Forwarded-Proto']
-    # if 'X-Forwarded-Port' in request.headers:
-    #    request.server_port = int(request.headers['X-Forwarded-Port'])
 
     return {
         'https': 'on' if request.scheme == 'https' else 'off',
         'http_host': request.host,
-        'server_port': request.server_port,
         'script_name': request.path,
         'get_data': request.GET.copy(),
         # Uncomment if using ADFS as IdP, https://github.com/onelogin/python-saml/pull/144

src/onelogin/saml2/utils.py

@@ -10,6 +10,7 @@
 """
 
 import base64
+import warnings
 from copy import deepcopy
 import calendar
 from datetime import datetime
@@ -254,27 +255,25 @@ def get_self_url_host(request_data):
         :rtype: string
         """
         current_host = OneLogin_Saml2_Utils.get_self_host(request_data)
-        port = ''
-        if OneLogin_Saml2_Utils.is_https(request_data):
-            protocol = 'https'
-        else:
-            protocol = 'http'
-
-        if 'server_port' in request_data and request_data['server_port'] is not None:
-            port_number = str(request_data['server_port'])
-            port = ':' + port_number
+        protocol = 'https' if OneLogin_Saml2_Utils.is_https(request_data) else 'http'
 
-            if protocol == 'http' and port_number == '80':
-                port = ''
-            elif protocol == 'https' and port_number == '443':
-                port = ''
+        if request_data.get('server_port') is not None:
+            warnings.warn(
+                'The server_port key in request data is deprecated. '
+                'The http_host key should include a port, if required.',
+                category=DeprecationWarning,
+            )
+            port_suffix = ':%s' % request_data['server_port']
+            if not current_host.endswith(port_suffix):
+                if not ((protocol == 'https' and port_suffix == ':443') or (protocol == 'http' and port_suffix == ':80')):
+                    current_host += port_suffix
 
-        return '%s://%s%s' % (protocol, current_host, port)
+        return '%s://%s' % (protocol, current_host)
 
     @staticmethod
     def get_self_host(request_data):
         """
-        Returns the current host.
+        Returns the current host (which may include a port number part).
 
         :param request_data: The request as a dict
         :type: dict
@@ -283,22 +282,11 @@ def get_self_host(request_data):
         :rtype: string
         """
         if 'http_host' in request_data:
-            current_host = request_data['http_host']
+            return request_data['http_host']
         elif 'server_name' in request_data:
-            current_host = request_data['server_name']
-        else:
-            raise Exception('No hostname defined')
-
-        if ':' in current_host:
-            current_host_data = current_host.split(':')
-            possible_port = current_host_data[-1]
-            try:
-                int(possible_port)
-                current_host = current_host_data[0]
-            except ValueError:
-                current_host = ':'.join(current_host_data)
-
-        return current_host
+            warnings.warn("The server_name key in request data is undocumented & deprecated.", category=DeprecationWarning)
+            return request_data['server_name']
+        raise Exception('No hostname defined')
 
     @staticmethod
     def is_https(request_data):
@@ -312,6 +300,7 @@ def is_https(request_data):
         :rtype: boolean
         """
         is_https = 'https' in request_data and request_data['https'] != 'off'
+        # TODO: this use of server_port should be removed too
         is_https = is_https or ('server_port' in request_data and str(request_data['server_port']) == '443')
         return is_https
 

tests/src/OneLogin/saml2_tests/response_test.py

@@ -1534,8 +1534,7 @@ def testIsInValidEncIssues(self):
         settings_2 = OneLogin_Saml2_Settings(settings_info_2)
 
         request_data = {
-            'http_host': 'pytoolkit.com',
-            'server_port': 8000,
+            'http_host': 'pytoolkit.com:8000',
             'script_name': '',
             'request_uri': '?acs',
         }

tests/src/OneLogin/saml2_tests/utils_test.py

@@ -190,7 +190,7 @@ def testGetselfhost(self):
         request_data = {
             'http_host': 'example.com:443'
         }
-        self.assertEqual('example.com', OneLogin_Saml2_Utils.get_self_host(request_data))
+        self.assertEqual('example.com:443', OneLogin_Saml2_Utils.get_self_host(request_data))
 
         request_data = {
             'http_host': 'example.com:ok'
@@ -211,11 +211,6 @@ def testisHTTPS(self):
         }
         self.assertTrue(OneLogin_Saml2_Utils.is_https(request_data))
 
-        request_data = {
-            'server_port': '80'
-        }
-        self.assertFalse(OneLogin_Saml2_Utils.is_https(request_data))
-
         request_data = {
             'server_port': '443'
         }