From 79b7b1dd5c356f8df8165e716100a15da6dd7135 Mon Sep 17 00:00:00 2001
From: Kevin Chung <kchung@nyu.edu>
Date: Sun, 17 Mar 2019 09:08:52 -0700
Subject: [PATCH] Fix removing profile details (Closes #894) (#899)

* Fix removing profile details (Closes #894)
* Update tests to properly check setting and removing profile values
---
 CTFd/schemas/users.py                    |  20 +++--
 CTFd/themes/core/static/js/settings.js   |   2 +-
 CTFd/themes/core/templates/settings.html |   2 +-
 CTFd/utils/validators/__init__.py        |   2 +
 tests/users/test_settings.py             | 104 +++++++++++++++++++++++
 tests/users/test_views.py                |  28 ------
 tests/utils/test_validators.py           |  11 +++
 7 files changed, 130 insertions(+), 39 deletions(-)
 create mode 100644 tests/users/test_settings.py
 create mode 100644 tests/utils/test_validators.py

diff --git a/CTFd/schemas/users.py b/CTFd/schemas/users.py
index 9bc82e7f..9b71ef29 100644
--- a/CTFd/schemas/users.py
+++ b/CTFd/schemas/users.py
@@ -39,10 +39,13 @@ class UserSchema(ma.ModelSchema):
     website = field_for(
         Users,
         'website',
-        validate=validate.URL(
-            error='Websites must be a proper URL starting with http or https',
-            schemes={'http', 'https'}
-        )
+        validate=[
+            # This is a dirty hack to let website accept empty strings so you can remove your website
+            lambda website: validate.URL(
+                error='Websites must be a proper URL starting with http or https',
+                schemes={'http', 'https'}
+            )(website) if website else True
+        ]
     )
     country = field_for(
         Users,
@@ -54,9 +57,6 @@ class UserSchema(ma.ModelSchema):
     password = field_for(
         Users,
         'password',
-        validate=[
-            validate.Length(min=1, error='Passwords must not be empty'),
-        ]
     )
 
     @pre_load
@@ -123,12 +123,11 @@ class UserSchema(ma.ModelSchema):
         password = data.get('password')
         confirm = data.get('confirm')
         target_user = get_current_user()
-        user_id = data.get('id')
 
         if is_admin():
             pass
         else:
-            if password and (confirm is None):
+            if password and (bool(confirm) is False):
                 raise ValidationError('Please confirm your current password', field_names=['confirm'])
 
             if password and confirm:
@@ -137,6 +136,9 @@ class UserSchema(ma.ModelSchema):
                     return data
                 else:
                     raise ValidationError('Your previous password is incorrect', field_names=['confirm'])
+            else:
+                data.pop('password', None)
+                data.pop('confirm', None)
 
     views = {
         'user': [
diff --git a/CTFd/themes/core/static/js/settings.js b/CTFd/themes/core/static/js/settings.js
index d4d5c8a1..d7d48220 100644
--- a/CTFd/themes/core/static/js/settings.js
+++ b/CTFd/themes/core/static/js/settings.js
@@ -17,7 +17,7 @@ $(function () {
     form.submit(function(e){
         e.preventDefault();
         $('#results').empty();
-        var params = $('#user-settings-form').serializeJSON(true);
+        var params = $('#user-settings-form').serializeJSON();
 
         CTFd.fetch('/api/v1/users/me', {
             method: 'PATCH',
diff --git a/CTFd/themes/core/templates/settings.html b/CTFd/themes/core/templates/settings.html
index 909e061e..c39506d6 100644
--- a/CTFd/themes/core/templates/settings.html
+++ b/CTFd/themes/core/templates/settings.html
@@ -72,7 +72,7 @@
 						Country
 					</label>
 					<select class="form-control" id="country-input" name="country">
-						<option></option>
+						<option value=""></option>
 						{% set countries = get_countries() %}
 						{% for country_code in countries.keys() %}
 							<option value="{{ country_code }}" {% if country == country_code %}selected{% endif %}>{{ countries[country_code] }}</option>
diff --git a/CTFd/utils/validators/__init__.py b/CTFd/utils/validators/__init__.py
index 3e66528f..b2b2c01a 100644
--- a/CTFd/utils/validators/__init__.py
+++ b/CTFd/utils/validators/__init__.py
@@ -32,5 +32,7 @@ def unique_email(email, model=Users):
 
 
 def validate_country_code(country_code):
+    if country_code.strip() == "":
+        return
     if lookup_country_code(country_code) is None:
         raise ValidationError('Invalid Country')
diff --git a/tests/users/test_settings.py b/tests/users/test_settings.py
new file mode 100644
index 00000000..6f080351
--- /dev/null
+++ b/tests/users/test_settings.py
@@ -0,0 +1,104 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+
+from CTFd.utils.crypto import verify_password
+from tests.helpers import *
+
+
+def test_user_set_profile():
+    """Test that a user can set and remove their information in their profile"""
+    app = create_ctfd()
+    with app.app_context():
+        register_user(app)
+        client = login_as_user(app)
+
+        data = {
+            'name': 'user',
+            'email': 'user@ctfd.io',
+            'confirm': '',
+            'password': '',
+            'affiliation': 'affiliation_test',
+            'website': 'https://ctfd.io',
+            'country': 'US',
+        }
+
+        r = client.patch('/api/v1/users/me', json=data)
+        assert r.status_code == 200
+
+        user = Users.query.filter_by(id=2).first()
+        assert user.affiliation == data['affiliation']
+        assert user.website == data['website']
+        assert user.country == data['country']
+
+        r = client.get('/settings')
+        resp = r.get_data(as_text=True)
+        for k, v in data.items():
+            assert v in resp
+
+        data = {
+            'name': 'user',
+            'email': 'user@ctfd.io',
+            'confirm': '',
+            'password': '',
+            'affiliation': '',
+            'website': '',
+            'country': '',
+        }
+
+        r = client.patch('/api/v1/users/me', json=data)
+        assert r.status_code == 200
+
+        user = Users.query.filter_by(id=2).first()
+        assert user.affiliation == data['affiliation']
+        assert user.website == data['website']
+        assert user.country == data['country']
+    destroy_ctfd(app)
+
+
+def test_user_can_change_password():
+    """Test that a user can change their password and is prompted properly"""
+    app = create_ctfd()
+    with app.app_context():
+        register_user(app)
+        client = login_as_user(app)
+
+        data = {
+            'name': 'user',
+            'email': 'user@ctfd.io',
+            'confirm': '',
+            'password': 'new_password',
+            'affiliation': '',
+            'website': '',
+            'country': '',
+        }
+
+        r = client.patch('/api/v1/users/me', json=data)
+        user = Users.query.filter_by(id=2).first()
+        assert verify_password(data['password'], user.password) is False
+        assert r.status_code == 400
+        assert r.get_json() == {
+            'errors': {
+                'confirm': ['Please confirm your current password']
+            },
+            'success': False
+        }
+
+        data['confirm'] = 'wrong_password'
+
+        r = client.patch('/api/v1/users/me', json=data)
+        user = Users.query.filter_by(id=2).first()
+        assert verify_password(data['password'], user.password) is False
+        assert r.status_code == 400
+        assert r.get_json() == {
+            'errors': {
+                'confirm': ['Your previous password is incorrect']
+            },
+            'success': False
+        }
+
+        data['confirm'] = 'password'
+        r = client.patch('/api/v1/users/me', json=data)
+        assert r.status_code == 200
+        user = Users.query.filter_by(id=2).first()
+        assert verify_password(data['password'], user.password) is True
+    destroy_ctfd(app)
diff --git a/tests/users/test_views.py b/tests/users/test_views.py
index 319cc53b..4fc3b0bf 100644
--- a/tests/users/test_views.py
+++ b/tests/users/test_views.py
@@ -124,34 +124,6 @@ def test_user_get_profile():
     destroy_ctfd(app)
 
 
-def test_user_set_profile():
-    """Can a registered user set their private profile (/profile)"""
-    app = create_ctfd()
-    with app.app_context():
-        register_user(app)
-        client = login_as_user(app)
-        r = client.get('/profile')
-        with client.session_transaction() as sess:
-            data = {
-                'name': 'user',
-                'email': 'user@ctfd.io',
-                # 'confirm': '',
-                # 'password': '',
-                'affiliation': 'affiliation_test',
-                'website': 'https://ctfd.io',
-                'country': 'US',
-            }
-
-        r = client.patch('/api/v1/users/me', json=data)
-        assert r.status_code == 200
-
-        user = Users.query.filter_by(id=2).first()
-        assert user.affiliation == 'affiliation_test'
-        assert user.website == 'https://ctfd.io'
-        assert user.country == 'US'
-    destroy_ctfd(app)
-
-
 def test_user_can_access_files():
     app = create_ctfd()
     with app.app_context():
diff --git a/tests/utils/test_validators.py b/tests/utils/test_validators.py
new file mode 100644
index 00000000..cf3720e5
--- /dev/null
+++ b/tests/utils/test_validators.py
@@ -0,0 +1,11 @@
+from CTFd.utils.validators import validate_country_code
+from marshmallow import ValidationError
+
+
+def test_validate_country_code():
+    assert validate_country_code('') is None
+    # TODO: This looks poor, when everything moves to pytest we should remove exception catches like this.
+    try:
+        validate_country_code('ZZ')
+    except ValidationError:
+        pass