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

API Access to Moderation Queue #1028

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

gflohr
Copy link

@gflohr gflohr commented May 31, 2024

Checklist

  • All new and existing tests are passing
  • (If adding features:) I have added tests to cover my changes
  • (If docs changes needed:) I have updated the documentation accordingly.
  • I have added an entry to CHANGES.rst because this is a user-facing change or an important bugfix
  • I have written proper commit message(s)

What changes does this Pull Request introduce?

This PR introduces a new API endpoint /pending which works like /latest with the following differences:

  • Requests must be authorized with HTTP basic authentication using username "admin" and the admin password.
  • The endpoint is enabled in the configuration with [general] pending-enabled set to "true".
  • The endpoint only returns posts in the moderation queue.
  • The endpoint additionally requires the admin interface to be enabled.

The last requirement is maybe a little bit paranoid. Rationale: the admin password is currently only used for the admin interface. The requirement is meant to ensure that people have changed the default password. On the other hand, the endpoint does not expose sensitive information but only potential spam.

The feature/endpoint only makes sense, when moderation is enabled. But this is not explicitly checked because it causes no harm if moderation is not enabled.

Why is this necessary?

If you want to implement an alternative notification mechanism for comments awaiting moderation, access to the moderation queue via the API is required. The existing /latest endpoint only returns accepted comments. The new endpoint should require authorization because it exposes comments that are not publicly visible.
...

gflohr added 4 commits May 31, 2024 10:15
It works exactly like `/latest` but returns only posts waiting
moderation.  The endpoint has to be explicitly enabled and requests must
be authorized with the admin password.  The admin interface also has to
be enabled to make sure that people have changed the password.
It should be mentioned that the endpoint /latest only returns accepted
comments.
Version 0.13.0 is already released. . Change to the probably next
version 0.13.1.
@ix5 ix5 added server (Python) server code feature needs-decision Architectural/Behavioral decision by maintainers needed labels Jun 5, 2024
@ix5 ix5 added this to the 0.14 milestone Jun 5, 2024
Copy link
Member

@ix5 ix5 left a comment

Choose a reason for hiding this comment

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

In general, I think this idea is sensible. A few comments inline.

I don't think you really need a new config option for this though. Either someone has already enabled the admin interface, giving anyone with admin credentials access to all comments, or admin is disabled, which will also disable the new /pending endpoint. My concern is that going at this rate, the whole routing system will at some point in the future be (poorly) exposed through the configuration file.

response = self.getAuthenticated('/pending?limit=5', 'admin', password)
self.assertEqual(response.status_code, 404)

def testPendingNotEnabled(self):
Copy link
Member

Choose a reason for hiding this comment

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

should be testPendingEnabled

"Unavailable because 'pending-enabled' not set by site admin"
)

return self._latest(environ, request, "2")
Copy link
Member

Choose a reason for hiding this comment

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

Rather than adding a new endpoint, wouldn't it make more sense to instead pass through a mode GET parameter? It would remain at "1" as a default for /latest and could be set to "2" to get pending comments (which would then require auth).

@@ -65,6 +65,10 @@ gravatar-url = https://www.gravatar.com/avatar/{}?d=identicon&s=55
# needing to previously know the posts URIs)
latest-enabled = false

# enable the "/pending" endpoint, that works likes "/latest" but only
# for comments waiting moderation
Copy link
Member

Choose a reason for hiding this comment

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

"that works like"
"awaiting"

@@ -844,6 +864,64 @@ def testModerateComment(self):
# Comment should no longer exist
self.assertEqual(self.app.db.comments.get(id_), None)

def testPendingWithoutAdmin(self):
self.conf.set("admin", "enabled", "false")
Copy link
Member

Choose a reason for hiding this comment

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

Are these tests guaranteed to be run in order? I'd prefer you un-set the admin.enabled value at test end.

@gflohr
Copy link
Author

gflohr commented Jul 7, 2024

I'm afraid, I will need a couple of weeks to come back to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature needs-decision Architectural/Behavioral decision by maintainers needed server (Python) server code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants