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.

Sign up for GitHub

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

Jump to bottom

refactor(IP Address): Update all IP Addresses in tests to use RFC-5735 TEST-NET-* addresses. #15470

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

refactor(IP Address): Update all IP Addresses in tests to use RFC-5735 TEST-NET-* addresses. #15470

wants to merge 25 commits into from

Conversation

Mr-Sunglasses
Copy link
Contributor

fix: #13186

@Mr-Sunglasses
refactor(IP Address): Update all IP Addresses in tests/unit/account t…
dc5b052 
…o use RFC-5735 TEST-NET-* addresses
@Mr-Sunglasses
refactor(IP Address): Update test/conftest.py to use RFC-5735 TEST-NET-*
c6e49e6
@Mr-Sunglasses
refactor(IP Address): Update test/unit/email/* to use RFC-5735 TEST-N…
31d1613 
…ET-*
@Mr-Sunglasses
refactor(IP Address): Update test/unit/events/* to use RFC-5735 TEST-…
98c3aa5 
…NET-*
@Mr-Sunglasses
refactor(IP Address): Update test/unit/manage/* to use RFC-5735 TEST-…
0b461a1 
…NET-*
@Mr-Sunglasses
refactor(IP Address): Update test/unit/manage/* to use RFC-5735 TEST-…
a317951 
…NET-*
@Mr-Sunglasses
refactor(IP Address): Update test/unit/utils/* to use RFC-5735 TEST-N…
3025736 
…ET-*
@Mr-Sunglasses
refactor(IP Address): Update test/unit/legacy/* to use RFC-5735 TEST-…
8f62827 
…NET-*
@Mr-Sunglasses
refactor(IP Address): Update test/unit/admin/* to use RFC-5735 TEST-N…
847bce4 
…ET-*
@Mr-Sunglasses
refactor(IP Address): Update test/unit/ip_addresses/* to use RFC-5735…
27defd1 
… TEST-NET-*
@Mr-Sunglasses
revert: Update test/unit/test_json/* to use RFC-5735 TEST-NET-*
58491b1
@Mr-Sunglasses
fix: broken tests
3419cc9
@Mr-Sunglasses Mr-Sunglasses marked this pull request as ready for review February 25, 2024 19:30
@Mr-Sunglasses Mr-Sunglasses requested a review from a team as a code owner February 25, 2024 19:30
@Mr-Sunglasses
Copy link
Contributor Author

Hey @miketheman , as far as I've researched on the issue #13186
The IP address ranges for RFC-5735 TEST-NET-*

TEST-NET-1: 192.0.2.0/24
TEST-NET-2: 198.51.100.0/24
TEST-NET-3: 203.0.113.0/24

I've updated all the IP Addresses in test/

miketheman
miketheman reviewed Feb 26, 2024
View reviewed changes
Copy link
Member

@miketheman miketheman left a comment

Choose a reason for hiding this comment

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

Nice work, a couple of comments inline to think about.

tests/conftest.py Show resolved Hide resolved
tests/unit/accounts/test_forms.py Outdated Show resolved Hide resolved
@miketheman miketheman added the testing Test infrastructure and individual tests label Feb 26, 2024
@Mr-Sunglasses
Copy link
Contributor Author

@miketheman I've made the changes you suggested me, let me know if there are any more changes to be made in this PR :)

miketheman
miketheman requested changes Mar 1, 2024
View reviewed changes
Copy link
Member

@miketheman miketheman left a comment

Choose a reason for hiding this comment

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

@Mr-Sunglasses This is coming along nicely, thanks for continuing the effort!

There's a few more cases of non-replaced IP address strings - I haven't exhaustively identified each one, but we're getting closer!

tests/unit/utils/test_wsgi.py
@@ -130,13 +138,14 @@ def test_accepts_x_forwarded_headers(self, remote_addr_salted):

resp = wsgi.ProxyFixer(app, token=None, ip_salt="pepa")(environ, start_response)

expected_remote_addr_hashed = calculate_hashed_value("1.2.3.4", "pepa")
Copy link
Member

Choose a reason for hiding this comment

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

question: Can you help me understand why we're back to using a 1.2.3.4 here?
In my head, all the REMOTE_ADDR entries above could be replaced by the remove_addr fixture, and the REMOTE_ADDR_HASHED below could be replaced with the remote_addr_hashed fixture, and the calculate_hashed_value() function could go away.‏

tests/conftest.py
Created statically to prevent needing to calculate it every run.
"""
return "6694f83c9f476da31f5df6bcc520034e7e57d421d247b9d34f49edbfc84a764c"
@pytest.fixture(scope="function")
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't all of these be scoped to session, so they effectively become set once per test suite run?

Suggested change
@pytest.fixture(scope="function")
@pytest.fixture(scope="session")

tests/conftest.py
Comment on lines +120 to +121
if remote_addr is None:
return None
Copy link
Member

Choose a reason for hiding this comment

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

question: How could remote_addr be None? Do we ever override the input to this fixture function, or should this condition be removed?

tests/unit/accounts/test_security_policy.py
@@ -101,12 +101,12 @@ def test_identity_credentials_fail(self, monkeypatch):
pretend.stub(
matched_route=None,
banned=pretend.stub(by_ip=lambda ip_address: False),
remote_addr="1.2.3.4",
remote_addr="192.0.2.1",
Copy link
Member

Choose a reason for hiding this comment

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

These should also probably be replaced by remote_addr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miketheman miketheman miketheman requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
testing Test infrastructure and individual tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update all IP Addresses in tests to use RFC-5735 TEST-NET-* addresses.
2 participants
@Mr-Sunglasses @miketheman