Add multiple role support #119

bturner-r7 · 2018-08-05T06:34:07Z

This adds a role switcher when multiple roles are passed via the SAML assertion. This is similar to how the AWS console handles multiple roles. When a single role is passed, the behavior is unchanged.

This also includes a few random fixes/improvements that I found along the way:

A copy button for the terminal copy/pasta text
Display credential data as a grid
Fix the bugs and URLs when running in react developer mode

Note to reviewers:
I am not a react or express developer, much less a react+redux+express+electron developer. Please keep this in mind when reviewing and feel free to suggest stylistic changes.

Note to Okta users:

AWS requires that multiple roles are passed as multiple values to the https://aws.amazon.com/SAML/Attributes/Role attribute (see the AWS docs). By default, Okta passes multiple values as a single value separated by commas. To fix this, you must contact Okta support and ask them to enable the SAML_SUPPORT_ARRAY_ATTRIBUTES feature flag. See this post.

Once this feature flag is enabled, you can add specify multiple roles using an Okta expression:

Arrays.flatten(
  "<role_arn>,<principal_arn>",
  "<role_arn>,<principal_arn>",
  "<role_arn>,<principal_arn>"
)

The `https://aws.amazon.com/SAML/Attributes/Role` attribute supports single or multiple values. This fixes Awsaml to work when a multiple-value attribute is passed. Currently, the first role is always used. Support for selecting from the roles may be added later. GH-105

This appears to not be needed and gets in the way when using react dev mode.

This adds a role switcher UI when multiple roles,saml-provider pairs are included in the SAML assertion. If only one role is present in the SAML assertion, the switcher is not displayed. GH-105

coveralls · 2018-08-05T07:20:14Z

Coverage remained the same at 90.625% when pulling bff6321 on multiple-role-support into 5f45c37 on master.

bturner-r7 · 2018-08-05T23:00:01Z

src/containers/select-role/SelectRole.js

+
+    if (status === 'selected') {
+      return <Redirect to="/refresh" />;
+    }


Is this the correct way to navigate to a different page after an action? This navigates to the refresh page after selecting a role. Doing this from a render method didn't feel natural to me. Is there a better way?

this is how react-router works, to redirect you render the redirect component. 🤷‍♂️

To avoid clutter, the profile name is only displayed if it has been customized.

erran-r7 · 2018-08-06T14:29:36Z

src/containers/configure/Configure.js

@@ -43,6 +43,7 @@ class Configure extends Component {
    this.state = {
      auth: (params['?auth'] && params['?auth'] === 'true'),
      loaded: false,
+      selectRole: (params['?select-role'] && params['?select-role'] === 'true'),


is it possible for select-role to be passed as a query parameter when auth is passed? assuming not 👍

Not currently.

erran-r7 · 2018-08-06T14:31:22Z

src/containers/select-role/SelectRole.js

+    }
+
+    return (
+      <RenderIfLoaded isLoaded={this.state.loaded}>


yikes, we should look at having skeleton content (instead of rendering based on async wait), but this is 👌 based on current setup.

dgreene-r7

The few comments I had around formatting components can be addressed with the following patches:

diff --git i/src/containers/refresh/Credentials.js w/src/containers/refresh/Credentials.js
index b9263fa..dbf2c20 100644
--- i/src/containers/refresh/Credentials.js
+++ w/src/containers/refresh/Credentials.js
@@ -16,13 +16,19 @@ const CredProps = styled.dl`
 
 const CredPropsKey = styled.dt`
   grid-column: 1;
-  margin-right: .5rem;
+  margin-right: 1rem;
+  margin-bottom: 10px;
+  line-height: 2.5rem;
 `;
 
 const CredPropsVal = styled.dd`
   grid-column: 2;
 `;
 
+const SmallMarginCardBody = styled(CardBody)`
+  padding: 1.25rem 0.5rem;
+`;
+
 export const Credentials = ({awsAccessKey, awsSecretKey, awsSessionToken}) => {
   const creds = new Map();
 
@@ -42,7 +48,7 @@ export const Credentials = ({awsAccessKey, awsSecretKey, awsSessionToken}) => {
     <details>
       <summary>Credentials</summary>
       <Card>
-        <CardBody className="bg-light">
+        <SmallMarginCardBody className="bg-light">
           <CredProps>
             {
               Array.from(creds).map(([name, value]) => {
@@ -61,7 +67,7 @@ export const Credentials = ({awsAccessKey, awsSecretKey, awsSessionToken}) => {
               })
             }
           </CredProps>
-        </CardBody>
+        </SmallMarginCardBody>
       </Card>
     </details>
   );

diff --git i/src/containers/refresh/Refresh.js w/src/containers/refresh/Refresh.js
index f583afd..c42126f 100644
--- i/src/containers/refresh/Refresh.js
+++ w/src/containers/refresh/Refresh.js
@@ -39,6 +39,8 @@ const AccountProps = styled.dl`
   grid-template-columns: auto 1fr;
   margin: 0;
   padding: .5rem;
+  font-family: Consolas,monospace;
+  font-size: 1rem;
 `;
 
 const AccountPropsKey = styled.dt`

dgreene-r7 · 2018-08-06T14:33:07Z

api/routes/auth.js

+      roleAttr = [roleAttr];
+    }
+
+    let roles = roleAttr.map((attr, i) => {


This should be a const. let is mutable.

Looking a little further down you could even just set it to session.roles and key off that value when you check the length.

dgreene-r7 · 2018-08-06T14:33:18Z

api/routes/auth.js

+    }
+
+    let roles = roleAttr.map((attr, i) => {
+      let arns = attr.split(',');


const as well.

Do we want to gracefully handle a case in which the value from the https://aws.amazon.com/SAML/Attributes/Role attribute isn't formatted like:

arn:aws:iam:role1,arn:aws:iam:provider arn:aws:iam:role2,arn:aws:iam:provider arn:aws:iam:role3,arn:aws:iam:provider

If so, do you think it makes sense to introduce some sort of arn parsing mechanism so that you can definitively know if any value in that attribute is formatted correctly and, if not, throw it away?

dgreene-r7 · 2018-08-06T14:35:10Z

api/routes/auth.js

+      let arns = attr.split(',');
+
+      return {
+        accountId: arns[0].split(':')[4],


This look pretty brittle. Any way you can add some validation to make sure it doesn't return undefined if the arns value isn't structured correctly?

dgreene-r7 · 2018-08-06T14:35:22Z

api/routes/auth.js

+        index: i,
+        principalArn: arns[1],
+        roleArn: arns[0],
+        roleName: arns[0].split(':')[5].replace('role/', ''),


Same as above re: brittle-ness.

As a suggestion, consider something like

const [roleArn, principalArn] = arns; const roleArnArray = roleArn.split(':'); const accountId = roleArnArray[4]; const roleName = roleArnArray[5]; return { accountId, index: i, principalArn, roleArn, roleName, };

dgreene-r7 · 2018-08-06T14:35:57Z

api/routes/auth.js

+      // the latest roles from the current SAML assertion.  If it
+      // doesn't match, wipe it from the session.
+      if (session.roleArn && session.principalArn) {
+        let found = roles.find((role) =>


dgreene-r7 · 2018-08-06T18:09:04Z

src/containers/refresh/Refresh.js

-                    <pre className={getLang(platform)}>
-                      {getEnvVars(this.props)}
-                    </pre>
+                    <InputGroupWithCopyButton


Love that you got the copy button working here but is there a way to maintain the pre formatting?

What formatting are you looking for. Are you actually looking for wrapping this text in <pre></pre>? Or are you looking for dark background and monospace font?

The current copy/paste stuff works with <textarea>, not <pre> so I don't think I can (easily) switch to pre. But I can get the dark background and monospace font if that is the goal.

dgreene-r7 · 2018-08-06T18:10:58Z

src/containers/refresh/Refresh.js

-                      <pre className="card-text language-markup">
-                        <code>{accountId}</code>
-                      </pre>
+                      <AccountProps className="bg-dark text-light">


Is there a way to maintain the pre formatting here?

Not technically with <pre> since I'm now laying this out with a grid layout. Personally I prefer non-monospaced font here since this isn't code or anything like that. I'm leaving as is. Hope that is ok.

dgreene-r7 · 2018-08-06T18:12:26Z

src/containers/select-role/Role.js

+  submitSelectRole: bindActionCreators(submitSelectRole, dispatch),
+});
+
+export const Role = connect(mapStateToProps, mapDispatchToProps)(RoleComponent);


If you don't need mapStateToProps you can just pass null to the connect function instead of a function that does nothing.

dgreene-r7 · 2018-08-08T14:52:57Z

api/routes/select-role.js

+  router.get('/', (req, res) => {
+    const session = req.session.passport;
+
+    if (session === undefined) {


This can be if (!session) {

dgreene-r7 · 2018-08-08T14:54:38Z

src/apis/index.js


 const axiosClient = axios.create({
-  baseURL: ENDPOINTS.electron,


Will this still work correctly without the baseURL option pointing to the backend? If I remember correctly I had some issues with it looking like it worked when testing but it failing using a built version.

In a built version, everything is port 2600 so it works. I've tested this.

I may be doing it wrong, but in a dev environment I run these two commands in separate terminals:

yarn electron-dev (runs on port 2600)

yarn react-start (runs on port 3000, and proxies non-react stuff to port 2600)

In this case, everything should go through port 3000. Previously, some stuff was going to port 2600 and some to port 3000 and this messed up some CORS stuff (I think).

Definitely worth a shot. If we build and release and see bugs here we can always roll this back.

This uses the new profile UUID when editing profile names instead of just the metadata URL.

req.body.index can be 0, so we need to explictly check for undefined.

dgreene-r7

Looks good. My feedback is mostly stylistic at this point and revolves around using forEach vs map.

dgreene-r7 · 2018-08-08T16:52:17Z

api/routes/configure.js

-      if (profileName && storedMetadataUrl.url === metadataUrl && storedMetadataUrl.name !== profileName) {
-        storedMetadataUrl.name = profileName;
+      if (profile.url !== metadataUrl) {
+        return res.status(422).json(Object.assign({}, ResponseObj, {


Do we want to return an error here or update the profile.url?

My thoughts are this should be an impossible state to get to. When you press the "Login" button now, it submits the metadata URL and the UUID for the profile. The metadata url box is not editable in the current UI. If the submitted metadata URL doesn't match the stored metadata URL (found using the uuid) then I assume corrupt input and throw an error.

If we ever update the UI to allow changing the metadata URL for existing entries, we can change this.

dgreene-r7 · 2018-08-08T17:26:53Z

api/routes/refresh.js


-        return storedMetadataUrl;
+      // Update the stored profile with account number(s) and profile names
+      metadataUrls.forEach((metadata) => {


I'm not a huge fan of using forEach as it tends to lead to mutation and side-effects. However, I recognize that's a stylistic issue and just something I want to call out here for future reference.

dgreene-r7 · 2018-08-08T17:27:59Z

src/apis/index.js


 const axiosClient = axios.create({
-  baseURL: ENDPOINTS.electron,


Definitely worth a shot. If we build and release and see bugs here we can always roll this back.

bturner-r7 added 6 commits August 5, 2018 01:12

Add a copy button for the terminal copy/pasta

77da67e

Format credentials in a grid

805c6f5

Remove the ENDPOINTS constant

74290e1

This appears to not be needed and gets in the way when using react dev mode.

Fix typo in development environment command

70d2dad

Support multiple roles via SAML assertion

5745ed3

This adds a role switcher UI when multiple roles,saml-provider pairs are included in the SAML assertion. If only one role is present in the SAML assertion, the switcher is not displayed. GH-105

bturner-r7 requested review from udangel-r7 and dgreene-r7 August 5, 2018 06:34

Fix linting errors

67fe640

bturner-r7 commented Aug 5, 2018

View reviewed changes

bturner-r7 added 2 commits August 5, 2018 23:44

Add profile name to refresh page

b38e609

To avoid clutter, the profile name is only displayed if it has been customized.

Clean up the grid layout html

c258a9b

erran-r7 approved these changes Aug 6, 2018

View reviewed changes

bturner-r7 added 2 commits August 7, 2018 09:39

Merge remote-tracking branch 'origin/master' into multiple-role-support

b985e1e

Add README notes about multiple role support

4cf9834

dgreene-r7 requested changes Aug 8, 2018

View reviewed changes

bturner-r7 added 7 commits August 8, 2018 10:04

Use the profile UUID when editing profile names

61f902b

This uses the new profile UUID when editing profile names instead of just the metadata URL.

Add support for searching by account number or role

68cadb0

Address code review comments

0d9489d

Use dark backgrounds for terminal copy/pasta

a703da9

Fix linting

7816392

Revert a code review change

d2d1b52

req.body.index can be 0, so we need to explictly check for undefined.

Address more code review comments

b822f09

dgreene-r7 approved these changes Aug 8, 2018

View reviewed changes

bturner-r7 added 3 commits August 8, 2018 12:51

Switch from forEach to map since we are altering the array

6c2d9e2

Fix invalid syntax

059af19

Fix invalid syntax

bff6321

bturner-r7 merged commit 3dc5ed9 into master Aug 8, 2018

bturner-r7 deleted the multiple-role-support branch August 8, 2018 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multiple role support #119

Add multiple role support #119

bturner-r7 commented Aug 5, 2018

coveralls commented Aug 5, 2018 •

edited

Loading

bturner-r7 Aug 5, 2018

erran-r7 Aug 6, 2018

erran-r7 Aug 6, 2018

bturner-r7 Aug 6, 2018

erran-r7 Aug 6, 2018

dgreene-r7 left a comment

dgreene-r7 Aug 6, 2018

dgreene-r7 Aug 8, 2018

dgreene-r7 Aug 6, 2018

dgreene-r7 Aug 8, 2018

dgreene-r7 Aug 6, 2018

dgreene-r7 Aug 6, 2018

dgreene-r7 Aug 6, 2018

dgreene-r7 Aug 6, 2018

bturner-r7 Aug 8, 2018

dgreene-r7 Aug 6, 2018

bturner-r7 Aug 8, 2018

dgreene-r7 Aug 6, 2018

dgreene-r7 Aug 8, 2018

dgreene-r7 Aug 8, 2018

bturner-r7 Aug 8, 2018

dgreene-r7 Aug 8, 2018

dgreene-r7 left a comment

dgreene-r7 Aug 8, 2018

bturner-r7 Aug 8, 2018 •

edited

Loading

dgreene-r7 Aug 8, 2018

dgreene-r7 Aug 8, 2018


		const axiosClient = axios.create({
		baseURL: ENDPOINTS.electron,

Add multiple role support #119

Add multiple role support #119

Conversation

bturner-r7 commented Aug 5, 2018

coveralls commented Aug 5, 2018 • edited Loading

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

dgreene-r7 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgreene-r7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bturner-r7 Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 5, 2018 •

edited

Loading

bturner-r7 Aug 8, 2018 •

edited

Loading