Skip to content

fix(api): honor dataset read grants for raw downloads#4163

Open
GautamSharma99 wants to merge 1 commit into
topoteretes:mainfrom
GautamSharma99:fix/dataset-read-authorization
Open

fix(api): honor dataset read grants for raw downloads#4163
GautamSharma99 wants to merge 1 commit into
topoteretes:mainfrom
GautamSharma99:fix/dataset-read-authorization

Conversation

@GautamSharma99

Copy link
Copy Markdown

Description

Closes #<ISSUE_NUMBER>

This PR fixes inconsistent authorization and missing-dataset handling in the dataset read endpoints:

  • GET /api/v1/datasets/{dataset_id}/data
  • GET /api/v1/datasets/{dataset_id}/data/{data_id}/raw

The raw-download endpoint first verified that the caller had read permission on the requested dataset and that the requested
data item belonged to that dataset. It then called get_data(user.id, data_id), which applied an additional owner-only check.

As a result, users with a valid dataset read grant could list a shared dataset's data but could not download the raw content
of those same data items.

This change uses the data record already obtained through the authorized dataset membership check. It does not relax the
owner-only behavior of get_data() globally.

The PR also fixes missing and inaccessible dataset handling. Both endpoints previously checked whether the authorization
result was None, although the helper returns a list or raises PermissionDeniedError. This made the intended 404 handling
ineffective and left empty results vulnerable to [0] indexing.

A shared response helper now:

  • Converts dataset read permission failures into the documented 404 response.
  • Handles empty authorization results safely.
  • Constructs ErrorResponseDTO using valid Pydantic keyword arguments.
  • Produces the same response shape for both dataset read endpoints.

Acceptance Criteria

  • Dataset owners can download raw data.
  • Users with a dataset-level read grant can download raw data belonging to that dataset.
  • The raw endpoint does not apply an additional data-owner requirement after dataset authorization.
  • A data ID belonging to another dataset is rejected.
  • Missing datasets return 404.
  • Inaccessible datasets return 404, matching the documented endpoint contract.
  • Empty authorization results do not cause IndexError.
  • Error responses are valid JSON and use valid Pydantic construction.
  • Local-file and S3 raw downloads continue to work.

Implementation Details

The raw-download flow now follows this authorization sequence:

  1. Verify that the caller has read permission on the requested dataset.
  2. Load data associated with the authorized dataset.
  3. Verify that data_id is present in that dataset.
  4. Use the matched data record to return the raw content.

This preserves the important cross-dataset boundary: possession of read access to one dataset cannot be used to download a
data item from another dataset.

The owner-only get_data() method was left unchanged because other operations may rely on its existing semantics.

Tests

Added or updated coverage for:

  • Raw downloads by an authorized dataset reader who does not own the data row.
  • Missing datasets represented by an empty authorization result.
  • Inaccessible datasets represented by PermissionDeniedError.
  • Both dataset-listing and raw-download routes returning 404.
  • Rejection of a data ID associated with another dataset.
  • Existing local-file, plain-path, encoded-path, S3, and unsupported-scheme behavior.

Commands run:

uv run ruff format --check \
  cognee/api/v1/datasets/routers/get_datasets_router.py \
  cognee/tests/unit/api/test_get_raw_data_endpoint.py

uv run ruff check \
  cognee/api/v1/datasets/routers/get_datasets_router.py \
  cognee/tests/unit/api/test_get_raw_data_endpoint.py

git diff --check

uv run pytest cognee/tests/unit/api/ -q

Results:

2 files already formatted
All Ruff checks passed
No whitespace errors
274 passed

The reported warnings are existing Pydantic, FastAPI, Starlette, Alembic, and datetime.utcnow() deprecation warnings.

## Type of Change

- [x] Bug fix (non-breaking change that fixes an issue)
- [ ] New feature (non-breaking change that adds functionality)
- [ ] Code refactoring
- [ ] Other (please specify):

## Screenshots
<!-- Add a screenshot showing the 274 passing unit API tests. -->

## Pre-submission Checklist

- [x] I have tested my changes thoroughly before submitting this PR
- [x] This PR contains minimal changes necessary to address the issue
- [x] My code follows the project's coding standards and style guidelines
- [x] I have added tests that prove my fix is effective
- [x] I have added necessary documentation (not applicable)
- [x] All new and existing relevant tests pass
- [ ] I have searched existing PRs to ensure this change hasn't been submitted already
- [x] I have linked the relevant issue in the description
- [x] My commit has a clear and descriptive message

## DCO Affirmation

I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of
Origin.

Signed-off-by: GautamSharma99 <gautamsharma99067@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant