Skip to content

✨ Ownership-based access #427

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

✨ Ownership-based access #427

wants to merge 16 commits into from

Conversation

brunobuddy
Copy link
Contributor

@brunobuddy brunobuddy commented Jun 11, 2025

Description

This PR implements a simple way of adding ownership access to items.

We can now add the self condition to an access rule to restrict the action to the owner of the item.

  Frog:
    properties:
      - name
    belongsTo:
      - User
    policies:
      create:
        - { access: restricted, allow: User, condition: "self" }

Docs PR mnfst/docs#33

How can it be tested?

From the website repo, visit the new docs doing:

cd docs
npm run fetch-content -- ownership

and play around !

Check list before submitting

  • This PR is wrote in a clear language and correctly labeled
  • I created the related changeset for my changes with npx changeset
  • I have performed a self-review of my code (no debugs, no commented code, good naming, etc.)
  • I wrote the relative tests
  • I created a PR for the documentation if necessary and attached the link to this PR

@brunobuddy brunobuddy changed the title ✨ Ownership based access ✨ Ownership-based access Jun 11, 2025
Copy link

codecov bot commented Jun 11, 2025

Bundle Report

Changes will increase total bundle size by 5.79kB (0.33%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
manifest 1.77MB 5.79kB (0.33%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: manifest

Assets Changed:

Asset Name Size Change Total Size Change (%)
admin/main.*.js 70 bytes 437.12kB 0.02%
manifest/src/crud/services/crud.service.js 1.03kB 16.46kB 6.66% ⚠️
types/src/manifests/ManifestSchema.d.ts 24 bytes 3.97kB 0.61%
manifest/src/policy/policy.guard.js 997 bytes 3.92kB 34.06% ⚠️
common/src/Helpers.js 2 bytes 3.88kB 0.05%
manifest/src/policy/policies.js 2.45kB 3.12kB 370.54% ⚠️
manifest/src/crud/services/crud.service.d.ts 35 bytes 2.41kB 1.48%
manifest/src/policy/policy.service.js 131 bytes 2.15kB 6.48% ⚠️
manifest/src/endpoint/endpoint.module.js 134 bytes 2.11kB 6.78% ⚠️
json-schema/src/schema/definitions/policies/policy-schema.json 237 bytes 1.24kB 23.65% ⚠️
manifest/src/policy/policy.module.js 141 bytes 1.21kB 13.25% ⚠️
json-schema/src/schema/schema.json 7 bytes 1.16kB 0.61%
manifest/src/policy/policy.guard.d.ts 133 bytes 749 bytes 21.59% ⚠️
manifest/src/policy/policies.d.ts 379 bytes 614 bytes 161.28% ⚠️
types/src/manifests/PolicyManifest.d.ts 24 bytes 159 bytes 17.78% ⚠️

Copy link

codecov bot commented Jun 11, 2025

Codecov Report

Attention: Patch coverage is 92.68293% with 6 lines in your changes missing coverage. Please review.

Project coverage is 89.53%. Comparing base (279d3c0) to head (5114e01).
Report is 36 commits behind head on master.

Files with missing lines Patch % Lines
...es/core/manifest/src/crud/services/crud.service.ts 90.47% 2 Missing ⚠️
packages/core/manifest/src/policy/policies.ts 95.12% 2 Missing ⚠️
packages/core/manifest/src/policy/policy.guard.ts 88.23% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #427      +/-   ##
==========================================
+ Coverage   89.44%   89.53%   +0.08%     
==========================================
  Files          58       58              
  Lines        1668     1729      +61     
  Branches      385      402      +17     
==========================================
+ Hits         1492     1548      +56     
- Misses        155      175      +20     
+ Partials       21        6      -15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@SebConejo
Copy link
Contributor

SebConejo commented Jun 18, 2025

  1. Create an item
  • POST to /api/collections/frogs with a valid token works
  • Creating with another userId in the body fails
  1. Read own items
  • GET /api/collections/frogs returns only items owned by the authenticated user
  • Another user cannot see these items
  1. Update own items
  • PUT on my own item works ❌
  • PATCH on my own item works
  • Updating another user's item fails
  1. Delete own items
  • DELETE on my own item works
  • DELETE on another user's item fails
  1. Change owner
  • Modifying userId or ownerId to hijack items is ignored or blocked
  • PATCH trying to change ownership fails ❌
  1. Admin check
  • An admin can see all items

@SebConejo

This comment was marked as resolved.

@SebConejo
Copy link
Contributor

❌ I'm able to transfer ownership by patching user field (expected: locked after creation)

Problem

When using belongsTo: User and condition: "self" on update, I can still patch the user field and assign my item to another user.

Since I am the current owner, I match the "self" condition, so Manifest allows the update. But it's dangerous. That means, I can give my invoices to other users.

Example

  Frog:
    properties:
      - name
      - color
      - { name: age, type: number }
    belongsTo:
      - User
      - Project
    policies:
      create:
        - { access: restricted, allow: User, condition: "self" }
      read:
        - { access: restricted, allow: User, condition: "self" }
      update:
        - { access: restricted, allow: User, condition: "self" }
      delete:
        - { access: restricted, allow: User, condition: "self" }

As a logged-in user, I run:

PATCH /api/collections/frogs/:id
Authorization: Bearer eyJhbGciOiJIUzI1NiIs...
Content-Type: application/json

{
  "user": "id-of-another-user"
}

The request succeeds.

Why this is risky

  • It allows ownership transfer without confirmation
  • Users can accidentally or maliciously give away control of their data
  • In most systems, ownership is immutable or requires admin-level confirmation

Expected behavior

To discuss. Example: If a relation is used to define ownership, then Manifest should block any attempt to change that field.

If a developer really wants to allow ownership transfer, it should require explicit opt-in, for example:

belongsTo:
  - { entity: User, allowOwnershipTransfer: true }

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.

2 participants