PKCE auth flow, code cleanup #10
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ThiefMaster
reviewed
Mar 31, 2021
ThiefMaster
reviewed
Mar 31, 2021
ThiefMaster
pushed a commit
to indico/indico
that referenced
this pull request
Mar 31, 2021
Related checkin app PR: indico/indico-checkin#10
Co-authored-by: Adrian <[email protected]>
ThiefMaster
approved these changes
Mar 31, 2021
DirkHoffmann
added a commit
to DirkHoffmann/indico
that referenced
this pull request
Apr 14, 2021
* Let cloners know about multiple clones And bypass VC cloner availability check after the 1st clone (useful for e.g. Zoom) * Patch plugin version suffix in setup.cfg * Stop setting unnecessary click option The warning is only triggered in Python 2 with unicode_literals * Update dev docs for Python 3 * Show timetable filter in meeting-like conferences * Rename copyright.yml to headers.yml The old filename broke GitHub's license detection * Move package metadata from setup.py to setup.cfg Except for some dynamic parts... * Pin versions of dev dependencies * Expose dev requirements via [dev] extra * Sort requirements.dev.txt entries * Move test dependencies to requirements.dev.txt And get rid of unused pojson dependency * Pin minimum versions of pytz and certifi * We no longer have *.tpl files * Do not cache JS i18n data for invalid locales Otherwise each request for an invalid locale will create a cache file. Security scanners and similar tools may request a lot of invalid locales, and thus create a large amount of small but useless files in the cache folder (which are eventually cleaned up but not creating them to begin with is clearly better). * Fix downloading abstract/paper file packages The temporary file was being deleted too early when using xsendfile * Fix review_editable_revision outside request ctx * Update Chinese translation 🇨🇳 🐉 * Add missing PR reference to changelog * Update French translation 🇫🇷 🥖 * Fix changelog wording * Release 2.3.3 * Bump version to 2.3.4-dev * Call metadata_postprocess for category event export When exporting all events from a category extending the metadata may also be useful, e.g. to add video conference join links. * Update url_for to default _external to False Flask's url_for _external defaults to True if there is no request context. * Make legacy event redirects routing-agnostic When changing the routing rules to require numeric event ids, we can no longer get leading-zero or non-numeric event ids from view_args * Always consider leading zeroes in event ids legacy Otherwise we can access a non-legacy event both with and without a leading zero, which is messy because there should be exactly one canonical URL * Replace confId with event_id everywhere Also make it numeric in the Flask routing rules * Update Jinja2 The previous version will soon receive a CVE which is not relevant for us, but let's make tools that only look at the version (e.g. GitHub's security notifications) happy. * Fix conference VC page if the VC plugin is missing * Fix deleting events with a VC room without the plugin * Update bleach to 3.3.0 The security issue does not affect our usage, but let's avoid warnings that may scare people... closes indico#4770 * Remove non-redis cache backends * Scope cache based on the indico version Like this we never risk errors due to cached data being from a previous version. * Add scoped caching layer on top of flask-caching Also improve the flask-caching behavior to not fail noisily in case redis is unreachable and support returning default values in case of cache misses. * Replace GenericCache with new scoped_cache * Remove GenericCache * Be more forgiving with invalid locales And warn if the configured default locale is invalid * Move indico.util.struct.* to indico.util.* * Add ID-1 page size for badge printing * Dashboard ical: Only query data that's needed * Fix eslint issues in Export.js * Hide semi-broken persistent ical links for contribs - They pointed to the event ical (without any contribution details) - When enabling (persistent) API keys from that widget, the URLs generated pointed to a legacy contribution-only ical api which 503s since v1.9.x - Persistent calendar links for a single contribution are not particularly useful * Fix event creation * Fix error in survey results - can't multiply strings with floats - dict_keys object can't be jsonified * Replace OrderedDicts with regular dicts (indico#4778) Regular dicts are sorted as of Python 3.7, so there is no need to use OrderedDict anymore. * Fix abstract submission `_.pluck()` was behaving very weirdly, and was supposedly removed in our lodash version anyway... * eslint person_link_widget.js * Allow timedelta for cache expiry * Use sane defaults in create_application fixture * Allow rejecting registrations with a reason (indico#4769) * Update *.pot files * cache: Fix timedelta conversion logic * cache: Fail noisily while in debug mode * Add temporary coverage file to gitignore * Use redis cache backend when running tests Using different backends in test and dev/prod is likely to result in errors that happen during regular usage but not in tests - and the fact that some tests actually contained bugs but passed nonetheless, proves this. * Support attaching iCal files to reminder/reg mails (indico#4780) * Fix dump_url_map That script initializes the app with testing=True, but we aren't inside pytest so we have no redis to connect to. * Fix dict ducktyping In Python 2 we used to check for iteritems+get which was safe, but items+get exists for quite a few motels, like the Survey one, and thus broke the ducktyping there and thus resulted in weird errors. * Avoid querying active registrations if not needed * Use 3.9 for RTD build * Remove unused schema instance * Remove weird session fallback in /api/user/ It was never used, and the idea of using the session user if there is any error with the oauth token (missing, invalid, whatever) is extremely obscure and prone to errors. * Add unit test for the complete oauth flow * Use Authlib for OAuth provider The implicit flow has been replaced with the more modern and in case of mobile apps more secure code+pkce flow. * Add token introspection endpoint * Disallow 'plain' PKCE code challenge It's less secure and we do not really care about clients that cannot create a SHA256 hash... * Add well-known oauth2 metadata endpoint * Allow CORS on token endpoint The PKCE flow can be used in browsers, so they should be able to exchange the code for a token via CORS * Improve error handling during authorization * Return more standard oauth errors * Remove obsolete oauth errors endpoint * Move oauth provider logic to core & split modules * Fix test_client with no webpack manifest If a test results in a templte being rendered this usually looks up some webpack bundles which we don't care about during tests and don't have during CI. * Add revocation endpoint * Apply app scope restrictions to existing tokens * Rename default_scopes to allowed_scopes The old name was just poor naming on the flask-oauthlib side, and never had anything to do with default scopes. * Support different tokens for different scopes Also refactor how user app authorizations are stored; instead of checking for a token, we now have a separate model holind the authorization and granted scopes, so if an app revokes its own token, the scopes will remain authorized in any future authorization flow the user does. * Remove unused property * Remove TODO comment about revoking tokens Deleting is fine, our tokens are unique so there is basically no risk of regenerating the same token that has been revoked before. * Fix error when jsonifying certain oauth errors * Add tests for insecure oauth logic * Stop expunging user from db session in tests Not sure why it now works without that ugly hack... * Add some app/user link assertions to the tests * Display metadata URL and add clipboard copy links * Remove unused authlib method upstream removed it as well * Make PKCE configurable Apps that are not targeting SPA/native apps do not need to allow flows without a client secret. * Test token reuse * Store OAuth tokens as SHA-256 hashes While not passwords, they are sensitive and there is no need to store plaintext versions of them. * Support implicit grant for the checkin app This also adds support for passing the token insecurely in the query string, but also ONLY for that app. Like this we are not under any pressure to update the app, and we can simply do it at some point after a proper Indico v3 release. * Add changelog entry for oauth * Fix merging users with oauth app auths * Use cascading FKs in oauth tables * Remove flower integration * ci: pin ubuntu version ubuntu-latest is now ubuntu 20 which doesn't work well with python 2 * ci: cache wheels + generate envs in setup This avoids weird random breakage with cached virtualenvs and is similar to what we already do for plugins * Remove deprecated registration form tab related css * Reduce label scope in the core css * Add primary_email_changed signal (indico#4802) Co-authored-by: Adrian Moennich <[email protected]> * Improve testing behavior - database: prevent rollbacks - test client: run each request in a separate app context - add `make_test_client` fixture to create new clients * Unify current-request-user logic * Better handle auth fails during exception handling 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. * Remove legacy url signing as much as possible We keep accepting the old links for the dashboard ical feed but no longer generate such URLs. * Simplify data in user_token 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. * Remove leading whitespace in registrants export Happened if someone had no title * Handle oauth tokens in query strings To be removed when cc2cab2 gets reverted (or earlier if we can ditch tokens-in-querystring before the implicit flow itself) * Use new user logic in oauth registrants API The CSRF checks no longer need to be disabled manually because any oauth-authenticated request is no longer subject to CSRF checks. * Do not register user picture url without user id We always put a user id in that url, and accessing it without one actually failed with a KeyError * Use new user logic in user info api * Remove override_request_user Looks like we don't need it in the end... * Preserve oauth errors if a bearer token is used The oauth RFCs actually specify statuscodes, so replacing everything with 400 is ugly * JSON requests are not likely seen by users * Make session user APIs clearer * Add changelog for session.user logic change * Correct some missing whitespace in templates * Fix URL signing when not logged in * Improve error handling in url signing * Use token-based urls for persistent ical links (indico#4801) Co-authored-by: Adrian Moennich <[email protected]> * Fix registration form management * Exclude some user agents from sso redirection * Add bulk export for editing data to JSON * UI: add horizontal scroll to body Provide scrollbar if content overflows. * Allow Editing team to see editables of unpublished contribs * Update indico-sui-theme * Remove obsolete contextlib2 dependency ExitStack is part of contextlib in Python 3 * Use pip-tools to manage requirements.txt * Update Python dependencies * Use new flask-caching cache factory * Update to Flask-Multipass without open redirects * Fix open redirects in signup/logout * Enforce the BASE_URL to match the actualr equest - Always set `SERVER_NAME` and `APPLICATION_ROOT` - Improve the handling of requests where this does not match (fail with a meaningful error) * Update French translation 🇫🇷 🥖 * Redirect to canonical URL in the web server * Release 2.3.4 * Bump version to 2.3.5-dev * Add 2.3.5 changelog stub * Fix react warning about duplicate key * Add better password security checks * Check password security during login * Add autocomplete="{new,current}-password" metadata * Add Flask-Limiter for rate limiting * Add rate limiting for failed login attempts * Fix incorrect cache/limiter initialization This broke building assets * Only disable jest tests instead of all JS tests We try building the assets which catches errors such as the url map extraction script being broken * Fix error on contribution author page * Fix static program page having header+footer * Don't show deleted long-lasting events in calendar * Allow filtering participant roles by unregistered * Fix custom role members not showing as registered (unless they had another event role) * Add bulk export for paper peer reviewing data to JSON * Fix oversized badges on buttons fixes one of the issues in indico#4821 * Fix i-button-label losing boldness on hover fixes one of the issues in indico#4821 * Improve iCal metadata (indico#4820) * Do not emit empty location property (indico#4785) * Include contact data in iCalendar (indico#4786) * Include event logo url in public events (indico#4787) * Fix icalendar property name * Update robots.txt * Remove legacy xmlGateway endpoint * Fix nonsense in the docs * Allow unstaging users by clicking them again * Allow unstaging users from staged users list * Flash the staged user count label when it appears Like this people are more likely to notice it * Editing: Only show menu links the user can access * Fix weeks view theme * Check for duplicate users during registration import * Fix "inheriting acls" info - show the message about inherited access again - properly show each acl type when viewing the list - remove obsolete code * Remove weird `is_*` attrs on principal models Using the enum is much cleaner, and they were only used in few places (which are now using the principal_type). * Allow groups/roles as authorized abstract submitters * Use StrictUndefined for top-level objects * Be explicit about undefined callers * Be a bit more strict with undefined attrs/items * Add tests for our undefined logic * Celery: execute tasks immediately while testing * Fix error when notifying new paper reviewing roles * Use new sentry-python SDK instead of raven * Fix user id not being logged with empty session * Fix typo in changelog * Disallow .bin for "all formats" http api endpoints It's specifically meant for the file apis, and for anything else it attempts to send the dict with the original data, which fails with an ugly error. * Fix error with flask-limiter and partials We do not use request-level rate limiting at the moment, so just disabling the before-request check avoids this bug which broke endpoints using `RHSimple.wrap_function`. Related upstream issue: alisaifee/flask-limiter#124 * Update lxml and cryptography Both updates contain security improvements * Fix incorrect i18n formatting * Fix Undefined error in registrant list * Redirect meeting elements to slug-based URL * Fix error when marking registration as paid * Update event QR code for latest checkin app (indico#4844) Related checkin app PR: indico/indico-checkin#10 * Remove legacy oauth features for checkin app (indico#4846) * Revert "Handle oauth tokens in query strings" This reverts commit e1e8dbb. Also fixes the unit test since the /api/user/ endpoint returns null if not authenticated instead of failing with 40x * Revert "Support implicit grant for the checkin app" This reverts commit cc2cab2. * Fix another UndefinedError * Fix subcontribution count check * Fix another UndefinedError * Add download option to ical export popups (indico#4850) * Fix UndefinedError * verbose_iterator: Support printing total duration * verbose_iterator: Fix error in case of no items * Update urllib3 (dependabot alert) * Remove invalid property * Remove horizontal scrollbar in Firefox * Remove unused get_nested_attached_items util indico/indico-plugins#109 removed the need for it, and in any case it would be something for the plugin if ever needed again. * Fix timetable details with explicit session access When a user has no access to the event but to a session, they should be able to view the timetable entry details for the contributions within that session. * Fix contribution access with explicit-only access ie a user who cannot access the event itself but is on the ACL for a contribution * Hide registration menu item if user has no access When an event is protected and the user has only access to a specific session/contribution inside, it makes no sense to show the registration menu unless the registrations are actually available to people with no event access. * Do not show empty conference menu We still have the blank area, but avoid showing just the border of the menu list. * Fix typo * Create SECURITY.md * Avoid inconsistent state when file deletion fails This can happen if there's a race condition between something claiming the file (and storing a reference to it in FK) + marking it as claimed and the file being deleted. Now we still fail in such a case, but no longer keep a file in the database that's gone from storage. * Fix reinitializing FileManager during "make changes" The tags are not guaranteed to be sorted, so sometimes the re-render caused by `setLoading(true)` resulted in the final-form's initialValues changing and thus reinitializing the form, triggering the file manager's logic to reset it as well and delete any pending files. Since this happened in parallel with the submission of the reviewing action, it at least resulted in the deleting request failing, but often also in a race between the deletion request and it checking whether the file has been claimed and the review claiming it, which resulted in the file being deleted from storage but not the database (fixed in a separate commit). * Add an option to keep Editing team anonymous * Add support for Polish translation 🇵🇱 * Add Polish translation 🇵🇱 * Update python deps; pin some trickier ones * Update to SQLAlchemy 1.4 * pytest: Only ignore a specific sqlalchemy warning * Fix some deprecation warnings * Cover `unknown` and mutability in webargs tests * Update to webargs 8 * Fix error when querying room statistics * Remove likely unnecessary joinedload This broke with SA 1.4 (sqlalchemy/sqlalchemy#6253) but doesn't seem to be necessary anyway. Also added some tests. They are kind of dumb (not testing the actual filtering logic) but they do cover the part that was broken. * Fix various Room Booking issues (indico#4861) * Make sure past bookings can't be cancelled * Show correct numbers when cancelling bookings * Fix timeline popup skidding * Fix fetchActiveBookings params * Ellipsize plugin version * Update JS deps within semver Pinned react-leaflet-draw to a specific version since newer versions within the 0.19.x line require react-leaflet v3... * Update package-lock.json * Update JS deps with semver-major changes * Update package-lock.json * Update package-lock.json to v2 (npm 7) * Update package-lock.json (npm upgrade) * Replace obsolete shortid package with nanoid * Fix eslint config The latest prettier plugin is no longer split into separate subconfigs * Enable legacy-peer-deps in npmrc enzyme and useAxios expect older react versions as peer deps and we don't care about this... * Add (n)pgettext support * Apply i18n to category event list date formats * Update *.pot files * Regenerate package-lock.json & fix ssh urls * `npm update` once again * Run latest pyupgrade * Use correct secure_filename fallback values * Be more verbose if plugin assets have not been built Co-authored-by: Pedro Ferreira <[email protected]> Co-authored-by: Adrian Moennich <[email protected]> Co-authored-by: Indico Team <[email protected]> Co-authored-by: Pedro Lourenço <[email protected]> Co-authored-by: Alejandro Avilés <[email protected]> Co-authored-by: Ergys Dona <[email protected]> Co-authored-by: Vasant Vohra <[email protected]> Co-authored-by: Javier Ferrer <[email protected]> Co-authored-by: Giorgio Pieretti <[email protected]> Co-authored-by: Javier Ferrer <[email protected]> Co-authored-by: Parth Shandilya <[email protected]>
javfg
added a commit
to javfg/indico-checkin
that referenced
this pull request
Jul 27, 2021
* Refactor code and add PKCE flow option * Add and apply indico linting rules * Use same date format everywhere
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses #9. Both using PKCE flow in compatible instances of Indico, and sending the token in the
Authorizationheader.Other improvements/fixes:
closes #7, closes #9