ValidationError output of SafeData

[Andrew Pinkham]

Hi,
I mentioned on the Django User Mailing List that the interaction of 
ValidationError, mark_safe, and ugettext was not what I expected.

https://groups.google.com/d/topic/django-users/Soik095MTaI/discussion

Succinctly, the two versions of clean_slug below behave differently, when they 
should not.

# results in  being printed for user
def clean_slug(self):
new_slug = self.cleaned_data['slug'].lower()
if new_slug == 'invalid_value':
raise ValidationError(
# _ is ugettext
mark_safe(_('SlugField may not be '
'"%(slug_value)s" '
'for URL reasons.')),
params={
'slug_value':
mark_safe('invalid_value')})
return new_slug

# results in  being valid HTML elements
def clean_slug(self):
new_slug = self.cleaned_data['slug'].lower()
if new_slug == 'invalid_value':
raise ValidationError(
# _ is ugettext
mark_safe(_('SlugField may not be '
'"invalid_value" '
'for URL reasons.')))
return new_slug 

The problem behavior is caused by the way ValidationError outputs its text; 
ValidationError behaves differently depending on whether params are used, which 
seems undesirable. We can demonstrate this in the Django shell (1.7.4).

>>> from django.core.exceptions import ValidationError
>>> from django.utils.safestring import mark_safe, SafeData
>>> from django.utils.six import text_type
>>> isinstance(mark_safe('hello'), SafeData)
True

When the ValidationError message is SafeData, the output of the ValidationError 
is also SafeData.

>>> ve = ValidationError('this is static')
>>> isinstance(list(ve)[0], text_type)
True
>>> ve = ValidationError(mark_safe('this is safe'))
>>> isinstance(list(ve)[0], SafeData)
True

... unless we use the params feature of the exception, as the documentation 
tells developers to.

>>> ve = ValidationError(mark_safe('%(r)s'), params={'r': 'replaced'})
>>> isinstance(list(ve)[0], SafeData)
False
>>> ve = ValidationError(mark_safe('%(r)s'), params={'r': 
mark_safe('replaced')})
>>> isinstance(list(ve)[0], SafeData)
False

The difference in behavior depending on whether params is passed should not 
happen.

I can see how in the first instance above---where the message is SafeData but 
the params values aren't---it is desirable to return text instead of SafeData 
from a security standpoint. However, in the second case, when both the message 
and the params are SafeData, it seems counter-intuitive to return text.

| message | params | output |
|-|||
| text| none   | text   |
| safe| none   | safe   |
| text| text   | text   |
| safe| text   | text   |
| text| safe   | text   |
| safe| safe   | safe   | <-- the problem: currently outputs text

The issues stems from ValidationError.__iter__(). In `add_error` of form 
validation, ValidationErrors are added on with the following:

# django/forms/forms.py: line 353
self._errors[field].extend(error_list)

This code results in a call to ValidationError.__iter__(),

# django/core/exceptions.py: line 151
def __iter__(self):
if hasattr(self, 'error_dict'):
for field, errors in self.error_dict.items():
yield field, list(ValidationError(errors))
else:
for error in self.error_list:
message = error.message
if error.params:
message %= error.params
yield force_text(message)

Turns out, __mod__ on SafeText is not defined, meaning the code `message %= 
error.params` always transforms SafeText to text.

>>> isinstance(mark_safe('hello %(r)s'), SafeData)
True
>>> isinstance(mark_safe('hello %(r)s') % {'r': 'boo'}, SafeData)
False
>>> isinstance(mark_safe('hello %(r)s') % {'r': mark_safe('boo')}, SafeData)
False

I can thus think of two ways to solve this:

1. Modify ValidationError.__iter__() to account for SafeText with params.
2. add __mod__() to SafeText

Given that ugettext (or rather, do_translate() in 
django/utils/translation/trans_real.py) takes the first approach, I've created 
a branch in my fork of Django to do just that.

https://github.com/jambonrose/django/commit/61612f38aa68efcb5038f51305359b25485e2f48

My djangocore-box isn't working quite the way it should (postgres tests will 
not run on master), but all the tests appear to run correctly on 
runtests2.7-sqlite and runtests3.3-sqlite.

With that said: while I think there is a problem in the difference in behavior, 
I'm not sure my solution is the best. More simply: is the following behavior 
table actually what we want?

| message | params | output |
|-|||
| text| none   | text   |
| safe| no

Re: ValidationError output of SafeData

[Andrew Pinkham]

I'm noting for posterity of this thread that I've opened a ticket and issued a 
pull-request.

Ticket:  https://code.djangoproject.com/ticket/24639
PR:  https://github.com/django/django/pull/4497

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/C009F6A7-1049-4494-AFD6-9285828F68D5%40andrewsforge.com.
For more options, visit https://groups.google.com/d/optout.

Re: Unifying locale time formats

[Andrew Pinkham]

On Sep 10, 2014, at 4:21 PM, Malte  wrote:
> That's a lot of locales to change though. I am still somewhat new to Git. How 
> would you go about it?

I asked about Git recently on the Django Core Mentorship list. Carl Meyer 
responded with a really good workflow for creating a PR. I have copied key 
parts of his response below.

Begin forwarded message:
> The best way [...] is to always use a new branch for
> each pull request, instead of making your changes directly in master.
> This way it doesn't matter whether your PR is merged exactly as-is, or
> is squashed or otherwise modified (as happened in this case) -- either
> way master always tracks exactly what happened upstream, without
> interference from your original version of the changes.
> 
> Sample sequence of git commands you might use to make a new local branch
> and make a PR from it (with interspersed commentary):
> 
> # First make sure your new branch will based on the latest master
> $ git checkout master
> # If your local master tracks upstream, you can omit "upstream master":
> $ git pull upstream master
> 
> # Create a branch: I often name it tX for the Trac ticket #
> $ git checkout -b t22951
> 
> # Make your changes and commit them...
> 
> # Then push your local branch to a branch of the same name
> # on your GitHub remote. The -u sets up a "tracking" relationship,
> # so in future you can just "git push" from this branch and it will
> # automatically know where to push to.
> $ git push -u origin t22951
> 
> Now if you visit GitHub shortly after this push, it'll automatically
> prompt you to create a pull-request from your recently-pushed t22951 branch.

Tim Graham also pointed me to a part of the documentation that may be helpful:

https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/submitting-patches/#patch-review-checklist

Hope that helps,
Andrew

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CBD94E54-A792-4C73-8699-9431AE6EE073%40andrewsforge.com.
For more options, visit https://groups.google.com/d/optout.

Re: Two proposals for the Django Code of Conduct.

[Andrew Pinkham]

Hi,
I am not qualified enough to express an opinion on the matter of the DCoC. 
However, I have a few questions:

- Have we consulted a psychologist or a specialist on the topics of community 
inclusion and protection? Their knowledge/research could be instrumental in 
determining the best way to write and implement a code of conduct.

- Am I correct in assuming that the sole aforementioned invocation of the DCoC 
was the following?
https://groups.google.com/d/msg/django-developers/Y_Tq7w4PAeI/WurT7J3Rcd0J

- Would it be correct to state that, with or without this wording, the results 
of breaking the DCoC would be identical? Is it correct to say that the new 
wording is for the benefit of being clear and open, such that new members 
better understand the rules?

- These rules have been referred to several times as guidelines to adhere to. 
Would it be in the interest of the community to say as much on the DCoC webpage 
(rather than or alongside "rules")?

- The issue of Free Speech (I'm assuming the American definition), has been 
invoked a few times. In what way would this new clause inhibit Free Speech?

Thanks you,
Andrew

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/A00F1B78-3CCF-4079-8EAA-D52B18B9869D%40andrewsforge.com.
For more options, visit https://groups.google.com/d/optout.

DjangoCon US 2016

[Andrew Pinkham]

We are pleased to announce that DjangoCon US will be hosted by the Wharton 
School at the University of Pennsylvania in Philadelphia from July 17-22!

We are looking for suggestions about topics that you would like to see at 
DjangoCon. If you have a suggestion, please add it to this gist document 
.

The call for proposals is open! 
 We want to 
hear about your experience with and around Django. However, talks: 

- Don't need to be about Django specifically
- Don't need to be 100% fleshed out.
- Don't need to be about software directly
- Don't need to be advanced

Apply now! The deadline is April 18th. 

Never spoken before? Not a problem! There are speaker mentors that will help 
you (see the link above).

Is cost a concern? Not a problem! Apply for financial aid (deadline April 
25th). 

You can follow all the latest on Twitter .

Hope to see you in Philly!

Andrew

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/7FD4A934-85F0-4206-BDCF-16C33CBC4CD2%40andrewsforge.com.
For more options, visit https://groups.google.com/d/optout.

Inconsistent Behavior in Auth with UserAttributeSimilarityValidator

[Andrew Pinkham]

Hi,
I've found some inconsistent behavior in how password validation via the 
`UserAttributeSimilarityValidator` is handled by `contrib/auth`.  I did not 
find any mention of this in Trac or on the mailing list, but apologies if this 
has already been discussed.

AFAICT: When creating a user, the password is only checked in similarity to the 
`username` field. When changing a password, the password is checked in 
similarity to `username`, `first_name`, `last_name`, and `email`.

This seems undesirable - it is possible to set a password at creation time that 
might be rejected if set during a password change.

In short:
When `SetPasswordForm` calls password validation, it has all of the fields on 
the `User` instance, but when `UserCreationForm` does so, it manually adds the 
one field being checked.

In depth:
In `django/contrib/auth/forms.py`, the `SetPasswordForm` and `UserCreationForm` 
both call `password_validation.validate_password()` in the `clean_password2()` 
method. In both instances, the forms pass `password2` and `self.instance`. In 
the `UserCreationForm`, the `ModelForm` hasn't yet created the `User` instance. 
`UserCreationForm` manually adds `username` to the empty `User` instance on 
line 105 to allow for user attribute validation.

Rather than check the validity of the passwords in the `password2` clean 
method, it seems like it would be better to wait until the `User` instance has 
been created. We can use the `ModelForm` `_post_clean()` hook to achieve this.

def _post_clean(self):
super()._post_clean()  # creates self.instance
password = self.cleaned_data.get("password1")
if password:
try:
password_validation.validate_password(password, self.instance)
except ValidationError as error:
self.add_error("password1", error)

This keeps the two forms' behaviors consistent. Note that I've opted to use 
`password1` instead of `password2` because it feels more logical to show the 
problem above both passwords, rather than between the two fields.

Is there a reason *not* to make this change? Are there specific reasons for why 
we are only checking the `username` field during creation? 

Feedback appreciated. If others approve of this, I will open a PR.

Andrew
http://jambonsw.com
http://django-unleashed.com

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/878428A0-4F59-403A-A611-DD9208A605C9%40andrewsforge.com.
For more options, visit https://groups.google.com/d/optout.