[draft] Update OAuthOpaqueAuthenticatorImpl.java #12989
Conversation
chamilaadhi
requested review from
tharindu1st,
AnuGayan,
Arshardh,
dushaniw and
tgtshanika
as code owners
WalkthroughThis pull request changes the initialization of the Changes
Sequence Diagram(s)sequenceDiagram
participant API as API Request
participant Auth as OAuthOpaqueAuthenticatorImpl
API->>Auth: Invoke getOrganizationInfo()
Note right of Auth: Initialize isSuperTenant = false
Auth->>Auth: Evaluate tenant domain logic
Auth-->>API: Return organization information based on tenant evaluation
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/apimgt/org.wso2.carbon.apimgt.rest.api.util/src/main/java/org/wso2/carbon/apimgt/rest/api/util/impl/OAuthOpaqueAuthenticatorImpl.java (2)
264-264: Initial value ofisSuperTenantis redundant.The initial value of
isSuperTenantis immediately overwritten at line 281 based on the tenant domain check. The initialization value doesn't affect the logic since it's not used before being overwritten.Consider removing the initialization and declare the variable where it's first used:
- Boolean isSuperTenant = false; int tenantId = MultitenantConstants.SUPER_TENANT_ID; APIManagerConfiguration config = ServiceReferenceHolder.getInstance(). getAPIManagerConfigurationService().getAPIManagerConfiguration(); String orgNameClaim = config.getOrgAccessControl().getOrgNameLocalClaim(); String orgIdClaim = config.getOrgAccessControl().getOrgIdLocalClaim(); + boolean isSuperTenant = tenantDomain.equals(MultitenantConstants.SUPER_TENANT_DOMAIN_NAME);Additionally:
- Use primitive
booleaninstead ofBooleansince null values aren't needed here- Move the declaration closer to where it's first used to improve code readability
256-333: Consider improving error handling and method organization.The
getOrganizationInfomethod has multiple responsibilities and could benefit from the following improvements:
- The error message in the catch block could be more specific about which operation failed
- The method could be split into smaller, focused methods for better maintainability
Consider refactoring into smaller methods:
+ private String resolveUsername(String username, String tenantDomain) { + if (MultitenantConstants.SUPER_TENANT_DOMAIN_NAME.equals(tenantDomain)) { + long count = username.chars().filter(ch -> ch == '@').count(); + boolean isEmailUsernameEnabled = Boolean.parseBoolean( + CarbonUtils.getServerConfiguration().getFirstProperty("EnableEmailUserName") + ); + if (isEmailUsernameEnabled || (username.endsWith(SUPER_TENANT_SUFFIX) && count <= 1)) { + return MultitenantUtils.getTenantAwareUsername(username); + } + } + return username; + } + private String[] getOrganizationClaims(APIManagerConfiguration config) { + String orgNameClaim = config.getOrgAccessControl().getOrgNameLocalClaim(); + String orgIdClaim = config.getOrgAccessControl().getOrgIdLocalClaim(); + + if (StringUtils.isBlank(orgNameClaim)) { + orgNameClaim = "http://wso2.org/claims/organization"; + } + if (StringUtils.isBlank(orgIdClaim)) { + orgIdClaim = "http://wso2.org/claims/organizationId"; + } + return new String[]{orgNameClaim, orgIdClaim}; + }Also, improve the error message:
- String error = "Error while checking user existence for " + username; + String error = "Error while retrieving organization claims for user: " + username;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/apimgt/org.wso2.carbon.apimgt.rest.api.util/src/main/java/org/wso2/carbon/apimgt/rest/api/util/impl/OAuthOpaqueAuthenticatorImpl.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-product (4, group4)
- GitHub Check: build-product (3, group3)
- GitHub Check: build-carbon
- GitHub Check: build-product (2, group2)
- GitHub Check: run-benchmark-test
- GitHub Check: build-product (1, group1)
Summary by CodeRabbit