Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fallback for ip optional #35

Closed
wants to merge 4 commits into from
Closed

fallback for ip optional #35

wants to merge 4 commits into from

Conversation

blodone
Copy link

@blodone blodone commented May 3, 2015

With this commit it would be optional to provide a fallback for reliable IP information.
otherwise we would get an integrity error so with the attribute error the user knows whats going wrong

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.35%) to 92.57% when pulling b1932c2 on induux:master into 555144f on Bouke:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.35%) to 92.57% when pulling b1932c2 on induux:master into 555144f on Bouke:master.

@Bouke
Copy link
Collaborator

Bouke commented May 3, 2015

Hi @blodone thanks again for your effort. Could you explain why you'd want to see the working of this changed? As I've explained elsewhere:

I don't think that merging your PR is a good idea, as Django doesn't include a similar middleware for good reasons. However I can see your need for a solution, so you could create a custom middleware that sets REMOTE_ADDR based on X-Forwarded-For. If the middleware appears before the user session middleware, everything works as intended.

@blodone
Copy link
Author

blodone commented May 3, 2015

i think for more then 50% of the people using this a module they need a way to get it working behind a proxy without writing own middleware to emulate REMOTE_ADDR just to supply django-user-sessions.

As i explained its optional, so it does not affect security unless you enable it explicitly in the settings.
Also the own middleware would also affect security, even though when bad written it exposes a greater security risk than enabling a fallback when REMOTE_ADDR is None

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.35%) to 92.57% when pulling 1da7b5e on induux:master into 555144f on Bouke:master.

@Bouke
Copy link
Collaborator

Bouke commented May 14, 2015

I won't merge this PR as explained above and in the issues I linked to. The goal of this package is to provide Session objects as first-class ORM models. How the IP address is derived from the request information is out-of-scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants