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 all 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
38 changes: 38 additions & 0 deletions ckanext/canada/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
from ckanext.xloader.utils import XLoaderFormats
except ImportError:
XLoaderFormats = None
import os
import ipaddress


ORG_MAY_PUBLISH_OPTION = 'canada.publish_datasets_organization_name'
Expand Down Expand Up @@ -774,6 +776,42 @@ 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
"""
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 or not os.path.isfile(netlist_path):
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
49 changes: 22 additions & 27 deletions ckanext/canada/templates/internal/error_document_template.html
Original file line number Diff line number Diff line change
Expand Up @@ -2,41 +2,36 @@

{% block page_heading %}
{% block subtitle %}
{% if g.debug %}
{{ gettext('Error %(error_code)s', error_code=code) }}
{% else %}
{{ _("Open Government Registry") }}
{% endif %}
{{ _("Open Government Registry") }}
{% endblock %}
{% endblock %}

{% block primary %}
<div class="row">
<div class="col-md-12">
{% if g.debug %}
{% if name %}
<h1>{{ code }} {{ name }}</h1>
{% endif %}
<div class="mrgn-tp-lg mrgn-bt-lg"><p><strong>{{ content}}</strong></p></div>
{% include 'snippets/debug.html' %}
{% else %}
<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>
<p class="mrgn-tp-lg"><strong>{{ content }}</strong></p>
{% if code == 403 %}
{% if not c.userobj %}
{% 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:open-ouvert@tbs-sct.gc.ca">open-ouvert@tbs-sct.gc.ca</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 %}
{% elif code == 404 %}
<p class="mrgn-tp-lg">{{ _('We couldn\'t find that Web page') }}</p>
{% elif code == 500 %}
<p class="mrgn-tp-lg">{{ _('We encountered an error and are unable to serve your request. Please try again in short time or contact <a href="mailto:open-ouvert@tbs-sct.gc.ca">open-ouvert@tbs-sct.gc.ca</a> if you need immediate assistance or if this issue persists. Note that your login expires after 1 hour of inactivity.') | safe }}</p>
{% else %}
<p class="mrgn-tp-lg">{{ _('We encountered an error') }}</p>
{% endif %}
{% elif code == 404 %}
<p class="mrgn-tp-lg">{{ _('We couldn\'t find that Web page') }}</p>
{% elif code == 500 %}
<p class="mrgn-tp-lg">{{ _('We encountered an error and are unable to serve your request. Please try again in short time or contact <a href="mailto:open-ouvert@tbs-sct.gc.ca">open-ouvert@tbs-sct.gc.ca</a> if you need immediate assistance or if this issue persists. Note that your login expires after 1 hour of inactivity.') | safe }}</p>
{% else %}
<p class="mrgn-tp-lg">{{ _('We encountered an error') }}</p>
{% endif %}
</div>
</div>
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
30 changes: 30 additions & 0 deletions ckanext/canada/tests/fixtures.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,37 @@
import os
import pytest
from io import StringIO
from ckan.lib import uploader
from pyfakefs import fake_filesystem

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_file_open = fake_filesystem.FakeFileOpen(_fs)


@pytest.fixture
def mock_uploads(ckan_config, monkeypatch, tmp_path):
monkeypatch.setitem(ckan_config, "ckan.storage_path", str(tmp_path))
monkeypatch.setattr(uploader, "_storage_path", str(tmp_path))


def _mock_open(*args, **kwargs):
try:
return real_open(*args, **kwargs)
except (OSError, IOError):
return _mock_file_open(*args, **kwargs)


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)
74 changes: 73 additions & 1 deletion ckanext/canada/tests/test_logic.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,19 @@
# -*- 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,
)
from ckanext.canada.tests.fixtures import (
mock_isfile,
mock_open_ip_list,
MOCK_IP_ADDRESS,
)


class TestCanadaLogic(CanadaTestBase):
Expand Down Expand Up @@ -45,3 +56,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('builtins.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('builtins.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('builtins.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('builtins.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('builtins.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('builtins.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
27 changes: 22 additions & 5 deletions ckanext/canada/tests/test_webforms.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# -*- coding: UTF-8 -*-
import mock
from ckanext.canada.tests import CanadaTestBase
import pytest
from urllib.parse import urlparse
Expand All @@ -15,7 +16,12 @@
from ckan.tests.factories import Sysadmin
from ckanext.canada.tests.factories import (
CanadaOrganization as Organization,
CanadaUser as User
CanadaUser as User,
)
from ckanext.canada.tests.fixtures import (
mock_isfile,
mock_open_ip_list,
MOCK_IP_ADDRESS,
)

from ckanext.recombinant.tables import get_chromo
Expand Down Expand Up @@ -183,10 +189,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('builtins.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 @@ -196,12 +205,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('builtins.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