Skip to content
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

Added missing permissions to Contributor & Editor #19881

Merged
merged 2 commits into from Mar 20, 2024
Merged

Conversation

allouis
Copy link
Contributor

@allouis allouis commented Mar 18, 2024

ref ENG-728
ref https://linear.app/tryghost/issue/ENG-728

This is NOT a functionality change. The Post#permissible method unit tests have been updated to pass true as hasUserPermission and we can see that the permission functionality remains the same.

The permissible method of the post model is responsible for removing permission based on the data that is being modified, but the permissions module is setup to allow the permissible method to grant permission - this means that we call permissible, even if the current actor doesn't have permission, this results in code that is hard to understand and manage.

We are going to be instead returning early if an actor does not have permission, this will allow permissible method signatures to be greatly simplified (removing the need for hasUserPermission, hasApiKeyPermission & hasMemberPermission arguments).

Copy link
Contributor

github-actions bot commented Mar 18, 2024

It looks like this PR contains a migration 👀
Here's the checklist for reviewing migrations:

General requirements

  • Satisfies idempotency requirement (both up() and down())
  • Does not reference models
  • Filename is in the correct format (and correctly ordered)
  • Targets the next minor version
  • All code paths have appropriate log messages
  • Uses the correct utils
  • Contains a minimal changeset
  • Does not mix DDL/DML operations

Schema changes

  • Both schema change and related migration have been implemented
  • For index changes: has been performance tested for large tables
  • For new tables/columns: fields use the appropriate predefined field lengths
  • For new tables/columns: field names follow the appropriate conventions
  • Does not drop a non-alpha table outside of a major version

Data changes

  • Mass updates/inserts are batched appropriately
  • Does not loop over large tables/datasets
  • Defends against missing or invalid data
  • For settings updates: follows the appropriate guidelines

@github-actions github-actions bot added the migration [pull request] Includes migration for review label Mar 18, 2024
ref ENG-728
ref https://linear.app/tryghost/issue/ENG-728

This is NOT a functionality change. The Post#permissible method unit tests have
been updated to pass `true` as `hasUserPermission` and we can see that the
permission functionality remains the same.

The permissible method of the post model is responsible for removing permission
based on the data that is being modified, but the permissions module is setup
to allow the permissible method to grant permission - this means that we call
permissible, even if the current actor doesn't have permission, this results in
code that is hard to understand and manage.

We are going to be instead returning early if an actor does not have
permission, this will allow permissible method signatures to be greatly
simplified (removing the need for hasUserPermission, hasApiKeyPermission &
hasMemberPermission arguments).
@allouis allouis merged commit 7cc65c1 into main Mar 20, 2024
20 checks passed
@allouis allouis deleted the add-missing-permission branch March 20, 2024 13:36
royalfig pushed a commit that referenced this pull request Mar 25, 2024
ref ENG-728
ref https://linear.app/tryghost/issue/ENG-728

This is NOT a functionality change. The Post#permissible method unit
tests have been updated to pass `true` as `hasUserPermission` and we can
see that the permission functionality remains the same.

The permissible method of the post model is responsible for removing
permission based on the data that is being modified, but the permissions
module is setup to allow the permissible method to grant permission -
this means that we call permissible, even if the current actor doesn't
have permission, this results in code that is hard to understand and
manage.

We are going to be instead returning early if an actor does not have
permission, this will allow permissible method signatures to be greatly
simplified (removing the need for hasUserPermission, hasApiKeyPermission
& hasMemberPermission arguments).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migration [pull request] Includes migration for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants