[WIP] Allow assuming all roles offered by IDP, regardless of account.

[bgshacklett]

Uses AssumeRoleWithSAML for all roles rather than just the main role.

I know we spoke about this being possible to disable, but I feel that I was able to keep the impact of the change down to a minimum and I don't believe this is likely to negatively impact users in any way. Please let me know what you think.

P.S.: Sorry for the wait; life got in the way a bit.

[bgshacklett]

Sorry to bother, but have you had any time to take a look? Do you have any concerns with how the roles are being assumed here?

Thanks!

[prolane]

Hi Brian Apologies for not coming back to you earlier. I was on holiday when you sent the PR, so I thought I'll have a look once I get back. But I simply forgot. So thanks for reminding me.

I did have a look now at the code and I'm not sure if I understand what your goal is? Or perhaps a better way to phrase it is I guess I misunderstood what you are trying achieve.

I see you changed the AssumeRole API action to the assumeRoleWithSAML API action for the additional roles configured in the options panel. But still, there will only be additional profiles in the credentials file when you configured additional roles in the options panel.

To be honest, I thought you meant you wanted to implement something that would check all the roles in the SAML payload, and would do the assumeRoleWithSAML action for every single role.

Unfortunately this PR is not something I can merge because it fundamentally changes the way how other users are using the additional roles option. With this PR users are now required to have set up SAML integration for the additional roles configured in the options panel. While where it was meant for (and how it currently works) is for users to sign in with SAML to one role in one AWS account which becomes the default profile. These users consider this one particular AWS account to be their master IAM account. The only AWS account where people are allowed to sign in. However, from this account they have setup cross account access. So with their credentials they get from assuming this role in this one AWS account, they can assume another role from another AWS account. And this is why this additional role panel is there in the options.

AWS used to promote this kind of setup as one of their best practices for IAM and securing signing in to your AWS environment. So I can understand why people wanted to see this in the Chrome extension.

Let me know if I somehow misunderstood what you want to do and then we'll try to figure it out together.

[bgshacklett]

Interesting. It sounds like there are two fundamentally different use-cases and I misunderstood the intended behavior of the specified list of ARNs. I think I have a much better understanding of why things are currently implemented as they are and I should be able to make a change to satisfy both.

To be honest, I thought you meant you wanted to implement something that would check all the roles in the SAML payload, and would do the assumeRoleWithSAML action for every single role.

That sounds like a very reasonable way of going about it. I'll work towards that as a solution. I think that's still within the scope of this PR so, if you agree, I'll update the commits here rather than cluttering up the issue history with another PR.

Thanks for getting back to me. I hope you had a great vacation!

[prolane]

Yeah its good that you mention it. When I have the time I'll update the documentation to make the intended behaviour more clear. And perhaps also a tooltip inside the options panel.

Okay cool! Sure, if you still feel you're up for the challenge, you're very welcome to contribute. And yes, no problem to keep using this PR.

I did had a great vacation, thanks for asking :-)

[bgshacklett]

Short update: I've got a bunch of people using samlogin now, so the functions in that tool are being tested pretty well and I'm finding a number of things which could stand to be improved.

In the meantime, I'm working on CI/CD tests/builds based on two things: 1. Your earlier comment that refactoring is on your list of TODOs; having this in place should make it significantly easier to use libraries from the NPM repository without much extra added effort during the development, build or release processes. 2. I'm using a libary, now, in samlogin that handles a lot of the parsing of SAML responses. It would be beneficial to be able to use that library for this extension as well. It would certainly be possible to just copy the code to a lib directory, but I feel that managing dependencies with NPM or Yarn will be beneficial in the long run.

I'd love to get your thoughts on item 2 in particular. The lib in question is at https://www.npmjs.com/package/libsaml.

[prolane]

Hi Brian, I see you are already actively working on this, so I needed to make some time to reply.

I don't want to discourage all your great efforts, but I do have some thought of my own for these particular topics.

CI/ID is great and it has been on my mind in the past. But frankly for completely different reasons. When I think about what is costing me the most time to maintain this extension, its the changing API's in Chrome. So what I once thought about is to create an automated pipeline for testing mainly. The pipeline would launch the latest beta release of Chromium with the extension, launch a docker container with an open source SAML provider (like shibboleth), and run some of the common use patterns. This would make me able to detect issues with the extension in a early stage, before users will actually notice it.

Building the extension is not costing me time at all. Its so simple I don't need to build. Even better, while coding the extension, all I have to do to see the changes in Chrome is reload the extension.

I appreciate your effort, but I'd like to believe we should only spend our time in things which makes our lives easier. I think for me adding build tools is only creating unnecessary overhead for something really simple. Feel free to convince me otherwise of course :-)

As for your second point: I'm not against using libraries. That would be really strange if I was. But I do like to keep it to a minimum. So only for those cases where it very clearly saves me coding dozens of extra lines. Obviously for jQuery and the AWS SDK this is the case. I'm not sure yet what the advantage would be of using the libsaml library, so please help me out here. The way I see it, once you have the saml document as string, it isn't that much effort to get an attribute value.

Sure, you need this one off:

  parser = new DOMParser()
  domDoc = parser.parseFromString(samlXmlDoc, "text/xml");

But after that, its really just simple one liner:

// Get a list of claims (= AWS roles) from the SAML assertion
var roleDomNodes = domDoc.querySelectorAll('[Name="https://aws.amazon.com/SAML/Attributes/Role"]')[0].childNodes

Don't get me wrong. I really appreciate the efforts you are putting into this in making the extension even better. But I'm also thinking about what it means for me, next time there is another issue with the extension because of a changed API in Chrome (or something else). As this extension is just one of my many hobby projects I don't like to be dependent on another library when I don't need to. I'm sure you know how these things work out in reality. The more complexity and the more dependencies, the harder it is to debug when shit hits the fan.

Just being honest here with you. Let me know your thoughts.

[bgshacklett]

I totally understand the concerns. I'll post more to explain my reasoning for the decisions I've made (including how they might benefit your goal of automated testing) shortly. For now, though, I'm calling it a night with the new-found understanding that I really don't care for jQuery. :-)

[bgshacklett]

Indeed, you're absolutely right about the libsaml. It's been a good amount of time since I began working on samlogin, so I completely forgot that I used that because querySelectorAll(...) doesn't exist in Node.js! So, I appreciate the push-back, as you're most certainly right about not needing that library in the browser.

As for the build process, I understand that you want to automate testing in chromium. I think Mocha and Puppeteer could help with that quite a bit. Both of these tools run in the Node.js/commonjs environment, which isn't directly compatible with the browser. That's really where builds are coming in at this point. If you take a look at gulpfile.js there's a step called browserify which essentially embeds scripts which are called by require(...). I suspect this will come into play for automated testing against chromium, but it probably isn't warranted for this particular PR. Let me see what I can do to slim things down a bit and maybe I can start experimenting on testing against Chromium in another branch after that.