🐛 Correctly handle nested configs #183

AlainODea · 2018-08-19T23:33:26Z

Problem Statement

Issue #141 states:

Existing ~/.aws/config with nested properties are overwritten improperly by this tool.
For example, s3 configuration uses indentation for its properties e.g.

[profile development]
aws_access_key_id=foo
aws_secret_access_key=bar
s3 =
  max_concurrent_requests = 20
  max_queue_size = 10000

When awscli is run, the config is written out like so:

[profile development]
aws_access_key_id=foo
aws_secret_access_key=bar
s3 =
max_concurrent_requests = 20
max_queue_size = 10000

which is not correctly read by aws s3 commands!

Specifically, the following test https://github.com/oktadeveloper/okta-aws-cli-assume-role/blob/054c45c5b0b58195db45bd25b528c0436331d7ed/src/test/java/com/okta/tools/aws/settings/ConfigurationTest.java#L110-L130 if modified to contain s3 keys as follows:

    @Test
    void addOrUpdateProfileToExistingProfile() throws IOException {
        final String existingCombined = manualRole + "\n"
                + "s3 =\n"
                + "    max_queue_size = 1000" + "\n\n" + existingProfile;

        final StringReader configurationReader = new StringReader(existingCombined);
        final StringWriter configurationWriter = new StringWriter();
        final Configuration configuration = new Configuration(configurationReader);

        final String updatedPrefix = "updated_";
        final String expected = "[profile " + profileName + "]\n"
                + Configuration.ROLE_ARN + " = " + updatedPrefix + role_arn + "\n"
                + SOURCE_PROFILE + " = " + profileName + "_source\n"
                + Configuration.REGION + " = " + region + "\n"
                + "s3 =\n"
                + "    max_queue_size = 1000"
                + "\n\n" + existingProfile;


        configuration.addOrUpdateProfile(profileName, updatedPrefix + role_arn);
        configuration.save(configurationWriter);

        String given = StringUtils.remove(configurationWriter.toString().trim(), '\r');

        assertEquals(expected, given);
    }

will not pass.

Solution

Add Apache Commons Configuration2
Adapt configuration2's INIConfiguration to support AWS quirks
Remove Ini4j (unmaintained, has park page for website)
Refactor file handling code

Resolves #141

Apologies for the extensiveness of this change. The configuration
handling likely needs a refactor to make it easier to support future
modification.

Apologies for the extensiveness of this change. The configuration handling likely needs a refactor to make it easier to support future modification. - Add Apache Commons Configuration2 - Adapt configuration2's INIConfiguration to support AWS quirks - Remove Ini4j (unmaintained, has park page for website) - Refactor file handling code Resolves oktadev#141

AlainODea

For some context on the over 1,800 lines of changes:

875 is directly copied with minimal modification from Apache Commons Configuration 2
294 lines are new tests

AlainODea · 2018-08-19T23:37:49Z

src/main/java/com/okta/tools/aws/settings/Settings.java


 /**
 * A simple abstract class used to load AWS CLI settings files like the `credentials` or `config` files.
 */
 public abstract class Settings {

- final Ini settings = new Ini();
+ private final AwsINIConfiguration settings;


A huge chunk of the change stems from hiding this as exposing it causing Ini4j's API to leak into everywhere and my original attempt at this had Apache Commons Configuration2's API leaked into everywhere.

AlainODea · 2018-08-19T23:38:14Z

src/main/java/com/okta/tools/aws/settings/AwsINIConfiguration.java

@@ -0,0 +1,875 @@
+/*


🤔 before calling me on this. I admit that this is a copy/paste from Apache Commons Configuration2. I tried to extend their class and it wasn't open to extension in this manner. I will look into contributing an extension point to their project so that we can later replace this with something much cleaner.

It's literally this code:
https://github.com/apache/commons-configuration/blob/trunk/src/main/java/org/apache/commons/configuration2/INIConfiguration.java

With some changes I'll highlight in the review.

AlainODea · 2018-08-19T23:41:25Z

src/main/java/com/okta/tools/aws/settings/AwsINIConfiguration.java

+import org.apache.commons.configuration2.ex.ConfigurationException;
+import org.apache.commons.configuration2.ex.ConfigurationRuntimeException;
+import org.apache.commons.configuration2.tree.*;
+import org.apache.commons.lang3.StringUtils;


➕ added this so I could effectively right trim.

AlainODea · 2018-08-19T23:42:18Z

src/main/java/com/okta/tools/aws/settings/AwsINIConfiguration.java

+ String line = in.readLine();
+ while (line != null)
+ {
+ line = StringUtils.stripEnd(line, null);


➕ right trim instead of trim the entire line:
https://github.com/apache/commons-configuration/blob/trunk/src/main/java/org/apache/commons/configuration2/INIConfiguration.java#L433

AlainODea · 2018-08-19T23:42:48Z

src/main/java/com/okta/tools/aws/settings/AwsINIConfiguration.java

+ {
+ key = line;
+ }
+ key = StringUtils.stripEnd(key, null);


➕ right trim instead of trim the key:
https://github.com/apache/commons-configuration/blob/trunk/src/main/java/org/apache/commons/configuration2/INIConfiguration.java#L461

AlainODea · 2018-08-19T23:47:05Z

src/test/java/com/okta/tools/aws/settings/ConfigurationTest.java

- void instantiateInvalidConfiguration() {
- assertThrows(IOException.class, () -> new Configuration(new StringReader("someinvalidini")));
- }
-


This is technically a valid INI file. I couldn't figure out how to construct an invalid INI file. Ini4j got this wrong.

AlainODea · 2018-08-19T23:47:55Z

src/main/java/com/okta/tools/helpers/SessionHelper.java

- Reader reader = FileHelper.getReader(FileHelper.getOktaDirectory(), "profiles");
-
- return new MultipleProfile(reader);
- }


🎨 I reordered the methods here to be in step-down order (Clean Code). Sorry for the diff carnage.

AlainODea · 2018-08-19T23:49:26Z

src/main/java/com/okta/tools/helpers/SessionHelper.java

+ FileHelper.usingPath(FileHelper.getOktaDirectory().resolve("profiles"), (reader, writer) -> {
+ MultipleProfile multipleProfile = new MultipleProfile(reader);
+ multipleProfile.addOrUpdateProfile(environment.oktaProfile, environment.awsRoleToAssume, start);
+ multipleProfile.save(writer);


🎨 I replaced the explicit resource management with a utility method that structurally ensures correct use without burdening every site of use.

AlainODea · 2018-08-19T23:50:17Z

src/main/java/com/okta/tools/helpers/SessionHelper.java

- private Path getSessionPath() throws IOException {
- return FileHelper.resolveFilePath(FileHelper.getOktaDirectory(), ".current-session");
- }
-


🎨 a good chunk of the diff in this file is method reordering to show methods in the order they are used (step down rule from Clean Code).

AlainODea · 2018-08-19T23:51:00Z

src/main/java/com/okta/tools/helpers/FileHelper.java

+ }
+
+ public interface PathW {
+ void useFile(Writer writer) throws IOException;


❓ I'm not sure I love the names of these utility methods or interfaces, but I couldn't come up with anything better. I like the outcome in code tho.

frosforever · 2018-08-20T19:48:32Z

src/main/java/com/okta/tools/aws/settings/Credentials.java

@@ -60,21 +59,8 @@ public Credentials(Reader reader) throws IOException {
 */
 public void addOrUpdateProfile(String name, String awsAccessKey, String awsSecretKey, String awsSessionToken) {
 name = "default".equals(name) ? "default" : name + environment.credentialsSuffix;


Should this also be using DEFAULT_PROFILE_NAME instead?

Definitely. I’ll fix that. Good catch.

- Use defined constant instead of magic string

AlainODea · 2018-08-21T01:48:21Z

@frosforever thank you for the code review. Any other concerns before I merge this?

frosforever

LGTM

frosforever · 2018-08-21T16:42:26Z

src/main/java/com/okta/tools/aws/settings/MultipleProfile.java

- Instant expiry = Instant.parse(profileExpiry);
- return expiry;
+ private Instant getExpiry(String profile) {
+ String profileExpiry = getProperty(profile, "profile_expiry");


Should this use static PROFILE_EXPIRY

frosforever · 2018-08-21T16:42:41Z

src/main/java/com/okta/tools/aws/settings/MultipleProfile.java

- public MultipleProfile(Reader reader) throws IOException {
- super(reader);
+ private String getRoleArn(String profile) {
+ return getProperty(profile, "okta_roleArn");


should this use static OKTA_ROLE_ARN

Thanks again. I've fixed these.

- Use defined constants instead of magic strings

AlainODea commented Aug 19, 2018

View reviewed changes

frosforever reviewed Aug 20, 2018

View reviewed changes

🎨 Address @frosforever's code review

c3170bb

- Use defined constant instead of magic string

frosforever approved these changes Aug 21, 2018

View reviewed changes

🎨 Address @frosforever's code review

785d642

- Use defined constants instead of magic strings

AlainODea merged commit 084209d into oktadev:master Aug 21, 2018

AlainODea deleted the ao-BUG-breaks-nested-config branch September 29, 2018 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Correctly handle nested configs #183

🐛 Correctly handle nested configs #183

AlainODea commented Aug 19, 2018

AlainODea left a comment

AlainODea Aug 19, 2018

AlainODea Aug 19, 2018

AlainODea Aug 19, 2018

AlainODea Aug 19, 2018

AlainODea Aug 19, 2018

AlainODea Aug 19, 2018

AlainODea Aug 19, 2018

AlainODea Aug 19, 2018

AlainODea Aug 19, 2018

AlainODea Aug 19, 2018

AlainODea Aug 19, 2018

frosforever Aug 20, 2018

AlainODea Aug 20, 2018

AlainODea commented Aug 21, 2018

frosforever left a comment

frosforever Aug 21, 2018

frosforever Aug 21, 2018

AlainODea Aug 21, 2018

🐛 Correctly handle nested configs #183

🐛 Correctly handle nested configs #183

Conversation

AlainODea commented Aug 19, 2018

Problem Statement

Solution

AlainODea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlainODea commented Aug 21, 2018

frosforever left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment