Skip to content

improvement: another approach on attachments field level authorization#1667

Merged
adrianthedev merged 3 commits into
mainfrom
improvement/apply_another_approach_on_field_level_authorization
Apr 15, 2023
Merged

improvement: another approach on attachments field level authorization#1667
adrianthedev merged 3 commits into
mainfrom
improvement/apply_another_approach_on_field_level_authorization

Conversation

@Paul-Bob

@Paul-Bob Paul-Bob commented Apr 4, 2023

Copy link
Copy Markdown
Contributor

Description

Fixes #1662

UPDATE

We have discovered that assigning different weights to two methods that police the same thing can be a complex and challenging task. As a result, we have decided to make upload_attachments?, download_attachments?, and delete_attachments? obsolete.

From now on, only field level authorization will be considered. If it is defined and returns true, the action will be granted; otherwise, it will not.

Previous approach

How it works:

upload_attachments?, download_attachments? and delete_attachments? are first level authorization for attachments.
upload_file_name?, download_file_name? and delete_file_name? are second level authorization for each field.

Lets pick uploads example:

If upload_attachments? is NOT defined only upload_file_name? will count for authorize an action on a field...
If upload_attachments? is defined, upload_file_name? will count only if upload_attachments? return true.

Why? This way you can do something like:

All admins and managers can upload attachments but only admins can upload that important document...

def upload_attachments?
  user.admin? || user.manager?
end

def upload_important_document?
   user.admin?
end

OR

Nobody can upload any attachments but admins can upload that important document...

def upload_important_document?
   user.admin?
end

OR

Anyone can upload attachments but nobody can upload that important document.

def upload_attachments?
  true
end

def upload_important_document?
   false
end

NOTICE:

The follow example don't make much sense to use it...
If upload_attachments are false, field level authorization will not be checked for uploads.
If you want to turn all uploads off and allow only specific uploads do NOT define uploads_attachments?

def upload_attachments?
  false
end

def upload_important_document?
   true
end

~~

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@Paul-Bob
Paul-Bob requested a review from adrianthedev April 4, 2023 22:35
@Paul-Bob Paul-Bob self-assigned this Apr 4, 2023
@qlty-cloud-legacy

qlty-cloud-legacy Bot commented Apr 4, 2023

Copy link
Copy Markdown

Code Climate has analyzed commit 8b1c848 and detected 0 issues on this pull request.

View more on Code Climate.

@adrianthedev adrianthedev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few comments on the docs repo.

@adrianthedev
adrianthedev merged commit 31b6ef9 into main Apr 15, 2023
@adrianthedev
adrianthedev deleted the improvement/apply_another_approach_on_field_level_authorization branch April 15, 2023 19:21
@github-actions

Copy link
Copy Markdown
Contributor

This PR has been merged into main. The functionality will be available in the next release.

Please check the release guide for more information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The download button is still visible if the specific file policy method returns false

2 participants