Merge pull request #122 from NaturalHistoryMuseum/dev

Weekly release 2023-09-25

.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
+

.gitignore

@@ -7,3 +7,4 @@ build/
 dist/
 .idea
 **/node_modules/
+.vscode/

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
    ```
 
 <!--testing-end-->

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

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):

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

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"]

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"))