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

Unit test on jwt exception type instead of string #3417

Merged
merged 2 commits into from
May 10, 2023
Merged

Unit test on jwt exception type instead of string #3417

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a1e8ef
unit test on jwt exception type instead of string
npalaska May 10, 2023
e316ecd
use isinstance to infer the type of cause
npalaska May 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions lib/pbench/server/auth/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,11 @@ def __str__(self) -> str:


class OpenIDTokenInvalid(Exception):
pass
def __init__(self, exc_type: Exception):
self.exc_type = exc_type

def __str__(self) -> str:
return str(f"Token invalid with exception: {self.exc_type}")
Copy link
Member

Choose a reason for hiding this comment

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

While I like the idea of having "our exception" be a wrapper for the underlying exception (e.g., having the stringification of our exception include the stringification of the underlying exception), the argument to the c'tor should be an exception instance, not an exception type -- when we stringify the underlying exception, any arguments with which it was created should be reflected in the string, and that won't happen if we reference the class instead of the instance.

Also, in general, when you derive a subclass from a superclass, you should invoke the superclass c'tor in the subclass c'tor (this is implicit in many languages, but not in Python). Moreover, the Exception class already provides a __str__() method which we can make use of, here. E.g.:

class OpenIDTokenInvalid(Exception):
    def __init__(self, exc: Exception):
        super().__init__(f"Token invalid with exception: {exc}")

That is, str(OpenIDTokenInvalid(e)) will use Exception.__str__() and it will return Token invalid with exception: followed by the stringification of e, and you don't need any more code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just reverted this change. If we use isinstance to check the type of the cause in the unit test, we don't need to touch this exception class.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good sign that using isinstance() is the right path. 😁

However, doing the super() call is "more correct" than the current code, and it enables direct (and useful) stringification of the resulting instance. (The current implementation will inherit the __str__() method, but, since it doesn't call the superclass c'tor, the method will produce an empty string, which is quite unsatisfying.)



class Connection:
Expand Down Expand Up @@ -377,4 +381,4 @@ def token_introspect(self, token: str) -> JSON:
jwt.InvalidAudienceError,
jwt.InvalidAlgorithmError,
) as exc:
raise OpenIDTokenInvalid() from exc
raise OpenIDTokenInvalid(exc_type=exc.__class__)
webbnh marked this conversation as resolved.
Show resolved Hide resolved
14 changes: 4 additions & 10 deletions lib/pbench/test/unit/server/auth/test_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,9 +378,7 @@ def test_token_introspect_exp(self, monkeypatch, rsa_keys):

with pytest.raises(OpenIDTokenInvalid) as exc:
oidc_client.token_introspect(token)
assert (
str(exc.value.__cause__) == "Signature has expired"
), f"{exc.value.__cause__}"
assert exc.value.exc_type == jwt.ExpiredSignatureError, f"{exc.value.exc_type}"
Copy link
Member

Choose a reason for hiding this comment

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

assert isinstance(exc.value.__cause__, jwt.ExpiredSignatureError) would allow you to keep using the raise [] from e.

And in any case, the comparison should be is rather than ==.


def test_token_introspect_aud(self, monkeypatch, rsa_keys):
"""Verify .token_introspect() failure via audience error"""
Expand All @@ -394,7 +392,7 @@ def test_token_introspect_aud(self, monkeypatch, rsa_keys):

with pytest.raises(OpenIDTokenInvalid) as exc:
oidc_client.token_introspect(token)
assert str(exc.value.__cause__) == "Invalid audience", f"{exc.value.__cause__}"
assert exc.value.exc_type == jwt.InvalidAudienceError, f"{exc.value.exc_type}"

def test_token_introspect_sig(self, monkeypatch, rsa_keys):
"""Verify .token_introspect() failure via signature error"""
Expand All @@ -411,9 +409,7 @@ def test_token_introspect_sig(self, monkeypatch, rsa_keys):
with pytest.raises(OpenIDTokenInvalid) as exc:
# Make the signature invalid.
oidc_client.token_introspect(token + "1")
assert (
str(exc.value.__cause__) == "Signature verification failed"
), f"{exc.value.__cause__}"
assert exc.value.exc_type == jwt.InvalidSignatureError, f"{exc.value.exc_type}"

def test_token_introspect_alg(self, monkeypatch, rsa_keys):
"""Verify .token_introspect() failure via algorithm error"""
Expand All @@ -430,9 +426,7 @@ def test_token_introspect_alg(self, monkeypatch, rsa_keys):

with pytest.raises(OpenIDTokenInvalid) as exc:
oidc_client.token_introspect(generated_api_key)
assert (
str(exc.value.__cause__) == "The specified alg value is not allowed"
), f"{exc.value.__cause__}"
assert exc.value.exc_type == jwt.InvalidAlgorithmError, f"{exc.value.exc_type}"


@dataclass
Expand Down