Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check network access for Request an Account page #1476

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changes/1476.a.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
check network access for account creation
1 change: 1 addition & 0 deletions changes/1476.b.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
check network access for login page
45 changes: 45 additions & 0 deletions ckanext/canada/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@
from ckanext.xloader.utils import XLoaderFormats
except ImportError:
XLoaderFormats = None
import os
import ipaddress
from flask import has_request_context


ORG_MAY_PUBLISH_OPTION = 'canada.publish_datasets_organization_name'
Expand Down Expand Up @@ -774,6 +777,48 @@ def date_field(field, pkg):
return pkg.get(field, None)


def registry_network_access():
"""
Only allow requests from GOC network to access
user account registration view
"""
if not has_request_context():
JVickery-TBS marked this conversation as resolved.
Show resolved Hide resolved
# outside of request (system functions), allow
return True

remote_addr = request.headers.get('X-Forwarded-For') or \
request.environ.get('REMOTE_ADDR')
try:
JVickery-TBS marked this conversation as resolved.
Show resolved Hide resolved
client_ip = ipaddress.ip_address(text_type(remote_addr))
except ValueError:
return False

netlist_path = config.get('ckanext.canada.registry_network_list', '')
if not netlist_path:
return False
if not os.path.isfile(netlist_path):
JVickery-TBS marked this conversation as resolved.
Show resolved Hide resolved
return False

with open(netlist_path) as allow_list:
for line in allow_list:
# the netlist_path file is a combination of text, ip addresses and subnets
line = text_type(line.strip())
try:
ip = ipaddress.ip_address(line)
if client_ip == ip:
return True
except ValueError:
pass

try:
ip_network = ipaddress.ip_network(line, False)
if client_ip in ip_network:
return True
except ValueError:
pass
return False


def split_piped_bilingual_field(field_text, client_lang):
if field_text is not None and ' | ' in field_text:
return field_text.split(' | ')[1 if client_lang == 'fr' else 0]
Expand Down
21 changes: 20 additions & 1 deletion ckanext/canada/plugins.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
ObjectNotFound,
_,
get_validator,
request
request,
abort,
)

from ckanext.canada import validators
Expand Down Expand Up @@ -68,6 +69,7 @@ class CanadaSecurityPlugin(CkanSecurityPlugin):
p.implements(p.IResourceController, inherit=True)
p.implements(p.IValidators, inherit=True)
p.implements(p.IConfigurer)
p.implements(p.IAuthenticator, inherit=True)

def update_config(self, config):
# Disable auth settings
Expand Down Expand Up @@ -102,6 +104,22 @@ def get_validators(self):
'canada_security_upload_presence':
validators.canada_security_upload_presence}

# IAuthenticator
def identify(self):
controller, action = p.toolkit.get_endpoint()
blueprint = '.'.join((controller, action))
restricted_blueprints = [
'canada.login',
'user.login',
'user.request_reset',
'canada.recover_username',
'canada.register',
'canada.action',
'api.action', # change if need to narrow down the scope
]
if blueprint in restricted_blueprints and not helpers.registry_network_access():
return abort(403)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might want to log failed access attempts so we can report on them

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Do you think we need a separate log handler for "Registry Access"? As we will need to also log failed login attempts and all login sessions.

We could have a new log handler for ckanext.canada.user_access to go to its own log file ckan_registry_access.log.

Or should we just keep it in the normal logs and have Log Analytics handle all this stuff @wardi ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JVickery-TBS I don't have a strong opinion either way. As long as the data can be pulled out of Log Analytics it shouldn't matter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wardi and @JVickery-TBS let's implement logging as a separate feature for both registry network access and login access. It is one of the requirements given to us by imtd/security. I would prefer to implement it as a separate log handler so that it is easier for us to investigate any issues reported by helpdesk.



class CanadaDatasetsPlugin(SchemingDatasetsPlugin):
"""
Expand Down Expand Up @@ -392,6 +410,7 @@ def get_helpers(self):
'canada_check_access',
'get_user_email',
'get_loader_status_badge',
'registry_network_access',
])

# IConfigurable
Expand Down
19 changes: 13 additions & 6 deletions ckanext/canada/templates/internal/error_document_template.html
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,19 @@
<p class="mrgn-tp-lg"><strong>{{ content }}</strong></p>
{% if code == 403 %}
{% if not c.userobj %}
<p class="mrgn-tp-lg">{{ _('You are not logged in to the Open Government Registry.
Note that your login expires after 1 hour of inactivity.
Click on the log in button below to log in.') }}</p>
<p><a href="{{ h.url_for('user.login') }}" class="btn btn-primary">
<span class="fa fa-sign-in"></span><span class="text">{{ _('Log in') }}</span>
</a></p>
{% if not h.registry_network_access() %}
<p class="mrgn-tp-lg">{{ _('This application is only available to authorized
Government of Canada departments and agencies. Please contact the support team at
<a href="mailto:[email protected]">[email protected]</a>
to request access.') }}</p>
{% else %}
<p class="mrgn-tp-lg">{{ _('You are not logged in to the Open Government Registry.
Note that your login expires after 1 hour of inactivity.
Click on the log in button below to log in.') }}</p>
<p><a href="{{ h.url_for('user.login') }}" class="btn btn-primary">
<span class="fa fa-sign-in"></span><span class="text">{{ _('Log in') }}</span>
</a></p>
{% endif %}
{% endif %}
{% elif code == 404 %}
<p class="mrgn-tp-lg">{{ _('We couldn\'t find that Web page') }}</p>
Expand Down
10 changes: 5 additions & 5 deletions ckanext/canada/templates/internal/header.html
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@
{% endblock %}
</ul>
</div>
{% else %}
{% elif h.registry_network_access() %}
<div id="usr-logged" class="col-md-6">
<ul class="list-inline">
{% block header_account_notlogged %}
<li>
<a href="{{ h.url_for('user.login') }}" class="btn btn-primary">
<span class="fa fa-sign-in"></span>
<span class="text">{{ _('Log in') }}</span>
</a>
<a href="{{ h.url_for('user.login') }}" class="btn btn-primary">
<span class="fa fa-sign-in"></span>
<span class="text">{{ _('Log in') }}</span>
</a>
</li>
{% endblock %}
</ul>
Expand Down
20 changes: 18 additions & 2 deletions ckanext/canada/tests/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import os
import mock
import functools
from io import StringIO
from pyfakefs import fake_filesystem
from cgi import FieldStorage
from factory import LazyAttribute, Sequence
Expand All @@ -14,12 +15,15 @@


real_open = open
real_isfile = os.path.isfile
MOCK_IP_ADDRESS = u'174.116.80.148'
MOCK_IP_LIST_FILE = u'test_ip_list'
_fs = fake_filesystem.FakeFilesystem()
_mock_os = fake_filesystem.FakeOsModule(_fs)
_mock_file_open = fake_filesystem.FakeFileOpen(_fs)


def _mock_open_if_open_fails(*args, **kwargs):
def _mock_open(*args, **kwargs):
try:
return real_open(*args, **kwargs)
except (OSError, IOError):
Expand All @@ -30,7 +34,7 @@ def mock_uploads(func):
@helpers.change_config('ckan.storage_path', '/doesnt_exist')
@mock.patch.object(ckan.lib.uploader, 'os', _mock_os)
@mock.patch.object(builtins, 'open',
side_effect=_mock_open_if_open_fails)
side_effect=_mock_open)
@mock.patch.object(ckan.lib.uploader, '_storage_path',
new='/doesnt_exist')
@functools.wraps(func)
Expand All @@ -51,6 +55,18 @@ def __init__(self, fp, filename):
self.list = None


def mock_isfile(filename):
if MOCK_IP_LIST_FILE in filename:
return True
return real_isfile(filename)


def mock_open_ip_list(*args, **kwargs):
if args and MOCK_IP_LIST_FILE in args[0]:
return StringIO(MOCK_IP_ADDRESS)
return _mock_open(*args, **kwargs)


class CanadaUser(User):
@classmethod
def _create(self, target_class, *args, **kwargs):
Expand Down
72 changes: 71 additions & 1 deletion ckanext/canada/tests/test_logic.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,17 @@
# -*- coding: UTF-8 -*-
import pytest
import mock

from ckanext.canada.tests import CanadaTestBase
from ckanapi import LocalCKAN
from ckan.plugins.toolkit import h

from ckanext.canada.tests.factories import CanadaResource as Resource
from ckanext.canada.tests.factories import (
CanadaResource as Resource,
mock_isfile,
mock_open_ip_list,
MOCK_IP_ADDRESS,
)


class TestCanadaLogic(CanadaTestBase):
Expand Down Expand Up @@ -45,3 +54,64 @@ def test_data_dictionary(self):
assert 'notes_fr' in ds_info['fields'][0]['info']
assert ds_info['fields'][0]['info']['notes_fr'] == 'Example Description FR'


@pytest.mark.usefixtures('with_request_context')
class TestPublicRegistry(CanadaTestBase):
@classmethod
def setup_method(self, method):
"""Method is called at class level before EACH test methods of the class are called.
Setup any state specific to the execution of the given class methods.
"""
super(TestPublicRegistry, self).setup_method(method)
self.extra_environ_tester = {'REMOTE_USER': str(u""), 'REMOTE_ADDR': MOCK_IP_ADDRESS}
self.extra_environ_tester_bad_ip = {'REMOTE_USER': str(u""), 'REMOTE_ADDR': '174.116.80.142'}

@mock.patch('os.path.isfile', mock_isfile)
@mock.patch('__builtin__.open', mock_open_ip_list)
def test_register_bad_ip_address(self, app):
offset = h.url_for('user.register')
response = app.get(offset, extra_environ=self.extra_environ_tester_bad_ip)

assert response.status_code == 403

@mock.patch('os.path.isfile', mock_isfile)
@mock.patch('__builtin__.open', mock_open_ip_list)
def test_register_good_ip_address(self, app):
offset = h.url_for('user.register')
response = app.get(offset, extra_environ=self.extra_environ_tester)

assert response.status_code == 200

@mock.patch('os.path.isfile', mock_isfile)
@mock.patch('__builtin__.open', mock_open_ip_list)
@pytest.mark.skip(reason="No mock for repoze handler in tests")
def test_login_bad_ip_address(self, app):
offset = h.url_for('canada.login')
response = app.get(offset, extra_environ=self.extra_environ_tester_bad_ip)
#FIXME: repoze handler in tests
assert response.status_code == 403

@mock.patch('os.path.isfile', mock_isfile)
@mock.patch('__builtin__.open', mock_open_ip_list)
@pytest.mark.skip(reason="No mock for repoze handler in tests")
def test_login_good_ip_address(self, app):
offset = h.url_for('canada.login')
response = app.get(offset, extra_environ=self.extra_environ_tester)
#FIXME: repoze handler in tests
assert response.status_code == 200

@mock.patch('os.path.isfile', mock_isfile)
@mock.patch('__builtin__.open', mock_open_ip_list)
def test_api_bad_ip_address(self, app):
offset = h.url_for('api.action', logic_function='status_show')
response = app.get(offset, extra_environ=self.extra_environ_tester_bad_ip)

assert response.status_code == 403

@mock.patch('os.path.isfile', mock_isfile)
@mock.patch('__builtin__.open', mock_open_ip_list)
def test_api_good_ip_address(self, app):
offset = h.url_for('api.action', logic_function='status_show')
response = app.get(offset, extra_environ=self.extra_environ_tester)

assert response.status_code == 200
28 changes: 22 additions & 6 deletions ckanext/canada/tests/test_webforms.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
# -*- coding: UTF-8 -*-
import os
import mock
from ckanext.canada.tests import CanadaTestBase
import pytest
from urlparse import urlparse
from StringIO import StringIO
from cStringIO import StringIO
from openpyxl.workbook import Workbook
from ckan.plugins.toolkit import h
from ckanapi import (
Expand All @@ -15,7 +17,10 @@
from ckan.tests.factories import Sysadmin
from ckanext.canada.tests.factories import (
CanadaOrganization as Organization,
CanadaUser as User
CanadaUser as User,
mock_isfile,
mock_open_ip_list,
MOCK_IP_ADDRESS,
)

from ckanext.recombinant.tables import get_chromo
Expand Down Expand Up @@ -182,10 +187,13 @@ def setup_method(self, method):
Setup any state specific to the execution of the given class methods.
"""
super(TestNewUserWebForms, self).setup_method(method)
self.extra_environ_tester = {'REMOTE_USER': str(u"")}
self.extra_environ_tester = {'REMOTE_USER': str(u""), 'REMOTE_ADDR': MOCK_IP_ADDRESS}
self.extra_environ_tester_bad_ip = {'REMOTE_USER': str(u""), 'REMOTE_ADDR': '174.116.80.142'}
self.org = Organization()


@mock.patch('os.path.isfile', mock_isfile)
@mock.patch('__builtin__.open', mock_open_ip_list)
def test_new_user_required_fields(self, app):
offset = h.url_for('user.register')
response = app.get(offset, extra_environ=self.extra_environ_tester)
Expand All @@ -195,12 +203,20 @@ def test_new_user_required_fields(self, app):
response = app.post(offset,
data=self._filled_new_user_form(),
extra_environ=self.extra_environ_tester,
follow_redirects=True)
follow_redirects=False)

# test environ does not work with GET requests, use headers instead
offset = _get_relative_offset_from_response(response)
response = app.get(offset, headers={'X-Forwarded-For': MOCK_IP_ADDRESS})

assert 'Account Created' in response.body
assert 'Thank you for creating your account for the Open Government registry' in response.body
assert response.status_code == 200
#FIXME: repoze handler in tests
# assert 'Account Created' in response.body
# assert 'Thank you for creating your account for the Open Government registry' in response.body


@mock.patch('os.path.isfile', mock_isfile)
@mock.patch('__builtin__.open', mock_open_ip_list)
def test_new_user_missing_fields(self, app):
offset = h.url_for('user.register')
response = app.get(offset, extra_environ=self.extra_environ_tester)
Expand Down
2 changes: 2 additions & 0 deletions test-core.ini
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ ckan.plugins = validation canada_forms canada_internal canada_public
# no default views for tests...
# ckan.views.default_views = []

ckanext.canada.registry_network_list = /test_ip_list

# we have tests for web user registration form
ckan.auth.create_user_via_web = true

Expand Down
Loading