Skip to content

Draft: oauth #54

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

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Draft: oauth #54

wants to merge 26 commits into from

Conversation

RensDimmendaal
Copy link
Contributor

This PR adds the oauth feature for plash apps.

Copy link

gitnotebooks bot commented Jul 10, 2025

Found 2 changed notebooks. Review the changes at https://app.gitnotebooks.com/AnswerDotAI/plash_cli/pull/54

Copy link

📖 Documentation Preview: https://plash-docs-pr-54.pla.sh

@algal
Copy link

algal commented Jul 10, 2025

This is a comment about the example code in the documentation.

In that code, an app checks if a user is logged in to an app by examining session.get('user_id'), and an app logs a user in by doing session['user_id'] = uid.

But then the app logs a user out by doing session.clear(), rather than clearing only the user_id property with a statement like session.pop('user_id', None).

I am not sure, but I think it might be better if the logout example showed clearing only the user_id property. Here's why:

  1. It's more consistent and locally comprehensible. If someone is reading the logout code session.pop('user_id',None) (or del session['user_id']) then they know that the effect of it is to clear the user_id property, and that the user_id property alone is what determines the user's login state wrt that app.

  2. It avoids unintended side-effects on the app. An app developer might store other data in the session besides the login state. Clearing all session state on logout might clear that other state unintentionally. If the developer wants to clear that other state at logout, they should do it explicitly and therefore intentionally.

  3. It avoids unintended side-effects on Plash. Plash also stores a token in the session associated with that app's domain. So session.clear() will also clear that state. Will it have a different effect on future login attempts if that state is also cleared -- that is, if the app does session.clear() vs session.pop('user_id',None) ? If there is a difference and it matters, then perhaps this doc should be more clear and prescriptive that one of the ways is right and the other is wrong. If there is no difference or a minor difference which does not matter, that would be great, but we should be sure of that.

@RensDimmendaal
Copy link
Contributor Author

@algal thanks for the comment. Ive adopted the changes you've suggested!

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.

2 participants