diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 1c19bf9..c2fab90 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -5,7 +5,7 @@ on: workflow_dispatch: jobs: - test: + test_latest: runs-on: ubuntu-latest steps: @@ -18,4 +18,18 @@ jobs: - name: Run tests env: COVERALLS_REPO_TOKEN: ${{ secrets.COVERALLS_REPO_TOKEN }} - run: docker-compose run -e COVERALLS_REPO_TOKEN ckan bash /opt/scripts/run-tests.sh -c ckanext.ldap + run: docker-compose run -e COVERALLS_REPO_TOKEN latest bash /opt/scripts/run-tests.sh -c ckanext.ldap + + test_next: + runs-on: ubuntu-latest + + steps: + - name: Checkout source code + uses: actions/checkout@v3 + + - name: Build images + run: docker-compose build + + - name: Run tests + run: docker-compose run next bash /opt/scripts/run-tests.sh -c ckanext.ldap + diff --git a/.gitignore b/.gitignore index f0abdeb..27abe72 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,4 @@ build/ dist/ .idea **/node_modules/ +.vscode/ diff --git a/README.md b/README.md index af223a7..fe7fd39 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ [![Tests](https://img.shields.io/github/actions/workflow/status/NaturalHistoryMuseum/ckanext-ldap/main.yml?style=flat-square)](https://github.com/NaturalHistoryMuseum/ckanext-ldap/actions/workflows/main.yml) [![Coveralls](https://img.shields.io/coveralls/github/NaturalHistoryMuseum/ckanext-ldap/main?style=flat-square)](https://coveralls.io/github/NaturalHistoryMuseum/ckanext-ldap) -[![CKAN](https://img.shields.io/badge/ckan-2.9.7-orange.svg?style=flat-square)](https://github.com/ckan/ckan) +[![CKAN](https://img.shields.io/badge/ckan-2.9.7+%20%7C%202.10-orange.svg?style=flat-square)](https://github.com/ckan/ckan) [![Python](https://img.shields.io/badge/python-3.6%20%7C%203.7%20%7C%203.8-blue.svg?style=flat-square)](https://www.python.org/) [![Docs](https://img.shields.io/readthedocs/ckanext-ldap?style=flat-square)](https://ckanext-ldap.readthedocs.io) @@ -175,7 +175,7 @@ To run the tests against ckan 2.9.x on Python3: configuration, so you should only need to rebuild the ckan image if you change the extension's dependencies. ```shell - docker-compose run ckan + docker-compose run latest ``` diff --git a/ckanext/ldap/plugin.py b/ckanext/ldap/plugin.py index b22bce8..561a116 100644 --- a/ckanext/ldap/plugin.py +++ b/ckanext/ldap/plugin.py @@ -154,8 +154,15 @@ def identify(self): # IAuthenticator def logout(self): + # Delete session items managed by ckanext-ldap self._delete_session_items() + # In CKAN 2.10.0+, we also need to invoke the toolkit's + # logout_user() command to clean up anything remaining + # on the CKAN side. + if toolkit.check_ckan_version(min_version="2.10.0"): + toolkit.logout_user() + # IAuthenticator def abort(self, status_code, detail, headers, comment): return status_code, detail, headers, comment diff --git a/ckanext/ldap/routes/_helpers.py b/ckanext/ldap/routes/_helpers.py index d978e09..2e6e263 100644 --- a/ckanext/ldap/routes/_helpers.py +++ b/ckanext/ldap/routes/_helpers.py @@ -11,7 +11,7 @@ import ldap import ldap.filter from ckan.common import session -from ckan.model import Session +from ckan.model import Session, User from ckan.plugins import toolkit from ckanext.ldap.lib.exceptions import UserConflictError @@ -36,13 +36,65 @@ def login_failed(notice=None, error=None): def login_success(user_name, came_from): ''' - Handle login success. Saves the user in the session and redirects to user/logged_in. + Handle login success. Behavior depends on CKAN version. + + The CKAN version is tested via ckan.plugins.toolkit.check_ckan_version(). + + CKAN < 2.10.0: + Saves user_name in the session under the 'ckanext-ldap-user' key, + then redirects to /user/logged_in. + + CKAN 2.10.0+: + Looks up the CKAN User object and invokes the login_user() command + (new in CKAN 2.10.0) to authenticate the user with flask-login. + If that succeeds, then this saves user_name in the session under + the 'ckanext-ldap-user' key before redirecting to /home/index. :param user_name: The user name + :param came_from: The value of the 'came_from' parameter sent with the + original login request ''' - session['ckanext-ldap-user'] = user_name + # Where to send the user after this function ends. + # Initialize this value with the default in CKAN < 2.10.0. + redirect_path = "user.logged_in" + + # In CKAN 2.10.0 (or, more precisely, ckan/ckan PR #6560 + # https://github.com/ckan/ckan/pull/6560), repoze.who was replaced by + # flask-login, and the `/user/logged_in` endpoint was removed. + # We need to retrieve the User object for the user and call + # toolkit.login_user(). + if toolkit.check_ckan_version(min_version="2.10.0"): + redirect_path = "home.index" + user_login_path = "user.login" + err_msg = ("An error occurred while processing your login. Please contact the " + "system administrators.") + + try: + # Look up the CKAN User object for user_name + user_obj = User.by_name(user_name) + except toolkit.ObjectNotFound as err: + log.error( + "User.by_name(%s) raised ObjectNotFound error: %s", user_name, err + ) + toolkit.h.flash_error(err_msg) + return toolkit.redirect_to(user_login_path) + + if user_obj is None: + log.error(f"User.by_name returned None for user '{user_name}'") + toolkit.h.flash_error(err_msg) + return toolkit.redirect_to(user_login_path) + + # Register the login with flask-login via the toolkit's helper function + ok = toolkit.login_user(user_obj) + if not ok: + log.error(f"toolkit.login_user() returned False for user '{user_name}'") + toolkit.h.flash_error(err_msg) + return toolkit.redirect_to(user_login_path) + + # Update the session & redirect + session["ckanext-ldap-user"] = user_name session.save() - return toolkit.redirect_to('user.logged_in', came_from=came_from) + return toolkit.redirect_to(redirect_path, came_from=came_from) def get_user_dict(user_id): diff --git a/docker-compose.yml b/docker-compose.yml index c83485e..0885664 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,10 +1,25 @@ version: "3" services: - ckan: + latest: build: context: . - dockerfile: docker/Dockerfile + dockerfile: docker/Dockerfile_latest + environment: + PYTHONUNBUFFERED: 1 + PYTHONDONTWRITEBYTECODE: 1 + depends_on: + - db + - solr + - redis + volumes: + - ./ckanext:/base/src/ckanext-ldap/ckanext + - ./tests:/base/src/ckanext-ldap/tests + + next: + build: + context: . + dockerfile: docker/Dockerfile_next environment: PYTHONUNBUFFERED: 1 PYTHONDONTWRITEBYTECODE: 1 diff --git a/docker/Dockerfile b/docker/Dockerfile_latest similarity index 100% rename from docker/Dockerfile rename to docker/Dockerfile_latest diff --git a/docker/Dockerfile_next b/docker/Dockerfile_next new file mode 100644 index 0000000..4a4363e --- /dev/null +++ b/docker/Dockerfile_next @@ -0,0 +1,21 @@ +FROM naturalhistorymuseum/ckantest:next + +# required by python-ldap +RUN apt-get -q -y install libldap2-dev libsasl2-dev \ + && apt-get -q clean \ + && rm -rf /var/lib/apt/lists/* + +WORKDIR /base/src/ckanext-ldap + +# copy over the source +COPY . . + +# install the base + test dependencies +RUN pip install -e .[test] + +# this entrypoint ensures our service dependencies (postgresql, solr and redis) are running before +# running the cmd +ENTRYPOINT ["/bin/bash", "/opt/waits/basic.sh"] + +# run the tests with coverage output +CMD ["bash", "/opt/scripts/run-tests.sh", "ckanext.ldap"] diff --git a/tests/routes/__init__.py b/tests/routes/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/routes/test_helpers.py b/tests/routes/test_helpers.py new file mode 100644 index 0000000..c773c1f --- /dev/null +++ b/tests/routes/test_helpers.py @@ -0,0 +1,165 @@ +from unittest.mock import patch, MagicMock + +import pytest + +from ckan.lib.helpers import url_for +from ckan.plugins import toolkit +from ckan.tests import factories +from ckanext.ldap.routes._helpers import login_failed, login_success + + +@pytest.mark.filterwarnings("ignore::sqlalchemy.exc.SADeprecationWarning") +@pytest.mark.usefixtures("with_request_context") +@patch("ckan.plugins.toolkit.h.flash_error") +@patch("ckan.plugins.toolkit.h.flash_notice") +def test_login_failed(flash_notice_mock: MagicMock, flash_error_mock: MagicMock): + notice = "A notice!" + error = "An error!" + response = login_failed(notice=notice, error=error) + + flash_notice_mock.assert_called_once_with(notice) + flash_error_mock.assert_called_once_with(error) + assert response.status_code == 302 + assert response.location.endswith(url_for("user.login")) + + +IS_CKAN_210_OR_HIGHER = toolkit.check_ckan_version(min_version="2.10.0") +IS_CKAN_29_OR_LOWER = not IS_CKAN_210_OR_HIGHER + + +@pytest.mark.skipif(IS_CKAN_210_OR_HIGHER, reason="requires CKAN 2.9 or lower") +@pytest.mark.filterwarnings("ignore::sqlalchemy.exc.SADeprecationWarning") +class TestLoginSuccess29: + # these tests are only run on CKAN 2.9 + + @pytest.mark.usefixtures("with_request_context") + @patch("ckanext.ldap.routes._helpers.session") + @patch("ckan.plugins.toolkit.h.flash_error") + def test_ok(self, flash_error_mock: MagicMock, mock_session: MagicMock): + username = "some_username" + came_from = "somewhere_else" + + response = login_success(username, came_from) + + # check no errors + flash_error_mock.assert_not_called() + # check the username was put in the session + mock_session.__setitem__.assert_called_once_with("ckanext-ldap-user", username) + + assert mock_session.save.called + assert response.status_code == 302 + + assert response.location.endswith( + f"{url_for('user.logged_in')}?came_from={came_from}" + ) + + +@pytest.mark.skipif(IS_CKAN_29_OR_LOWER, reason="requires CKAN 2.10 or higher") +@pytest.mark.filterwarnings("ignore::sqlalchemy.exc.SADeprecationWarning") +class TestLoginSuccess210: + # these tests are only run on CKAN 2.10 + @pytest.mark.usefixtures("with_request_context") + @patch("ckan.plugins.toolkit.login_user", return_value=True) + @patch("ckanext.ldap.routes._helpers.session") + @patch("ckan.plugins.toolkit.h.flash_error") + def test_ok( + self, + flash_error_mock: MagicMock, + mock_session: MagicMock, + login_user: MagicMock, + ): + user = factories.User() + username = user["name"] + came_from = "somewhere_else" + + response = login_success(username, came_from) + + # check no errors + flash_error_mock.assert_not_called() + # check the username was put in the session + mock_session.__setitem__.assert_called_once_with("ckanext-ldap-user", username) + + assert mock_session.save.called + assert response.status_code == 302 + + assert login_user.called + + assert response.location.endswith( + f"{url_for('home.index')}?came_from={came_from}" + ) + + @pytest.mark.usefixtures("with_request_context") + @patch("ckan.plugins.toolkit.login_user", return_value=True) + @patch("ckanext.ldap.routes._helpers.session") + @patch("ckan.plugins.toolkit.h.flash_error") + @patch("ckan.model.User.by_name", side_effect=toolkit.ObjectNotFound()) + def test_user_not_found( + self, + mock_by_name: MagicMock, + flash_error_mock: MagicMock, + mock_session: MagicMock, + login_user: MagicMock, + ): + username = "not_in_the_db" + came_from = "somewhere_else" + + response = login_success(username, came_from) + + # check an error was flashed + flash_error_mock.assert_called_once() + # check the username was not put in the session + assert not mock_session.__setitem__.called + assert not mock_session.save.called + assert not login_user.called + assert response.status_code == 302 + assert response.location.endswith(url_for("user.login")) + + @pytest.mark.usefixtures("with_request_context") + @patch("ckan.plugins.toolkit.login_user", return_value=True) + @patch("ckanext.ldap.routes._helpers.session") + @patch("ckan.plugins.toolkit.h.flash_error") + @patch("ckan.model.User.by_name", return_value=None) + def test_user_object_is_none( + self, + mock_by_name: MagicMock, + flash_error_mock: MagicMock, + mock_session: MagicMock, + login_user: MagicMock, + ): + username = "not_in_the_db" + came_from = "somewhere_else" + + response = login_success(username, came_from) + + # check an error was flashed + flash_error_mock.assert_called_once() + # check the username was not put in the session + assert not mock_session.__setitem__.called + assert not mock_session.save.called + assert not login_user.called + assert response.status_code == 302 + assert response.location.endswith(url_for("user.login")) + + @pytest.mark.usefixtures("with_request_context") + @patch("ckan.plugins.toolkit.login_user", return_value=False) + @patch("ckanext.ldap.routes._helpers.session") + @patch("ckan.plugins.toolkit.h.flash_error") + def test_login_user_not_ok( + self, + flash_error_mock: MagicMock, + mock_session: MagicMock, + login_user: MagicMock, + ): + username = "not_in_the_db" + came_from = "somewhere_else" + + response = login_success(username, came_from) + + # check an error was flashed + flash_error_mock.assert_called_once() + # check the username was not put in the session + assert not mock_session.__setitem__.called + assert not mock_session.save.called + assert not login_user.called + assert response.status_code == 302 + assert response.location.endswith(url_for("user.login"))