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

Improve Quart example #1229

Merged
merged 2 commits into from Aug 1, 2019

Conversation

@pgjones
Copy link
Contributor

commented Jul 9, 2019

As the client seems tied to a specific login, I've changed it from
being a global to a local in the request handler. This should prevent
one user logging in and another unrelated user having access (due to a
global shared client). I've then made use of session storage to store
the phone and code (session storage is per user and deleted when the
browser tab is closed). I wasn't sure if the code was sensitive, if so
it is potentially problematic to store in the default session system
(cookies) - a server storage solution (e.g. redis or in memory) would
be better.

Note that if the client has to be global the Quart before/after
serving functions should be used to connect and disconnect, as so,

client = ...

@app.before_serving
async def startup():
    await client.connect()

@app.after_serving
async def cleanup():
    await client.disconnect()

as this creates the client within the event-loop managed by
Quart. Alternatively if the event-loop must be managed outside of
Quart the following should be used,

from hypercorn.asyncio import serve

loop.run_until_complete(serve(app))

instead of app.run().

I've then made a number of more minor changes to use the template
rendering system within Quart (more features, better example) and
to clean up the code.

Improve Quart example
As the client seems tied to a specific login, I've changed it from
being a global to a local in the request handler. This should prevent
one user logging in and another unrelated user having access (due to a
global shared client). I've then made use of session storage to store
the phone and code (session storage is per user and deleted when the
browser tab is closed). I wasn't sure if the code was sensitive, if so
it is potentially problematic to store in the default session system
(cookies) - a server storage solution (e.g. redis or in memory) would
be better.

Note that if the client has to be global the Quart before/after
serving functions should be used to connect and disconnect, as so,

    client = ...

    @app.before_serving
    async def startup():
        await client.connect()

    @app.after_serving
    async def cleanup():
        await client.disconnect()

as this creates the client within the event-loop managed by
Quart. Alternatively if the event-loop must be managed outside of
Quart the following should be used,

    from hypercorn.asyncio import serve

    loop.run_until_complete(serve(app))

instead of `app.run()`.

I've then made a number of more minor changes to use the template
rendering system within Quart (more features, better example) and
to clean up the code.
@@ -1,10 +1,38 @@
import base64
import os

from quart import Quart, request
from quart import Quart, render_template_string, request, session

This comment has been minimized.

Copy link
@Lonami

Lonami Jul 9, 2019

Member

Heh, I guess render_template_string is better than my hand-rolled html(inner) function.

@Lonami

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Thank you for this, however the point of the example was to showcase some of the problems people might encounter (with this whole implicit loop mess), and a global client was the easiest way to demonstrate the issue and how it could be solved.

I agree that the new code you propose is a lot better but notice you create the client inside the async def, as you note, if we wanted to use a global client, we should use @before/after_serving, and using serve(app) instead of app.run().

I think the best for this simple example is to keep the client global and ready to be used. Maybe we should add a note in the file security-wise, with things like setting secret_key and being careful with the fact anyone can access this logged-in client. Fixing this would complicate the example more than I would like.

Also, please make consistent use of single quotes in the files, since they are used everywhere for string literals ('string' and """docstring""").

@pgjones

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

Is there another way to demonstrate how to control the event loop usage (does the client have a multi user mode)? I'd prefer not to contribute an example that is known to be insecure as the examples tend to be copied verbatim by beginners.

In terms of Quart I've added extra documentation I hope helps people understand this.

@Lonami

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

Thank you for the extra documentation, I actually couldn't find an asynchronous alternative to app.run() with a quick search so this is welcome :)

Maybe we should just make both Quart's app and Telethon's client global variables, with the explicit loop set for both and a comment explaining why it's done this way, as well as connecting and disconnecting the client how you explained above. If we're going to be following good practices security-wise we might as well just get things right for the rest of things too.

@Lonami

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Finally got around trying this properly. Anyway, your code had some issues, so instead I adapted what you had in the old code with the new code and your suggestions on the first comment. If you still see something off, please let me know.

@Lonami Lonami merged commit e1355ae into LonamiWebs:master Aug 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.