Password generator in Django

I am a system admin, not a developer, so this might be pretty horrible code. This is a password generator. The point of the attempt was to become more familiar with Django at a lower level. I current host Django sites other developers write and sometimes it would help to have a better understanding of how the sites work at a lower level. I realize a password generator should run client side not server side but a highly secure, strong password generator was not my primary concern in writing this. It just seemed like an appropriately sized problem to jump into this with.

Any feedback on either Python or Django best practices would be appreciated. The logic of the password generation is not my primary concern however I would be willing to hear any thoughts on that as well.

Since I am not using a database or any models I am sure I will be told some other framework is probably more appropriate but this was just an iterative step in learning Django. My next project will contain a database.

views.py

```
from django.http import HttpResponse
from django.shortcuts import render, render_to_response
from django.views import generic

import datetime
import random
import string

from .forms import PasswordGeneratorForm

def generate_password(request):
'''Generate a random password. Uses request POST data to allow user
to set length and character set.'''

pw_charset_lower = list(string.ascii_lowercase)
pw_charset_upper = list(string.ascii_uppercase)
pw_charset_digit = list(string.digits)
pw_charset_special = list(string.punctuation)
pw_charset_similar = ['o', 'O', '0', 'I', 'l', '1', '|']

if request.method == 'POST':
form = PasswordGeneratorForm(request.POST)

if form.is_valid():
pw_len = form.cleaned_data['length']

if request.POST.get('avoid_similar', False):
for x in pw_charset_similar:
if x in pw_charset_lower: pw_charset_lower.remove(x)
if x in pw_charset_upper: pw_charset_upper.remove(x)
if x in pw

Don't reinvent the wheel

Since you're handling a security feature, you shouldn't roll your own. As often with python, Django comes bateries included. And since they already need to generate random passwords (when prepopulating SECRET_KEY for instance), chances are they already have a more secure way of doing.

Indeed, django.utils.crypto.get_random_string() uses random.SystemRandom instead of random when available and try its best to avoid predictability when not. Chances are it will keep up with Python's security features in future releases.

Thus you just need to call get_random_string with the desired output length and a string of characters to choose from.
View

I said you should pass a string to get_random_string but any sequence of character would do. However, turning strings into lists here is really not needed. Strings already have all required features: they are iterables, indexables and have a len. Keep it simple.

You also handle forms a bit wrong: grabbing values from request.POST directly defeat the purpose of using forms in the first place and especialy form validation. Besides, what happens when the form is not valid (form.is_valid() returns False)? You'll get a NameError on pw_len.

From the documentation, a classic layout for form validation is as follow:

def generate_password(request):
    # if this is a POST request we need to process the form data
    if request.method == 'POST':
        # create a form instance and populate it with data from the request:
        form = PasswordGeneratorForm(request.POST)
        # check whether it's valid:
        if form.is_valid():
            # process the data in form.cleaned_data as required
            # ...
            # redirect to a new URL:
            return HttpResponseRedirect('/thanks/')

    # if a GET (or any other method) we'll create a blank form
    else:
        form = PasswordGeneratorForm()

    return render(request, 'password/index.html', {'form': form})

However, here you don't want to redirect to a success page, you want to stay on the same. We can't rely on early returns to stay DRY. An other common way is to use form = PasswordGeneratorForm(request.POST or None) without checking for request.method and rely on the fact that PasswordGeneratorForm(None).is_valid() will return False.
Style

From PEP8:

• Order your imports: standard library, other system packages, personal modules;

• Use 4 space per indentation level, and stay consistent: no 4 and then 2.

• Never put code after a colon.

• Extract your constant from your function, and name them all caps.

Proposed improvements

import string

from django.shortcuts import render
from django.utils.crypto import get_random_string 

from .forms import PasswordGeneratorForm

SIMILARS = {'o', 'O', '0', 'I', 'l', '1', '|'}

def generate_password(request):
    '''Generate a random password. Uses request POST data to allow user
    to set length and character set.
    '''

    form = PasswordGeneratorForm(request.POST or None)
    if not form.is_valid():
        context = {'password' : 'Your password will show here', 'length': 12, 'form': form}
    else:
        data = form.cleaned_data
        charset = ''
        if data['use_lower']:
            charset += string.ascii_lowercase
        if data['use_upper']:
            charset += string.ascii_uppercase
        if data['use_digits']:
            charset += string.digits
        if data['use_specials']:
            charset += string.punctuation

        if data['avoid_similar']:
            # keeping a list is fine here, get_random_string only need an indexable object
            charset = [c for c in charset if c not in SIMILARS]

        length = data['length']
        password = get_random_string(length, charset)
        context = {'password': password, 'length': length, 'form': form}

  return render(request, 'password/index.html', context)

We can also see that if no option is selected, the view will have a hard time terminating. It would be wise to add a custom validation step to your form to ensure that at least one option is selected.

StackExchange Code Review Q#152408, answer score: 6