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

Add invalid algorithm exception check #3399

Merged
merged 3 commits into from
Apr 25, 2023

Conversation

npalaska
Copy link
Member

We need to catch an invalid algorithm error when we decode the SSO token and raise it as OpenIDTokenInvalid. This is because we are using HS256 for our internal api key encode-decode.

PBENCH_1136

@npalaska npalaska self-assigned this Apr 25, 2023
@npalaska npalaska added this to the v0.72 milestone Apr 25, 2023
dbutenhof
dbutenhof previously approved these changes Apr 25, 2023
Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

NOTE: I don't think Jira will link this because it's "PBENCH-1136" not "PBENCH_1136"; but it's good enough for human readers, and we can add a manual link if necessary. (But if you have reason for edits or a rebase, changing the commit message to link would be good.)

webbnh
webbnh previously approved these changes Apr 25, 2023
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Just nits.

lib/pbench/server/auth/__init__.py Show resolved Hide resolved
lib/pbench/test/unit/server/auth/test_auth.py Outdated Show resolved Hide resolved
lib/pbench/test/unit/server/auth/test_auth.py Outdated Show resolved Hide resolved
@npalaska npalaska dismissed stale reviews from webbnh and dbutenhof via cedffa6 April 25, 2023 19:19
dbutenhof
dbutenhof previously approved these changes Apr 25, 2023
lib/pbench/server/auth/__init__.py Show resolved Hide resolved
@dbutenhof
Copy link
Member

Intriguing: you have "real" unit test failures related to authentication. Did you run the full server unit test suite before submitting? Is this one of those bizarre differences between local and CI environments? 😦

WARNING pbench.server.api:init.py:1731 Datasets: Unauthenticated client is not authorized to DELETE a resource owned by drb with private access
_____________ TestDatasetsDetail.test_http_exception[exceptions1] ______________
[gw25] linux -- Python 3.9.16 /var/tmp/jenkins/tox/py39/bin/python
Traceback (most recent call last):
File "/var/tmp/jenkins/tox/py39/lib/python3.9/site-packages/pbench/test/unit/server/query_apis/commons.py", line 445, in test_http_exception
query_api(
File "/var/tmp/jenkins/tox/py39/lib/python3.9/site-packages/pbench/test/unit/server/query_apis/conftest.py", line 67, in query_api
response = client_method(
File "/var/tmp/jenkins/tox/py39/lib/python3.9/site-packages/responses/init.py", line 929, in exit
self.stop(allow_assert=success)
File "/var/tmp/jenkins/tox/py39/lib/python3.9/site-packages/responses/init.py", line 1126, in stop
raise AssertionError(
AssertionError: Not all requests have been executed [('GET', 'http://elasticsearch.example.com:7080/unit-test.v6.run-data.2020-08/_search?ignore_unavailable=true')]

@npalaska
Copy link
Member Author

Intriguing: you have "real" unit test failures related to authentication. Did you run the full server unit test suite before submitting? Is this one of those bizarre differences between local and CI environments?

This is very bizarre, I don't see it when I ran locally with pytest before submitting. I am trying to see what's really happening here.

@dbutenhof
Copy link
Member

Intriguing: you have "real" unit test failures related to authentication. Did you run the full server unit test suite before submitting? Is this one of those bizarre differences between local and CI environments?

This is very bizarre, I don't see it when I ran locally with pytest before submitting. I am trying to see what's really happening here.

I always do a jenkins/run tox server python before raising a PR; a local pytest is convenient for development but can have a lot of differences from the CI environment, and that can trip you up.

@npalaska
Copy link
Member Author

I always do a jenkins/run tox server python before raising a PR; a local pytest is convenient for development but can have a lot of differences from the CI environment, and that can trip you up.

Yeah, I usually do that but I didn't think that it's gonna break everything. However, what I don't understand is that the unit tests are failing on changes from the main branch as well so I not sure whether the failures here are related to these changes. 😨

@dbutenhof
Copy link
Member

I always do a jenkins/run tox server python before raising a PR; a local pytest is convenient for development but can have a lot of differences from the CI environment, and that can trip you up.

Yeah, I usually do that but I didn't think that it's gonna break everything. However, what I don't understand is that the unit tests are failing on changes from the main branch as well so I not sure whether the failures here are related to these changes. fearful

Indeed. Riya's cache manager PR has similar failures; and running the test set locally now also fails. I wonder if we had another package upgrade behind our backs? 😦

In fact, something seems to have re-triggered every PR, and they've all failed. Wow.

@dbutenhof
Copy link
Member

I just compared packages between a re-run that failed this afternoon and an earlier run that worked. The differences are:

+blinker==1.6.2
-Flask==2.2.3
+Flask==2.3.0
-Werkzeug==2.2.3
+Werkzeug==2.3.0

I'm assuming blinker must be a new dependency of flask, but I didn't investigate; werkzeug is a flask dependency.

So ...

@dbutenhof
Copy link
Member

And ... the weird thing is that changing our flask dependency in requirements.txt to Flask==2.2.3 doesn't "take" ... the containerized tests still run with 2.3.0 and fail. I'm assuming those two facts are connected...

@dbutenhof
Copy link
Member

Intriguing: there's suddenly a Flask 2.3.1 with the one change "Restore deprecated from flask import Markup. #5084". Hmm...

@npalaska
Copy link
Member Author

npalaska commented Apr 25, 2023

Intriguing: there's suddenly a Flask 2.3.1 with the one change "Restore deprecated from flask import Markup. #5084". Hmm...

Yes I was gonna say they just upgraded to 2.3.1, I'll try this version and see if it fixes everything. but... its not available on PyPi I guess.

@dbutenhof
Copy link
Member

but... its not available on PyPi I guess.

Yeah, I've been trying ... maybe it'll fix the problem ... and maybe it'll be available soon. Sigh.

@webbnh
Copy link
Member

webbnh commented Apr 25, 2023

See this comment.

@npalaska
Copy link
Member Author

See this comment.

Okay that explains.

@dbutenhof
Copy link
Member

See this comment.

The context makes me fear that 2.3.1 is not a fix for whatever we're hitting. And for some reason I'm having no luck locking Flask to 2.2.x... it's ignoring me and using 2.3.0 anyway.

We need to catch invalid algorithm error when we
decode the SSO token and raise it as OpenIDTokenInvalid.
This is because we are using HS256 for our internal api
key encode-decode.

PBENCH-1136
lib/pbench/server/auth/__init__.py Show resolved Hide resolved
@npalaska npalaska merged commit 92fed82 into distributed-system-analysis:main Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants