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

Jump to bottom

fix: really assign string scp claim #6336

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: really assign string scp claim #6336

wants to merge 1 commit into from
+2 −1

Conversation

vlada-dudr
Copy link

@vlada-dudr vlada-dudr commented Mar 12, 2025 •
edited by flyte-bot
Loading

Why are the changes needed?

because before this change string formatted claims were ignored.

How was this patch tested?

I did run it against keycloak, which was forced to send scp claim as string. I.e. "scp": "all"

Labels

  • fixed: String formatted scp claim now works

Summary by Bito

This PR fixes a critical authentication bug by properly assigning scopes from string-formatted claims. The fix corrects an improper assignment that would have ignored the claim. A test file update adds client_id parameter support to validate the fix, improving overall claim handling reliability.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 1

@welcome Welcome
Copy link

welcome bot commented Mar 12, 2025

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

  • Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
  • Sign off your commits (Reference: DCO Guide).

@flyte-bot
Copy link
Collaborator

flyte-bot commented Mar 12, 2025 •
edited
Loading

Code Review Agent Run #e56972

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: b1d908e..b1d908e
    • flyteadmin/auth/authzserver/claims_verifier.go
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

Refer to the documentation for additional commands.

Configuration

This repository uses code_review_bito You can customize the agent settings here or contact your Bito workspace admin at [email protected].

Documentation & Help

AI Code Review powered by Bito Logo

@flyte-bot
Copy link
Collaborator

flyte-bot commented Mar 12, 2025 •
edited
Loading

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Bug Fix - Bug Fix - Correct Scopes Assignment

- claims_verifier.go - Changed variable assignment to correctly capture the string formatted scopes claim.
Testing - Testing Update - Expanded Test Parameter

- claims_verifier_test.go - Added a new test line with a client_id parameter to enhance test coverage.

@flyte-bot
Copy link
Collaborator

flyte-bot commented Mar 12, 2025 •
edited
Loading

Code Review Agent Run #8a2bba

Actionable Suggestions - 0
Review Details
  • Files reviewed - 1 · Commit Range: 0fbd346..0fbd346
    • flyteadmin/auth/authzserver/claims_verifier.go
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

Refer to the documentation for additional commands.

Configuration

This repository uses code_review_bito You can customize the agent settings here or contact your Bito workspace admin at [email protected].

Documentation & Help

AI Code Review powered by Bito Logo

eapolinario
eapolinario reviewed Mar 12, 2025
View reviewed changes
Copy link
Contributor

@eapolinario eapolinario left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good catch. Can you fix the unit test?

fix: really assign string scp claim
49dd9d9 
Signed-off-by: Vladimír Dudr <[email protected]>
@vlada-dudr
Copy link
Author

Like this?

@flyte-bot
Copy link
Collaborator

flyte-bot commented Mar 13, 2025 •
edited
Loading

Code Review Agent Run #a97aef

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 49dd9d9..49dd9d9
    • flyteadmin/auth/authzserver/claims_verifier.go
    • flyteadmin/auth/authzserver/claims_verifier_test.go
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

Refer to the documentation for additional commands.

Configuration

This repository uses code_review_bito You can customize the agent settings here or contact your Bito workspace admin at [email protected].

Documentation & Help

AI Code Review powered by Bito Logo

@codecov Codecov
Copy link

codecov bot commented Mar 14, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.49%. Comparing base (c02dfd3) to head (49dd9d9).
Report is 5 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6336      +/-   ##
==========================================
- Coverage   58.50%   58.49%   -0.01%     
==========================================
  Files         937      937              
  Lines       71091    71107      +16     
==========================================
+ Hits        41589    41594       +5     
- Misses      26350    26359       +9     
- Partials     3152     3154       +2
Flag Coverage Δ
unittests-datacatalog 59.03% <ø> (-0.04%) ⬇️
unittests-flyteadmin 56.28% <100.00%> (-0.02%) ⬇️
unittests-flytecopilot 30.99% <ø> (ø)
unittests-flytectl 64.70% <ø> (-0.06%) ⬇️
unittests-flyteidl 76.12% <ø> (ø)
unittests-flyteplugins 61.00% <ø> (ø)
unittests-flytepropeller 54.82% <ø> (+0.01%) ⬆️
unittests-flytestdlib 64.02% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

Sovietaced
Sovietaced reviewed Mar 14, 2025
View reviewed changes
flyteadmin/auth/authzserver/claims_verifier_test.go
@@ -78,6 +78,7 @@ func Test_verifyClaims(t *testing.T) {
t.Run("String scope", func(t *testing.T) {
identityCtx, err := verifyClaims(sets.NewString("https://myserver", "https://myserver2"),
map[string]interface{}{
"client_id": "my-client",
"aud": []string{"https://myserver"},
"scp": "all",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use something other than the default all scope so its clear that it was not injected.

@Sovietaced
Copy link
Contributor

Also make sure to run make lint-fix under /flyteadmin to resolve lint issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eapolinario eapolinario eapolinario left review comments

@Sovietaced Sovietaced Sovietaced left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vlada-dudr @flyte-bot @Sovietaced @eapolinario