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

Fix: remove admin.logentry perm, use admin (staff) status #6380

Merged
merged 2 commits into from
Apr 14, 2024
Merged

Conversation

shamoon
Copy link
Member

@shamoon shamoon commented Apr 12, 2024

Proposed change

This turned out to be a bit more complicated than I expected.

The problem: we now have a collision for permissions named add_logentry because there is both admin.add_logentry and auditlog.add_logentry, and the problem is our serializer uses the codename as the slugfield. Perhaps there is an easier way to be able to differentiate the two (e.g. the frontend can specify admin.add_logentry) but didnt see a way to do that. So....

  1. We remove the 'Admin' View/Edit etc. permission which was a weird proxy anyway and replace it with the pre-existing is_staff setting from Django. Note that is_staff is what actually determines backend access by Django, so this really is more accurate. I did rename it Admin in the web UI for clarity.
  2. The rest of this PR is really just accommodating that change, again it's more than I expected but not so crazy.
  3. To be transparent about this I think we should label it a breaking change.

Welcome any other thoughts, of course.

See https://matrix.to/#/!lxUkPrXfbmPsCrNwHb:adnidor.de/$raooANjAp2c-24DvBD_6OvIedMqs3bGu56V9XxGAQfg?via=adnidor.de&via=matrix.org&via=tchncs.de

Type of change

  • Bug fix: non-breaking change which fixes an issue.
  • New feature / Enhancement: non-breaking change which adds functionality. Please read the important note above.
  • Breaking change: fix or feature that would cause existing functionality to not work as expected.
  • Documentation only.
  • Other. Please explain:

Checklist:

  • I have read & agree with the contributing guidelines.
  • If applicable, I have included testing coverage for new code in this PR, for backend and / or front-end changes.
  • If applicable, I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.
  • If applicable, I have checked that all tests pass, see documentation.
  • I have run all pre-commit hooks, see documentation.
  • I have made corresponding changes to the documentation as needed.
  • I have checked my modifications for any breaking changes.

@shamoon shamoon added this to the Next Release milestone Apr 12, 2024
@shamoon shamoon requested review from a team as code owners April 12, 2024 08:43
@paperless-ngx-secretary paperless-ngx-secretary bot added backend documentation Improvements or additions to documentation frontend non-trivial Requires approval by several team members labels Apr 12, 2024
@github-actions github-actions bot added the bug Bug report or a Bug-fix label Apr 12, 2024
@shamoon shamoon enabled auto-merge (squash) April 12, 2024 08:44
@shamoon shamoon removed the documentation Improvements or additions to documentation label Apr 12, 2024
@shamoon shamoon force-pushed the fix-admin-perm branch 2 times, most recently from bba15d6 to 8b61416 Compare April 12, 2024 08:51
Copy link

codecov bot commented Apr 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.12%. Comparing base (47b4a60) to head (42dc7bf).

❗ Current head 42dc7bf differs from pull request most recent head 7e9975e. Consider uploading reports for the commit 7e9975e to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #6380   +/-   ##
=======================================
  Coverage   97.12%   97.12%           
=======================================
  Files         420      420           
  Lines       16919    16923    +4     
  Branches     1208     1211    +3     
=======================================
+ Hits        16433    16437    +4     
  Misses        486      486           
Flag Coverage Δ
backend 95.94% <100.00%> (ø)
frontend 98.56% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@stumpylog
Copy link
Member

So the break here would be in what other applications see coming from the backend right?

@shamoon
Copy link
Member Author

shamoon commented Apr 12, 2024

The break is that if you previously used admin permissions to eg determine access to the logs it’s now different

@shamoon
Copy link
Member Author

shamoon commented Apr 12, 2024

But actually you made me realize that there needs to be more to this, ugh. I have an idea, will draft for a second

@shamoon shamoon disabled auto-merge April 12, 2024 22:32
@shamoon shamoon marked this pull request as draft April 12, 2024 22:32
@shamoon shamoon marked this pull request as ready for review April 12, 2024 22:46
@shamoon
Copy link
Member Author

shamoon commented Apr 12, 2024

Ok, good not a big deal I think, should be good to go, just will double-check coverage / tests weren't affected

@shamoon shamoon enabled auto-merge (squash) April 12, 2024 22:47
@shamoon shamoon changed the title Fix: remove admin.logentry perm, add admin (staff) status Fix: remove admin.logentry perm, use admin (staff) status Apr 12, 2024
@shamoon
Copy link
Member Author

shamoon commented Apr 12, 2024

One point about the breaking change is that I think for the most part it will restrict access that users currently have (they will now have to set the "Admin" toggle aka is_staff) as opposed this allowing access that was previously not allowed (which would only happen if someone was explicitly already is_staff in which case I dont think that would bother the person who set it up).

Id guess there will be questions about this, I just didnt see another way, again open to thoughts of course.

@shamoon shamoon merged commit f812f2a into dev Apr 14, 2024
25 checks passed
@shamoon shamoon deleted the fix-admin-perm branch April 14, 2024 00:35
@Pudda
Copy link

Pudda commented Apr 15, 2024

See #6396

Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns. See our contributing guidelines for more details.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backend breaking-change bug Bug report or a Bug-fix frontend non-trivial Requires approval by several team members
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[BUG] HTTP 500 Error when trying to add or save group with Admin permissions
3 participants