-
Notifications
You must be signed in to change notification settings - Fork 6
feat: GCP Auth Login #11
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.
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
Changes from 3 commits
0a407c5
85f39e7
f63e3c4
f14a599
94aed32
341046d
9e7465e
6440704
a3ede1c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
package com.infisical.sdk.auth; | ||
|
||
import java.io.IOException; | ||
import java.util.Arrays; | ||
import java.util.HashMap; | ||
|
||
import com.google.auth.oauth2.GoogleCredentials; | ||
import com.google.auth.oauth2.IdTokenCredentials; | ||
import com.google.auth.oauth2.IdTokenProvider; | ||
import com.google.auth.oauth2.IdTokenProvider.Option; | ||
|
||
public class GCPAuthProvider { | ||
|
||
public static HashMap<String,String> getGCPAuthInput(String identityId){ | ||
|
||
|
||
try{ | ||
|
||
// This will fetch credentials from environment variable named GOOGLE_APPLICATION_CREDENTIALS or | ||
// or if it's running in a GCP instance it will get them from the instance itself (GCP service account attached) | ||
GoogleCredentials googleCredentials = GoogleCredentials.getApplicationDefault(); | ||
|
||
IdTokenCredentials idTokenCredentials = | ||
IdTokenCredentials.newBuilder() | ||
.setIdTokenProvider((IdTokenProvider) googleCredentials) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Unsafe cast to IdTokenProvider without type checking - could throw ClassCastException if GoogleCredentials doesn't implement IdTokenProvider Prompt To Fix With AIThis is a comment left during a code review.
Path: src/main/java/com/infisical/sdk/auth/GCPAuthProvider.java
Line: 24:24
Comment:
**logic:** Unsafe cast to IdTokenProvider without type checking - could throw ClassCastException if GoogleCredentials doesn't implement IdTokenProvider
How can I resolve this? If you propose a fix, please make it concise. |
||
.setTargetAudience(identityId) | ||
.setOptions(Arrays.asList(Option.FORMAT_FULL, Option.LICENSES_TRUE)) | ||
.build(); | ||
|
||
// Get the ID token. | ||
String idToken = idTokenCredentials.refreshAccessToken().getTokenValue(); | ||
|
||
// Body cannot be a string so... HashMap can use bulider, POJO etc | ||
|
||
HashMap<String, String> body = new HashMap<>(); | ||
body.put("identityId", identityId); | ||
body.put("jwt", idToken); | ||
|
||
return body; | ||
|
||
} catch (IOException e){ | ||
throw new RuntimeException(e); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Converting IOException to RuntimeException may hide important credential configuration issues. Consider providing more specific error messages or allowing IOException to propagate with better context. Prompt To Fix With AIThis is a comment left during a code review.
Path: src/main/java/com/infisical/sdk/auth/GCPAuthProvider.java
Line: 39:41
Comment:
**style:** Converting IOException to RuntimeException may hide important credential configuration issues. Consider providing more specific error messages or allowing IOException to propagate with better context.
How can I resolve this? If you propose a fix, please make it concise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I saw this in AWSAuthProvider.java the same did this in here, hence did it, let me know if I need to change this. |
||
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
package com.infisical.sdk.auth; | ||
|
||
import com.infisical.sdk.InfisicalSdk; | ||
import com.infisical.sdk.config.SdkConfig; | ||
import com.infisical.sdk.util.EnvironmentVariables; | ||
import org.junit.jupiter.api.Test; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
import static org.junit.jupiter.api.Assertions.*; | ||
|
||
public class GCPAuthIntegrationTest{ | ||
|
||
|
||
private static final Logger logger = LoggerFactory.getLogger(GCPAuthIntegrationTest.class); | ||
@Test | ||
public void testGCPAuthAndFetchSecrets() throws Exception { | ||
|
||
// Load env variables | ||
var envVars = new EnvironmentVariables(); | ||
|
||
// Get Machine Identity Id | ||
String machineIdentityId = System.getenv("INFISICAL_MACHINE_IDENTITY_ID"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider using envVars.getMachineIdentityId() if this method exists, for consistency with other environment variable access patterns in the codebase Prompt To Fix With AIThis is a comment left during a code review.
Path: src/test/java/com/infisical/sdk/auth/GCPAuthIntegrationTest.java
Line: 22:22
Comment:
**style:** Consider using envVars.getMachineIdentityId() if this method exists, for consistency with other environment variable access patterns in the codebase
How can I resolve this? If you propose a fix, please make it concise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't wanted to update existing code hence did this, let me know if I need to change this. |
||
|
||
|
||
// Check if env variable machine identity is set others are already tested via env tests | ||
assertNotNull(machineIdentityId, "INFISICAL_MACHINE_IDENTITY_ID env variable must be set"); | ||
|
||
|
||
// Create SDK instance | ||
var sdk = new InfisicalSdk(new SdkConfig.Builder() | ||
.withSiteUrl(envVars.getSiteUrl()) | ||
.build() | ||
); | ||
|
||
// Authenticate using GCP Auth | ||
assertDoesNotThrow(() -> { | ||
sdk.Auth().GCPAuthLogin(machineIdentityId); | ||
}); | ||
|
||
|
||
|
||
try { | ||
|
||
// Test if we have correctly logged in and we can list the secrets | ||
var secrets = sdk.Secrets().ListSecrets( | ||
envVars.getProjectId(), | ||
"dev", | ||
"/", | ||
null, | ||
null, | ||
null); | ||
|
||
assertNotNull(secrets, "Secrets list should not be null"); | ||
assertFalse(secrets.isEmpty(), "Secrets list should not be empty"); | ||
Comment on lines
+53
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Test assumes secrets exist in the 'dev' environment. Consider adding a comment explaining this prerequisite or making the test more robust by checking if no secrets exist. Prompt To Fix With AIThis is a comment left during a code review.
Path: src/test/java/com/infisical/sdk/auth/GCPAuthIntegrationTest.java
Line: 53:54
Comment:
**style:** Test assumes secrets exist in the 'dev' environment. Consider adding a comment explaining this prerequisite or making the test more robust by checking if no secrets exist.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
||
logger.info("TestGCPAuth Successful"); | ||
logger.info("Secrets length : {}", secrets.size()); | ||
|
||
} catch (Exception e) { | ||
throw new AssertionError(e); | ||
} | ||
} | ||
} |
Uh oh!
There was an error while loading. Please reload this page.