Feature/add account number verification #14
Conversation
|
@pandian912 Looks like the build failed installing the JDK. I don't think that's related to my PR though. |
take a pull from master plz |
|
I pulled from master but this PR is for develop so it pulled in other changes from master. Did you mean for me to rebase with develop? |
|
I fixed it by cherry picking the travis.yml change from master. |
| @@ -150,14 +167,24 @@ public BasicCredentials() { | |||
| //ignore to use amazon default provider | |||
| } | |||
| } | |||
|
|
|||
| public BasicCredentials(AWSAccountValidator validator) { | |||
There was a problem hiding this comment.
this is a new constructor which will not be invoked by default... would recommend adding this logic in the default constructor so that all existing usages get this feature
There was a problem hiding this comment.
I added this so that I could inject a mock instance in order to test with. The logic in getCredentials will create an instance of AWSAccountValidator if one does not exist so existing usages will get this feature
|
|
||
| public BasicCredentials(AWSAccountValidator validator) { | ||
| this.validator = validator; | ||
| } | ||
|
|
||
| /** | ||
| * Returns AWSCredentials for the given user/account. Reverts to default credentials provider if no credentials exist. | ||
| * @return AWSCredentials | ||
| */ | ||
| public AWSCredentials getAwsCredentials() { |
There was a problem hiding this comment.
- This function is a failsafe and the actual logic of fetching the credentials(via various methods) is done in the default constructor.... I would leave this function alone and make the validator call from the default constructor...
- Also i would pass the validator function the AWSCredentials object or pass the account number fetched from the account as validator function does not have access to credentials
There was a problem hiding this comment.
Ideally, there should be one method that creates the credentials. So I'd like to consolidate that code into one method and then put the validation logic there. Does that make sense?
| public void validateAccount(String targetAccount) { | ||
|
|
||
| if (targetAccount != null) { | ||
| GetCallerIdentityRequest getCallerIdentityRequest = new GetCallerIdentityRequest(); |
There was a problem hiding this comment.
.withCredentials(new AWSStaticCredentialsProvider(awsCredentials))
.withRegion(region)
.build()
.getCallerIdentity(new GetCallerIdentityRequest())
.getAccount();
There was a problem hiding this comment.
If the code builds the AWSCredentialsProvider on the fly, there's no real way to unit test the code without having valid credentials (ie, no way to inject a mock provider).
What Changed
Added AWS Account verification
Why
Solves issue #8
Todo:
yarn contributors:add)