feat(printing): add rate limit to print job requests (closes tjcsl#662) #1666

aarushtools · 2024-04-28T21:21:33Z

Proposed changes

Add a rate limit of 10 POST requests per minute to ion printing
Fix some linting errors from previous contributors in the printing/views.py file so checks can pass

Brief description of rationale

Blocks excessive printing by users to prevent abuse or overload of the printers

coveralls · 2024-04-28T21:31:16Z

coverage: 79.342% (-0.06%) from 79.406%
when pulling ffcb59a on aarushtools:dev
into 94f02ed on tjcsl:dev.

NotFish232

Hi Aarush, thanks for the PR! Nice implementation with SImpleRateThrottle. I have a couple of suggestions.

A rate of 10/minute is probably too lenient, maybe try something more along the lines of a request every 5 minutes?
The current implementation ratelimits even if the request is not successful, i.e. the form is invalid. If a user tries to print and accidentally selects the wrong options (without an actual print job requested), they shouldn't have to wait for the ratelimit to expire.
Let us know if you need any help with this!

alanzhu0 · 2024-05-02T13:47:34Z

I think 2 requests / 5 minutes is a good ratelimit for successful POST requests.

I don't think turning the printing view into an API view is a good idea. It doesn't give a good error screen when the requests are throttled. Instead, the user should be presented with a good error screen, perhaps using the messages module we already have, and told they cannot print more than twice every 5 minutes. Perhaps you will find another package to accomplish this, but keep in mind only successful POST requests should be tracked. Otherwise, you may need to write your own backend and model for this.

aarushtools · 2024-05-03T01:49:02Z

I tried to use django-ratelimit but decided that the package is just not meant to do what I was looking for and decided to use caching to make a sort of DIY ratelimit. I implemented the django cache system into two functions that I check for directly in the print_job function. The cache key holds an int that is incremented each time a request is made, and it expires in 300 seconds or 5 minutes.

alanzhu0

This is good! Can you make the ratelimit a setting variable, instead of directly hardcoded? For example,

# 2 print jobs every 5 minutes
PRINT_RATELIMIT_FREQUENCY = 2
PRINT_RATELIMIT_MINUTES = 5

would control the ratelimit behavior.

aarushtools · 2024-05-04T22:52:34Z

Done 👍
I put those variables in the main settings file for everything

alanzhu0 · 2024-05-11T02:47:14Z

This is great! Last few comments:

Can you write some text on the printing page alerting users to the ratelimit? Use the variables from the settings file.
Can you make the ratelimit not apply to administrators (admin_all)? Still send a message along the lines of "you have reached the ratelimit of 2 req / 5 min, but since you are an administrator it will still go through." (for debugging purposes).
Same thing for teachers - except don't send the message. So no ratelimit at all for teachers. While you're at it, maybe increase the number of pages teachers can print? 50 is probably a good value. That can go in settings too.

aarushtools · 2024-05-18T00:19:34Z

Everything implemented except for point 2 (partially). I use logger.error() to send a message since messages.error requires request as a property, which I don't have access to in the print_job function. I could add request to the obj object or as a param for the print_job func, but I think that isn't really necessary (but I can still do it if you believe it's the best way)

alanzhu0 · 2024-05-18T00:39:44Z

That's fine. Good call. Almost ready to merge, I'm assuming you'll fix CI, last thing it looks like "WizardoWaldo" (your alt?) is included in the commit. I don't really care about that but if you do you might want to take that off.

aarushtools · 2024-05-18T00:40:50Z

Yea, sorry about that I'm trying to fix it. Pushing using the PyCharm gui did that for some reason

aarushtools · 2024-05-18T01:23:45Z

I'm not sure why the linting is failing. It seems like it has to do with modules I never edited?

alanzhu0 · 2024-05-19T16:20:19Z

intranet/templates/printing/print_form.html

@@ -23,6 +23,7 @@
    <b>The following restrictions apply:</b>
    <ul>
        <li>Print jobs can have a maximum of {{ DJANGO_SETTINGS.PRINTING_PAGES_LIMIT }} pages.</li>


You need to change this to reflect the new teacher/student settings.

alanzhu0 · 2024-05-19T16:21:17Z

intranet/templates/printing/print_form.html

@@ -23,6 +23,7 @@
    <b>The following restrictions apply:</b>
    <ul>
        <li>Print jobs can have a maximum of {{ DJANGO_SETTINGS.PRINTING_PAGES_LIMIT }} pages.</li>
+        <li>You can send {{ DJANGO_SETTINGS.PRINT_RATELIMIT_FREQUENCY}} print jobs every {{ DJANGO_SETTINGS.PRINT_RATELIMIT_MINUTES }} minutes.</li>


Small reword - You may send up to XX print jobs every XX minutes.

alanzhu0 · 2024-05-19T16:21:33Z

intranet/apps/printing/views.py

+        if get_user_ratelimit_status(obj.user.username):
+            # Bypass rate limit for admins but still send error message for debugging purposes
+            if obj.user.has_admin_permission("printing"):
+                logger.error(


logger.debug is more appropriate here

alanzhu0 · 2024-05-19T16:27:26Z

intranet/apps/printing/views.py

@@ -349,17 +380,44 @@ def print_job(obj: PrintJob, do_print: bool = True):
        if obj.page_range:
            if not range_count:
                raise InvalidInputPrintingError("You specified an invalid page range.")
-            elif range_count > settings.PRINTING_PAGES_LIMIT:
+            elif range_count > settings.PRINTING_PAGES_LIMIT_STUDENTS and obj.user.is_student:


There are user types other than students and teachers. Apply the teacher limit to teachers and counselors (and, while we're at it, if the user is an admin they can get the teacher limit too). Apply the student limit to everyone else. Same logic for ratelimiting. No ratelimit for teachers and counselors. Ratelimit for everyone else (except admins).

Actually, Justin reminded me is_teacher checks for teacher or counselor. So that's ok. But you will still need to cover other user types.

I just made the first if statement check for printing admin perms or if they are a teacher, and anyone else gets the student limit because of the order the elifs appear in

alanzhu0

Very very close. Sorry for all the comments about small details.

Can you do a favor for me? user.has_print_permission is a weird property. All the other admin ones are named like user.is_printing_admin. Could you global replace has_print_permission with is_printing_admin? This isn't strictly related to your PR but it'd be a good change long-term for Ion.

alanzhu0 · 2024-05-20T02:41:35Z

intranet/templates/printing/print_form.html

+        {% else %}
+        <li>Print jobs can have a maximum of {{ DJANGO_SETTINGS.PRINTING_PAGES_LIMIT_STUDENTS }} pages.</li>
+        {% endif %}
+        {% if user.is_student %}


Small nitpick. Following convention this should be not user.is_teacher and not user.has_print_permission

aarushtools · 2024-05-20T16:55:04Z

Very very close. Sorry for all the comments about small details.

No worries, hopefully it's all good now 👍

alanzhu0 · 2024-05-25T04:08:39Z

intranet/apps/printing/views.py

+                    settings.PRINT_RATELIMIT_MINUTES,
+                )
+            # If user needs to be rate limited
+            elif obj.user.is_student:  # Don't rate limit teachers


This goes back to what we were discussing earlier - if printing admin or teacher, bypass, else apply ratelimit logic. Not just students.

aarushtools requested a review from a team as a code owner April 28, 2024 21:21

aarushtools force-pushed the dev branch from 1fcbc20 to be07aa9 Compare April 28, 2024 21:24

NotFish232 requested changes May 1, 2024

View reviewed changes

aarushtools force-pushed the dev branch from be07aa9 to eb37463 Compare May 3, 2024 01:29

aarushtools requested a review from NotFish232 May 3, 2024 01:32

aarushtools force-pushed the dev branch 2 times, most recently from 9366b9e to 0287f01 Compare May 3, 2024 01:42

aarushtools changed the title ~~feat(printing): add rate limiting to printing (closes tjcsl#662)~~ feat(printing): add rate limit to print job requests (closes tjcsl#662) May 3, 2024

aarushtools force-pushed the dev branch from 66e8e85 to 0287f01 Compare May 3, 2024 23:16

alanzhu0 requested changes May 4, 2024

View reviewed changes

aarushtools force-pushed the dev branch 5 times, most recently from 4cef985 to 8cef1cc Compare May 4, 2024 22:49

aarushtools force-pushed the dev branch from 8cef1cc to 6475b38 Compare May 18, 2024 00:19

aarushtools force-pushed the dev branch from 6475b38 to f8ef3d7 Compare May 18, 2024 00:25

aarushtools force-pushed the dev branch 5 times, most recently from 8e42e7a to 2ce3d7d Compare May 18, 2024 01:02

aarushtools force-pushed the dev branch from 2ce3d7d to 0d0ad90 Compare May 18, 2024 01:16

aarushtools force-pushed the dev branch 3 times, most recently from 6982c00 to bebf1df Compare May 19, 2024 15:47

alanzhu0 requested changes May 19, 2024

View reviewed changes

aarushtools force-pushed the dev branch from bebf1df to f3abd79 Compare May 19, 2024 23:49

alanzhu0 requested changes May 20, 2024

View reviewed changes

aarushtools force-pushed the dev branch from f3abd79 to c4feff6 Compare May 20, 2024 16:52

alanzhu0 requested changes May 25, 2024

View reviewed changes

aarushtools force-pushed the dev branch from c4feff6 to ba37ec2 Compare May 27, 2024 00:17

feat(printing): add rate limit to print job requests (closes tjcsl#662)

ffcb59a

aarushtools force-pushed the dev branch from ba37ec2 to ffcb59a Compare May 27, 2024 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(printing): add rate limit to print job requests (closes tjcsl#662) #1666

feat(printing): add rate limit to print job requests (closes tjcsl#662) #1666

aarushtools commented Apr 28, 2024

coveralls commented Apr 28, 2024 •

edited

NotFish232 left a comment

alanzhu0 commented May 2, 2024

aarushtools commented May 3, 2024

alanzhu0 left a comment

aarushtools commented May 4, 2024

alanzhu0 commented May 11, 2024

aarushtools commented May 18, 2024 •

edited

alanzhu0 commented May 18, 2024

aarushtools commented May 18, 2024

aarushtools commented May 18, 2024

alanzhu0 May 19, 2024

alanzhu0 May 19, 2024

alanzhu0 May 19, 2024

alanzhu0 May 19, 2024

alanzhu0 May 19, 2024

aarushtools May 19, 2024

alanzhu0 left a comment

alanzhu0 May 20, 2024

aarushtools commented May 20, 2024

alanzhu0 May 25, 2024

feat(printing): add rate limit to print job requests (closes tjcsl#662) #1666

Are you sure you want to change the base?

feat(printing): add rate limit to print job requests (closes tjcsl#662) #1666

Conversation

aarushtools commented Apr 28, 2024

Proposed changes

Brief description of rationale

coveralls commented Apr 28, 2024 • edited

NotFish232 left a comment

Choose a reason for hiding this comment

alanzhu0 commented May 2, 2024

aarushtools commented May 3, 2024

alanzhu0 left a comment

Choose a reason for hiding this comment

aarushtools commented May 4, 2024

alanzhu0 commented May 11, 2024

aarushtools commented May 18, 2024 • edited

alanzhu0 commented May 18, 2024

aarushtools commented May 18, 2024

aarushtools commented May 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanzhu0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarushtools commented May 20, 2024

Choose a reason for hiding this comment

coveralls commented Apr 28, 2024 •

edited

aarushtools commented May 18, 2024 •

edited