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

add tests for scenarios to cover retrieving caller claims from multiple tokens #26878

Merged
merged 2 commits into from
Nov 13, 2023
Merged

add tests for scenarios to cover retrieving caller claims from multiple tokens #26878

merged 2 commits into from
Nov 13, 2023

Conversation

arunavemulapalli
Copy link
Contributor

@arunavemulapalli arunavemulapalli commented Nov 9, 2023 •
edited
Loading

@arunavemulapalli
Copy link
Contributor Author

#build
#spawn.fullfat.buckets=com.ibm.ws.security.oidc.client_fat.4,com.ibm.ws.security.oidc.client_fat.2,com.ibm.ws.security.social_fat.LibertyOP.2

@LibbyBot
Copy link

LibbyBot commented Nov 9, 2023

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_By9lYH6uEe6pmuZkFgD-jA

Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

LibbyBot commented Nov 9, 2023

The build arunavemulapalli-26878-20231108-2022
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_By9lYH6uEe6pmuZkFgD-jA
completed and has errors or failures.

For help analyzing your personal build, go to https://libh-proxy1.fyre.ibm.com/cognitive/buildAnalysis.html?uuid=_By9lYH6uEe6pmuZkFgD-jA

@posmith
Copy link

posmith commented Nov 10, 2023

L2 message review not required.

helyarp
helyarp reviewed Nov 10, 2023
View reviewed changes
Copy link

@helyarp helyarp left a comment

Choose a reason for hiding this comment

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

Suggested changes

dev/com.ibm.ws.security.openidconnect.client/resources/OSGI-INF/l10n/metatype.properties Outdated
@@ -298,3 +298,8 @@ pkceCodeChallengeMethod.S256=S256

tokenRequestOriginHeader=Token request origin header
tokenRequestOriginHeader.desc=Specifies the value to use in the Origin HTTP header that is included in the HTTP POST request to the token endpoint of the OpenID Connect provider. If not specified, an Origin HTTP header is not included in the request.

tokenOrderToFetchCallerClaims=Specify one or more tokens to fetch caller claims
tokenOrderToFetchCallerClaims.desc=Specifies the order of the token/s to fetch the caller name and group claim values. If the claim does not exist in the first token, then the OpenID Connect client will continue the search with other tokens in the list
Copy link

Choose a reason for hiding this comment

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

  • Change "the order of the token/s" to "token order"

Info: Current style guidelines ask us to use plural ("tokens" instead of "token/s"). With the suggested change you don't need to specify plural.

  • Change "will continue" to "continues"
  • Add a period to the end of the last sentence

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I addressed the review comments and added this commit (ee5dcdd) to #26719

thanks

dev/com.ibm.ws.security.openidconnect.client/resources/OSGI-INF/l10n/metatype.properties Outdated
tokenOrderToFetchCallerClaims=Specify one or more tokens to fetch caller claims
tokenOrderToFetchCallerClaims.desc=Specifies the order of the token/s to fetch the caller name and group claim values. If the claim does not exist in the first token, then the OpenID Connect client will continue the search with other tokens in the list
tokenOrderToFetchCallerClaims.IDToken=Use IDToken only to determine the caller claims
tokenOrderToFetchCallerClaims.ACCESSAndIDAndUITokens=Use AccessToken, IDToken and Userinfo tokens in this order, to determine the caller claims.
Copy link

Choose a reason for hiding this comment

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

Add a comma after "IDToken" and remove the comma after "order" >>

"Use AccessToken, IDToken, and Userinfo tokens in this order to determine the caller claims."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed this review comment in the runtime , config PR - #26719

Copy link

Choose a reason for hiding this comment

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

#26719 looks good. I'll add the ID reviewed label to this PR. Thanks for making the changes!

c00crane
c00crane previously approved these changes Nov 10, 2023
View reviewed changes
Copy link
Member

@c00crane c00crane left a comment

Choose a reason for hiding this comment

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

I think my comments can be addressed with a follow on work item

...t_fat.4/fat/src/com/ibm/ws/security/openidconnect/client/fat/IBM/OidcClientCallerClaims.java
/**
* testing tokenOrderTofetchCallerClaims="Access Token IDToken Userinfo"
* caller group claim exist in idtoken and userinfo
*
Copy link
Member

Choose a reason for hiding this comment

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

IdToken and acess_token

...t_fat.4/fat/src/com/ibm/ws/security/openidconnect/client/fat/IBM/OidcClientCallerClaims.java
}

/**
* testing tokenOrderTofetchCallerClaims="Access Token IDToken Userinfo"
Copy link
Member

Choose a reason for hiding this comment

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

IDToken

...t_fat.4/fat/src/com/ibm/ws/security/openidconnect/client/fat/IBM/OidcClientCallerClaims.java

/******************************* tests *******************************/
/**
* testing tokenOrderTofetchCallerClaims="Access Token IDToken Userinfo"
Copy link
Member

Choose a reason for hiding this comment

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

IDToken

...t_fat.4/fat/src/com/ibm/ws/security/openidconnect/client/fat/IBM/OidcClientCallerClaims.java
/******************************* tests *******************************/
/**
* testing tokenOrderTofetchCallerClaims="Access Token IDToken Userinfo"
* caller group claim exist in all tokens only
Copy link
Member

Choose a reason for hiding this comment

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

access_token and userinfo only

...t_fat.4/fat/src/com/ibm/ws/security/openidconnect/client/fat/IBM/OidcClientCallerClaims.java
}

/**
* testing tokenOrderTofetchCallerClaims="Access Token IDToken Userinfo"
Copy link
Member

Choose a reason for hiding this comment

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

IDToken

...t_fat.4/fat/src/com/ibm/ws/security/openidconnect/client/fat/IBM/OidcClientCallerClaims.java

}


Copy link
Member

Choose a reason for hiding this comment

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

  1. I would suggest adding some tests that contain the claim that is being tested with different/alt values. If the search order is TOKEN_ORDER_ACCESSTOKEN_IDTOKEN_USERINFO, have access_token contain the claim as expected and then have id_token and userinfo contain some other value (cases with different values and others with a superset of the value in the access_token)
  2. Have configs with different multiple token orders - such as TOKEN_ORDER_ACCESSTOKEN_IDTOKEN_USERINFO and TOKEN_ORDER_USERINFO_IDTOKEN_ACCESSTOKEN, ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Will add more test cases for #1.

For the #2, the valid configuration value are either TOKEN_ORDER_ACCESSTOKEN_IDTOKEN_USERINFO or TOKEN_ORDER_IDTOKEN, which is specified in the metatype.xml.

@arunavemulapalli
Copy link
Contributor Author

thanks @c00crane for the review
I will go ahead and open an issue

@arunavemulapalli
Copy link
Contributor Author

#libby

@arunavemulapalli
Copy link
Contributor Author

#build

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_HC7TwIAaEe6pmuZkFgD-jA

Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

The build arunavemulapalli-26878-20231110-1601
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_HC7TwIAaEe6pmuZkFgD-jA
completed successfully!

@arunavemulapalli arunavemulapalli force-pushed the caller-from-multiple-tokens-tests branch from 3af0967 to e74de8a Compare November 12, 2023 00:53
@arunavemulapalli
Copy link
Contributor Author

#build
#spawn.fullfat.buckets=com.ibm.ws.security.oidc.client_fat.2,com.ibm.ws.security.social_fat.LibertyOP.2

@LibbyBot
Copy link

Your personal build request is at https://libh-proxy1.fyre.ibm.com/cognitive/pipelineAnalysis.html?uuid=56aca1b4-0b79-473f-a9fa-987ae1414ce2

Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_2zIRAYDtEe6tffXfLjKGSA

Target locations of links might be accessible only to IBM employees.

@LibbyBot
Copy link

The build arunavemulapalli-26878-20231111-1704
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_2zIRAYDtEe6tffXfLjKGSA
completed and has errors or failures.

For help analyzing your personal build, go to https://libh-proxy1.fyre.ibm.com/cognitive/buildAnalysis.html?uuid=_2zIRAYDtEe6tffXfLjKGSA

@LibbyBot
Copy link

https://libh-proxy1.fyre.ibm.com/cognitive/pipelineAnalysis.html?uuid=56aca1b4-0b79-473f-a9fa-987ae1414ce2 has errors/failures.

@arunavemulapalli
Copy link
Contributor Author

#libby

@LibbyBot
Copy link

Code analysis and actions

DO NOT DELETE THIS COMMENT.

  • 1 product code files were changed.

  • Please describe in a separate comment how you tested your changes.

  • 4 FAT files were changed, added, or removed.

  • Check that the build did not break the affected FAT suite(s).

@arunavemulapalli arunavemulapalli merged commit 83b526f into OpenLiberty:integration Nov 13, 2023
@arunavemulapalli arunavemulapalli mentioned this pull request Nov 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@helyarp helyarp helyarp left review comments

@felix-liang felix-liang felix-liang left review comments

@c00crane c00crane c00crane approved these changes

@ayoho ayoho Awaiting requested review from ayoho

Assignees
No one assigned
Labels
CLA Signed ID reviewed L2 ReviewNotRequired
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fetch caller claims from multiple tokens - tests
6 participants
@arunavemulapalli @LibbyBot @posmith @felix-liang @helyarp @c00crane