From 9ad7e9121d42fffa7f481edb1e4233b61cbf935f Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Fri, 22 Nov 2024 12:35:19 +0000 Subject: [PATCH] Improve add-member API functests The functional tests for the add-member API are a mess: they're in kind of an old-fashioned coding style, but more importantly there are some missing test cases and there are several tests that aren't actually testing what they're supposed to be testing because the test setup is wrong, and at least one of these tests was passing when there was actually a crash in the code. A difficulty is that the h API doesn't return specific error responses. Whenever anything goes wrong with an add-member API request h returns a 404 with the same fixed body that doesn't say specifically *what* was wrong with the request. This means that tests can't assert that they've triggered the error that they intended to trigger. They can only assert that the response is a 404. Several of add-member's functests were actually triggering a *different* 404 than what the test was intending to trigger because the test setup was wrong, but the tests were still passing. For example this test doesn't actually test what it's intending to test: def test_it_returns_404_if_authority_mismatch_on_user( self, app, factories, auth_client ): group = factories.Group() user = factories.User(authority="somewhere-else.org") res = app.post_json( "/api/groups/{pubid}/members/{userid}".format( pubid=group.pubid, userid=user.userid ), headers=self.authorization_header(auth_client), expect_errors=True, ) assert res.status_code == 404 The `group` that the test has created has an authority that doesn't match that of the `auth_client` so the request will 404 for that reason before the code ever gets to the `user`'s authority. In addition the `db_session` hasn't been commited so neither `group` or `user` have been written to the DB, which will also cause the request to 404. The test needs to create a group with the right authority, a user with the wrong authority, and add both to the DB. In this commit I've adopted an approach where class-level test fixtures set up everything that's needed for the API request to succeed. The test for the happy-path doesn't need to do any setup beyond what the fixtures have already done. Tests for error cases then change one particular thing before calling the API. This means the test can be sure that the 404 was triggered by the one thing it changed rather than by anything else. For example: class TestAddMember: def test_it(self, do_request, group, user): do_request() assert user in group.members def test_it_errors_if_the_request_isnt_authenticated(self, do_request, headers): del headers["Authorization"] do_request(status=404) ... @pytest.fixture def authclient(self): """Return a group with an authority matching authclient's.""" @pytest.fixture def headers(self, authclient): """Return an `Authorization` header for authclient.""" @pytest.fixture def group(self, authclient): """Return a group with an authority matching authclient's.""" @pytest.fixture def user(self, authclient): """Return a user with an authority matching authclient's.""" @pytest.fixture def do_request(self, db_session, group, user, headers): def do_request(...): """Commit the db_session then make an API request with group, user and headers.""" return do_request The happy path test (`test_it()`) verifies that, with everything as set up by the test fixtures, the API request succeeds and adds the user to the group. The error test (`test_it_errors_if_the_request_isnt_authenticated()`) changes one thing about the fixtures set up (it deletes the `Authorization` header) and asserts that the request 404s. This approach makes mistakes in tests much less likely. I have also gone through breaking specific parts of the context, security, view and service code and testing that the expected test fails. I did this for every functest in the class. This commit also fixes one bug in the code: if the add-member API is called with an invalid userid (one that isn't in `acct:{username}@{authority}` format) the app crashes. The previous functests actually had a test for this case, but the test was wrong: def test_it_returns_404_if_malformed_userid( self, app, factories, auth_client, db_session ): group = factories.Group() db_session.commit() res = app.post_json( "/api/groups/{pubid}/members/{userid}".format( pubid=group.pubid, userid="foo@bar.com" ), headers=self.authorization_header(auth_client), expect_errors=True, ) assert res.status_code == 404 This test creates a `group` whose authority doesn't match that of the `auth_client` and the request 404s for that reason before the code ever gets to parsing the malformed userid. So the test passes. In fact if the test had set up the group's authority correctly the app would crash on the malformed userid. This commit replaces this test with a correct one. --- h/views/api/groups.py | 3 +- tests/functional/api/groups/members_test.py | 227 ++++++++------------ tests/pyproject.toml | 4 + tests/unit/h/views/api/groups_test.py | 3 +- 4 files changed, 102 insertions(+), 135 deletions(-) diff --git a/h/views/api/groups.py b/h/views/api/groups.py index c1bdb3c4101..751c28aed12 100644 --- a/h/views/api/groups.py +++ b/h/views/api/groups.py @@ -5,6 +5,7 @@ HTTPNotFound, ) +from h.exceptions import InvalidUserId from h.i18n import TranslationString as _ from h.presenters import GroupJSONPresenter, GroupsJSONPresenter, UserJSONPresenter from h.schemas.api.group import CreateGroupAPISchema, UpdateGroupAPISchema @@ -195,7 +196,7 @@ def add_member(context: GroupContext, request): try: user = user_svc.fetch(request.matchdict["userid"]) - except ValueError as err: + except InvalidUserId as err: raise HTTPNotFound() from err if user is None: diff --git a/tests/functional/api/groups/members_test.py b/tests/functional/api/groups/members_test.py index c8afe1ea47a..5c3eee38090 100644 --- a/tests/functional/api/groups/members_test.py +++ b/tests/functional/api/groups/members_test.py @@ -3,7 +3,7 @@ import pytest from h.models import GroupMembership, Token -from h.models.auth_client import GrantType +from h.models.auth_client import AuthClient, GrantType class TestReadMembers: @@ -36,7 +36,7 @@ def test_it_returns_list_of_members_if_user_has_access_to_private_group( res = app.get( "/api/groups/{pubid}/members".format(pubid=group.pubid), - headers=self.authorization_header(token), + headers=token_authorization_header(token), ) returned_usernames = [member["username"] for member in res.json] @@ -51,7 +51,7 @@ def test_it_returns_404_if_user_does_not_have_read_access_to_group( res = app.get( "/api/groups/{pubid}/members".format(pubid=group.pubid), - headers=self.authorization_header(factories.DeveloperToken()), + headers=token_authorization_header(factories.DeveloperToken()), expect_errors=True, ) @@ -62,179 +62,135 @@ def test_it_returns_empty_list_if_no_members_in_group(self, app): assert res.json == [] - @staticmethod - def authorization_header(token) -> dict: - """Return an Authorization header for the given developer token.""" - return {"Authorization": "Bearer {}".format(token.value)} - class TestAddMember: - def test_it_returns_http_204_when_successful( - self, app, auth_client, db_session, factories + def test_it(self, do_request, group, user): + do_request() + + assert user in group.members + + def test_it_does_nothing_if_the_user_is_already_a_member_of_the_group( + self, do_request, group, user ): - third_party_user = factories.User(authority="thirdparty.com") - third_party_group = factories.Group(authority="thirdparty.com") - db_session.commit() + group.memberships.append(GroupMembership(user=user)) - res = app.post_json( - "/api/groups/{pubid}/members/{userid}".format( - pubid=third_party_group.pubid, userid=third_party_user.userid - ), - headers=self.authorization_header(auth_client), - ) + do_request() - assert res.status_code == 204 + assert user in group.members - def test_it_adds_member_to_group(self, app, auth_client, db_session, factories): - third_party_user = factories.User(authority="thirdparty.com") - third_party_group = factories.Group(authority="thirdparty.com") - db_session.commit() + def test_it_errors_if_the_pubid_is_unknown(self, do_request): + do_request(pubid="UNKNOWN_PUBID", status=404) - app.post_json( - "/api/groups/{pubid}/members/{userid}".format( - pubid=third_party_group.pubid, userid=third_party_user.userid - ), - headers=self.authorization_header(auth_client), - ) + def test_it_errors_if_the_userid_is_unknown(self, do_request, authclient): + do_request(userid="acct:UNKOWN_USERNAME@{authclient.authority}", status=404) - assert third_party_user in third_party_group.members + def test_it_errors_if_the_userid_is_invalid(self, do_request): + do_request(userid="INVALID_USERID", status=404) - def test_it_ignores_forwarded_user_header( - self, app, factories, db_session, auth_client - ): - third_party_user = factories.User(authority="thirdparty.com") - third_party_group = factories.Group(authority="thirdparty.com") - headers = self.authorization_header(auth_client) - user2 = factories.User(authority="thirdparty.com") - db_session.commit() + def test_it_errors_if_the_request_isnt_authenticated(self, do_request, headers): + del headers["Authorization"] - headers["X-Forwarded-User"] = third_party_user.userid + do_request(status=404) - res = app.post_json( - "/api/groups/{pubid}/members/{userid}".format( - pubid=third_party_group.pubid, userid=third_party_user.userid - ), - headers=headers, - ) + def test_it_errors_if_the_request_has_token_authentication( + self, do_request, factories, user, headers + ): + token = factories.DeveloperToken(user=user) + headers.update(token_authorization_header(token)) - assert third_party_user in third_party_group.members - assert user2 not in third_party_group.members - assert res.status_code == 204 + do_request(status=404) - def test_it_is_idempotent(self, app, auth_client, db_session, factories): - third_party_user = factories.User(authority="thirdparty.com") - third_party_group = factories.Group(authority="thirdparty.com") - db_session.commit() + def test_it_errors_if_the_groups_authority_doesnt_match( + self, do_request, factories + ): + group = factories.Group(authority="other") - app.post_json( - "/api/groups/{pubid}/members/{userid}".format( - pubid=third_party_group.pubid, userid=third_party_user.userid - ), - headers=self.authorization_header(auth_client), - ) + do_request(pubid=group.pubid, status=404) - res = app.post_json( - "/api/groups/{pubid}/members/{userid}".format( - pubid=third_party_group.pubid, userid=third_party_user.userid - ), - headers=self.authorization_header(auth_client), - ) + def test_it_errors_if_the_users_authority_doesnt_match(self, do_request, factories): + user = factories.User(authority="other") - assert third_party_user in third_party_group.members - assert res.status_code == 204 + do_request(userid=user.userid, status=404) - def test_it_returns_404_if_authority_mismatch_on_user( - self, app, factories, auth_client + def test_it_errors_if_the_groups_and_users_authorities_both_dont_match( + self, do_request, factories ): - group = factories.Group() - user = factories.User(authority="somewhere-else.org") + # The user and group have the same authority but it is different from + # the authclient's authority. + group = factories.Group(authority="other") + user = factories.User(authority=group.authority) - res = app.post_json( - "/api/groups/{pubid}/members/{userid}".format( - pubid=group.pubid, userid=user.userid - ), - headers=self.authorization_header(auth_client), - expect_errors=True, - ) + do_request(pubid=group.pubid, userid=user.userid, status=404) - assert res.status_code == 404 - - def test_it_returns_404_if_malformed_userid( - self, app, factories, auth_client, db_session + def test_it_ignores_forwarded_users( + self, do_request, factories, authclient, headers, user, group ): - group = factories.Group() - db_session.commit() + forwarded_user = factories.User(authority=authclient.authority) + headers["X-Forwarded-User"] = forwarded_user.userid - res = app.post_json( - "/api/groups/{pubid}/members/{userid}".format( - pubid=group.pubid, userid="foo@bar.com" - ), - headers=self.authorization_header(auth_client), - expect_errors=True, - ) + do_request() - assert res.status_code == 404 + # It has added the user from the URL to the group. + assert user in group.members + # It has *not* added the user from the X-Forwarded-User header to the group. + assert forwarded_user not in group.members - def test_it_returns_404_if_authority_mismatch_on_group( - self, app, factories, user, auth_client, db_session + @pytest.mark.xfail + def test_me_alias_with_forwarded_user( + self, do_request, factories, authclient, headers, group ): - group = factories.Group(authority="somewhere-else.org") - db_session.commit() + forwarded_user = factories.User(authority=authclient.authority) + headers["X-Forwarded-User"] = forwarded_user.userid - res = app.post_json( - "/api/groups/{pubid}/members/{userid}".format( - pubid=group.pubid, userid=user.userid - ), - headers=self.authorization_header(auth_client), - expect_errors=True, - ) + do_request(userid="me") - assert res.status_code == 404 + # If the "me" alias is used with an X-Forwarded-User header it should + # add the forwarded user to the group. + assert forwarded_user in group.members - def test_it_returns_404_if_missing_auth(self, app, user, db_session, factories): - group = factories.Group() - db_session.commit() + def test_me_alias_with_forwarded_user_with_wrong_authority( + self, do_request, factories, headers + ): + forwarded_user = factories.User(authority="other") + headers["X-Forwarded-User"] = forwarded_user.userid - res = app.post_json( - "/api/groups/{pubid}/members/{userid}".format( - pubid=group.pubid, userid=user.userid - ), - expect_errors=True, - ) + do_request(userid="me", status=404) - assert res.status_code == 404 + def test_me_alias_without_forwarded_user(self, do_request): + do_request(userid="me", status=404) - def test_it_returns_404_with_token_auth( - self, app, token_auth_header, user, db_session, factories - ): - group = factories.Group() - db_session.commit() - - res = app.post_json( - "/api/groups/{pubid}/members/{userid}".format( - pubid=group.pubid, userid=user.userid - ), - headers=token_auth_header, - expect_errors=True, - ) + @pytest.fixture + def do_request(self, db_session, app, group, user, headers): + def do_request(pubid=group.pubid, userid=user.userid, status=204): + db_session.commit() + return app.post_json( + f"/api/groups/{pubid}/members/{userid}", headers=headers, status=status + ) - assert res.status_code == 404 + return do_request @pytest.fixture - def auth_client(self, factories): + def authclient(self, factories): return factories.ConfidentialAuthClient( authority="thirdparty.com", grant_type=GrantType.client_credentials ) - @staticmethod - def authorization_header(auth_client) -> dict: - """Return an Authorization header for the given AuthClient.""" + @pytest.fixture + def headers(self, authclient): user_pass = "{client_id}:{secret}".format( - client_id=auth_client.id, secret=auth_client.secret + client_id=authclient.id, secret=authclient.secret ) encoded = base64.standard_b64encode(user_pass.encode("utf-8")) return {"Authorization": "Basic {creds}".format(creds=encoded.decode("ascii"))} + @pytest.fixture + def group(self, authclient, factories): + return factories.Group(authority=authclient.authority) + + @pytest.fixture + def user(self, authclient, factories): + return factories.User(authority=authclient.authority) + class TestRemoveMember: def test_it(self, app, db_session, factories): @@ -257,3 +213,8 @@ def test_it(self, app, db_session, factories): assert user not in group.members assert user in other_group.members assert other_user in group.members + + +def token_authorization_header(token) -> dict: + """Return an Authorization header for the given developer token.""" + return {"Authorization": "Bearer {}".format(token.value)} diff --git a/tests/pyproject.toml b/tests/pyproject.toml index c63660dd238..a897e94a12a 100644 --- a/tests/pyproject.toml +++ b/tests/pyproject.toml @@ -82,6 +82,10 @@ disable = [ # tests/unit/h/models/document/_uri_test.py:17:0: I0021: Useless suppression of 'comparison-with-callable' (useless-suppression) # Just disable the whole rule globally. "comparison-with-callable", + + # This is intermittently causing false-positives with SQLAlchemy's Mapped, for example: + # h/models/token.py:57:10: E1136: Value 'Mapped' is unsubscriptable (unsubscriptable-object) + "unsubscriptable-object", ] # Just disable PyLint's name style checking for the tests, because we diff --git a/tests/unit/h/views/api/groups_test.py b/tests/unit/h/views/api/groups_test.py index b7deeb76d74..23e21687465 100644 --- a/tests/unit/h/views/api/groups_test.py +++ b/tests/unit/h/views/api/groups_test.py @@ -10,6 +10,7 @@ import h.views.api.groups as views from h import presenters +from h.exceptions import InvalidUserId from h.models import User from h.schemas.base import ValidationError from h.traversal.group import GroupContext @@ -468,7 +469,7 @@ def test_it( assert isinstance(response, HTTPNoContent) def test_it_with_malformed_userid(self, context, pyramid_request, user_service): - user_service.fetch.side_effect = ValueError() + user_service.fetch.side_effect = InvalidUserId("invalid_userid") with pytest.raises(HTTPNotFound) as exc_info: views.add_member(context, pyramid_request)