Code

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#13331 closed (invalid)

Strange behavior of lists of objects with overriden changelist_view in admin after re-login.

Reported by: minder Owned by: nobody
Component: contrib.admin Version: 1.1
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I spotted a strange behavior in the admin:

I've made an Admin for my model. It looks like this:

class EntryAdmin(admin.ModelAdmin):
	(...)
	def changelist_view(self, request, extra_context=None):
		if request.user.has_perm('blog.can_approve_entry'):
			self.list_editable = ['is_approved']
		return super(EntryAdmin, self).changelist_view(request, extra_context)

	def get_form(self, request, obj=None, **kwargs):
		if not request.user.has_perm('blog.can_approve_entry'):
			self.exclude = ['is_approved']
		return super(EntryAdmin, self).get_form(request, obj, **kwargs)

I've created two users: one with can_approve_entry permission (Alice) and the other without it (Bob). Then I created some Entries logged in as Bob. Entry list had no list_editable items, just nice pictures showing that his posts are not yet approved. Then I logged in as Alice and the list containted nice tickboxes and a Save button on bottom. So far, so good. Then I logged Alice out and logged back in as Bob. The list contained tickboxes instead of pictures. Now Bob could change the status of all Entries. I think showing these tickboxes is a security hole. Or maybe I'm doing something wrong? This shows up both on test server and Apache (mod_wsgi).

Attachments (0)

Change History (5)

comment:1 Changed 4 years ago by kmtracey

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

You are doing something wrong. Note a single ModelAdmin instance may get used for handling multiple requests, from different users. So if you set self.list_editable to ['is_approved'] when you get a request from a user authorized to change that, you had best take care to set it to an empty list when you get called to handle a request for a user that is not authorized to change that, otherwise your previous setting for an authorized user may still be in effect.

comment:2 Changed 4 years ago by minder

  • Resolution invalid deleted
  • Status changed from closed to reopened

After setting 'list_editable' and 'exclude' to empty lists, this still shows up after editing an object. I have no idea how to fix this.

Complete ModelAdmin:

class EntryAdmin(admin.ModelAdmin):
	prepopulated_fields={'url': ('title',)}
	save_on_top = True

	date_hierarchy = 'publish_date'
	search_fields = ['title']
	list_display = ['title', 'unit', 'is_approved']
	list_filter = ['unit',]

	list_editable = []
	exclude = []

	def formfield_for_foreignkey(self, db_field, request, **kwargs):
		if db_field.name == 'unit':
			if request.user.is_superuser:
				kwargs['queryset'] = Unit.objects.all()
			else:
				kwargs['queryset'] = Unit.objects.filter(editors__in=request.user.groups.all())
			return db_field.formfield(**kwargs)
		return super(EntryAdmin, self).formfield_for_foreignkey(db_field, request, **kwargs)

	def changelist_view(self, request, extra_context=None):
		if request.user.has_perm('blog.can_approve_entry'):
			self.list_editable = ['is_approved']
		return super(EntryAdmin, self).changelist_view(request, extra_context)

	def get_form(self, request, obj=None, **kwargs):
		if not request.user.has_perm('blog.can_approve_entry'):
			self.exclude = ['is_approved']
		return super(EntryAdmin, self).get_form(request, obj, **kwargs)

	def queryset(self, request):
		qs = super(EntryAdmin, self).queryset(request)
		if not request.user.is_superuser:
			units = Unit.objects.filter(editors__in=request.user.groups.all())
			qs = qs.filter(unit__in=units)
		return qs

	def save_model(self, request, obj, form, change):
		if not change:
			obj.author = request.user
		if not request.user.has_perm('blog.can_approve_entry'):
			obj.is_approved = False
		if not bool(obj.approved_by) and obj.is_approved:
			obj.approved_by = request.user
		obj.save()

comment:3 Changed 4 years ago by Alex

Just don't do this. It's complete un-thread safe and therefore a terrible idea. We should add a get_list_editable function which takes request and such.

comment:4 follow-up: Changed 4 years ago by gabrielhurley

  • Resolution set to invalid
  • Status changed from reopened to closed

I'm gonna third the "please don't do this." The bug you're seeing is the inevitable consequence of doing something that's not thread-safe. It's not meant to be done.

Re-closing as invalid as per kmtracey above.

comment:5 in reply to: ↑ 4 Changed 4 years ago by erob

Replying to gabrielhurley:

I'm gonna third the "please don't do this." The bug you're seeing is the inevitable consequence of doing something that's not thread-safe. It's not meant to be done.

Re-closing as invalid as per kmtracey above.

Using function methods as middlewares in a subclass such as ModelAdmin has nothing to do with thread-safety, sorry. Its fully supported by Django to use
classes methods as generic views. So this sounds more like yet another Django weird admin-policy-design-issue thing, rather than a limitation in the framework
itself.

Good luck! ;)


Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.