iam: implement substrate neutral policy document model#327
iam: implement substrate neutral policy document model#327sandeepvinayak merged 1 commit intosalesforce:mainfrom
Conversation
SriramGuduri
force-pushed
the
iam_policy_doc
branch
3 times, most recently
from
7b1e45d to
4bc6d1e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #327 +/- ##
============================================
+ Coverage 82.06% 82.36% +0.30%
- Complexity 553 625 +72
============================================
Files 183 191 +8
Lines 11238 11503 +265
Branches 1498 1525 +27
============================================
+ Hits 9222 9475 +253
- Misses 1351 1357 +6
- Partials 665 671 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| .statement(Statement.builder() | ||
| .sid("StorageReadAccess") | ||
| .effect("Allow") | ||
| .action("storage:GetObject") | ||
| .action("storage:ListBucket") | ||
| .resource("storage://demo-bucket/*") | ||
| .build()) |
There was a problem hiding this comment.
Client code will be much clean and compact, Please explore the below inputs:
Can we enable validations by exploring the enums way of setting up the effect, actions and other properties
Something like this or a better way you can think of
Statement.builder()
.effect(Effect.ALLOW)
.action(StorageActions.GET_OBJECT)
.action(StorageActions.PUT_OBJECT)
.build()
public final class Action {
private final String service;
private final String operation;
private Action(String service, String operation) {
this.service = service;
this.operation = operation;
}
public static Action of(String action) {
// parses "storage:GetObject" → service="storage", operation="GetObject"
// validates format
}
}
public final class StorageActions {
public static final Action GET_OBJECT = Action.of("storage:GetObject");
public static final Action PUT_OBJECT = Action.of("storage:PutObject");
public static final Action ALL = Action.wildcard("storage");
}
| private static final String REGION = "us-central-1"; | ||
| private static final String TENANT_ID = "projects/substrate-sdk-gcp-poc1"; | ||
| private static final String SERVICE_ACCOUNT = "serviceAccount:multicloudjexample@substrate-sdk-gcp-poc1.iam.gserviceaccount.com"; | ||
| private static final String SERVICE_ACCOUNT = "serviceAccount:chameleon@substrate-sdk-gcp-poc1.iam.gserviceaccount.com"; |
There was a problem hiding this comment.
Let's avoid hardcoding specific GCP service account identifiers (like chameleon@...) directly in the codebase. Since this is an open-source project, we shouldn't expose internal project IDs or account details.
| private static String translateAction(String action) { | ||
| // Handle wildcard actions (e.g., storage:*, compute:*, iam:*) | ||
| if (action.endsWith(":*")) { | ||
| String service = action.substring(0, action.length() - 2); |
There was a problem hiding this comment.
This is a case-sensitive check. Do we want to use .toLowerCase() here just in case to match the service name ?
| case "iam": | ||
| return "iam:*"; | ||
| default: | ||
| throw new SubstrateSdkException( |
There was a problem hiding this comment.
nit : UnknownException ?
There was a problem hiding this comment.
We've been using UnknownException for Server Side exceptions. Probably UnSupportedOperationException is a close fit rather.
| // Handle specific actions | ||
| String awsAction = ACTION_MAPPINGS.get(action); | ||
| if (awsAction == null) { | ||
| throw new SubstrateSdkException( |
There was a problem hiding this comment.
nit : UnknownException ?
There was a problem hiding this comment.
We've been using UnknownException for Server Side exceptions. Probably UnSupportedOperationException is a close fit rather.
| } | ||
|
|
||
| // Add Effect | ||
| awsStatement.put("Effect", statement.getEffect()); |
There was a problem hiding this comment.
Unlike Sid, the Effect field is mandatory in AWS IAM. Currently, if statement.getEffect() is null or empty, we are putting a null/empty value into the map, which will result in an invalid AWS policy.
We should add a null check here and either throw a valid Exception (as defined in your Javadoc) or provide a sensible default if the substrate-neutral format allows it.
| Map<String, Object> awsStatement = new LinkedHashMap<>(); | ||
|
|
||
| // Add Sid if present | ||
| if (statement.getSid() != null && !statement.getSid().isEmpty()) { |
There was a problem hiding this comment.
Nit: We are calling statement.getSid() three times here. While the null check is safely handled by the && short-circuit, it’s cleaner to assign the SID to a local variable first. This avoids redundant method calls and makes the logic easier to follow.
| awsStatement.put("Effect", statement.getEffect()); | ||
|
|
||
| // Translate and add Principals if present | ||
| if (statement.getPrincipals() != null && !statement.getPrincipals().isEmpty()) { |
SriramGuduri
force-pushed
the
iam_policy_doc
branch
5 times, most recently
from
bd3dd79 to
78c1570
Compare
SriramGuduri
force-pushed
the
iam_policy_doc
branch
from
78c1570 to
6149ad0
Compare
5 participants
Summary
< Provide a brief description of the changes in this PR >
Some conventions to follow
docstore:for document store module,blobstorefor Blob Store moduletest:perf: