Account ID support for Crendentials Providers #262
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #262 +/- ##
==========================================
+ Coverage 80.50% 80.64% +0.13%
==========================================
Files 33 33
Lines 6099 6158 +59
==========================================
+ Hits 4910 4966 +56
- Misses 1189 1192 +3 ☔ View full report in Codecov by Sentry. |
waahm7
changed the title
| struct aws_credentials *aws_credentials_new_from_string_with_account_id( | ||
| struct aws_allocator *allocator, | ||
| const struct aws_string *access_key_id, | ||
| const struct aws_string *secret_access_key, | ||
| const struct aws_string *session_token, | ||
| const struct aws_string *account_id, | ||
| uint64_t expiration_timepoint_seconds); |
There was a problem hiding this comment.
There's a time-honored tradition in this codebase, when new arguments keep creeping into constructors...
| struct aws_credentials *aws_credentials_new_from_string_with_account_id( | |
| struct aws_allocator *allocator, | |
| const struct aws_string *access_key_id, | |
| const struct aws_string *secret_access_key, | |
| const struct aws_string *session_token, | |
| const struct aws_string *account_id, | |
| uint64_t expiration_timepoint_seconds); | |
| struct aws_credentials *aws_credentials_new_from_options( | |
| struct aws_allocator *allocator, | |
| const struct aws_credentials_options *options); |
| /* The format of the Arn is arn:partition:service:region:account-id:resource-ID and we need to parse the | ||
| * account-id out of it which is the fifth element. */ | ||
| for (int i = 0; i < 5; i++) { | ||
| if (!aws_byte_cursor_next_split(&arn_cursor, ':', &account_id)) { |
There was a problem hiding this comment.
note: its valid for arn not to have account id field. most fields are optional from what i remember (s3 is an example of arns without a bunch of fields). do we want to have logic that accounts for that? or just setting it to empty string is fine?
| struct aws_byte_cursor arn_cursor; | ||
| AWS_ZERO_STRUCT(arn_cursor); | ||
|
|
||
| if (aws_byte_cursor_eq_c_str_ignore_case(&node_name, "Arn")) { |
There was a problem hiding this comment.
maybe pull arn parsing into a separate helper that we can test standalone?
| if (options->token_name) { | ||
| token = aws_json_value_get_from_object(document_root, aws_byte_cursor_from_c_str((char *)options->token_name)); | ||
| if (!aws_json_value_is_string(token) || aws_json_value_get_string(token, &token_cursor) == AWS_OP_ERR) { | ||
| if (!aws_json_value_is_string(token) || aws_json_value_get_string(token, &token_cursor) == AWS_OP_ERR || | ||
| token_cursor.len == 0) { |
There was a problem hiding this comment.
why is this one getting a length check, but access_key_id and secret_access_key above don't get a length check?
worth breaking these 4 checks out into a helper function?
Issue: awslabs/aws-crt-swift#303
Description of changes:
Update the following providers to provide account_id if present.
AWS_ACCOUNT_IDenv variable.sso_account_idconfig parameter which was also required for the request.AccountIdfield in the response if present.AccountIdfield in the response if present.aws_account_idin the config file.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.