Skip to content

Conversation

casabre
Copy link

@casabre casabre commented Jul 20, 2025

Description

Adding Sequence support for audience in JWT Token.

Closes

#4240

@casabre casabre requested review from a team as code owners July 20, 2025 10:52
@casabre casabre changed the title Audience should support sequence too fix: audience should support sequence too Jul 20, 2025
Copy link

codecov bot commented Jul 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.24%. Comparing base (65b0bd2) to head (49489d8).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4241   +/-   ##
=======================================
  Coverage   98.24%   98.24%           
=======================================
  Files         344      344           
  Lines       15889    15890    +1     
  Branches     1755     1755           
=======================================
+ Hits        15610    15611    +1     
  Misses        139      139           
  Partials      140      140           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/4241

@igorcoding
Copy link

igorcoding commented Sep 17, 2025

Hi! Is there any chance that this would be merged soon? Our team is currently experiencing an issue with the aud being an array. Or Maybe you know of any workaround to mitigate this issue without patching source code?

@Harshal6927
Copy link
Member

Hello @casabre , thanks for the PR! Could you please add a test case so we can go ahead and merge it?

@pytest.mark.parametrize("token_audience", [None, "627224198b4245ed91cf8353e4ccdf1650728c7ee92748f55fe1e9a9c4d961df"])
@pytest.mark.parametrize(
"token_unique_jwt_id", [None, "10f5c6967783ddd6bb0c4e8262d7097caeae64705e45f83275e3c32eee5d30f2"]
"algorithm",
Copy link
Member

Choose a reason for hiding this comment

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

This doubles the amount of tests run, due to the multiple parametrize decorators. Could you maybe just add one simple case for the sequence audience?

@casabre
Copy link
Author

casabre commented Sep 18, 2025

@Harshal6927 @provinzkraut I added an own test for audience validation. Hope that works for you.

@casabre
Copy link
Author

casabre commented Sep 18, 2025

@provinzkraut I didn't change the imports but now pytest is complaining about a missing Sequence import during runtime. Actually, I don't know if this is related to a pytest version change and if I should fix the imports or leave it as it is...

@casabre
Copy link
Author

casabre commented Sep 19, 2025

@provinzkraut In the end, a missing import makes the pytests fail

FAILED tests/unit/test_security/test_jwt/test_auth.py::test_jwt_auth - NameError: name 'Sequence' is not defined

e.g. at https://github.com/litestar-org/litestar/actions/runs/17854788852/job/50771227842?pr=4241#step:10:9032

@provinzkraut
Copy link
Member

@provinzkraut I didn't change the imports but now pytest is complaining about a missing Sequence import during runtime. Actually, I don't know if this is related to a pytest version change and if I should fix the imports or leave it as it is...

Well it's because you are using Sequence but don't import it anywhere 😉

@casabre
Copy link
Author

casabre commented Sep 19, 2025

@provinzkraut I didn't change the imports but now pytest is complaining about a missing Sequence import during runtime. Actually, I don't know if this is related to a pytest version change and if I should fix the imports or leave it as it is...

Well it's because you are using Sequence but don't import it anywhere 😉

Yes, you right 😅. But I was relying on the correctness of previous logic --> because Sequence was used before my change I thus, I thought, it is covered. E.g here

issuer: str | Sequence[str] | None = None,

It's not a problem for me to pull the collections import out of the type checking guard, but I am really wondering how it was working before 🤔

@provinzkraut
Copy link
Member

It's not a problem for me to pull the collections import out of the type checking guard, but I am really wondering how it was working before 🤔

The function signature is not validated during runtime. The dataclass itself is.

@casabre
Copy link
Author

casabre commented Sep 19, 2025

It's not a problem for me to pull the collections import out of the type checking guard, but I am really wondering how it was working before 🤔

The function signature is not validated during runtime. The dataclass itself is.

@provinzkraut Yes, I understand that but why was it failing now but before not, when Sequence was accessed too. Anyway... let't move ahead --> if you are satisfied with the change, can you just approve it 😊.

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.

4 participants