Secure various handlers

[jimchamp]

Affected handlers

Ensure Alpha Endpoints enforce correct Usergroup

What follows are links to affected POST and DELETE handlers, and a short description of what each of them do. Entries in the "Verified locally" section have been confirmed to make unauthenticated writes on my local development environment.

It's possible that validations for some of the untested handlers is happening downstream of the handler. It'd be great if we cultivated a practice of validating given data in the handlers, before any data is processed/written to DB. I recommend doing this as we shore up the unsecured handlers.

POST handlers

Verified locally:

• bulk_tag_works.POST handler allows unauthenticated writes.
  • Allows patrons to add or remove one or more subjects from one or more works.
• add_author.POST handler allows unauthenticated writes.
  • Handler creates new Author records

Not yet verified

• lists_delete.POST json handler allows unauthenticated deletions.
  • This handler deletes lists.
• author_edit.POST handler allows unauthenticated writes.
  • This handler updates/deletes author records
• work_identifiers.POST handler appears to allow unauthenticated writes.
  • This handler adds ISBNs to editions.
  • It's unclear to me if this handler is being used today
• manage_covers.POST handler allow unauthenticated requests.
  • Handler to add new covers to an edition (it may also handle image deletions)
  • The manage work covers handler and manage author photo handler both extend this handler, and have the same issues.
• merge_authors_json.POST json handler appears to allow unauthenticated requests.
  • EDIT: The class's is_enabled method effectively provides authentication
    • Page will 404 if is_enabled is falsey
• recentchanges_view.POST handler appears to allow unauthenticated writes.
  • This handler appears to undo previous revisions of a record.
  • Is this handler being used today?
• borrow_receive_notification.POST handler allows unauthenticated requests, but does not appear to write to the DB.
  • This looks like it parses and returns given web.input() input
  • Appears to be related to ACS4, so can likely be deprecated or removed safely
• ia_borrow_notify.POST handler allows unauthenticated requests.
  • This handler updates waiting lists and calls lending.sync_loan when called with a valid identifier
• community_edits_queue.POST handler doesn't check permissions of requester.
  • This handler creates and updates merge requests.

DELETE handlers

• patrons_observations.DELETE handler allows authenticated patrons to delete the observations of other patrons.
  • This works as expected -- username is pulled from authenticated account and passed to method that writes observations to the DB.

Progress towards a solution can be checked here. If a fix has been pushed to the advisory-fix branch, it will be checked off.

• bulk_tag_works.POST handler
• add_author.POST handler
• lists_delete.POST json handler
• author_edit.POST handler
• work_identifiers.POST handler
• manage_covers.POST handler
• Manage work covers POST handler
• Manage author photos POST handler
• recentchanges_view.POST handler
• borrow_receive_notification.POST handler
• ia_borrow_notify.POST handler
• community_edits_queue.POST handler

As of writing, none of these changes have been tested.

[Copilot]

Pull request overview

This PR enhances security by adding authentication and authorization checks to various HTTP POST handlers and removing insecure endpoints. The changes prevent unauthorized users from performing write operations and restrict certain administrative functions to appropriate user groups.

Changes:

• Added authentication checks to POST handlers for covers, authors, editions, and lists
• Added authorization checks restricting merge requests and bulk tagging to librarians, super-librarians, and admins
• Removed insecure endpoints: undo functionality from recentchanges and notification handlers from borrow
• Added a helper method is_member_of_any() to the User model for checking multiple usergroup memberships

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
openlibrary/plugins/upstream/recentchanges.py	Removed POST handler for undo functionality to prevent unauthorized change reversals
openlibrary/plugins/upstream/edits.py	Added authorization check restricting merge request operations to librarians and admins
openlibrary/plugins/upstream/covers.py	Added authentication check to cover management POST handler
openlibrary/plugins/upstream/borrow.py	Removed insecure notification handlers that accepted external requests
openlibrary/plugins/upstream/addbook.py	Added authentication checks to author editing and work identifier POST handlers
openlibrary/plugins/openlibrary/lists.py	Added authorization check for list deletion with owner/admin verification
openlibrary/plugins/openlibrary/code.py	Added authentication check to author creation POST handler
openlibrary/plugins/openlibrary/bulk_tag.py	Added authorization check restricting bulk tagging to librarians and admins
openlibrary/core/models.py	Added is_member_of_any() helper method to User model for checking multiple usergroup memberships

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The authorization logic should use the list document's get_owner() method to check ownership instead of relying on string prefix matching. The current approach using key.startswith(user.key) is fragile and may not work correctly for all list path formats. The List model already has a get_owner() method that properly extracts the owner from the list key. Consider checking ownership like: list_owner = doc.get_owner(); if not user.is_admin() and (not list_owner or list_owner.key != user.key): raise web.unauthorized()

Suggested change

	# Check if current user is admin or list owner
	if not user.is_admin() and (user.key and not key.startswith(user.key)):
	raise web.unauthorized()
	doc = web.ctx.site.get(key)
	if doc is None or doc.type.key != '/type/list':
	raise web.notfound()
	doc = web.ctx.site.get(key)
	if doc is None or doc.type.key != '/type/list':
	raise web.notfound()
	# Check if current user is admin or list owner using the list's get_owner()
	if not user.is_admin():
	get_owner = getattr(doc, "get_owner", None)
	list_owner = get_owner() if callable(get_owner) else None
	if not list_owner or getattr(list_owner, "key", None) != user.key:
	raise web.unauthorized()

[Copilot]

Unnecessary parentheses around get_current_user() call. The walrus operator is not being used here, so the extra parentheses are redundant. Consider simplifying to: if not get_current_user():

Suggested change

	if not (get_current_user()):
	if not get_current_user():