feat: Create OAuth client from config & Bump version to 0.2.1 #29
Conversation
Change-Id: Ia2a29307ca80f36faa079e1f1d2cba8832fe600f
|
Warning Rate limit exceeded@hanzeINGH has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughThis pull request introduces version updates and enhancements to the OAuth configuration and client management across the Coze API project. The changes primarily focus on adding a new Changes
Possibly related PRs
Poem
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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #29 +/- ##
============================================
- Coverage 78.14% 76.57% -1.58%
Complexity 203 203
============================================
Files 41 41
Lines 1121 1144 +23
Branches 83 83
============================================
Hits 876 876
- Misses 187 210 +23
Partials 58 58
Flags with carried forward coverage won't be shown. Click here to find out more.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
api/pom.xml (1)
Line range hint
143-147: Update dependencies with known vulnerabilities.Several dependencies have known security vulnerabilities:
- junit:junit:4.12
- ch.qos.logback:logback-classic:1.2.3
Apply these updates:
<dependency> <groupId>junit</groupId> <artifactId>junit</artifactId> - <version>4.12</version> + <version>4.13.2</version> <scope>test</scope> </dependency> <dependency> <groupId>ch.qos.logback</groupId> <artifactId>logback-classic</artifactId> - <version>1.2.3</version> + <version>1.2.13</version> <scope>test</scope> </dependency>Let's verify if there are any other dependencies with known vulnerabilities:
#!/bin/bash # Use GitHub API to check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: MAVEN, package: "junit") { nodes { advisory { summary severity } vulnerableVersionRange } } } ' gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: MAVEN, package: "logback-classic") { nodes { advisory { summary severity } vulnerableVersionRange } } } 'Also applies to: 156-161
🧹 Nitpick comments (1)
api/src/main/java/com/coze/openapi/service/auth/PKCEOAuthClient.java (1)
11-11: Consider using specific imports instead of wildcard import.Replace the wildcard import with specific imports to improve code maintainability and prevent potential naming conflicts.
-import com.coze.openapi.client.auth.*; +import com.coze.openapi.client.auth.OAuthConfig; +import com.coze.openapi.client.auth.OAuthToken; +import com.coze.openapi.client.auth.GetAccessTokenReq; +import com.coze.openapi.client.auth.GrantType;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/pom.xml(1 hunks)api/src/main/java/com/coze/openapi/client/auth/OAuthConfig.java(1 hunks)api/src/main/java/com/coze/openapi/client/auth/OAuthToken.java(1 hunks)api/src/main/java/com/coze/openapi/service/auth/DeviceOAuthClient.java(1 hunks)api/src/main/java/com/coze/openapi/service/auth/JWTOAuthClient.java(2 hunks)api/src/main/java/com/coze/openapi/service/auth/PKCEOAuthClient.java(2 hunks)api/src/main/java/com/coze/openapi/service/auth/WebOAuthClient.java(2 hunks)api/src/main/java/com/coze/openapi/service/utils/UserAgentInterceptor.java(1 hunks)example/pom.xml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/src/main/java/com/coze/openapi/service/utils/UserAgentInterceptor.java
🧰 Additional context used
🪛 GitHub Check: codecov/patch
api/src/main/java/com/coze/openapi/service/auth/WebOAuthClient.java
[warning] 16-21: api/src/main/java/com/coze/openapi/service/auth/WebOAuthClient.java#L16-L21
Added lines #L16 - L21 were not covered by tests
api/src/main/java/com/coze/openapi/service/auth/JWTOAuthClient.java
[warning] 36-42: api/src/main/java/com/coze/openapi/service/auth/JWTOAuthClient.java#L36-L42
Added lines #L36 - L42 were not covered by tests
api/src/main/java/com/coze/openapi/service/auth/DeviceOAuthClient.java
[warning] 19-23: api/src/main/java/com/coze/openapi/service/auth/DeviceOAuthClient.java#L19-L23
Added lines #L19 - L23 were not covered by tests
api/src/main/java/com/coze/openapi/service/auth/PKCEOAuthClient.java
[warning] 37-41: api/src/main/java/com/coze/openapi/service/auth/PKCEOAuthClient.java#L37-L41
Added lines #L37 - L41 were not covered by tests
🔇 Additional comments (4)
api/src/main/java/com/coze/openapi/client/auth/OAuthToken.java (1)
29-30: LGTM! Addition of token_type field follows OAuth2 standard.The addition of the
tokenTypefield with proper JSON mapping annotation follows the OAuth2 specification and maintains consistency with existing field patterns.api/src/main/java/com/coze/openapi/service/auth/WebOAuthClient.java (1)
15-22: Add test coverage for loadFromFile method.The new
loadFromFilemethod lacks test coverage. Consider adding tests for:
- Successful configuration loading
- Invalid file path handling
- Invalid configuration handling
Would you like me to help create comprehensive test cases for this method?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 16-21: api/src/main/java/com/coze/openapi/service/auth/WebOAuthClient.java#L16-L21
Added lines #L16 - L21 were not covered by testsexample/pom.xml (1)
19-19: LGTM! Version update is consistent.The version update aligns with the version change in api/pom.xml.
api/pom.xml (1)
46-46: LGTM! Version update follows semantic versioning.The version update is consistent across the project files.
| @JsonProperty("client_type") | ||
| private String clientType; | ||
|
|
||
| @JsonProperty("client_id") | ||
| private String clientId; | ||
|
|
||
| @JsonProperty("client_secret") | ||
| private String clientSecret; | ||
|
|
||
| @JsonProperty("private_key") | ||
| private String privateKey; | ||
|
|
||
| @JsonProperty("public_key_id") | ||
| private String publicKeyId; | ||
|
|
||
| @JsonProperty("coze_api_base") | ||
| private String cozeApiBase; | ||
|
|
||
| @JsonProperty("coze_www_base") | ||
| private String cozeWwwBase; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for required fields.
The configuration class contains multiple fields but lacks validation. Some fields might be required depending on the OAuth flow type (e.g., clientId is always required, while privateKey is only needed for JWT flow).
Consider adding a validation method:
public void validate() {
if (clientId == null || clientId.trim().isEmpty()) {
throw new IllegalStateException("client_id is required");
}
// Add conditional validation based on client_type
if ("jwt".equals(clientType)) {
if (privateKey == null || privateKey.trim().isEmpty()) {
throw new IllegalStateException("private_key is required for JWT client type");
}
}
}| private static OAuthConfig doLoad(String configFilePath) { | ||
| try (InputStream inputStream = new FileInputStream(configFilePath)) { | ||
| ObjectMapper mapper = new ObjectMapper(); | ||
| mapper.findAndRegisterModules(); | ||
|
|
||
| return mapper.readValue(inputStream, OAuthConfig.class); | ||
|
|
||
| } catch (FileNotFoundException e) { | ||
| throw new RuntimeException("Config file not found: " + configFilePath, e); | ||
| } catch (StreamReadException e) { | ||
| throw new RuntimeException("JSON file format error: " + e.getMessage(), e); | ||
| } catch (DatabindException e) { | ||
| throw new RuntimeException("Failed to map config file to object: " + e.getMessage(), e); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("I/O error while reading config file: " + e.getMessage(), e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance security and error handling in configuration loading.
While the error handling is good, there are several security considerations for loading sensitive configuration:
- File permissions are not checked
- Sensitive data remains in memory as String
- Exception messages might expose sensitive paths
Consider these improvements:
private static OAuthConfig doLoad(String configFilePath) {
+ File configFile = new File(configFilePath);
+ if (!configFile.exists() || !configFile.canRead()) {
+ throw new RuntimeException("Config file is not accessible");
+ }
try (InputStream inputStream = new FileInputStream(configFilePath)) {
ObjectMapper mapper = new ObjectMapper();
mapper.findAndRegisterModules();
- return mapper.readValue(inputStream, OAuthConfig.class);
+ OAuthConfig config = mapper.readValue(inputStream, OAuthConfig.class);
+ config.validate(); // Add validation call
+ return config;
} catch (FileNotFoundException e) {
- throw new RuntimeException("Config file not found: " + configFilePath, e);
+ throw new RuntimeException("Config file not found", e);
} catch (StreamReadException e) {
throw new RuntimeException("JSON file format error: " + e.getMessage(), e);
} catch (DatabindException e) {Committable suggestion skipped: line range outside the PR's diff.
| public static DeviceOAuthClient loadFromFile(String configFilePath) { | ||
| OAuthConfig config = OAuthConfig.load(configFilePath); | ||
| return new DeviceOAuthClient.DeviceOAuthBuilder() | ||
| .clientID(config.getClientId()) | ||
| .baseURL(config.getCozeApiBase()) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Review comment is accurate - shared test utilities needed and loadFromFile requires test coverage
The codebase analysis confirms:
- Multiple OAuth clients with similar test patterns but no shared test utilities
- Missing test coverage for loadFromFile implementations
- The suggested OAuthTestUtils would reduce duplication and improve maintainability
🔗 Analysis chain
Consider creating shared test utilities for loadFromFile methods.
Multiple OAuth clients now implement similar loadFromFile methods. Consider:
- Creating shared test utilities to avoid test duplication
- Adding test coverage for this implementation
Example test utility class:
public class OAuthTestUtils {
public static String createTestConfig(String clientType, String clientId) {
return String.format("""
{
"client_type": "%s",
"client_id": "%s",
"coze_api_base": "https://api.test"
}
""", clientType, clientId);
}
public static File writeTestConfig(String config) throws IOException {
File tempFile = File.createTempFile("test-oauth-config", ".json");
tempFile.deleteOnExit();
Files.writeString(tempFile.toPath(), config);
return tempFile;
}
}🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find OAuth client implementations
ast-grep --pattern 'class $_OAuthClient {
$$$
}'
# Find test files related to OAuth
rg -l "OAuthClient.*Test"
# Look for existing loadFromFile implementations and tests
rg -l "loadFromFile|OAuthTestUtils"
Length of output: 686
Script:
#!/bin/bash
# Check test implementations for loadFromFile
rg "loadFromFile" api/src/test/java/com/coze/openapi/service/auth/*Test.java -B 1 -A 3
# Look for existing test utilities or helper methods
rg "class|void.*test" api/src/test/java/com/coze/openapi/service/auth/*Test.java
Length of output: 5449
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 19-23: api/src/main/java/com/coze/openapi/service/auth/DeviceOAuthClient.java#L19-L23
Added lines #L19 - L23 were not covered by tests
| public static PKCEOAuthClient loadFromFile(String configFilePath) { | ||
| OAuthConfig config = OAuthConfig.load(configFilePath); | ||
| return new PKCEOAuthClient.PKCEOAuthBuilder() | ||
| .clientID(config.getClientId()) | ||
| .baseURL(config.getCozeApiBase()) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add error handling and test coverage for loadFromFile method
The verification confirms the need for both error handling and test coverage. The method requires:
-
Error handling for:
- Null/empty config file path
- File access/parsing errors
- Null/empty client ID or base URL in config
-
Test coverage for:
- Successful config loading
- Invalid file path scenarios
- Malformed configuration
- Missing required fields
The original review comment's suggested changes are appropriate, but should also include validation for required config fields:
public static PKCEOAuthClient loadFromFile(String configFilePath) {
+ if (configFilePath == null || configFilePath.trim().isEmpty()) {
+ throw new IllegalArgumentException("Config file path cannot be null or empty");
+ }
OAuthConfig config = OAuthConfig.load(configFilePath);
+ if (config == null) {
+ throw new IllegalStateException("Failed to load OAuth configuration from file");
+ }
+ String clientId = config.getClientId();
+ String baseUrl = config.getCozeApiBase();
+ if (clientId == null || clientId.trim().isEmpty() || baseUrl == null || baseUrl.trim().isEmpty()) {
+ throw new IllegalStateException("Config must contain non-empty clientId and baseURL");
+ }
return new PKCEOAuthClient.PKCEOAuthBuilder()
.clientID(config.getClientId())
.baseURL(config.getCozeApiBase())
.build();
}🔗 Analysis chain
Add error handling and test coverage for the loadFromFile method.
The method should handle potential file-related exceptions and include comprehensive test coverage.
- Add error handling:
public static PKCEOAuthClient loadFromFile(String configFilePath) {
+ if (configFilePath == null || configFilePath.trim().isEmpty()) {
+ throw new IllegalArgumentException("Config file path cannot be null or empty");
+ }
OAuthConfig config = OAuthConfig.load(configFilePath);
+ if (config == null) {
+ throw new IllegalStateException("Failed to load OAuth configuration from file");
+ }
return new PKCEOAuthClient.PKCEOAuthBuilder()
.clientID(config.getClientId())
.baseURL(config.getCozeApiBase())
.build();
}- Let's verify the test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing test files
fd -e java -p "test.*PKCEOAuthClient.*Test\.java$"
# Look for test methods related to loadFromFile
rg -l "test.*[Ll]oad.*[Ff]ile.*PKCEOAuthClient" -t java
Length of output: 183
Script:
#!/bin/bash
# Check test file content
cat api/src/test/java/com/coze/openapi/service/auth/PKCEOAuthClientTest.java
# Check OAuthConfig implementation
ast-grep --pattern 'class OAuthConfig {
$$$
static OAuthConfig load($_) {
$$$
}
$$$
}'
Length of output: 6529
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 37-41: api/src/main/java/com/coze/openapi/service/auth/PKCEOAuthClient.java#L37-L41
Added lines #L37 - L41 were not covered by tests
| public static JWTOAuthClient loadFromFile(String configFilePath) throws Exception { | ||
| OAuthConfig config = OAuthConfig.load(configFilePath); | ||
| return new JWTOAuthClient.JWTOAuthBuilder() | ||
| .privateKey(config.getPrivateKey()) | ||
| .publicKey(config.getPublicKeyId()) | ||
| .clientID(config.getClientId()) | ||
| .baseURL(config.getCozeApiBase()) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
Improve error handling and exception specificity.
The method needs better error handling and should use more specific exceptions.
- Improve error handling and exception specificity:
-public static JWTOAuthClient loadFromFile(String configFilePath) throws Exception {
+public static JWTOAuthClient loadFromFile(String configFilePath) throws IOException, InvalidKeyException {
+ if (configFilePath == null || configFilePath.trim().isEmpty()) {
+ throw new IllegalArgumentException("Config file path cannot be null or empty");
+ }
OAuthConfig config = OAuthConfig.load(configFilePath);
+ if (config == null) {
+ throw new IllegalStateException("Failed to load OAuth configuration from file");
+ }
+ if (config.getPrivateKey() == null || config.getPublicKeyId() == null) {
+ throw new IllegalStateException("Missing required JWT configuration");
+ }
return new JWTOAuthClient.JWTOAuthBuilder()
.privateKey(config.getPrivateKey())
.publicKey(config.getPublicKeyId())
.clientID(config.getClientId())
.baseURL(config.getCozeApiBase())
.build();
}- Let's verify the test coverage:
#!/bin/bash
# Check for existing test files
fd -e java -p "test.*JWTOAuthClient.*Test\.java$"
# Look for test methods related to loadFromFile
rg -l "test.*[Ll]oad.*[Ff]ile.*JWTOAuthClient" -t java🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 36-42: api/src/main/java/com/coze/openapi/service/auth/JWTOAuthClient.java#L36-L42
Added lines #L36 - L42 were not covered by tests
chyroc
changed the title
chyroc
changed the title
feat: add load oauth client by file