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

Handle facebook limited login #113

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

Conversation

kadyvillicana
Copy link
Collaborator

No description provided.

src/auth.py Outdated Show resolved Hide resolved
src/auth.py Outdated Show resolved Hide resolved
src/auth.py Outdated Show resolved Hide resolved
src/auth.py Outdated Show resolved Hide resolved
src/auth.py Outdated Show resolved Hide resolved
src/auth.py Outdated
public_key_pem = jwt.algorithms.RSAAlgorithm.from_jwk(key_data)

return public_key_pem
@lru_cache(maxsize=1)
Copy link
Member

Choose a reason for hiding this comment

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

Use @cache here, @lru_cache is not needed. But how often does the Facebook public key change? If it changes e.g. every 24 hours we need a mechanism to refetch it if the last value fetched is > 12 hours old.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The documentation doesn’t specify a time frame for public key changes, but I believe we should refetch it every week.

src/auth.py Outdated Show resolved Hide resolved
src/auth.py Outdated Show resolved Hide resolved
@kadyvillicana kadyvillicana force-pushed the facebook-limited-login branch from b62bea4 to 46375ea Compare August 30, 2024 13:12
return (
jsonify(
{"status": "invalid", "msg": "Invalid Facebook nonce token",}
),
401,
)
account = decoded_token.get('sub', '')
account = decoded_token.get("sub", "")
Copy link
Member

Choose a reason for hiding this comment

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

In the other case (non-limited login), the account id is compared with the original string from the login request, and if different, the login is aborted. Does the same logic apply here, in the limited login case? @kadyvillicana

@@ -398,6 +398,11 @@ def oauth_fb(request: Request) -> ResponseType:
),
401,
)
if account != user.get("id"):
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me like we need instead, in line 386, something like this:

account_in_token = decoded_token.get("sub", "")
if account_in_token != account:
    return (jsonify(...), 401)  # User account mismatch

We already assigned the account variable in line 326 and the user id in the token ("sub") must match that account id.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants