Add tenant domain to jwt payload #2384
Merged
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
This was referenced Feb 28, 2024
| @@ -641,6 +645,11 @@ public String getDeviceAuthzEPUrl() { | |||
|
|
|||
| return deviceAuthzEPUrl; | |||
| } | |||
|
|
|||
| public boolean isAddTenantDomainToTokenEnabled() { | |||
| return addTenantDomainToTokenEnabled; | |||
There was a problem hiding this comment.
Add a new line after the method definition. Check other places as well.
There was a problem hiding this comment.
fixed with 5ec7232
...tity.oauth/src/main/java/org/wso2/carbon/identity/oauth/config/OAuthServerConfiguration.java
Show resolved
Hide resolved
| @@ -3348,6 +3369,8 @@ private class ConfigElements { | |||
| private static final String OPENID_CONNECT_ADD_TENANT_DOMAIN_TO_ID_TOKEN = "AddTenantDomainToIdToken"; | |||
| // Property to decide whether to add userstore domain to id_token. | |||
| private static final String OPENID_CONNECT_ADD_USERSTORE_DOMAIN_TO_ID_TOKEN = "AddUserstoreDomainToIdToken"; | |||
| // Enable/Disable adding domain information to the token | |||
There was a problem hiding this comment.
Suggested change
| // Enable/Disable adding domain information to the token | |
| // Enable/Disable adding domain information to the token. |
Comment on lines
105
to
106
| private static final String APP_DOMAIN = "app_domain"; | ||
| private static final String USER_DOMAIN = "user_domain"; |
There was a problem hiding this comment.
shall we have the word tenant domain in the claim. specially when using user_domain this could be confused with userstore domian.
some suggetions,
app_domain -> app_td
user_domain -> user_td
There was a problem hiding this comment.
changed as suggested
| @@ -3348,6 +3370,8 @@ private class ConfigElements { | |||
| private static final String OPENID_CONNECT_ADD_TENANT_DOMAIN_TO_ID_TOKEN = "AddTenantDomainToIdToken"; | |||
| // Property to decide whether to add userstore domain to id_token. | |||
| private static final String OPENID_CONNECT_ADD_USERSTORE_DOMAIN_TO_ID_TOKEN = "AddUserstoreDomainToIdToken"; | |||
| // Enable/Disable adding domain information to the token. | |||
| private static final String ADD_DOMAIN_TO_ACCESS_TOKEN = "AddTenantDomainToAccessToken"; | |||
There was a problem hiding this comment.
Suggested change
| private static final String ADD_DOMAIN_TO_ACCESS_TOKEN = "AddTenantDomainToAccessToken"; | |
| private static final String ADD_TENANT_DOMAIN_TO_ACCESS_TOKEN = "AddTenantDomainToAccessToken"; |
Comment on lines
105
to
106
| private static final String APP_DOMAIN = "app_td"; | ||
| private static final String USER_DOMAIN = "user_td"; |
There was a problem hiding this comment.
Suggested change
| private static final String APP_DOMAIN = "app_td"; | |
| private static final String USER_DOMAIN = "user_td"; | |
| private static final String APP_TENANT_DOMAIN = "app_td"; | |
| private static final String USER_TENANT_DOMAIN = "user_td"; |
| @@ -479,6 +481,11 @@ protected JWTClaimsSet createJWTClaimSet(OAuthAuthzReqMessageContext authAuthzRe | |||
| jwtClaimsSetBuilder.notBeforeTime(new Date(curTimeInMillis)); | |||
| jwtClaimsSetBuilder.claim(CLIENT_ID, consumerKey); | |||
|
|
|||
| if (OAuthServerConfiguration.getInstance().isAddTenantDomainToAccessTokenEnabled()) { | |||
| jwtClaimsSetBuilder.claim(APP_DOMAIN, oAuthAppDO.getAppOwner().getTenantDomain()); | |||
There was a problem hiding this comment.
oAuthAppDO.getAppOwner().getTenantDomain() this seems to be resolving the tenant domain of the app through the appowners tenant domain. lets directly resolve the apps tenant domain.
There was a problem hiding this comment.
janakamarasena
approved these changes
Mar 8, 2024
HiranyaKavishani
approved these changes
Mar 12, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Proposed changes in this pull request
When SP is created with saas app enabled, it is not possible to identify the tenant domain of the user. Also, it is not possible to identify the SP's tenant domain as well. For that had a chat with @janakamarasena and decided to introduce two new custom fields to the JWT token (
user_domainandapp_domain). It was decided to introduce a flag to enable this to the jwt.This will be enabled only if the following config is added
PR related to the config wso2/carbon-identity-framework#5547