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

Unify handling of the current user of a request #4803

Merged
merged 14 commits into from Mar 4, 2021

Conversation

ThiefMaster
Copy link
Member

@ThiefMaster ThiefMaster commented Feb 26, 2021

My approach is to first use the slightly ugly solution where session.user returns the current user, and then later (in a separate PR) replace session.user with something else.

@ThiefMaster ThiefMaster added this to the v3 milestone Feb 26, 2021
@ThiefMaster ThiefMaster added this to In progress in Release 3.0 via automation Feb 26, 2021
@ThiefMaster ThiefMaster force-pushed the session-user branch 5 times, most recently from 8f1da08 to ed3da3f Compare March 3, 2021 17:20
- database: prevent rollbacks
- test client: run each request in a separate app context
- add `make_test_client` fixture to create new clients
In that case the original exception is likely more relevant so we avoid
re-raising the authentication error but rather silently discard the user
that failed to authenticate.
We keep accepting the old links for the dashboard ical feed but no
longer generate such URLs.
We do not need to encode the data in the token, a simple signature is
actually enough since the user id is prepended to the signature part of
the token

Also use sha256 instead of sha1 for the HMAC signature. While SHA1 is
still fine for HMAC purposes, using a stronger algorithm doesn't have
any disadvantages and the tokens are still reasonably short.
Happened if someone had no title
To be removed when cc2cab2 gets reverted (or earlier if we can ditch
tokens-in-querystring before the implicit flow itself)
The CSRF checks no longer need to be disabled manually because any
oauth-authenticated request is no longer subject to CSRF checks.
We always put a user id in that url, and accessing it without one
actually failed with a KeyError
Looks like we don't need it in the end...
The oauth RFCs actually specify statuscodes, so replacing everything
with 400 is ugly
@ThiefMaster ThiefMaster marked this pull request as ready for review March 3, 2021 19:14
Copy link
Member

@plourenco plourenco left a comment

Choose a reason for hiding this comment

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

Nice, I like the way this is heading 👍

For future reference, we talked about the possibility of keeping the simple assignment session.user = instead of the setter, but that behaviour is only meant to be restored later when session.user is replaced and setting and reading to the exact same thing.

indico/modules/events/registration/util.py Show resolved Hide resolved
indico/web/util.py Show resolved Hide resolved
Release 3.0 automation moved this from In progress to Awaiting review/merge Mar 4, 2021
@ThiefMaster ThiefMaster merged commit 6017748 into indico:master Mar 4, 2021
Release 3.0 automation moved this from Awaiting review/merge to Done Mar 4, 2021
@ThiefMaster ThiefMaster deleted the session-user branch March 4, 2021 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Release 3.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants