test: Improved test coverage in #31
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| package com.kinde.admin; | ||
|
|
||
| import com.kinde.KindeClient; | ||
| import com.kinde.KindeClientSession; | ||
| import com.kinde.client.OidcMetaData; | ||
| import com.kinde.config.KindeConfig; | ||
| import com.kinde.token.AccessToken; | ||
| import com.kinde.token.IDToken; | ||
| import com.kinde.token.KindeTokens; | ||
| import com.kinde.token.RefreshToken; | ||
| import com.kinde.token.jwt.JwtGenerator; | ||
| import com.nimbusds.oauth2.sdk.id.Issuer; | ||
| import com.nimbusds.openid.connect.sdk.SubjectType; | ||
| import com.nimbusds.openid.connect.sdk.op.OIDCProviderMetadata; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.mockito.Mockito; | ||
| import org.openapitools.client.ApiClient; | ||
|
|
||
| import java.net.URI; | ||
| import java.util.Arrays; | ||
|
|
||
| import static org.mockito.Mockito.when; | ||
|
|
||
| public class KindeAdminSessionImplTest { | ||
|
|
||
| @Test | ||
| public void testInitClient() throws Exception { | ||
| KindeAdminSessionImpl kindeAdminSession1 = new KindeAdminSessionImpl(null); | ||
| ApiClient apiClient = kindeAdminSession1.initClient(); | ||
|
|
||
| KindeClient kindeClient = Mockito.mock(KindeClient.class); | ||
| KindeClientSession kindeClientSession = Mockito.mock(KindeClientSession.class); | ||
| when(kindeClient.clientSession()).thenReturn(kindeClientSession); | ||
| KindeConfig kindeConfig = Mockito.mock(KindeConfig.class); | ||
| when(kindeClient.kindeConfig()).thenReturn(kindeConfig); | ||
| OidcMetaData oidcMetaData = Mockito.mock(OidcMetaData.class); | ||
| when(kindeClient.oidcMetaData()).thenReturn(oidcMetaData); | ||
| OIDCProviderMetadata oidcProviderMetadata = new OIDCProviderMetadata(new Issuer("https://kinde.com"), | ||
| Arrays.asList(SubjectType.PUBLIC), new URI("https://kinde.com")); | ||
| oidcProviderMetadata.setTokenEndpointURI(new URI("https://kinde.com")); | ||
| when(oidcMetaData.getOpMetadata()).thenReturn(oidcProviderMetadata); | ||
| IDToken idToken = IDToken.init(JwtGenerator.generateIDToken(),true); | ||
| AccessToken accessToken = AccessToken.init(JwtGenerator.generateAccessToken(),true); | ||
| RefreshToken refreshToken = RefreshToken.init(JwtGenerator.refreshToken(),true); | ||
| KindeTokens kindeTokens = new KindeTokens(Arrays.asList(),idToken,accessToken,refreshToken); | ||
| when(kindeClientSession.retrieveTokens()).thenReturn(kindeTokens); | ||
| KindeAdminSessionImpl kindeAdminSession2 = new KindeAdminSessionImpl(kindeClient); | ||
| kindeAdminSession2.initClient(); | ||
| } | ||
|
Comment on lines
+47
to
+49
There was a problem hiding this comment. Enhance test assertions and scenario verification. The test creates two
Example improvements: ApiClient apiClient1 = kindeAdminSession1.initClient();
assertNotNull(apiClient1, "ApiClient should not be null for null KindeClient");
KindeAdminSessionImpl kindeAdminSession2 = new KindeAdminSessionImpl(kindeClient);
ApiClient apiClient2 = kindeAdminSession2.initClient();
assertNotNull(apiClient2, "ApiClient should not be null for mocked KindeClient");
// Verify that the mocked methods were called
Mockito.verify(kindeClient).clientSession();
Mockito.verify(kindeClient).kindeConfig();
Mockito.verify(kindeClient).oidcMetaData();
Mockito.verify(kindeClientSession).retrieveTokens();
// Compare the two ApiClient instances if they should be different
assertNotEquals(apiClient1, apiClient2, "ApiClient instances should be different for null and mocked KindeClient"); |
||
| } | ||
|
Comment on lines
+24
to
+50
There was a problem hiding this comment. 🛠️ Refactor suggestion Good start, but expand test coverage and add assertions. The test class provides a good foundation for testing the
Example additional test case: @Test
public void testInitClientWithException() {
KindeClient mockClient = Mockito.mock(KindeClient.class);
when(mockClient.clientSession()).thenThrow(new RuntimeException("Session error"));
KindeAdminSessionImpl kindeAdminSession = new KindeAdminSessionImpl(mockClient);
assertThrows(RuntimeException.class, () -> kindeAdminSession.initClient(),
"initClient should throw RuntimeException when client session fails");
} |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,117 @@ | ||
| package com.kinde.token.jwt; | ||
|
|
||
| import com.nimbusds.jose.JWSAlgorithm; | ||
| import com.nimbusds.jose.JWSHeader; | ||
| import com.nimbusds.jose.JWSSigner; | ||
| import com.nimbusds.jose.crypto.RSASSASigner; | ||
| import com.nimbusds.jose.jwk.RSAKey; | ||
| import com.nimbusds.jose.jwk.gen.RSAKeyGenerator; | ||
| import com.nimbusds.jwt.JWTClaimsSet; | ||
| import com.nimbusds.jwt.SignedJWT; | ||
| import lombok.SneakyThrows; | ||
|
|
||
| import java.util.*; | ||
|
|
||
| public class JwtGenerator { | ||
| @SneakyThrows | ||
| public static String generateAccessToken() { | ||
|
|
||
| // generate a new signed token | ||
| RSAKey rsaJWK = new RSAKeyGenerator(2048) | ||
| .keyID("123") | ||
| .generate(); | ||
|
Comment on lines
+20
to
+22
There was a problem hiding this comment. 🛠️ Refactor suggestion Reuse RSA key instead of generating a new one in each method Currently, each method generates a new RSA key pair every time it's called. This can be inefficient and unnecessary for testing purposes. Consider generating the RSA key once and reusing it across all methods to improve efficiency. For example, you can declare the RSA key as a static variable: private static final RSAKey RSA_JWK = new RSAKeyGenerator(2048)
.keyID("123")
.generate();
private static final JWSSigner SIGNER = new RSASSASigner(RSA_JWK);Then, use Also applies to: 51-53, 90-92 |
||
| RSAKey rsaPublicJWK = rsaJWK.toPublicJWK(); | ||
|
There was a problem hiding this comment. Remove unused variable The variable Apply this diff to remove the unused variable: - RSAKey rsaPublicJWK = rsaJWK.toPublicJWK();Also applies to: 54-54, 93-93 |
||
|
|
||
| JWSSigner signer = new RSASSASigner(rsaJWK); | ||
|
|
||
| Date now = new Date(); | ||
|
|
||
| JWTClaimsSet jwtClaims = new JWTClaimsSet.Builder() | ||
| .issuer("https://openid.net") | ||
| .subject("test") | ||
| .audience(Arrays.asList("https://kinde.com")) | ||
| .expirationTime(new Date(now.getTime() + 1000*60*10)) // expires in 10 minutes | ||
| .notBeforeTime(now) | ||
| .issueTime(now) | ||
| .claim("permissions",Arrays.asList("test1","test1")) | ||
| .jwtID(UUID.randomUUID().toString()) | ||
| .build(); | ||
|
|
||
| SignedJWT signedJWT = new SignedJWT( | ||
| new JWSHeader.Builder(JWSAlgorithm.RS256).keyID(rsaJWK.getKeyID()).build(), | ||
| jwtClaims); | ||
|
|
||
| signedJWT.sign(signer); | ||
| return signedJWT.serialize(); | ||
| } | ||
|
Comment on lines
+17
to
+46
There was a problem hiding this comment. 🛠️ Refactor suggestion Refactor common code to reduce duplication The methods For example, you can create a private method to generate the RSA key and signer: @SneakyThrows
private static RSASSASigner getSigner() {
RSAKey rsaJWK = new RSAKeyGenerator(2048)
.keyID("123")
.generate();
return new RSASSASigner(rsaJWK);
}And another private method to build the common JWT claims: private static JWTClaimsSet.Builder getCommonClaims(Date now) {
return new JWTClaimsSet.Builder()
.issuer("https://openid.net")
.subject("test")
.audience(Collections.singletonList("https://kinde.com"))
.expirationTime(new Date(now.getTime() + 1000 * 60 * 10)) // expires in 10 minutes
.notBeforeTime(now)
.issueTime(now)
.claim("permissions", Arrays.asList("test1", "test1"))
.jwtID(UUID.randomUUID().toString());
}This refactoring will make your test code cleaner and more manageable. Also applies to: 49-85, 88-116 |
||
|
|
||
| @SneakyThrows | ||
| public static String generateIDToken() { | ||
| // generate a new signed token | ||
| RSAKey rsaJWK = new RSAKeyGenerator(2048) | ||
| .keyID("123") | ||
| .generate(); | ||
| RSAKey rsaPublicJWK = rsaJWK.toPublicJWK(); | ||
|
|
||
| JWSSigner signer = new RSASSASigner(rsaJWK); | ||
|
|
||
| Date now = new Date(); | ||
|
|
||
| Map<String,Object> featureFlags = new HashMap<>(); | ||
|
|
||
| featureFlags.put("test_str","test_str"); | ||
| featureFlags.put("test_integer",Integer.valueOf(1)); | ||
| featureFlags.put("test_boolean",Boolean.valueOf(false)); | ||
|
|
||
| JWTClaimsSet jwtClaims = new JWTClaimsSet.Builder() | ||
| .issuer("https://openid.net") | ||
| .subject("test") | ||
| .audience(Arrays.asList("https://kinde.com")) | ||
| .expirationTime(new Date(now.getTime() + 1000*60*10)) // expires in 10 minutes | ||
| .notBeforeTime(now) | ||
| .issueTime(now) | ||
| .claim("permissions",Arrays.asList("test1","test1")) | ||
| .claim("org_codes",Arrays.asList("test1","test1")) | ||
| .claim("feature_flags",featureFlags) | ||
| .jwtID(UUID.randomUUID().toString()) | ||
| .build(); | ||
|
|
||
| SignedJWT signedJWT = new SignedJWT( | ||
| new JWSHeader.Builder(JWSAlgorithm.RS256).keyID(rsaJWK.getKeyID()).build(), | ||
| jwtClaims); | ||
|
|
||
| signedJWT.sign(signer); | ||
| return signedJWT.serialize(); | ||
| } | ||
|
|
||
| @SneakyThrows | ||
| public static String refreshToken() { | ||
| // generate a new signed token | ||
| RSAKey rsaJWK = new RSAKeyGenerator(2048) | ||
| .keyID("123") | ||
| .generate(); | ||
| RSAKey rsaPublicJWK = rsaJWK.toPublicJWK(); | ||
|
|
||
| JWSSigner signer = new RSASSASigner(rsaJWK); | ||
|
|
||
| Date now = new Date(); | ||
|
|
||
| JWTClaimsSet jwtClaims = new JWTClaimsSet.Builder() | ||
| .issuer("https://openid.net") | ||
| .subject("test") | ||
| .audience(Arrays.asList("https://kinde.com")) | ||
| .expirationTime(new Date(now.getTime() + 1000*60*10)) // expires in 10 minutes | ||
| .notBeforeTime(now) | ||
| .issueTime(now) | ||
| .claim("permissions",Arrays.asList("test1","test1")) | ||
| .jwtID(UUID.randomUUID().toString()) | ||
| .build(); | ||
|
|
||
| SignedJWT signedJWT = new SignedJWT( | ||
| new JWSHeader.Builder(JWSAlgorithm.RS256).keyID(rsaJWK.getKeyID()).build(), | ||
| jwtClaims); | ||
|
|
||
| signedJWT.sign(signer); | ||
| return signedJWT.serialize(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| package com.kinde.spring.config; | ||
|
|
||
| import org.junit.Assert; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.mockito.Mockito; | ||
| import org.springframework.boot.autoconfigure.security.oauth2.client.OAuth2ClientProperties; | ||
|
|
||
| public class KindeOAuth2PropertiesTest { | ||
|
|
||
| @Test | ||
| public void testKindeOAuth2Properties() { | ||
| OAuth2ClientProperties clientProperties = Mockito.mock(OAuth2ClientProperties.class); | ||
| KindeOAuth2Properties kindeOAuth2Properties = new KindeOAuth2Properties(clientProperties); | ||
|
|
||
| Assert.assertNull(kindeOAuth2Properties.getRedirectUri()); | ||
| Assert.assertNull(kindeOAuth2Properties.getClientId()); | ||
| Assert.assertNull(kindeOAuth2Properties.getClientSecret()); | ||
| Assert.assertNull(kindeOAuth2Properties.getAuthorizationGrantType()); | ||
| Assert.assertNull(kindeOAuth2Properties.getDomain()); | ||
| Assert.assertEquals("api://default",kindeOAuth2Properties.getAudience()); | ||
| Assert.assertNull(kindeOAuth2Properties.getPostLogoutRedirectUri()); | ||
| Assert.assertNull(kindeOAuth2Properties.getProxy()); | ||
| Assert.assertNull(kindeOAuth2Properties.getScopes()); | ||
|
|
||
| Assert.assertNotNull(kindeOAuth2Properties.getPermissionsClaim()); | ||
|
|
||
| kindeOAuth2Properties.setRedirectUri("https://kinde.com"); | ||
| Assert.assertEquals("https://kinde.com",kindeOAuth2Properties.getRedirectUri()); | ||
|
|
||
| kindeOAuth2Properties.setClientId("client-id"); | ||
| Assert.assertEquals("client-id",kindeOAuth2Properties.getClientId()); | ||
|
|
||
| kindeOAuth2Properties.setClientSecret("client-secret"); | ||
| Assert.assertEquals("client-secret",kindeOAuth2Properties.getClientSecret()); | ||
|
|
||
| kindeOAuth2Properties.setDomain("https://kinde.com"); | ||
| Assert.assertEquals("https://kinde.com",kindeOAuth2Properties.getDomain()); | ||
|
|
||
| kindeOAuth2Properties.setAudience("https://kinde.com/api"); | ||
| Assert.assertEquals("https://kinde.com/api",kindeOAuth2Properties.getAudience()); | ||
| } | ||
|
|
||
|
|
||
| } | ||
|
Comment on lines
+1
to
+44
There was a problem hiding this comment. 🛠️ Refactor suggestion Good start on testing, but room for improvement in coverage and structure. The test provides a solid foundation for verifying the basic functionality of
Here's an example of how you might restructure the tests: public class KindeOAuth2PropertiesTest {
private OAuth2ClientProperties clientProperties;
private KindeOAuth2Properties kindeOAuth2Properties;
@BeforeEach
void setUp() {
clientProperties = Mockito.mock(OAuth2ClientProperties.class);
kindeOAuth2Properties = new KindeOAuth2Properties(clientProperties);
}
@Test
void testDefaultValues() {
// Test all properties for their default values
}
@Test
void testSetterMethods() {
// Test all setter methods
}
@ParameterizedTest
@ValueSource(strings = {"https://kinde.com", "", "invalid-uri"})
void testSetRedirectUri(String uri) {
kindeOAuth2Properties.setRedirectUri(uri);
assertEquals(uri, kindeOAuth2Properties.getRedirectUri());
}
// Additional test methods for other properties and edge cases
}This structure would provide more comprehensive and organized testing for the |
||
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.
💡 Codebase verification
Hardcoded URLs Identified in Multiple Locations
The review has uncovered that hardcoded URLs
"https://kinde.com"and"http://kinde.com/oauth2/token"are present in several Java files, including both main and test directories. This widespread use of hardcoded authentication endpoints poses the following concerns:Recommendations:
Centralize Configuration: Refactor the code to use configuration files or environment variables for authentication URLs.
Implement Validation and Error Handling: Ensure that configurations are loaded correctly and handle scenarios where configurations might be missing or incorrect.
Review All Instances: Conduct a comprehensive review of all occurrences of these hardcoded URLs and apply the refactoring consistently across the codebase.
🔗 Analysis chain
Avoid using hardcoded URLs and improve error handling
While the addition of a default authentication mechanism when
kindeClientis null is a good idea, there are several concerns with the current implementation:Hardcoded URLs: The URLs "https://kinde.com" and "http://kinde.com/oauth2/token" are hardcoded. This reduces flexibility and may cause issues if these URLs change or differ across environments.
Lack of error handling: There's no error handling or logging for this new code path, which could make debugging difficult if issues arise.
Security implications: Using hardcoded authentication endpoints could pose a security risk if these URLs change or if different environments require different URLs.
Maintainability: Hardcoded values reduce maintainability and make it harder to update the code for different environments or configurations.
To address these concerns, consider the following improvements:
kindeClientis null.To ensure these URLs are not used elsewhere in the codebase, you can run the following command:
This will help identify if these hardcoded URLs are used in other parts of the codebase, which might also need to be updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 134
Script:
Length of output: 3628