fix: reject fields without profile, remove duplicate fields and count field filtering by set

.github/CONTRIBUTING.md

@@ -196,7 +196,6 @@ suite automatically before every commit.
 The following checks are enforced:
 
 - ✅ `ruff` for linting and formatting (with auto-fix), running from `uv.lock` so it is the same version as CI
-- ✅ `compileall` to catch syntax errors in `tests/`, which ruff does not lint
 - ✅ `uv-lock` to check that `uv.lock` matches `pyproject.toml`
 - ✅ `mdformat` to format Markdown files (with GFM support)
 - ✅ `end-of-file-fixer`, `trailing-whitespace`, `check-yaml`, `check-toml`, `check-added-large-files` for formatting

.pre-commit-config.yaml

@@ -28,7 +28,7 @@ repos:
     hooks:
       # Ruff runs from uv.lock, so it is the same version CI runs. --frozen leaves a stale lock for
       # the uv-lock hook to report instead of rewriting it, and --force-exclude applies ruff's
-      # exclude list (tests/) to the file names pre-commit passes.
+      # exclude list (AGENTS.md, .agents/) to the file names pre-commit passes.
       - id: ruff-check
         name: ruff check
         entry: uv run --frozen ruff check --force-exclude --fix --exit-non-zero-on-fix
@@ -43,13 +43,6 @@ repos:
         types_or: [ python, pyi ]
         require_serial: true
 
-      # Catches syntax errors in tests/, which ruff does not lint
-      - id: compileall
-        name: Compile Python sources
-        entry: uv run --frozen python -m compileall -q
-        language: system
-        types: [ python ]
-
       - id: uv-lock
         name: Check uv.lock matches pyproject.toml
         entry: uv lock --check

README.md

@@ -172,16 +172,16 @@ object, with the user's profile information if requested.
 
 #### Request Parameters
 
-| **Parameter** | **Optional** | **Type**    | **Default** | **Description**                                                                                                                                                                                                     |
-| ------------- | ------------ | ----------- | ----------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| `username`    | No           | `str`       |             | The user's SRN, PRN, email address, or phone number. Maximum length is **96 characters**.                                                                                                                           |
-| `password`    | No           | `str`       |             | The user's password, up to **128 characters**, counted exactly as typed. It is sent only to PESU Academy, and never stored or logged.                                                                               |
-| `profile`     | Yes          | `boolean`   | `False`     | Whether to fetch profile information. This makes a second call to PESU Academy, so it takes longer                                                                                                                  |
-| `fields`      | Yes          | `list[str]` | `None`      | Which [`ProfileObject`](#profileobject) fields to return. Only used when `profile` is `true`. If not provided, all fields are returned. Fields come back in the table's order, whatever order they are asked for in |
+| **Parameter** | **Optional** | **Type**    | **Default** | **Description**                                                                                                                                                                                                                                                                                            |
+| ------------- | ------------ | ----------- | ----------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `username`    | No           | `str`       |             | The user's SRN, PRN, email address, or phone number. Maximum length is **96 characters**.                                                                                                                                                                                                                  |
+| `password`    | No           | `str`       |             | The user's password, up to **128 characters**, counted exactly as typed. It is sent only to PESU Academy, and never stored or logged.                                                                                                                                                                      |
+| `profile`     | Yes          | `boolean`   | `False`     | Whether to fetch profile information. This makes a second call to PESU Academy, so it takes longer                                                                                                                                                                                                         |
+| `fields`      | Yes          | `list[str]` | `None`      | Which [`ProfileObject`](#profileobject) fields to return. A list can only be given when `profile` is `true`; otherwise the request is a `400`. If not provided or `null`, all fields are returned. Repeated names are ignored. Fields come back in the table's order, whatever order they are asked for in |
 
 The request body is validated strictly. A missing or empty `username` or `password`, one that is not valid text, a `username` with a
 line break or another invisible character, a value of the wrong type (such as
-the string `"true"` for `profile`), an unknown key, an empty `fields` list, or an unknown field name is rejected with a
+the string `"true"` for `profile`), an unknown key, an empty `fields` list, `fields` without `profile: true`, or an unknown field name is rejected with a
 `400`. So is a body that cannot be read at all, such as JSON that is not valid UTF-8, with the fixed message
 `Could not parse request body.`
 
@@ -368,7 +368,7 @@ A few definitions that are easy to assume wrongly:
 | ---------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
 | `authentication_requests_total{profile}` | Authentication requests, split by whether profile data was asked for                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                    |
 | `authentication_results_total{result}`   | Attempts by outcome: `success` or `failure`. Deliberately only those two; `errors_total` already names the exception class, and recording the reason here too would put one fact in two places. This family exists for the login **success rate**, where success and failure share a denominator                                                                                                                                                                                                                                                                                                                                                                                        |
-| `profile_field_filtering_total{enabled}` | Profile fetches, split by whether the caller narrowed the returned fields. Recorded where the branch is taken, so a caller passing exactly the default list counts as `false`                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                           |
+| `profile_field_filtering_total{enabled}` | Profile fetches, split by whether the caller narrowed the returned fields. Recorded where the branch is taken, so a caller passing exactly the default list, in any order, counts as `false`                                                                                                                                                                                                                                                                                                                                                                                                                                                                                            |
 | `profile_parse_errors_total{reason}`     | Profile response problems by what broke: `response_structure` (the response could not be parsed, a `422`), `unknown_campus_code` (a campus name with no known campus code), `missing_field` (a field PESU Academy did not send at all) and `unexpected_value` (a field sent in an unexpected shape). The last two are counted once per field, returned to the caller as `null`, and named in the log. `response_structure`, `missing_field` and `unexpected_value` mean PESU Academy's API changed; `unknown_campus_code` usually means a new campus, or a new way of writing a campus's name. A field PESU Academy sends with no value is not counted: that is a student with no value |
 
 **Upstream (PESU Academy)**

app/docs/authenticate.py

@@ -156,8 +156,9 @@
             "description": (
                 "Bad Request - The request body failed validation: a body that is not valid JSON, a missing, empty, "
                 "over-long or invalid username or password, a value of the wrong type, an unknown key, an unknown "
-                "profile field, or an empty fields list. The message names each field that failed and why. A body "
-                "that cannot be read at all, such as JSON that is not valid UTF-8, gets a fixed message instead."
+                "profile field, an empty fields list, or fields without profile set to true. The message names each "
+                "field that failed and why. A body that cannot be read at all, such as JSON that is not valid UTF-8, "
+                "gets a fixed message instead."
             ),
             "headers": CACHE_CONTROL_HEADER,
             "model": ResponseModel,
@@ -225,6 +226,17 @@
                                 "timestamp": "2024-07-28T22:30:10.103368+05:30",
                             },
                         },
+                        "fields_without_profile": {
+                            "summary": "Fields without profile set to true",
+                            "value": {
+                                "status": False,
+                                "message": (
+                                    "Could not validate request data - body.fields: Value error, Fields can only be "
+                                    "requested when profile is true."
+                                ),
+                                "timestamp": "2024-07-28T22:30:10.103368+05:30",
+                            },
+                        },
                         "username_too_long": {
                             "summary": "Username too long",
                             "value": {

app/models/request.py

@@ -73,8 +73,9 @@ class RequestModel(BaseModel):
         title="Profile Fields",
         description=(
             "Which profile fields to return, from those listed in `ProfileModel`. Every field is returned when "
-            "this is omitted. Only used when profile is `true`. Fields come back in `ProfileModel`'s order, "
-            "whatever order they are asked for in, and a name that is not in `ProfileModel` is rejected."
+            "this is omitted or `null`. A list can only be given when `profile` is `true`; otherwise the request is "
+            "rejected with a 400. Repeated names are ignored. Fields come back in `ProfileModel`'s order, whatever "
+            "order they are asked for in, and a name that is not in `ProfileModel` is rejected."
         ),
         json_schema_extra={"example": ["name", "email", "campus", "branch", "semester", "firstName", "mobile"]},
     )
@@ -100,13 +101,18 @@ def validate_password(cls, v: str) -> str:
             raise ValueError("Password cannot be empty.")
         return v
 
-    @field_validator("fields")
+    @field_validator("fields", mode="after")
     @classmethod
-    def validate_fields(cls, v: list[str] | None) -> list[str] | None:
-        """Validate that fields is either None or a non-empty list containing only allowed field names."""
-        if v is not None and not v:
+    def validate_fields(cls, v: list[str] | None, info: ValidationInfo) -> list[str] | None:
+        """Validate fields: a non-empty list, only with profile set to true, with duplicates removed."""
+        if v is None:
+            return v
+        if not v:
             raise ValueError("Fields must be a non-empty list or None.")
-        return v
+        # profile is declared before fields, so it is in info.data unless it failed its own validation
+        if info.data.get("profile") is False:
+            raise ValueError("Fields can only be requested when profile is true.")
+        return list(dict.fromkeys(v))
 
     @field_validator("username", "password", mode="before")
     @classmethod

app/pesu.py

@@ -453,8 +453,8 @@ async def authenticate(
         """
         # Default fields to fetch if fields is not provided
         fields = self.DEFAULT_FIELDS if fields is None else fields
-        # Check if fields is not the default fields and enable field filtering
-        field_filtering = fields != self.DEFAULT_FIELDS
+        # Field filtering is on only if the requested set differs from the default set; order and repeats don't count.
+        field_filtering = set(fields) != set(self.DEFAULT_FIELDS)
 
         logging.info(
             f"Connecting to PESU Academy with user={username}, profile={profile}, fields={fields} ...",

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "pesu-auth"
-version = "5.4.0"
+version = "5.5.0"
 description = "A simple API to authenticate PESU credentials using PESU Academy."
 readme = "README.md"
 requires-python = ">=3.14"
@@ -76,6 +76,10 @@ select = [
     "RET", # Return type rules
 ]
 
+[tool.ruff.lint.per-file-ignores]
+# A test's name says what it checks, and pytest fixture arguments need no annotations
+"tests/**" = ["ANN", "D1"]
+
 [tool.ruff.lint.pydocstyle]
 convention = "google"
 
@@ -86,7 +90,6 @@ runtime-evaluated-base-classes = ["pydantic.BaseModel"]
 line-length = 120
 target-version = "py314"
 exclude = [
-    "tests/",
     # Agent instructions and skills from the pesu-dev/skills submodule, which lints them itself.
     # A local `ruff check .` before the submodule is initialized would otherwise follow dangling links.
     "AGENTS.md",

tests/integration/test_app_integration.py

@@ -311,7 +311,7 @@ def test_integration_health_check(client):
     response = client.get("/health")
     assert response.status_code == 200
     data = response.json()
-    assert data["status"] == True
+    assert data["status"] is True
     assert data["message"] == "ok"
 
 
@@ -448,9 +448,11 @@ def test_integration_authenticate_snake_case_fields_rejected(client, field):
     assert "body.fields.0" in data["message"]
 
 
-@pytest.mark.parametrize("field", ["bloodGroup", "photo", "profilePicture", "programShortCode", "phone", "institute", "rollNumber"])
+@pytest.mark.parametrize(
+    "field", ["bloodGroup", "photo", "profilePicture", "programShortCode", "phone", "institute", "rollNumber"]
+)
 def test_integration_authenticate_fields_the_api_does_not_return_rejected(client, field):
-    """PESU sends some of these and this API returned the others before 5.0.0; none is returned now, so asking is a 400."""
+    """PESU sends some of these and this API returned the others before 5.0.0; asking for any of them now is a 400."""
     payload = {"username": "username", "password": "password", "profile": True, "fields": [field]}
 
     response = client.post("/authenticate", json=payload)
@@ -603,3 +605,23 @@ async def fake_crash(*args, **kwargs):
     assert response.status_code == 500
     assert_security_headers(response)
     assert response.headers["Cache-Control"] == "no-store"
+
+
+def test_integration_profile_key_omitted(client):
+    """Profile key omitted but fields are still included."""
+    payload = {"username": "username", "password": "password", "fields": ["name", "email", "srn", "prn"]}
+
+    response = client.post("/authenticate", json=payload)
+    data = response.json()
+    assert response.status_code == 400
+    assert "Fields can only be requested when profile is true" in data["message"]
+
+
+def test_integration_profile_is_false(client):
+    """Profile set to false."""
+    payload = {"username": "username", "password": "password", "profile": False, "fields": ["name", "email", "srn"]}
+
+    response = client.post("/authenticate", json=payload)
+    data = response.json()
+    assert response.status_code == 400
+    assert "Fields can only be requested when profile is true" in data["message"]