Skip to content

Conversation

@SemVerTsar
Copy link
Contributor

ProfileAvatar doesn't set permission_classes. This should use IsAuthenticated to avoid the possibility of misconfiguration.

@LilyFirefly
Copy link
Author

@meshy @nologo As we discussed.

@meshy
Copy link
Contributor

meshy commented Oct 24, 2014

👍

@SemVerTsar
Copy link
Contributor

Review Please? Yes I'm aware of the typo in the branch name.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 8db59a2 on profile-avatat-perm into b94a288 on master.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you setting request.user for an unauthenticated request?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think IsAuthenticatedOrReadOnly is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The avatar will need to be gotten by unauthenticated users, so I think IsAuthenticatedOrReadOnly is correct.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what the UserAvatar view is for I think.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling f1501a9 on profile-avatat-perm into b94a288 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling f3c632c on profile-avatat-perm into b94a288 on master.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't care about this user any more.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 4c51b3f on profile-avatat-perm into b94a288 on master.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avatars.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not actually testing post here.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 3e62254 on profile-avatat-perm into b94a288 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 70fa00b on profile-avatat-perm into b94a288 on master.

@SemVerTsar
Copy link
Contributor

@ian-foote That should do it

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling f4a30e3 on profile-avatat-perm into b94a288 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling f4a30e3 on profile-avatat-perm into b94a288 on master.

LilyFirefly pushed a commit that referenced this pull request Oct 28, 2014
ProfileAvatar doesn't specify permissions
@LilyFirefly LilyFirefly merged commit 2a610b9 into master Oct 28, 2014
@LilyFirefly LilyFirefly deleted the profile-avatat-perm branch October 28, 2014 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants