From e4deacd9c898b6a627d892ef09d3d6efeb88ac52 Mon Sep 17 00:00:00 2001 From: Rodney Ewing Date: Fri, 21 Jun 2013 14:14:40 -0700 Subject: [PATCH] changes after cwebb's review --- mediagoblin/auth/__init__.py | 12 --------- mediagoblin/auth/forms.py | 4 +-- mediagoblin/auth/tools.py | 19 ++------------ mediagoblin/auth/views.py | 23 ++++++++--------- mediagoblin/config_spec.ini | 4 --- mediagoblin/db/migrations.py | 4 +++ mediagoblin/plugins/basic_auth/forms.py | 4 +-- .../templates/mediagoblin/auth/change_fp.html | 4 +-- .../mediagoblin/auth/forgot_password.html | 4 +-- .../templates/mediagoblin/auth/login.html | 4 +-- .../templates/mediagoblin/auth/register.html | 4 +-- .../templates/mediagoblin/utils/wtforms.html | 16 +++++++++--- .../tests/appconfig_context_modified.ini | 1 - mediagoblin/tests/appconfig_static_plugin.ini | 1 - .../authentication_disabled_appconfig.ini | 1 - .../auth_configs/no_auth_plugin_appconfig.ini | 25 ------------------- mediagoblin/tests/test_auth.py | 20 +-------------- 17 files changed, 36 insertions(+), 114 deletions(-) delete mode 100644 mediagoblin/tests/auth_configs/no_auth_plugin_appconfig.ini diff --git a/mediagoblin/auth/__init__.py b/mediagoblin/auth/__init__.py index ab3d37e7..be5d0eed 100644 --- a/mediagoblin/auth/__init__.py +++ b/mediagoblin/auth/__init__.py @@ -35,14 +35,6 @@ def extra_validation(register_form): return extra_validation_passes -def get_login_form(request): - return hook_handle("auth_get_login_form", request) - - -def get_registration_form(request): - return hook_handle("auth_get_registration_form", request) - - def gen_password_hash(raw_pass, extra_salt=None): return hook_handle("auth_gen_password_hash", raw_pass, extra_salt) @@ -50,7 +42,3 @@ def gen_password_hash(raw_pass, extra_salt=None): def check_password(raw_pass, stored_hash, extra_salt=None): return hook_handle("auth_check_password", raw_pass, stored_hash, extra_salt) - - -def fake_login_attempt(): - return hook_handle("auth_fake_login_attempt") diff --git a/mediagoblin/auth/forms.py b/mediagoblin/auth/forms.py index 7a67285b..dad5dd86 100644 --- a/mediagoblin/auth/forms.py +++ b/mediagoblin/auth/forms.py @@ -29,9 +29,7 @@ class ForgotPassForm(wtforms.Form): class ChangePassForm(wtforms.Form): password = wtforms.PasswordField( - 'Password', - [wtforms.validators.Required(), - wtforms.validators.Length(min=5, max=1024)]) + 'Password') userid = wtforms.HiddenField( '', [wtforms.validators.Required()]) diff --git a/mediagoblin/auth/tools.py b/mediagoblin/auth/tools.py index f69d35ad..71f824de 100644 --- a/mediagoblin/auth/tools.py +++ b/mediagoblin/auth/tools.py @@ -169,7 +169,7 @@ def check_login_simple(username, password): user = auth.get_user(username=username) if not user: _log.info("User %r not found", username) - auth.fake_login_attempt() + hook_handle("auth_fake_login_attempt") return None if not auth.check_password(password, user.pw_hash): _log.warn("Wrong password for %r", username) @@ -178,23 +178,8 @@ def check_login_simple(username, password): return user -class AuthError(Exception): - def __init__(self): - self.value = 'No Authentication Plugin is enabled and' \ - ' authentication_disabled = False in config!' - - def __str__(self): - return repr(self.value) - - def check_auth_enabled(): - authentication_disabled = mg_globals.app_config['authentication_disabled'] - auth_plugin = hook_handle('authentication') - - if authentication_disabled is False and not auth_plugin: - raise AuthError - - if authentication_disabled: + if not hook_handle('authentication'): _log.warning('No authentication is enabled') return False else: diff --git a/mediagoblin/auth/views.py b/mediagoblin/auth/views.py index b407c6ba..d7535ef0 100644 --- a/mediagoblin/auth/views.py +++ b/mediagoblin/auth/views.py @@ -22,6 +22,7 @@ from mediagoblin.db.models import User from mediagoblin.tools.response import render_to_response, redirect, render_404 from mediagoblin.tools.translate import pass_to_ugettext as _ from mediagoblin.tools.mail import email_debug_message +from mediagoblin.tools.pluginapi import hook_handle from mediagoblin.auth import forms as auth_forms from mediagoblin.auth.tools import (send_verification_email, register_user, send_fp_verification_email, @@ -45,10 +46,11 @@ def register(request): return redirect(request, "index") if 'pass_auth' not in request.template_env.globals: - if 'openid' in request.template_env.globals: - return redirect(request, 'mediagoblin.plugins.openid.register') + redirect_name = hook_handle('auth_no_pass_redirect') + return redirect(request, 'mediagoblin.plugins.{0}.register'.format( + redirect_name)) - register_form = auth.get_registration_form(request) + register_form = hook_handle("auth_get_registration_form", request) if request.method == 'POST' and register_form.validate(): # TODO: Make sure the user doesn't exist already @@ -65,7 +67,6 @@ def register(request): request, 'mediagoblin/auth/register.html', {'register_form': register_form, - 'focus': 'username', 'post_url': request.urlgen('mediagoblin.auth.register')}) @@ -84,10 +85,11 @@ def login(request): return redirect(request, 'index') if 'pass_auth' not in request.template_env.globals: - if 'openid' in request.template_env.globals: - return redirect(request, 'mediagoblin.plugins.openid.login') + redirect_name = hook_handle('auth_no_pass_redirect') + return redirect(request, 'mediagoblin.plugins.{0}.login'.format( + redirect_name)) - login_form = auth.get_login_form(request) + login_form = hook_handle("auth_get_login_form", request) login_failed = False @@ -115,7 +117,6 @@ def login(request): {'login_form': login_form, 'next': request.GET.get('next') or request.form.get('next'), 'login_failed': login_failed, - 'focus': 'username', 'post_url': request.urlgen('mediagoblin.auth.login'), 'allow_registration': mg_globals.app_config["allow_registration"]}) @@ -217,8 +218,7 @@ def forgot_password(request): if not (request.method == 'POST' and fp_form.validate()): # Either GET request, or invalid form submitted. Display the template return render_to_response(request, - 'mediagoblin/auth/forgot_password.html', {'fp_form': fp_form, - 'focus': 'username'}) + 'mediagoblin/auth/forgot_password.html', {'fp_form': fp_form,}) # If we are here: method == POST and form is valid. username casing # has been sanitized. Store if a user was found by email. We should @@ -314,8 +314,7 @@ def verify_forgot_password(request): return render_to_response( request, 'mediagoblin/auth/change_fp.html', - {'cp_form': cp_form, - 'focus': 'password'}) + {'cp_form': cp_form,}) # in case there is a valid id but no user with that id in the db # or the token expired diff --git a/mediagoblin/config_spec.ini b/mediagoblin/config_spec.ini index 4eb69da8..b213970d 100644 --- a/mediagoblin/config_spec.ini +++ b/mediagoblin/config_spec.ini @@ -31,10 +31,6 @@ email_smtp_pass = string(default=None) # Set to false to disable registrations allow_registration = boolean(default=True) -# Set to true to run an instance with no authentication plugins enabled. -# You will not be able to login or register -authentication_disabled = boolean(default=False) - # tag parsing tags_max_length = integer(default=255) diff --git a/mediagoblin/db/migrations.py b/mediagoblin/db/migrations.py index 1f92417e..50fccd78 100644 --- a/mediagoblin/db/migrations.py +++ b/mediagoblin/db/migrations.py @@ -297,4 +297,8 @@ def pw_hash_nullable(db): user_table.c.pw_hash.alter(nullable=True) + if db.bind.url.drivername is 'sqlite': + constraint = UniqueConstraint('username', table=user_table) + constraint.create() + db.commit() diff --git a/mediagoblin/plugins/basic_auth/forms.py b/mediagoblin/plugins/basic_auth/forms.py index f389b21e..72d99dff 100644 --- a/mediagoblin/plugins/basic_auth/forms.py +++ b/mediagoblin/plugins/basic_auth/forms.py @@ -40,6 +40,4 @@ class LoginForm(wtforms.Form): [wtforms.validators.Required(), normalize_user_or_email_field()]) password = wtforms.PasswordField( - _('Password'), - [wtforms.validators.Required(), - wtforms.validators.Length(min=5, max=1024)]) + _('Password')) diff --git a/mediagoblin/templates/mediagoblin/auth/change_fp.html b/mediagoblin/templates/mediagoblin/auth/change_fp.html index afffeadd..a3cf9cb9 100644 --- a/mediagoblin/templates/mediagoblin/auth/change_fp.html +++ b/mediagoblin/templates/mediagoblin/auth/change_fp.html @@ -34,12 +34,10 @@ {{ csrf_token }}

{% trans %}Set your new password{% endtrans %}

- {{ wtforms_util.render_divs(cp_form) }} + {{ wtforms_util.render_divs(cp_form, True) }}
- - {% endblock %} diff --git a/mediagoblin/templates/mediagoblin/auth/forgot_password.html b/mediagoblin/templates/mediagoblin/auth/forgot_password.html index a6c9e1e9..6cfd2c85 100644 --- a/mediagoblin/templates/mediagoblin/auth/forgot_password.html +++ b/mediagoblin/templates/mediagoblin/auth/forgot_password.html @@ -29,12 +29,10 @@ {{ csrf_token }}

{% trans %}Recover password{% endtrans %}

- {{ wtforms_util.render_divs(fp_form) }} + {{ wtforms_util.render_divs(fp_form, True) }}
- - {% endblock %} diff --git a/mediagoblin/templates/mediagoblin/auth/login.html b/mediagoblin/templates/mediagoblin/auth/login.html index 2adbe547..d9f92557 100644 --- a/mediagoblin/templates/mediagoblin/auth/login.html +++ b/mediagoblin/templates/mediagoblin/auth/login.html @@ -45,7 +45,7 @@ {%- trans %}Create one here!{% endtrans %}

{% endif %} - {{ wtforms_util.render_divs(login_form) }} + {{ wtforms_util.render_divs(login_form, True) }} {% if pass_auth %}

@@ -61,6 +61,4 @@ {% endif %} - - {% endblock %} diff --git a/mediagoblin/templates/mediagoblin/auth/register.html b/mediagoblin/templates/mediagoblin/auth/register.html index 755d5418..b315975c 100644 --- a/mediagoblin/templates/mediagoblin/auth/register.html +++ b/mediagoblin/templates/mediagoblin/auth/register.html @@ -34,7 +34,7 @@ method="POST" enctype="multipart/form-data">

{% trans %}Create an account!{% endtrans %}

- {{ wtforms_util.render_divs(register_form) }} + {{ wtforms_util.render_divs(register_form, True) }} {{ csrf_token }}
- - {% endblock %} diff --git a/mediagoblin/templates/mediagoblin/utils/wtforms.html b/mediagoblin/templates/mediagoblin/utils/wtforms.html index be6976c2..90b237ee 100644 --- a/mediagoblin/templates/mediagoblin/utils/wtforms.html +++ b/mediagoblin/templates/mediagoblin/utils/wtforms.html @@ -33,10 +33,14 @@ {%- endmacro %} {# Generically render a field #} -{% macro render_field_div(field) %} +{% macro render_field_div(field, autofocus_first=False) %} {{- render_label_p(field) }}
- {{ field }} + {% if autofocus_first %} + {{ field(autofocus=True) }} + {% else %} + {{ field }} + {% endif %} {%- if field.errors -%} {% for error in field.errors %}

{{ error }}

@@ -49,9 +53,13 @@ {%- endmacro %} {# Auto-render a form as a series of divs #} -{% macro render_divs(form) -%} +{% macro render_divs(form, autofocus_first=False) -%} {% for field in form %} - {{ render_field_div(field) }} + {% if autofocus_first and loop.first %} + {{ render_field_div(field, True) }} + {% else %} + {{ render_field_div(field) }} + {% endif %} {% endfor %} {%- endmacro %} diff --git a/mediagoblin/tests/appconfig_context_modified.ini b/mediagoblin/tests/appconfig_context_modified.ini index 251c083c..80ca69b1 100644 --- a/mediagoblin/tests/appconfig_context_modified.ini +++ b/mediagoblin/tests/appconfig_context_modified.ini @@ -2,7 +2,6 @@ direct_remote_path = /test_static/ email_sender_address = "notice@mediagoblin.example.org" email_debug_mode = true -authentication_disabled = true # TODO: Switch to using an in-memory database sql_engine = "sqlite:///%(here)s/user_dev/mediagoblin.db" diff --git a/mediagoblin/tests/appconfig_static_plugin.ini b/mediagoblin/tests/appconfig_static_plugin.ini index dbb789e0..dc251171 100644 --- a/mediagoblin/tests/appconfig_static_plugin.ini +++ b/mediagoblin/tests/appconfig_static_plugin.ini @@ -2,7 +2,6 @@ direct_remote_path = /test_static/ email_sender_address = "notice@mediagoblin.example.org" email_debug_mode = true -authentication_disabled = true # TODO: Switch to using an in-memory database sql_engine = "sqlite:///%(here)s/user_dev/mediagoblin.db" diff --git a/mediagoblin/tests/auth_configs/authentication_disabled_appconfig.ini b/mediagoblin/tests/auth_configs/authentication_disabled_appconfig.ini index bbcb9163..a64e9e40 100644 --- a/mediagoblin/tests/auth_configs/authentication_disabled_appconfig.ini +++ b/mediagoblin/tests/auth_configs/authentication_disabled_appconfig.ini @@ -2,7 +2,6 @@ direct_remote_path = /test_static/ email_sender_address = "notice@mediagoblin.example.org" email_debug_mode = true -authentication_disabled = true # TODO: Switch to using an in-memory database sql_engine = "sqlite:///%(here)s/user_dev/mediagoblin.db" diff --git a/mediagoblin/tests/auth_configs/no_auth_plugin_appconfig.ini b/mediagoblin/tests/auth_configs/no_auth_plugin_appconfig.ini deleted file mode 100644 index a64e9e40..00000000 --- a/mediagoblin/tests/auth_configs/no_auth_plugin_appconfig.ini +++ /dev/null @@ -1,25 +0,0 @@ -[mediagoblin] -direct_remote_path = /test_static/ -email_sender_address = "notice@mediagoblin.example.org" -email_debug_mode = true - -# TODO: Switch to using an in-memory database -sql_engine = "sqlite:///%(here)s/user_dev/mediagoblin.db" - -# Celery shouldn't be set up by the application as it's setup via -# mediagoblin.init.celery.from_celery -celery_setup_elsewhere = true - -[storage:publicstore] -base_dir = %(here)s/user_dev/media/public -base_url = /mgoblin_media/ - -[storage:queuestore] -base_dir = %(here)s/user_dev/media/queue - -[celery] -CELERY_ALWAYS_EAGER = true -CELERY_RESULT_DBURI = "sqlite:///%(here)s/user_dev/celery.db" -BROKER_HOST = "sqlite:///%(here)s/user_dev/kombu.db" - -[plugins] diff --git a/mediagoblin/tests/test_auth.py b/mediagoblin/tests/test_auth.py index 83e71580..9f70c5ff 100644 --- a/mediagoblin/tests/test_auth.py +++ b/mediagoblin/tests/test_auth.py @@ -22,7 +22,6 @@ from mediagoblin import mg_globals from mediagoblin.db.models import User from mediagoblin.tests.tools import get_app, fixture_add_user from mediagoblin.tools import template, mail -from mediagoblin.auth.tools import AuthError from mediagoblin.auth import tools as auth_tools @@ -273,7 +272,6 @@ def test_authentication_views(test_app): context = template.TEMPLATE_TEST_CONTEXT['mediagoblin/auth/login.html'] form = context['login_form'] assert form.username.errors == [u'This field is required.'] - assert form.password.errors == [u'This field is required.'] # Failed login - blank user # ------------------------- @@ -291,9 +289,7 @@ def test_authentication_views(test_app): response = test_app.post( '/auth/login/', { 'username': u'chris'}) - context = template.TEMPLATE_TEST_CONTEXT['mediagoblin/auth/login.html'] - form = context['login_form'] - assert form.password.errors == [u'This field is required.'] + assert 'mediagoblin/auth/login.html' in template.TEMPLATE_TEST_CONTEXT # Failed login - bad user # ----------------------- @@ -359,20 +355,6 @@ def test_authentication_views(test_app): assert urlparse.urlsplit(response.location)[2] == '/u/chris/' -# App with authentication_disabled and no auth plugin enabled -def no_auth_plugin_app(request): - return get_app( - request, - mgoblin_config=pkg_resources.resource_filename( - 'mediagoblin.tests.auth_configs', - 'no_auth_plugin_appconfig.ini')) - - -def test_auth_plugin_raises(request): - with pytest.raises(AuthError): - no_auth_plugin_app(request) - - @pytest.fixture() def authentication_disabled_app(request): return get_app( -- 2.25.1