-
Notifications
You must be signed in to change notification settings - Fork 20
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
atproto OAuth Flask Backend Demo #11
Conversation
python-oauth-web-app/app.py
Outdated
abort(400, "OAuth request not found") | ||
|
||
# delete row to prevent replay | ||
query_db("DELETE FROM oauth_auth_request WHERE state = ?;", [request.args["state"]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might run in a race where two read occur in // before the row is deleted. With Postgres, you can do DELETE ... RETURNING *
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree that is a best practice, but it feels a bit low-risk in this particular case? The state needs to match exactly, and the user is being redirected back from the auth flow... I guess a possible issue would be browser or some proxy doing a HEAD request or something?
hardened_http = requests_hardened.Manager( | ||
requests_hardened.Config( | ||
default_timeout=(2, 10), | ||
never_redirect=True, | ||
ip_filter_enable=True, | ||
ip_filter_allow_loopback_ips=False, | ||
user_agent_override="AtprotoCookbookOAuthFlaskDemo", | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to verify the url as part of hardened_http
manager ? It will probably be less error prone than having to always have to call is_safe_url
before using hardened_http
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does some similar checks internally, but is an externally-maintained package. We also need to do the basic URL check for the authorization request URL before forwarding the browser; that code path doesn't involve the hardened client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to have some hardened_http_for_safe_url
version of the hardened_http
that allows to replace:
assert is_safe_url(pds_url)
with hardened_http.get_session() as sess:
with a single:
with hardened_http_for_safe_url.get_session() as sess:
I feel like it would be less error prone (but that may just be a nit from me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are definitely correct, I just don't want to fork/maintain requests_hardened
just for this example code
Can this proof-of-concept also include JWT validation for tokens sent from the PDS? |
@ngerakines the current spec semantics are that the authorization server tokens are opaque strings. they are indeed JWTs in the case of the bsky PDS/entryway implementation, but I think this demo shouldn't assume that |
There is a demo version of this deployed at: https://oauth-flask.demo.bsky.dev/ It has been updated to be inline with the current draft spec (bluesky-social/atproto-website#326), as of just now. Any discrepancies are a bug! This is ready for final review and merge. |
The README gives an overview and getting started directions.
Progress/status:
It might also be helpful to have a "public" client example in python? But don't want to over-complicate this codebase.