-
Notifications
You must be signed in to change notification settings - Fork 396
Introduce POST_ISSUE_ACCESS_TOKEN_V2 event #2815
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: master
Are you sure you want to change the base?
Introduce POST_ISSUE_ACCESS_TOKEN_V2 event #2815
Conversation
8208c3a
to
419a098
Compare
419a098
to
bbe6d59
Compare
bbe6d59
to
f919fec
Compare
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.
Pull Request Overview
This PR introduces a new POST_ISSUE_TOKEN
event in the OAuth2 flow, encapsulating token issuance details in a structured TokenIssuanceDO
object, and fires this event after access token issuance.
- Added constants to
OIDCConstants
for new event attributes and token billing categories. - Introduced
TokenIssuanceDO
andOAuth2TokenUtil.postIssueToken(...)
to build and publish issuance events. - Updated
AccessTokenIssuer
to invoke the new event on token issue and adjusted related imports and WSDL definitions.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
components/.../OIDCConstants.java | Added constants and new TokenBillingCategory enum |
components/.../OAuth2TokenUtil.java | New postIssueToken method and helper event builder |
components/.../AccessTokenIssuer.java | Hooked in new event in triggerPostListeners |
components/.../TokenIssuanceDO.java | New data object and builder for token issuance data |
components/.../OAuthAdminService.wsdl | Renamed <tokenType> element to <tokenCategory> |
components/.../OAuth2TokenValidationService.wsdl | Renamed <tokenType> to <tokenCategory> in three places |
Comments suppressed due to low confidence (5)
components/org.wso2.carbon.identity.oauth.stub/src/main/resources/OAuthAdminService.wsdl:440
- Renaming the
<tokenType>
element to<tokenCategory>
is a breaking change in the WSDL API. Ensure all downstream consumers are updated or provide a migration path.
<xs:element minOccurs="0" name="tokenCategory" nillable="true" type="xs:string"/>
components/org.wso2.carbon.identity.oauth.stub/src/main/resources/OAuth2TokenValidationService.wsdl:105
- This change renames the
<tokenType>
element to<tokenCategory>
, which breaks the existing validation service contract. Confirm clients are updated accordingly.
<xs:element minOccurs="0" name="tokenCategory" nillable="true" type="xs:string"/>
components/org.wso2.carbon.identity.oauth.stub/src/main/resources/OAuth2TokenValidationService.wsdl:134
- Similarly,
<tokenType>
was replaced with<tokenCategory>
in this element; ensure compatibility with existing clients.
<xs:element minOccurs="0" name="tokenCategory" nillable="true" type="xs:string"/>
components/org.wso2.carbon.identity.oauth.stub/src/main/resources/OAuth2TokenValidationService.wsdl:155
- The
<tokenType>
element has been renamed to<tokenCategory>
here as well, which impacts the introspection response schema; verify client updates.
<xs:element minOccurs="0" name="tokenCategory" nillable="true" type="xs:string"/>
components/org.wso2.carbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2TokenUtil.java:116
- The code references
OIDCConstants.Event.TOKEN_ID
but noTOKEN_ID
constant is declared inOIDCConstants.Event
. Add this constant or correct the reference.
tokenIssuanceData.put(OIDCConstants.Event.TOKEN_ID, tokenIssuanceDO.getTokenId());
...arbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/util/OAuth2TokenUtil.java
Outdated
Show resolved
Hide resolved
...on.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/AccessTokenIssuer.java
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2815 +/- ##
============================================
- Coverage 57.39% 55.80% -1.60%
+ Complexity 9757 9098 -659
============================================
Files 664 664
Lines 52656 53757 +1101
Branches 11618 12323 +705
============================================
- Hits 30223 30000 -223
- Misses 18220 19436 +1216
- Partials 4213 4321 +108
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
96d52d5
to
daaf5a8
Compare
daaf5a8
to
77e7522
Compare
568ad0c
to
04badc2
Compare
…dOrganizationId from TokenIssuanceDO
...rbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/model/TokenIssuanceDO.java
Outdated
Show resolved
Hide resolved
...on.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/token/AccessTokenIssuer.java
Outdated
Show resolved
Hide resolved
...dentity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/token/AccessTokenIssuerTest.java
Show resolved
Hide resolved
...dentity.oauth/src/test/java/org/wso2/carbon/identity/oauth2/token/AccessTokenIssuerTest.java
Outdated
Show resolved
Hide resolved
...rbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/model/TokenIssuanceDO.java
Outdated
Show resolved
Hide resolved
...rbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/model/TokenIssuanceDO.java
Outdated
Show resolved
Hide resolved
...rbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/model/TokenIssuanceDO.java
Outdated
Show resolved
Hide resolved
...rbon.identity.oauth/src/main/java/org/wso2/carbon/identity/oauth2/model/TokenIssuanceDO.java
Outdated
Show resolved
Hide resolved
cf245f1
to
c435fb5
Compare
PR builder started |
PR builder completed |
PR builder started |
PR builder completed |
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.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/16201997704
4057bbb
to
de171a8
Compare
de171a8
to
227e256
Compare
tenantDomain = tokReqMsgCtx.getOauth2AccessTokenReqDTO().getTenantDomain(); | ||
} | ||
if (tokReqMsgCtx.getOauth2AccessTokenReqDTO().getClientId() != null) { | ||
clientId = tokReqMsgCtx.getOauth2AccessTokenReqDTO().getClientId(); |
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.
Do we need to handle null checks for userType, tenantDomain, clientId, tokenId and grantType. Because these are mandatory items in a token issuance flow.
Also we can simplify these resolving logic for b2b cases
if (StringUtils.isNotEmpty(tenantDomain)) {
organizationId = OAuthComponentServiceHolder.getInstance()
.getOrganizationManager()
.resolveOrganizationId(tenantDomain);
}
String accessingOrganizationId = user != null ? user.getAccessingOrganization() : StringUtils.EMPTY;
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.
The null checks are removed with 4e0802c
However, the checks for accessingOrganizationId need to be kept since user.getAccessingOrganization can also return a null. If we go with the above approach it'll result in empty string when user is null , and null when user.getAccessingOrganization returns null.
} | ||
return; | ||
} | ||
if (existingTokenUsed(tokReqMsgCtx)) { |
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.
Why do we need to skip in here? This is a token issuance event, and it should be aware of the token issuance flow. The consumer can check this and handle the scenarios as per their usecses
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.
Agree. Changed with 4e0802c
* Unit test cases for {@link AccessTokenIssuer} | ||
*/ | ||
|
||
public class DeprecatedAccessTokenIssuerTest { |
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.
what this test calss do? Why we introduced new test class instead of AccessTokenIssuerTest?
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.
The AccessaTokenIssuerTest was already existing. This was commented out by @madurangasiriwardena since it was not functional and needed an extra effort. Therefore it was renamed and kept as deprecated for future reference and a new test class was introduced to cover the changes being introduced by the PR.
4e0802c
to
57fbe1e
Compare
Purpose
PR Description
This pull request introduces a new feature for publishing token issuance events in the OAuth2 component, along with associated refactoring and test enhancements. The changes include the addition of a post-issue token event mechanism, updates to constants, and the introduction of unit tests to ensure the new functionality works as expected.
New Feature: Token Issuance Event Publishing
postIssueToken
inOAuth2TokenUtil
to publish token issuance details as events. This includes creating and handling an event with properties like token ID, client ID, grant type, and issued time.triggerPostIssueTokenEvent
inAccessTokenIssuer
to handle the logic for triggering the post-issue token event. This method validates input and skips the event if certain conditions are not met.POST_ISSUE_TOKEN
inOIDCConstants.Event
to represent the event name for token issuance.Refactoring and Code Enhancements
OIDCConstants.Event
for additional token properties such asGRANT_TYPE
,CLIENT_ID
,TENANT_DOMAIN
, andISSUED_TIME
.existingTokenUsed
inAccessTokenIssuer
to determine whether an existing token was reused.Unit Test Additions
OAuth2TokenUtilTest
to validate the behavior of thepostIssueToken
method. This includes tests for successful event handling and scenarios where exceptions are thrown.testng.xml
to include theAccessTokenIssuerTest
class for execution. [1] [2]Minor Changes
pom.xml
to exclude theDeprecatedAccessTokenIssuerTest
instead ofAccessTokenIssuerTest
during Checkstyle checks.AccessTokenIssuer
. [1] [2] [3]