Skip to content

feat: Create OAuth client from config & Bump version to 0.2.1 #29

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 17, 2025
Merged

feat: Create OAuth client from config & Bump version to 0.2.1 #29

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cc1cbdd
feat: add load oauth client by file
hanzeINGH Jan 17, 2025
0e1f731
update
hanzeINGH Jan 17, 2025
2cc6b39
update
hanzeINGH Jan 17, 2025
b2c9070
update
hanzeINGH Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
</parent>

<artifactId>coze-api</artifactId>
<version>0.2.0</version>
<version>0.2.1</version>

<scm>
<connection>scm:git:git://github.com/coze-dev/coze-java.git</connection>
Expand Down
65 changes: 65 additions & 0 deletions api/src/main/java/com/coze/openapi/client/auth/OAuthConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package com.coze.openapi.client.auth;

import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.exc.StreamReadException;
import com.fasterxml.jackson.databind.DatabindException;
import com.fasterxml.jackson.databind.ObjectMapper;

import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Data;
import lombok.NoArgsConstructor;

@Data
@Builder
@NoArgsConstructor
@AllArgsConstructor
public class OAuthConfig {
@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;
Comment on lines +23 to +42
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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");
        }
    }
}


public static OAuthConfig load(String configFilePath) {
return doLoad(configFilePath);
}

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);
}
}
Comment on lines +48 to +64
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. File permissions are not checked
  2. Sensitive data remains in memory as String
  3. 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.

}
3 changes: 3 additions & 0 deletions api/src/main/java/com/coze/openapi/client/auth/OAuthToken.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,7 @@ public class OAuthToken extends BaseResp {

@JsonProperty("refresh_token")
private String refreshToken;

@JsonProperty("token_type")
private String tokenType;
}
8 changes: 8 additions & 0 deletions api/src/main/java/com/coze/openapi/service/auth/DeviceOAuthClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@
super(builder);
}

public static DeviceOAuthClient loadFromFile(String configFilePath) {
OAuthConfig config = OAuthConfig.load(configFilePath);
return new DeviceOAuthClient.DeviceOAuthBuilder()
.clientID(config.getClientId())
.baseURL(config.getCozeApiBase())
.build();

Check warning on line 23 in api/src/main/java/com/coze/openapi/service/auth/DeviceOAuthClient.java

View check run for this annotation

Codecov / codecov/patch

api/src/main/java/com/coze/openapi/service/auth/DeviceOAuthClient.java#L19-L23 

Added lines #L19 - L23 were not covered by tests
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. Creating shared test utilities to avoid test duplication
  2. 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


private static final Logger logger = AuthLogFactory.getLogger();

public DeviceAuthResp getDeviceCode() {
Expand Down
11 changes: 11 additions & 0 deletions api/src/main/java/com/coze/openapi/service/auth/JWTOAuthClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import com.coze.openapi.client.auth.GetAccessTokenReq;
import com.coze.openapi.client.auth.GrantType;
import com.coze.openapi.client.auth.OAuthConfig;
import com.coze.openapi.client.auth.OAuthToken;
import com.coze.openapi.client.auth.scope.Scope;
import com.coze.openapi.service.utils.Utils;
Expand All @@ -31,6 +32,16 @@
this.ttl = builder.ttl;
}

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();

Check warning on line 42 in api/src/main/java/com/coze/openapi/service/auth/JWTOAuthClient.java

View check run for this annotation

Codecov / codecov/patch

api/src/main/java/com/coze/openapi/service/auth/JWTOAuthClient.java#L36-L42 

Added lines #L36 - L42 were not covered by tests
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve error handling and exception specificity.

The method needs better error handling and should use more specific exceptions.

  1. 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();
 }
  1. 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


@Override
public OAuthToken refreshToken(String refreshToken) {
return null;
Expand Down
13 changes: 9 additions & 4 deletions api/src/main/java/com/coze/openapi/service/auth/PKCEOAuthClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

import com.coze.openapi.client.auth.GetAccessTokenReq;
import com.coze.openapi.client.auth.GetPKCEAuthURLResp;
import com.coze.openapi.client.auth.GrantType;
import com.coze.openapi.client.auth.OAuthToken;
import com.coze.openapi.client.auth.*;
import com.coze.openapi.service.utils.Utils;

import lombok.Getter;
Expand All @@ -36,6 +33,14 @@
super(builder);
}

public static PKCEOAuthClient loadFromFile(String configFilePath) {
OAuthConfig config = OAuthConfig.load(configFilePath);
return new PKCEOAuthClient.PKCEOAuthBuilder()
.clientID(config.getClientId())
.baseURL(config.getCozeApiBase())
.build();

Check warning on line 41 in api/src/main/java/com/coze/openapi/service/auth/PKCEOAuthClient.java

View check run for this annotation

Codecov / codecov/patch

api/src/main/java/com/coze/openapi/service/auth/PKCEOAuthClient.java#L37-L41 

Added lines #L37 - L41 were not covered by tests
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

Add error handling and test coverage for loadFromFile method

The verification confirms the need for both error handling and test coverage. The method requires:

  1. Error handling for:

    • Null/empty config file path
    • File access/parsing errors
    • Null/empty client ID or base URL in config

  2. 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.

  1. 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();
 }
  1. 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 GetPKCEAuthURLResp genOAuthURL(@NotNull String redirectURI, String state) {
return genOAuthURL(redirectURI, state, CodeChallengeMethod.Plain);
}
Expand Down
10 changes: 10 additions & 0 deletions api/src/main/java/com/coze/openapi/service/auth/WebOAuthClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import org.jetbrains.annotations.NotNull;

import com.coze.openapi.client.auth.GrantType;
import com.coze.openapi.client.auth.OAuthConfig;
import com.coze.openapi.client.auth.OAuthToken;

public class WebOAuthClient extends OAuthClient {
Expand All @@ -11,6 +12,15 @@
super(builder);
}

public static WebOAuthClient loadFromFile(String configFilePath) {
OAuthConfig config = OAuthConfig.load(configFilePath);
return new WebOAuthClient.WebOAuthBuilder()
.clientID(config.getClientId())
.clientSecret(config.getClientSecret())
.baseURL(config.getCozeApiBase())
.build();

Check warning on line 21 in api/src/main/java/com/coze/openapi/service/auth/WebOAuthClient.java

View check run for this annotation

Codecov / codecov/patch

api/src/main/java/com/coze/openapi/service/auth/WebOAuthClient.java#L16-L21 

Added lines #L16 - L21 were not covered by tests
}

@Override
public String getOAuthURL(@NotNull String redirectURI, String state) {
return super.getOAuthURL(redirectURI, state);
Expand Down
2 changes: 1 addition & 1 deletion api/src/main/java/com/coze/openapi/service/utils/UserAgentInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public Response intercept(Chain chain) throws IOException {
return chain.proceed(request);
}

public static final String VERSION = "0.2.0";
public static final String VERSION = "0.2.1";
private static final ObjectMapper objectMapper = new ObjectMapper();

/** 获取操作系统版本 */
Expand Down
2 changes: 1 addition & 1 deletion example/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
<dependency>
<groupId>com.coze</groupId>
<artifactId>coze-api</artifactId>
<version>${project.parent.version}</version>
<version>0.2.1</version>
</dependency>
</dependencies>
</project>
Loading