changes after cwebb's review

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")

mediagoblin/auth/forms.py

@@ -29,9 +29,7 @@ class ForgotPassForm(wtforms.Form):
 
 class ChangePassForm(wtforms.Form):
     password = wtforms.PasswordField(
-        'Password',
+        'Password')
-        [wtforms.validators.Required(),
-         wtforms.validators.Length(min=5, max=1024)])
     userid = wtforms.HiddenField(
         '',
         [wtforms.validators.Required()])

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']
+    if not hook_handle('authentication'):
-    auth_plugin = hook_handle('authentication')
-
-    if authentication_disabled is False and not auth_plugin:
-        raise AuthError
-
-    if authentication_disabled:
         _log.warning('No authentication is enabled')
         return False
     else:

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:
+        redirect_name = hook_handle('auth_no_pass_redirect')
-            return redirect(request, 'mediagoblin.plugins.openid.register')
+        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:
+        redirect_name = hook_handle('auth_no_pass_redirect')
-            return redirect(request, 'mediagoblin.plugins.openid.login')
+        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,
+            'mediagoblin/auth/forgot_password.html', {'fp_form': fp_form,})
-                                                      'focus': 'username'})
 
     # 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,
+                {'cp_form': cp_form,})
-                 'focus': 'password'})
 
     # in case there is a valid id but no user with that id in the db
     # or the token expired

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)
 

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()

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'),
+        _('Password'))
-        [wtforms.validators.Required(),
-         wtforms.validators.Length(min=5, max=1024)])

mediagoblin/templates/mediagoblin/auth/change_fp.html

@@ -34,12 +34,10 @@
     {{ csrf_token }}
     <div class="form_box">
       <h1>{% trans %}Set your new password{% endtrans %}</h1>
-      {{ wtforms_util.render_divs(cp_form) }}
+      {{ wtforms_util.render_divs(cp_form, True) }}
       <div class="form_submit_buttons">
         <input type="submit" value="{% trans %}Set password{% endtrans %}" class="button_form"/>
       </div>
     </div>
-</form><!-- Focus the field passed in with the focus arg-->
-<script>$(document).ready(function(){$({{ focus }}).focus();});</script>
 {% endblock %}
 

mediagoblin/templates/mediagoblin/auth/forgot_password.html

@@ -29,12 +29,10 @@
     {{ csrf_token }}
     <div class="form_box">
       <h1>{% trans %}Recover password{% endtrans %}</h1>
-      {{ wtforms_util.render_divs(fp_form) }}
+      {{ wtforms_util.render_divs(fp_form, True) }}
       <div class="form_submit_buttons">
         <input type="submit" value="{% trans %}Send instructions{% endtrans %}" class="button_form"/>
       </div>
     </div>
   </form>
-<!-- Focus the field passed in with the focus arg-->
-<script>$(document).ready(function(){$({{ focus }}).focus();});</script>
 {% endblock %}

mediagoblin/templates/mediagoblin/auth/login.html

@@ -45,7 +45,7 @@
             {%- trans %}Create one here!{% endtrans %}</a>
         </p>
       {% endif %}
-      {{ wtforms_util.render_divs(login_form) }}
+      {{ wtforms_util.render_divs(login_form, True) }}
       {% if pass_auth %}
         <p>
           <a href="{{ request.urlgen('mediagoblin.auth.forgot_password') }}" id="forgot_password">
@@ -61,6 +61,4 @@
       {% endif %}
     </div>
   </form>
-<!-- Focus the field passed in with the focus arg-->
-<script>$(document).ready(function(){$({{ focus }}).focus();});</script>
 {% endblock %}

mediagoblin/templates/mediagoblin/auth/register.html

@@ -34,7 +34,7 @@
         method="POST" enctype="multipart/form-data">
     <div class="form_box">
       <h1>{% trans %}Create an account!{% endtrans %}</h1>
-      {{ wtforms_util.render_divs(register_form) }}
+      {{ wtforms_util.render_divs(register_form, True) }}
       {{ csrf_token }}
       <div class="form_submit_buttons">
         <input type="submit" value="{% trans %}Create{% endtrans %}"
@@ -42,6 +42,4 @@
       </div>
     </div>
   </form>
-<!-- Focus the field passed in with the focus arg-->
-<script>$(document).ready(function(){$({{ focus }}).focus();});</script>
 {% endblock %}

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) }}
   <div class="form_field_input">
-    {{ field }}
+    {% if autofocus_first %}
+      {{ field(autofocus=True) }}
+    {% else %}
+      {{ field }}
+    {% endif %}
     {%- if field.errors -%}
       {% for error in field.errors %}
         <p class="form_field_error">{{ error }}</p>
@@ -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 %}
 

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"

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"

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"

mediagoblin/tests/auth_configs/no_auth_plugin_appconfig.ini

@@ -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]

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']
+    assert 'mediagoblin/auth/login.html' in template.TEMPLATE_TEST_CONTEXT
-    form = context['login_form']
-    assert form.password.errors == [u'This field is required.']
 
     # 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(