simplesamlphp / SAML-tracer

Browser extension for examining SAML messages
https://addons.mozilla.org/nl/firefox/addon/saml-tracer/
BSD 2-Clause "Simplified" License
141 stars 39 forks source link

New tab with exrtacted params #58

Closed amnemonic closed 5 years ago

amnemonic commented 5 years ago

New feature - new tab with essential parameters of AuthnRequest, LogoutRequest and Response. It is very useful for me so I though why not to create pull request.

Preview of functionality: screenshot

khlr commented 5 years ago

Hi Adam,

it's really cool, that you opened this pull request and contributed to SAML-tracer! I think this feature may come in handy for some folks.

On the other hand, I like examining "the whole" token because in something like a summary there's probably missing that single detail I'm interesed in. But that's just my personal opinion ;-) What do you think, @jaimeperez, @thijskh and @tvdijen ?

If this PR should be merged, I'd propose an adjustments:

I'd not distinguish between a Request- and a Response-tab. Why not just have a Summary-tab (or Details-tab or something like that)? I think that would simplify the code since the functions and properties for the distinction between request and response wouldn't be necessary. And no information would be lost; you could still write a "headline" (Response, AuthnRequest, LogoutRequest) in the content-div.

A side-effect of this would be, that you wouldn't need a showResponse and a showRequest function. This could be a single (e.g.) showSummary function. This could probably avoid some code duplications, too.

I'll leave some comments on the code in minute to clarify my thoughts. Hope this'll help ;-)

Thank you & best regards, Jan

khlr commented 5 years ago

If there are multiple claims of the same type within an AttributeStatement, they'll be concatenated whereas one would expect them to be listed separately.

<saml2:AttributeStatement>
  <saml2:Attribute Name="http://schemas.xmlsoap.org/ws/2005/05/identity/claims/country">
    <saml2:AttributeValue>no</saml2:AttributeValue>
    <saml2:AttributeValue>es</saml2:AttributeValue>
    <saml2:AttributeValue>nl</saml2:AttributeValue>
    <saml2:AttributeValue>de</saml2:AttributeValue>
  </saml2:Attribute>
  <saml2:Attribute Name="http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname">
    <saml2:AttributeValue>John</saml2:AttributeValue>
  </saml2:Attribute>
  <saml2:Attribute Name="http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname">
    <saml2:AttributeValue>Doe</saml2:AttributeValue>
  </saml2:Attribute>
</saml2:AttributeStatement>

leads to this:

~ Attributes ~ 
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/country = noesnlde 
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname = John 
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname = Doe

whereas I'd expect:

~ Attributes ~ 
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/country = no 
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/country = es 
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/country = nl 
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/country = de 
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname = John 
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname = Doe

One more time I used Anders Abels Stub-IdP (https://stubidp.sustainsys.com/) for creating the SAML-Response. Maybe this is useful for your testing purposes, too.

khlr commented 5 years ago

I really like this 😊👍

Could it be you accidently dropped some keys? E.g. Audience vanished and Destination will only be printed for AuthnRequests but not for Responses.


A minor suggestion: If you add a function like the following, you could ease the additions to samlSummary:

const writeLine = (key, value) => {
  if (value) {
    samlSummary += key.padEnd(70, " ") + " = " + value + "\n";
  } else {
    samlSummary += key + "\n";
  }
};

Use it like:

writeLine("Destination", destination.value);
…
writeLine("Subject", xmldoc.getElementsByTagNameNS('*','Subject')[0].textContent);
…
writeLine("AttributeStatement:");

Just an idea 😉


Good thing, that you fixed the multiple value issue as well. You solved this by comma-joining the values. This is fine but can easily lead to avoidable line breaks if you have to handle large values:

image

What about listing them separately? One could additionally use an asterisk for it:

image

for (let i = 0; i < attrStatement.length; i++) {
  let attribute = attrStatement[i];
  let attributeName = attribute.getAttribute('Name');
  for (let j = 0; j < attribute.childNodes.length; j++) {
    writeLine(" * " + attributeName, attribute.childNodes[j].textContent);
  }
}

You can use this test-IdP if you like: https://stubidp.sustainsys.com/d6c8b68a-3c2b-4966-85ff-52be55fb83be

khlr commented 5 years ago

Yep, it'd be great if the output looked a bit nicer. This is the current output:

Issuer                                                                = https://stubidp.sustainsys.com/d6c8b68a-3c2b-4966-85ff-52be55fb83be/Metadata
Subject                                                               = JohnDoe

AttributeStatement:
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname       = John
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname         = Doe
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/country         = es
                                                                      = nl
http://schemas.xmlsoap.org/ws/2005/05/identity/claims/webpage         = https://github.com/Uninett/SAML-tracer/
                                                                      = https://stubidp.sustainsys.com/
http://schemas.foo.bar/veeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeerylooooooooooongclaimtype= baz
NameID  

How about this?

Issuer              = https://stubidp.sustainsys.com/d6c8b68a-3c2b-4966-85ff-52be55fb83be/Metadata
Subject             = JohnDoe
NameID              = JohnDoe

AttributeStatement:
* http://schemas.xmlsoap.org/ws/2005/05/identity/claims/givenname     = John
* http://schemas.xmlsoap.org/ws/2005/05/identity/claims/surname       = Doe
* http://schemas.xmlsoap.org/ws/2005/05/identity/claims/country       = es
* http://schemas.xmlsoap.org/ws/2005/05/identity/claims/country       = nl
* http://schemas.xmlsoap.org/ws/2005/05/identity/claims/webpage       = https://github.com/Uninett/SAML-tracer/
* http://schemas.xmlsoap.org/ws/2005/05/identity/claims/webpage       = https://stubidp.sustainsys.com/
* http://schemas.foo.bar/veeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeerylooooooooooongclaimtype = baz

Differences:

khlr commented 5 years ago

Hello Adam!

Sorry for my long absence. I've been on vacation 🏝 I think everything works as expected now. 😊👍

I'll leave just some small comments directly at the code. Then this PR could be merged from my point of view.

amnemonic commented 5 years ago

Hello Adam!

Sorry for my long absence. I've been on vacation desert_island I think everything works as expected now. blush+1

I'll leave just some small comments directly at the code. Then this PR could be merged from my point of view.

Sure... I made some updates :)

jaimeperez commented 5 years ago

This is great, thanks a lot for your contribution @amnemonic!

I merged it now, although I think we could definitely improve the look of the new tab, since right now it looks very much like a plaintext file. I have no specific suggestions for improvement, though. @khlr maybe you do?

khlr commented 5 years ago

I think we could definitely improve the look of the new tab

Adam had that in mind, too 😉

Technically isn't bad but it isn't visually pleasing for me 👀

My opinion is: It's fine as it is. Sure, it could look more fancy, but the main purpose of that tab is to provide some key facts. No matter if they're styled or not 😉