#8936 closed New feature (fixed)
Add view (read-only) permission to admin
Reported by: | simon | Owned by: | Olivier Dalang |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Simon Charette, Petr Dlouhý, Aaron McMillin, cmawebsite@…, manelclos@…, serhiy.int@…, jeroen@…, olivier.dalang@…, Jeppe Vesterbæk, Carlton Gibson | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Adrian and Jacob on stage at DjangoCon.
Attachments (2)
Change History (57)
comment:1 Changed 15 years ago by
Triage Stage: | Unreviewed → Someday/Maybe |
---|
comment:2 Changed 14 years ago by
Component: | Uncategorized → Contrib apps |
---|---|
Keywords: | django pony added |
comment:3 Changed 13 years ago by
Component: | Contrib apps → contrib.databrowse |
---|
comment:4 Changed 12 years ago by
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:5 Changed 12 years ago by
Easy pickings: | unset |
---|---|
UI/UX: | unset |
comment:6 Changed 12 years ago by
See also #15956 for a list of specific problems when trying to use the admin merely for browsing data.
comment:7 Changed 12 years ago by
Component: | contrib.databrowse → contrib.admin |
---|---|
Summary: | Integrate databrowse functionality in to the admin → add databrowse-like functionality to the admin |
Since Databrowse is now deprecated, this should be considered as a proposed enhancement to the admin
Databrowse is now deprecated, see #16907
Changed 12 years ago by
Attachment: | options.py added |
---|
comment:8 Changed 12 years ago by
hi, i've added a small patch to django.contrib.admin.options
i've tested it and it works, it's pretty basic and there is more work to do.
what i've done is to add a method has_view_permission, and check a view permission in every view
i also created a get_list_editable, and i add in get_list_editable and in get_readonly_fields a check, and it returns original read_only and list_editable only if user has got a change_pemission
i also raise a permission error every time there is a post request and user do not have a change_permission
a also ask for view_permission in history_view.
todo:
fix inlines
fix submit_row
add a opts.has_view_permission()
comment:9 Changed 12 years ago by
Another discussion here: https://groups.google.com/group/django-developers/browse_thread/thread/ad9e4fb7d47de1de
comment:10 Changed 10 years ago by
What is the current status of this one? Is the provided patch of any help or do we need a new patch?
comment:11 Changed 10 years ago by
It's probably best to start from scratch on this one. The patch is in a weird format for one, and it's difficult for me to tell what's going on with it. Instead of just diving in and writing code for this, what we really need to move forward with this is a well thought out design document that can be reviewed. See Aymeric's post on the mailing list thread above for more details.
comment:12 Changed 10 years ago by
I think a simple POC could be implemented by doing the following:
- Create a
view
default permission. - Add
has_view_permission
methods toBaseModelAdmin
andInlineModelAdmin
. - Use the built-in
readonly_fields
logic to provide views of models. - Do the same thing with the listing of models.
- Add a new icon to link to the read-only version of models.
Then some permission checks will need to be changed in order to fallback to the view mode of models if the user doesn't have the change
permission but has the view
one.
comment:13 Changed 10 years ago by
Cc: | Simon Charette added |
---|
comment:14 Changed 9 years ago by
It's weird to see a feature that was requested 6 years ago, by multiple people, still not being implemented. Instead, the "admins" refused this a number of times, calling it a pony, and generally ignoring what actual users want. It's very sad.
comment:15 Changed 9 years ago by
If you want to see this, make a proposal (comment 12 looks good) and implement it with tests and documentation. That will be more useful than lamenting the fact that no one has done it.
comment:16 Changed 8 years ago by
Hi,
I have started to work on this issue. It is not complete yet, so any help with this will be appreciated. The commit is following:
https://github.com/PetrDlouhy/django/commit/19b5582c8604cb8e96914aa382de18f2a1060ea7
- It should not break current permission model, so change permission also qualifies to view data in admin.
- All fields are made readonly, "Safe" button is hidden, and POST requests are denied without change permission.
- There is "change" link on index admin page for all models which links to list view. The name not very accurate, but I didn't rename it nor change the icon.
- Tests should be written for this.
I am new to Django contributing, so if someone helps me with finishing this and making it part of the project, I would be very glad.
comment:17 Changed 8 years ago by
Cc: | Petr Dlouhý added |
---|---|
Has patch: | set |
Patch needs improvement: | set |
Hello,
I have added few corrections to my patch, fixed tests, added new tests and made few updates to the documentation. I don't think, that all issues connected with this are solved yet. Anyway, I would like my work to be reviewed.
The new patches are part of (yet uncreated) pull request:
https://github.com/django/django/compare/master...PetrDlouhy:view_permission_master?expand=1
There few weak spots, that I am feeling that should be discussed with someone more experienced:
- Model situation: User has defined
ModelAdmin
with customhas_change_permission()
such that it disallows to change (consequently also view) anything.- Then adding view permissions allows superuser to view that
ModelAdmin
. - I had to change
RowLevelChangePermissionModelAdmin
in tests for this.
- Then adding view permissions allows superuser to view that
- How should change
RelatedFieldWidgetWrapper
comment:18 Changed 8 years ago by
As advised on Django development mailing list, I have made PR for this ticket:
https://github.com/django/django/pull/5108
comment:19 Changed 8 years ago by
Keywords: | django pony removed |
---|---|
Patch needs improvement: | unset |
Triage Stage: | Someday/Maybe → Accepted |
Version: | 1.0 → master |
comment:20 Changed 8 years ago by
Triage Stage: | Accepted → Ready for checkin |
---|
comment:21 Changed 8 years ago by
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
I left some comments for improvement on the pull request.
comment:22 Changed 8 years ago by
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:23 Changed 8 years ago by
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
Please uncheck "Patch needs improvement" when you update the pull request, but don't promote it to "Ready for checkin" until someone has reviewed the changes. Thanks!
comment:24 Changed 8 years ago by
Cc: | Aaron McMillin added |
---|
comment:25 Changed 8 years ago by
Owner: | changed from nobody to Petr Dlouhý |
---|---|
Patch needs improvement: | unset |
Status: | new → assigned |
comment:27 Changed 8 years ago by
Patch needs improvement: | set |
---|
comment:28 Changed 8 years ago by
Cc: | cmawebsite@… added |
---|---|
Summary: | add databrowse-like functionality to the admin → admin databrowse (read-only view-only permissions) |
comment:29 Changed 8 years ago by
This would solve #11561 (raw_id_fields requires that the user has change permissions on the model class that is being linked to)
comment:30 Changed 8 years ago by
Cc: | manelclos@… added |
---|
comment:31 Changed 8 years ago by
Patch needs improvement: | unset |
---|
comment:32 Changed 8 years ago by
Cc: | serhiy.int@… added |
---|
comment:34 Changed 8 years ago by
Cc: | jeroen@… added |
---|
comment:35 Changed 8 years ago by
Cc: | olivier.dalang@… added |
---|
comment:36 Changed 7 years ago by
Patch needs improvement: | unset |
---|
Hi,
I fixed the merge conflicts on the patch today : https://github.com/olivierdalang/django/commits/view_permission_master
See discussion on the PR :
https://github.com/django/django/pull/5297#issuecomment-223714731
It should apply to master now.
I think we need a core dev to review this, so I unset the "patch needs improvement" tag. Sorry if this is not the right process (I'm new to contributing to django).
Please let me know if there's anything else I can do to help getting those merged into core.
Bests
comment:37 Changed 7 years ago by
Summary: | admin databrowse (read-only view-only permissions) → Add view (read-only) permission to admin |
---|
Since "databrowse" is not in the main focus anymore now.
comment:38 Changed 7 years ago by
Patch needs improvement: | set |
---|
comment:39 Changed 7 years ago by
Patch needs improvement: | unset |
---|
comment:40 Changed 7 years ago by
Patch needs improvement: | set |
---|
There are comments for improvement from Nick Pope on the PR.
comment:41 Changed 6 years ago by
Owner: | changed from Petr Dlouhý to Olivier Dalang |
---|---|
Patch needs improvement: | unset |
comment:42 Changed 6 years ago by
Cc: | Jeppe Vesterbæk added |
---|
comment:43 Changed 6 years ago by
Patch needs improvement: | set |
---|
comment:44 Changed 6 years ago by
I'm copying here my comment from the pull request:
It is important to be able to see the read-only page even if one has change permission, because long-term the read-only page will be more helpful than the form. For example, the form shows foreign keys as dropdowns, while the read-only page could show them as links to the foreign object. The read-only page is not constrained by the need to show input widgets, so it is more powerful for visualizing information.
comment:45 Changed 5 years ago by
Cc: | Carlton Gibson added |
---|---|
Patch needs improvement: | unset |
Unchecking "Patch needs improvement" to add this back to the review queue. I will review over the next week with a possible inclusion (or not) in 2.1 in mind.
comment:46 Changed 5 years ago by
Patch needs improvement: | set |
---|
Having reviewed, there are a few small changes needed.
Olivier, please uncheck "Patch needs improvement" when you've taken those on. (Feel free to @menion me on GitHub if you want input or help!)
comment:47 Changed 5 years ago by
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
This looks ready to go now. A lot of points have been picked up and all addressed.
It's a big patch, with security implications, so a further review is justified.
These seem like the main areas:
- Manual testing looks good.
- Tests on permission checks when POSTing have been added.
- Permission checks on accessing inlines are also tested.
(Note: PR thread is so long that it's taking several refreshes to load when logged in to GitHub — it does load if you keep refreshing though.)
comment:48 follow-up: 50 Changed 5 years ago by
I pushed a commit with some edits. I'm okay with merging this tomorrow, although I find the tests a bit lacking. In particular, many of the template changes are untested. I added some assertContains()
assertions but more are needed. This could be done after the alpha release, I suppose.
comment:50 Changed 5 years ago by
Replying to Tim Graham:
I'm okay with merging this tomorrow, although I find the tests a bit lacking. In particular, many of the template changes are untested. I added some
assertContains()
assertions but more are needed. This could be done after the alpha release, I suppose.
Thanks for the merge ! Yes I think I could spare some time to add some tests. But would you have some concrete examples of tests you'd like to have added ?
comment:51 Changed 5 years ago by
Try reverting various changes in templates and you'll find that no tests fail.
A summary is here: https://code.djangoproject.com/wiki/SummerOfCode2011#Integratedatabrowseintotheadmin
This project didn't happen in 2011.