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

(PXP-7565): User registration, plus CSRF changes #906

Merged
merged 27 commits into from
Aug 26, 2021
Merged

Conversation

vpsx
Copy link
Contributor

@vpsx vpsx commented Apr 19, 2021

Jira Ticket: PXP-7565

Two pieces to this PR: (1) Add user registration endpoints, and (2) update CSRF handling.

Registration means that a user provides their name, organization, and email, in order to gain some predefined permissions (i.e. to be automatically added to a preconfigured Arborist group). For more information see the docstrings in the diff. This PR adds the registration form itself along with an admin endpoint to list registered users and their information. Register Users is an optional feature. The user information provided during registration IS NOT VALIDATED IN ANY WAY currently. The registration page is not customizable at the moment.

The registration form presents some difficulties with existing CSRF protection, so this PR also includes CSRF changes alongside a sister cloud-automation PR uc-cdis/cloud-automation#1580. More info below.


Instructions for testing/using:

  • Set up a useryaml that has a GROUP with a policy (for example: "registered_users" with "data_upload" policy)
  • Your test user should not be in this group
  • Set up and run your Arborist
  • Run usersync
  • Put the name of the group in fence config's REGISTERED_USERS_GROUP; also set fence REGISTER_USERS_ON to true.
  • Look in the Fence db and confirm that your test user is not registered (he has no registration_info block in his additional_info column)
  • Look in Arborist db and confirm that your test user is not part of the group and does not have the group's policies
  • Set up and run your Fence, which is talking to your Arborist
  • Log in. You should be redirected to the registration form!
  • Register.
  • Check Arborist db and confirm that now your test user IS part of the group and DOES have the group's policies.
  • Check Fence db and confirm that the registration info you entered is now in the additional_info column.
  • Log out. Log in again. There should be no redirect to the registration form.
  • You can hit /user/register directly to re-register.
  • Now log in with a user that is an admin. Hit the /user/admin/register endpoint. You should see your first test user's registration info.

NOT implemented in this PR:

  • Nicer UI feedback upon successful registration!
  • On login and redirect to registration form, if user declines to register, don't ask again on next login. (Do we want this?) Could set flag in additional_info or could do it cookie-based.

Background on CSRF:

Prior to this PR, CSRF protection was being implemented at two levels--once in Fence, and the other in the revproxy. Both Fence and the revproxy used the cookie-to-header method for CSRF prevention. There were multiple problems with this.

First problem: HTML forms. The new registration form, like other HTML forms, is not compatible with the cookie-to-header method. (The usual way to implement CSRF protection with HTML forms is using a hidden form field.) Fence's other, existing HTML form, the OAuth consent form, up until recently was using AJAX instead of a plain form submit to get around this. This additionally necessitated some interesting gymnastics for redirects to work. Then, recently, Fence began to only issue HttpOnly cookies in response to pen testing, and so the AJAX method technically broke--except that the revproxy was rewriting the csrf cookies anyway, see below, and so things happened to keep working.

Second problem: Fence cookie was being overwritten. Fence has been operating under the assumption that it manages its own CSRF token. But actually the revproxy rewrites the token and performs its own CSRF checking. Apart from being confusing (see above), this also meant that Fence effectively couldn't implement its own CSRF protection (for example, requests with the correct token in a form field would still fail the revproxy CSRF check), while at the same time the revproxy CSRF fails to accommodate verification via HTML hidden form fields.

A full reconsideration of CSRF protection in Gen3 (involving revproxy, Fence, and all the other services) was outside the scope of this ticket. The present solution makes partial improvements while maintaining existing behavior around CSRF checks. (NB: This existing behavior is also the reason why this PR does not fully discard all the current CSRF code in favor of built-in WTForms protection. Further work in that direction might want to start here.) The changes consist of the following:

  1. Remove the Fence code that sets the CSRF cookie
  2. In Fence, use Flask-WTF, which wraps WTForms, to generate a CSRF token that is stored in the session
  3. Extend the Fence-specific CSRF check to allow verification against this token either via a header or via a hidden form field
  4. Exclude the registration endpoints from the revproxy CSRF check.

See uc-cdis/cloud-automation#1580 for the required revproxy change.

New Features

Add registration endpoints (registration form endpoint and admin-only list endpoint)

Breaking Changes

Bug Fixes

Improvements

Improvements around CSRF handling

Dependency updates

New dependency Flask-WTF

Deployment changes

Requires these cloud-automation updates to revproxy: uc-cdis/cloud-automation#1580

@github-actions
Copy link

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

@@ -802,6 +805,11 @@ SYNAPSE_URI: 'https://repo-prod.prod.sagebase.org/auth/v1'
SYNAPSE_JWKS_URI:
SYNAPSE_DISCOVERY_URL:
SYNAPSE_AUTHZ_TTL: 86400
# MIDRC user registration feature: Ask users to register (provide name/org/email) on login.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not mention specific projects, but just call this an optional feature

Copy link
Contributor

Choose a reason for hiding this comment

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

bumping this, we should remove the reference to MIDRS. Call this an optional feature

{% block content %}
<form method="post">

<h1 class="introduction">Register in order to get access to download data</h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

feels like this text should be configurable, a template var and some cfg thrown in? What happens if someone wants to use a different HTML form though, with different fields? Maybe there could be different "registration forms" and each one is tied to a template in the templates folder?

I'm just trying to think how we can make this more generic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very true, but I was thinking that I should keep it simple until the need arises for very configurable registration forms (if it ever does), rather than adding complexity straight away that may never become useful. Also, the different-registration-forms thing would need not only different templates but also different form classes and maybe different field validators..?

In any case I can definitely still push for this if you want 👍 lmk!

@coveralls
Copy link

coveralls commented Aug 5, 2021

Pull Request Test Coverage Report for Build 11604

  • 25 of 71 (35.21%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.3%) to 72.138%

Changes Missing Coverage Covered Lines Changed/Added Lines %
fence/blueprints/login/base.py 1 3 33.33%
fence/blueprints/login/fence_login.py 1 3 33.33%
fence/blueprints/admin.py 3 6 50.0%
fence/init.py 3 12 25.0%
fence/blueprints/register.py 17 47 36.17%
Files with Coverage Reduction New Missed Lines %
fence/init.py 1 90.8%
Totals Coverage Status
Change from base Build 11532: -0.3%
Covered Lines: 6408
Relevant Lines: 8883

💛 - Coveralls

@rnerella92 rnerella92 force-pushed the feat/register branch 6 times, most recently from bc8e995 to 69da6b4 Compare August 10, 2021 21:28
vpsx and others added 12 commits August 17, 2021 21:30
* Except recent pentest PR sets cookie HttpOnly to True
* So XHR won't work here; also means existing consent form code is broken
* No longer cookie-based: WTForms manages a session CSRF token to check against
* Retained most of the old CSRF logic but accepts hidden HTML form field as well as header
* Added benefit of signed CSRF tokens
* NB: turns out Fence-issued csrftoken cookies were being overwritten by cloud-auto anyway
@rnerella92 rnerella92 force-pushed the feat/register branch 2 times, most recently from d5b50e0 to a29d1eb Compare August 18, 2021 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants