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

feat: Implement Flask-Limiter on the search module #37

Merged
merged 18 commits into from
Jul 26, 2024
Merged

Conversation

petesfrench
Copy link
Contributor

@petesfrench petesfrench commented Jun 21, 2024

@carkod
Copy link
Contributor

carkod commented Jun 25, 2024

Can you fix the test, I think the test can pretty much be a QA

@@ -46,18 +44,16 @@ def build_search_view(
)
"""

app = flask.Flask(__name__)
limiter = Limiter(
app, key_func=get_remote_address, default_limits=[request_limit]
Copy link
Contributor

@carkod carkod Jun 25, 2024

Choose a reason for hiding this comment

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

Out of scope for this PR, but @samhotep we need IP address for search to track agents who spam us and store the rate limit in memory, since IPs are proxied, what would you recommend for this? I guess we can't use the same solution of the telephone number because this endpoint could be any ?search=xxx and this module only accepts a remote address function.

Copy link
Member

Choose a reason for hiding this comment

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

We'd need to force the content-cache to forward a custom header with the ip address probably, to preserve both the content-cache and remote IPs, and then update the limiter to use that header..
It's just an idea for now, but I'll check to see if the content-cache supports this

setup.py Outdated Show resolved Hide resolved
get_remote_address, app=app, default_limits=[request_limit]
)

@limiter.limit(request_limit)
Copy link
Contributor

@carkod carkod Jun 25, 2024

Choose a reason for hiding this comment

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

This implementation doesn't work, the moment I removed this decorator, everything passed locally. So I came up with another better implementation:

with limiter.limit(request_limit):
  flask.render_template(
                    template_path,
                    query=query,
                    start=start,
                    num=num,
                    results=results,
                    siteSearch=site_search,
                ),
                {"X-Robots-Tag": "noindex"},
            )

This seems to pass the tests. Of course you'd need to do it twice, in both returns.

@@ -46,18 +44,16 @@ def build_search_view(
)
"""

app = flask.Flask(__name__)
Copy link
Contributor

@carkod carkod Jun 25, 2024

Choose a reason for hiding this comment

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

This doesn't work also because you are creating and overriding an exisiting Flask app. So you need to

from flask import current_app
.....

limiter = Limiter(
        get_remote_address, app=current_app, default_limits=[request_limit]
    )

Copy link
Member

Choose a reason for hiding this comment

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

@petesfrench this one in particular

Copy link
Member

@samhotep samhotep left a comment

Choose a reason for hiding this comment

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

@petesfrench Carlos' comments are worth looking into :)

@petesfrench
Copy link
Contributor Author

petesfrench commented Jul 25, 2024

Thanks @samhotep! Can you take another look pls

Copy link
Member

@samhotep samhotep left a comment

Choose a reason for hiding this comment

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

LGTM!

@petesfrench petesfrench merged commit 4850a23 into main Jul 26, 2024
4 checks passed
@petesfrench petesfrench deleted the wd-12317 branch July 26, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants