This repository has been archived by the owner on Oct 9, 2023. It is now read-only.
Allow tokens with multiple audiences as long as one matches #285
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@pmahindrakar-oss this is roughly what's needed... feel free to validate and test though.. |
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Prafulla Mahindrakar <[email protected]>
pmahindrakar-oss
force-pushed
the
multiple-aud
branch
from
8abda03 to
6d61085
Compare
Codecov Report
@@ Coverage Diff @@
## master #285 +/- ##
==========================================
+ Coverage 58.50% 58.53% +0.02%
==========================================
Files 152 152
Lines 11018 11021 +3
==========================================
+ Hits 6446 6451 +5
+ Misses 3873 3872 -1
+ Partials 699 698 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
pmahindrakar-oss
requested review from
anandswaminathan,
katrogan,
kumare3 and
wild-endeavor
as code owners
|
@EngHabu this is ready for review. Verified this using keycloak |
kumare3
previously approved these changes
Feb 3, 2022
|
Wait - should you change line# 177 |
Signed-off-by: Prafulla Mahindrakar <[email protected]>
Signed-off-by: Prafulla Mahindrakar <[email protected]>
|
Thanks @kumare3 for pointing that out. Adding the matched audience to the identity context now |
kumare3
approved these changes
Feb 3, 2022
eapolinario
pushed a commit
that referenced
this pull request
Sep 6, 2023
* Allow tokens with multiple audiences as long as one matches Signed-off-by: Haytham Abuelfutuh <[email protected]> * Added unit tests Signed-off-by: Prafulla Mahindrakar <[email protected]> * Adding matched audience index in the context Signed-off-by: Prafulla Mahindrakar <[email protected]> * Update mocks Signed-off-by: Prafulla Mahindrakar <[email protected]> Co-authored-by: Prafulla Mahindrakar <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Haytham Abuelfutuh [email protected]
TL;DR
Tested this using keycloak as an external auth server.
The token generated by keycloak come with account as audience and verified that with admin not configured with this audience rejects the token
Unit tests added for multiple audience.
Tried adding keycloak token mapper to generate multiple audience but couldn't get that to work . But this code works in all cases which are required for this ticket.
Type
Are all requirements met?
Complete description
How did you fix the bug, make the feature etc. Link to any design docs etc
Tracking Issue
Remove the 'fixes' keyword if there will be multiple PRs to fix the linked issue
fixes flyteorg/flyte#1809