Describe the SAML Response in greater detail #497

vrajmohan · 2024-09-19T17:41:47Z

The existing documentation for the SAML Authentication response
(https://developers.login.gov/saml/authentication/#authentication-response)
is low on detail - it just states that the response contains encrypted
data. The example provided is for the encrypted response
and does not help in understanding the payload.

A question asked in Slack
could not be easily answered without digging through the actual code and tests.

This change:

provides a description of the actual data elements returned
adds an example of the decrypted response

zachmargolis · 2024-09-19T18:32:25Z

_pages/saml/authentication.md

+        <p>{{ response_elements | markdownify }}</p>
+        <p>For example, {{ decrypted_response | markdownify }}</p>


markdown inside HTML is not automatic but you can do:

Suggested change

<p>{{ response_elements | markdownify }}</p>

<p>For example, {{ decrypted_response | markdownify }}</p>

<div markdown="1">

## markdown content here

</div>

Thank you, that was what I was looking for!

vrajmohan · 2024-09-20T15:44:54Z

Struggled with deciding between Markdown and HTML, open to suggestions.

ajfarkas · 2024-09-20T17:32:24Z

_includes/snippets/saml/auth/response_example.md

-                Consent="urn:oasis:names:tc:SAML:2.0:consent:unspecified"
-                InResponseTo="_6fca7b78-9ab7-49f5-bd62-18c48eac3c68"
-                xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol">
+<samlp:Response xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" ID="_7f3d8cd9-d3f8-4b47-a571-5272810d5073" Version="2.0" IssueInstant="2024-09-18T16:20:36Z" Destination="https://sp.int.identitysandbox.gov/auth/saml/callback" Consent="urn:oasis:names:tc:SAML:2.0:consent:unspecified" InResponseTo="_bf054c05-5b2c-4773-a6a9-9ba075a87bc9">


[Question] Are the Response attributes intentionally placed on a single line? Is there a benefit over the existing layout?

It wasn't intentional. It is what SAML Tool's XML formatter gave me. It does substitute vertical scrolling (which I prefer) for horizontal scrolling. Do we have a preference?

Good question, I only noticed that it was a change. I don't think either way is particularly readable.

In my opinion, the horizontal scrolling is better as it allows you to understand the structure from the "pretty-printed" indentation. Would anyone else like to weigh in with their opinions?

Note: Existing content in https://developers.login.gov/saml/authentication/#example-specifying-ial-aal-and-attributes follows horizontal scrolling.

ajfarkas · 2024-09-20T17:35:20Z

_includes/snippets/saml/auth/response_example.md

+              <ds:X509Certificate>
+MIIDgDCCAmgCCQCwpieA9CKuDDANBgkqhkiG9w0BAQUFADCBgTEYMBYGA1UEAwwP
+U1AgU2luYXRyYSBEZW1vMQwwCgYDVQQKDANHU0ExDDAKBgNVBAsMAzE4ZjETMBEG
+<!-- X509Certificate elided -->


[Request] Could you use a more common word than "elide"?

"abridged", "truncated", "clipped", "hidden". I think elide isn't a commonly used word: for instance, this is not how I use it (whether or not I use it correctly).

Thanks, I'll go with truncated.

Correction, abridged is more accurate.

ajfarkas · 2024-09-20T17:40:02Z

_pages/saml/authentication.md

+    <ds:SignatureValue>XT9CguQWKBvbqVsJ+Khu5/eyl09JVhHkUuyFHa98ViZUBVgL/Hc9gzwUr43CA7OVOO+uMfCc6WvPKeADF9w9kqJaUgsi8LiKC/nfDCY6+UiRoep2zmXyFJRAvrD/HbgVfayx/4Nn3ponRPZ/T/oezhimssFF66m+/UAwJekO9kuob+5n+uaOiFOMuHEycSdASH/iFnTSR1ajdo6AaLomG6YT8zJbuRzcKmesouAKPiQCJFt2cgstEs1zw8dvTgmozy4qd/0aMiZ52eGcXoORD8VZOQiY63HT8F4wkhk5eGU05sFcyfpg7dXNtKOfCddHwyngmgmPhpRN30ew5njg7w==</ds:SignatureValue>
+    <ds:KeyInfo>
+    <ds:X509Data>
+        <ds:X509Certificate>MIID+TCCAuGgAwIBAgIUUS6s9Rb+KY0fT0qKKgqPPJij/HMwDQYJKoZIhvcNAQELBQAwgYsxCzAJBgNVBAYTAlVTMR0wGwYDVQQIDBREaXN0cmljdCBvZiBDb2x1bWJpYTETMBEGA1UEBwwKV2FzaGluZ3RvbjEMMAoGA1UECgwDR1NBMRIwEAYDVQQLDAlMb2dpbi5nb3YxJjAkBgNVBAMMHWxvZ2luLmdvdi5pZGVudGl0eXNhbmRib3guZ292MB4XDTI0MDEyMjIwMTcwN1oXDTI1MDQwMTIwMTcwN1owgYsxCzAJBgNVBAYTAlVTMR0wGwYDVQQIDBREaXN0cmljdCBvZiBDb2x1bWJpYTETMBEGA1UEBwwKV2FzaGluZ3RvbjEMMAoGA1UECgwDR1NBMRIwEAYDVQQLDAlMb2dpbi5nb3YxJjAkBgNVBAMMHWxvZ2luLmdvdi5pZGVudGl0eXNhbmRib3guZ292MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAhmcFFn4b56vHlGBQ1Lx6AXz17sqKnCc6sJ+9csP1RtQBI0NpPHB2z9Di1PNk/ElK7V7yh3uMu4FJYw30GZFUl2f/ttsDkNHrwfh/jzbMNjrOSc0P25oem4uOUfeGH9jtMhKa+HZLOaOmcyWFKkYR2mwacEbQJ1CWviHtP8AzHUPSbHklAmusRLuygTjq0+QRJZgSezGqwU1L3ixPq+gMzPtMS+fxsMOVo2eosip440gz4rcqUUogtD2hV8EQi3+GIkGYuMTS81ug/385TCPEhzWMnNmDi3HykOZeRNb4GfCYw0Yx+v+cb7BPD5EdxUHNwliHvSiRAeYqLjBjuNUfKQIDAQABo1MwUTAdBgNVHQ4EFgQUusictYnNM2TbIt5STz2lkYN1sI8wHwYDVR0jBBgwFoAUusictYnNM2TbIt5STz2lkYN1sI8wDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAQEATuLF4kHeP7FY9Wzm3DfF+m/5wUhJEtbsF8J9Wq8duhQ4/gtZVJgMDUKLsnSDLCtWiRlsFXquI8tlo32JsVo5NfZI9WYsub7192iCYpqE+x5G+94tt5vAayoF7GKGPxatyldxAQUz7RUzwqas7NCYXQ0p7wZrMqF8z2yvaUgL55v8TJIb7RP+D8b47Cmzx7IYmx3Co30vZWysQe61Bv880hG11YJsBAc0hmyWlokJYZZVm+xcjKkm6aFyyAbeCe0Kh68QU7f9YkpFv/sW2RIvZ/Z0gvxjJE+YJBwOwPDDHdkb0ZmKOJvlaabi5lkTZvUtTHXb5Hu7DxRRt91dm77MlQ==</ds:X509Certificate>


[Nitpick] <ds:X509Certificate> open and close tags could be moved to different lines from the text contents (add linebreaks).

This would make it consistent with response_example.md (I'm also not sure how this affects syntax highlighting)

I'll take a crack at it.

ajfarkas · 2024-09-20T17:44:26Z

_pages/saml/authentication.md

+        <p markdown="1">After the user authenticates, Login.gov will redirect and POST a form back to your registered Assertion Consumer Service URL with a hidden form control named `SAMLResponse`.</p>
+        <p markdown="1">`SAMLResponse` contains a base64-encoded XML payload that contains data that is encrypted with the service provider's public key.</p>
+        <p markdown="1"> The decrypted `SAMLResponse` contains a `<saml:Assertion>` element, which in turn contains the following elements: </p>
+        <dl>


[Praise] Description List in the wild! Thanks for using semantic HTML

I'm still unhappy about mixing Markdown and HTML.

I find CMSes to be consistently bizarre in their authoring choices. The ones I've seen that are very straightforward for the author tend to be shockingly expensive. (or, I guess, we could just host a wiki)

Sgtpluck · 2024-09-24T16:37:51Z

_pages/saml/authentication.md

+            <dt markdown="1">`AuthnStatement`</dt>
+            <dd>Contains the AAL that was used.</dd>
+        </dl>
+        <p>For example, {{ decrypted_response | markdownify }}</p>


Suggested change

<p>For example, {{ decrypted_response | markdownify }}</p>

<p>For example: {{ decrypted_response | markdownify }}</p>

i am wondering if it is more useful to have this example in the example tab than what is currently there. (although to be honest i can't see someone spending a lot of time looking at either, since as noted above by aj, it's pretty unreadable in its current state)

I thought about that and felt that it was also useful to retain an example of the encrypted response.

i think maybe it would make more sense than as adding another tab to the thing on the right - it doesn't really make sense to have an XML example in the body and in the tab. we could have Encrypted Response and Decrypted Response.

Unfortunately, that's a bigger lift. There's a lot of JavaScript that assumes that the "code" section has only 2 tabs.

then i think we should pause on this -- it'll need a UX/content review, and i hesitate to add that to ursula's plate.

Sgtpluck · 2024-09-26T13:53:04Z

@vrajmohan please add more details in the opening comment about what this change is, what problem it's solving/why it's needed

mmagsa · 2024-09-26T14:13:22Z

Please add more detail as suggested by @Sgtpluck. Since this is a relatively significant change, we'll need UX to take a look. Once there's enough context in the PR description, I'll add a ticket to our backlog.

The existing documentation for the SAML Authentication response (https://developers.login.gov/saml/authentication/#authentication-response) is low on detail - it just states that the response contains encrypted data. The example provided is for the _encrypted_ response and does not help in understanding the payload. This change attempts to: - provide a description of the actual data elements returned - adds an example of the decrypted response

Introduces horizontal scrolling

Sgtpluck · 2024-09-27T12:35:36Z

@vrajmohan sorry, to be clearer, please update the initial PR comment where instead of describing the ticket you wrote:

Still figuring out why I can't simply use markdown and need to assign complex fragments using {% capture %} and render with markdownify

When you open a PR, the description needs to clear to anyone looking at this PR, not just folks who know to look at the commits. Thanks!

vrajmohan marked this pull request as draft September 19, 2024 17:42

vrajmohan requested a review from nprimak September 19, 2024 17:42

zachmargolis reviewed Sep 19, 2024

View reviewed changes

vrajmohan requested review from Jeremy1026 and ajfarkas September 20, 2024 15:45

vrajmohan force-pushed the vm-update-saml-response branch from 01d662d to b25f87c Compare September 20, 2024 15:47

vrajmohan marked this pull request as ready for review September 20, 2024 17:00

vrajmohan changed the title ~~[WIP] Describe the SAML Response in greater detail~~ Describe the SAML Response in greater detail Sep 20, 2024

ajfarkas reviewed Sep 20, 2024

View reviewed changes

vrajmohan force-pushed the vm-update-saml-response branch from 88a2ab1 to 02cf5de Compare September 20, 2024 21:14

vrajmohan requested review from ajfarkas and Sgtpluck September 20, 2024 21:14

Sgtpluck reviewed Sep 24, 2024

View reviewed changes

vrajmohan force-pushed the vm-update-saml-response branch from 02cf5de to 431b9b4 Compare September 24, 2024 17:03

vrajmohan added 3 commits September 26, 2024 07:38

Use abridged instead of elided

87b0cc7

Try to retain XML formatting

1851500

Introduces horizontal scrolling

vrajmohan force-pushed the vm-update-saml-response branch from 431b9b4 to 1851500 Compare September 26, 2024 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Describe the SAML Response in greater detail #497

Describe the SAML Response in greater detail #497

vrajmohan commented Sep 19, 2024 •

edited

Loading

zachmargolis Sep 19, 2024

vrajmohan Sep 19, 2024

vrajmohan commented Sep 20, 2024

ajfarkas Sep 20, 2024

vrajmohan Sep 20, 2024

ajfarkas Sep 23, 2024

vrajmohan Sep 23, 2024

ajfarkas Sep 20, 2024

vrajmohan Sep 20, 2024

vrajmohan Sep 20, 2024

ajfarkas Sep 20, 2024

vrajmohan Sep 20, 2024

ajfarkas Sep 20, 2024

vrajmohan Sep 20, 2024

ajfarkas Sep 23, 2024

Sgtpluck Sep 24, 2024

Sgtpluck Sep 24, 2024

vrajmohan Sep 24, 2024

Sgtpluck Sep 24, 2024

vrajmohan Sep 25, 2024

Sgtpluck Sep 26, 2024

Sgtpluck commented Sep 26, 2024

mmagsa commented Sep 26, 2024

Sgtpluck commented Sep 27, 2024

		<p>{{ response_elements \| markdownify }}</p>
		<p>For example, {{ decrypted_response \| markdownify }}</p>

	<p>For example, {{ decrypted_response \| markdownify }}</p>
	<p>For example: {{ decrypted_response \| markdownify }}</p>

Describe the SAML Response in greater detail #497

Are you sure you want to change the base?

Describe the SAML Response in greater detail #497

Conversation

vrajmohan commented Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrajmohan commented Sep 20, 2024

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

Choose a reason for hiding this comment

Sgtpluck commented Sep 26, 2024

mmagsa commented Sep 26, 2024

Sgtpluck commented Sep 27, 2024

vrajmohan commented Sep 19, 2024 •

edited

Loading