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

Fix tests on main are broken #178

Merged
merged 8 commits into from
Jan 21, 2025
Merged

Fix tests on main are broken #178

merged 8 commits into from
Jan 21, 2025

Conversation

chrisvanrun
Copy link
Contributor

Also adds a few minor tweaks to make local testing more efficient.

@chrisvanrun chrisvanrun requested a review from amickan January 16, 2025 14:02
@chrisvanrun chrisvanrun linked an issue Jan 16, 2025 that may be closed by this pull request
gcapi/client.py Outdated Show resolved Hide resolved
gcapi/client.py Outdated Show resolved Hide resolved
local_api_url = "https://gc.localhost/api/v1/"
local_api_url = os.environ.get(
"LOCAL_API_URL",
"https://gc.localhost/api/v1/",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use http://gc.localhost:8000 locally.

Copy link
Member

Choose a reason for hiding this comment

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

Might be good to namespace the env var, GCAPI_TESTS_LOCAL_API_URL maybe?

Copy link
Member

@jmsmkn jmsmkn left a comment

Choose a reason for hiding this comment

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

The changes to the tests look good but please can you revert the checks against https checking? The checks are there not protect against MITM - they are to protect against the auth token being sent over HTTP where everyone has access to it unencrypted. I would very much prefer that we maintained these checks as it would otherwise mean that it is very easy to send the users auth token to GC over http which would result in account compromise.

@jmsmkn
Copy link
Member

jmsmkn commented Jan 16, 2025

BTW not tried it myself but you might be able to run the test server locally with SSL directly https://django-extensions.readthedocs.io/en/latest/runserver_plus.html#ssl

@chrisvanrun
Copy link
Contributor Author

BTW not tried it myself but you might be able to run the test server locally with SSL directly https://django-extensions.readthedocs.io/en/latest/runserver_plus.html#ssl

Good idea. I'll try that route next time I'm working on gcapi locally.

@chrisvanrun
Copy link
Contributor Author

The changes to the tests look good but please can you revert the checks against https checking? The checks are there not protect against MITM - they are to protect against the auth token being sent over HTTP where everyone has access to it unencrypted. I would very much prefer that we maintained these checks as it would otherwise mean that it is very easy to send the users auth token to GC over http which would result in account compromise.

Reverted. Let's be double sure the users are using https when sending the tokens!

@chrisvanrun chrisvanrun requested a review from jmsmkn January 17, 2025 08:37
@pkcakeout
Copy link
Member

Quickly writing it here as well: https://github.com/DIAGNijmegen/rse-cirrus-core/issues/3377 CIRRUS is broken, I suspect due to the ordering thing, maybe? I did not have time to sink my teeth in this yet, needs to wait until Monday.

Copy link
Member

@jmsmkn jmsmkn left a comment

Choose a reason for hiding this comment

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

Looks good! Couple of minor questions.

pyproject.toml Outdated Show resolved Hide resolved
local_api_url = "https://gc.localhost/api/v1/"
local_api_url = os.environ.get(
"LOCAL_API_URL",
"https://gc.localhost/api/v1/",
Copy link
Member

Choose a reason for hiding this comment

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

Might be good to namespace the env var, GCAPI_TESTS_LOCAL_API_URL maybe?

@chrisvanrun chrisvanrun requested a review from jmsmkn January 21, 2025 09:39
Copy link
Member

@jmsmkn jmsmkn left a comment

Choose a reason for hiding this comment

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

Thanks!

@jmsmkn jmsmkn merged commit a802507 into main Jan 21, 2025
14 checks passed
@jmsmkn jmsmkn deleted the 177-tests-on-main-are-broken branch January 21, 2025 11:42
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.

Tests on main are broken
3 participants