-
Notifications
You must be signed in to change notification settings - Fork 31
Fix token discovery logic #2846
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
base: main
Are you sure you want to change the base?
Conversation
- Fixed bug in tests - Added token location tracking - Refactored token accebility policy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks mostly good, but I would like to see some explicit unit tests for the helper functions. I'd also like to see some Wlcg token testing (because you switched out wlcg for scitoken, and I think we should have both)
|
|
||
| // matchesResource checks if the target resource matches the scope resource. | ||
| // For shared URLs, exact matches are preferred, but prefix matching is also acceptable. | ||
| func matchesResource(targetResource, scopeResource string, operation config.TokenOperation) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have unit tests for these helper functions. Helps identify more specifically when/if we introduce bugs that cause token verification issues.
| } | ||
|
|
||
| // Verify if a scitoken‐profile JWT is acceptable for a given namespace | ||
| func TestTokenIsAcceptableForSciTokens(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this because you altered the test. This really feels like it should be in acquire_token_test.go file rather than main.
| strings.HasPrefix(targetNorm, scopeNorm) | ||
| } | ||
|
|
||
| func isValidWLCGScope(authz string, operation config.TokenOperation) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment
| } | ||
| } | ||
|
|
||
| func isValidSciScope(authz string, operation config.TokenOperation) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment
Fixes #2812 - Token discovery was incorrectly rejecting valid SciTokens and logging empty token location. The cause of this was that token validation only checked for WLCG tokens, causing valid SciTokens to be incorrectly flagged as unacceptable.