Closed Charlotte-Holt closed 10 months ago
Bruce's comments, The preferred approach to configure SAML is by using metadata exchange as follows: Supply the above URL to the saml identity provider to establish federation between this sp and Identity provider. If the identity provider requires a file instead of a url, use a browser to retrieve the data from that URL and supply the file. Next, obtain the metadata file from the provider, rename it to idpMetadata.xml, and copy it to the resources/security directory on the server. The issuer…. After the example of enabled=false, there are a bunch of elements that aren’t part of server.xml, that can all be removed. You can mention something like Additional attributes can be optionally specified to override the default behavior. In most cases, exchange of the metadata files is sufficient.
Bruce's comment "Hi, that looks fine, my only suggestion is to change SAML Identity Provider --> SAML Identity Provider (IDP) IP --> IDP"
looks good!
@lauracowen Hi Laura. Can you review this draft https://draft-openlibertyio.mybluemix.net/docs/ref/feature/#samlWeb-2.0.html?
samlWebSso20
element, we should say that and that actually all that you need to do to configure SAML (at the simplest config, anyway) is to enable the feature. I think that this is the case. In which case, it might be better worded as "When you enable the SAML Web Single Sign-On feature in Open Liberty, the following configuration is automatically generated:". (If that's not the case, it needs to be clearer that the user needs to add the simple line of configuration shown in the example.<samlWebSso20 id="defaultSP" />
(do this for both examples). Aside from being tidier, having this first example take up three lines makes it look like there should be more to it than there is.valuexxx1
and xxxx2 to valuexxx2
. The hostname
value is obtained from...xxx and the sslport
value is obtained from."samlWebSso20
you can see what config attributes are valid on that element). I don't think they're valid attributes either, eg samlsp
doesn't seem to exist for the samlWebSso20
.Some additional comments Alasdair made last week:
@lauracowen I worked with Bruce in a meeting yesterday and discussed all the points you made and addressed them based on his suggestions. Can you review the draft https://draft-openlibertyio.mybluemix.net/docs/ref/feature/#samlWeb-2.0.html?
[x] When introducing an example, focus on what it provides rather than just describing the elements. So saying that the element is configured by default doesn't tell the user anything. The thing that's important to the user is what that provides. (Also, technically the element itself isn't really configured; it is the configuration.) So introduce it with something more like "SAML Web Single Sign-On is configured by default in Open Liberty:"
[x] However, that sentence is ambiguous and doesn't really make sense because if SAML is configured by default, why do you need to put some configuration in the server.xml? If Liberty automatically generates that line with the samlWebSso20 element, we should say that and that actually all that you need to do to configure SAML (at the simplest config, anyway) is to enable the feature. I think that this is the case. In which case, it might be better worded as "When you enable the SAML Web Single Sign-On feature in Open Liberty, the following configuration is automatically generated:". (If that's not the case, it needs to be clearer that the user needs to add the simple line of configuration shown in the example.
[x] After the example, don't talk about it being a default configuration. Just say what this example does (by definition, the first example is the most commonly useful and simplest possible config).
[x] In the example, use the simpler format of just using a closing slash in the opening tag eg
[x] It also needs to be clearer where the information comes from. I think just have a simple paragraph under the example then to say "This configuration sets the xxxx1 to valuexxx1 and xxxx2 to valuexxx2. The hostname value is obtained from...xxx and the sslport value is obtained from."
[x] What are the "AssertionConsumerService URL" and the "Service Provider (SP) metadata URL" and what are those URLs used for?
[x] The configuration elements listed in the text don't seem to exist (if you scroll down the page, you can see the list of valid elements for this feature; if you click an element eg samlWebSso20 you can see what config attributes are valid on that element). I don't think they're valid attributes either, eg samlsp doesn't seem to exist for the samlWebSso20.
[x] In general though, the text here is not appropriate for this type of topic. It's trying to be full task documentation for how to configure SAML; this page is meant to provide just config examples. If there's value in having a separate task topic on how to configure SAML, we can raise an issue to do that but when it's prioritised it will be need to be clear why it's commonly useful to anyone trying to use SAML on Open Liberty. Talk to the SME about this (btw, Bruce has moved to a different org now so might need someone else as SME).
[x] Also, the text says the preferred approach is by metadata exchange but doesn't explain how to do this - if we expect this to instruct users (if we produce a topic about configuring SAML), it needs to be more detailed, eg metadata exchange suggests it's automatic? how do you supply the url?
[x] If the second example is useful to users, then it needs to be formatted as described for the first example, and it needs to be in a separate sub-section with a heading (the first example does too) and little introduction and explanation of what it does. See other config topics for the standard format.
Some additional comments Alasdair made last week:
[x] There's a NameID format (not an element) - I don't know if this is still relevant re above comments?
[x] princidPal is a typo.
[x] source highlighting of the code examples needs to show colour - need to specify that it's XML in the source.
[source,xml]
at the start (see other topics for the exact asciidoc syntax).<samlWebSso20 id="defaultSP" enabled="false"/>
@lauracowen I worked on your edit suggesions.
[x] Default configuration. This needs to start by saying how you get the default configuration, and then give a code example of the default configuration that is generated. Explaining what the URLs are doesn't make much sense without it - it's not clear where those URLs are taken from.
[x] I don't think the XML code snippet is marked up properly. It still doesn't have syntax highlighting. It needs to specify [source,xml] at the start (see other topics for the exact asciidoc syntax).
[x] Also, put a closing slash in the opening tag in each line instead of having a separate closing tag. eg
[x] Don't prefix every heading with "Example for" - it's unnecssary as this is the Examples section.
[x] Don't provide instructional text that lists possible alternatives. Show it as an example. So, for instance, show an example of how the SAML configuration uses a metadata url. Then, if providing a file to the config instead of a URL is a valid thing to want to do to, insert another heading and show an example of this (then describe what the values show and where they're from).
@dmuelle Can you peer review this draft https://draft-openlibertyio.mybluemix.net/docs/ref/feature/#samlWeb-2.0.html?
Looks good- just a few things:
- AssertionConsumerService URL: `https://<hostname>:<sslport>/ibm/saml20/defaultSP/acs`
The AssertionConsumerService URL is the endpoint on the service provider point of contact server that receives assertions.
but however you organize them, make the two bullets the same format.
You can use a browser to download the metadata for this SP by using this URL and provide the URL to the SAML IdP to establish federation between this SP and IdP
.
suggested edit: You can use a browser to download the metadata for this SP from this URL. Provide the URL to the SAML IdP to establish federation between this SP and IdP."Thanks.
samleWeb-2.0
to the feature list does OL automatically generate a line of config that says <samlWebSso20 />
and nothing else? If OL generates a line of config like that, that's your config example for this section and it should be added as an XML config snippet (but in the lead-in sentence be clear that OL generates that line for you). If it doesn't, that first sentence doesn't make sense.+
on the blank line between the URL and the bullet point to which it belongs - that tells asciidoc that they're meant to be part of the same bullet point even though you've added a line break.Custom config section
defaultSP
seems to just be a name given here the default service provider instance (ie it's the value of the id
attribute). Don't use it as a shortcut instead of the actual thing you're talking about (it just means the reader has to guess what the name refers to and it isn't clear whether that value must always be used or not - maybe it is but it's not clear from the text here).defaultSP
, is disabled and the new service provider instance, newSP
, is configured."@dmuelle I worked on your peer edit suggestions.
[x] I think you could remove "of the SAML web SSO" from your subject headings as it's implied
[x] the two URL bullet points are not parallel. One uses a colon and the other doesn't. It might be more readable if you put the explanation on a separate line.
[x] You can use a browser to download the metadata for this SP by using this URL and provide the URL to the SAML IdP to establish federation between this SP and IdP. suggested edit: You can use a browser to download the metadata for this SP from this URL. Provide the URL to the SAML IdP to establish federation between this SP and IdP."
[x] In most cases, exchange of the metadata files is sufficient." Sufficient for what? does this mean that most cases will only need the default? It's not clear.
[x] Additional attributes can be specified if necessary." Use active voice- also it might help to refer them to the feature configuration elements, which detail the available attributes. "For more information, see the feature configuration elements.
@lauracowen I worked on your suggestions.
[x] I'm still not clear what this means "Open Liberty provides a default samlWebSso20 element.". When does OL provide it - is there anything the user needs to do? eg when you add samleWeb-2.0 to the feature list does OL automatically generate a line of config that says
[x] I think don't use the "SP" and "IdP" abbreviations - I know they're technically correct, and it's fine to put them in parentheses for people looking out for them, but there are so few mentions of each term here it's unnecessary here and if you're not familiar with them, you have to keep checking back to find out what they're referring to. It's much easier to read the full name instead.
[x] The URLs need to be indented as part of the bullet point to which they belong. To do that, put a + on the blank line between the URL and the bullet point to which it belongs - that tells asciidoc that they're meant to be part of the same bullet point even though you've added a line break.
[x] Actually, I'd put the URL before the explanation of what it is because that is the endpoint that's activated (which is what the lead-in sentence implies the bulleted list as listing). The text just explains what it is so should follow the URL.
[x] I don't understand what the sentences about metadata mean. The heading of this section implies there's no config to do, but then the end of the section talks about a "preferred approach to configure SAML". It's not clear if/why this is necessary and how you "supply the metadata URL to the Identity Provider".
[x] I don't understand what "exchange of the metadata files" means here.
[x] The literal value defaultSP seems to just be a name given here the default service provider instance (ie it's the value of the id attribute). Don't use it as a shortcut instead of the actual thing you're talking about (it just means the reader has to guess what the name refers to and it isn't clear whether that value must
[x] Then, underneath the example code, add "The default service provider instance, defaultSP, is disabled and the new service provider instance, newSP, is configured."
Hi - can you add a comment to this issue with answers to the questions above (in the issue, not necessarily in the topic)? I can see you've removed all the queried text from the topic but it's not clear why you've done so I can't tell whether it should be in the topic or not. Specifically points 1, 5, and 6. eg if you've had a discussion with the technical reviewer, add a note in here saying what the answer was and why the info is no longer required. For point 1, is there a line of automatically generated XML or not? and so on.
The formatting looks better, thanks:
newSP
in the final sentence (needs to be highlighted like defaultSP
is.defaultSP
..." still has the problem described in my point 7 above; ie don't use the name of the thing as part of the sentence; in this case it's not clear what a "defaultSP instance is" - it should probably say "You can disable the default service provider instance by..."?@lauracowen I made the edits per your review.
For point 1, I made the current changes after discussion with Bruce. When you add samlWebinto your server.xml file, inside the featureManager element, the SAML Web Single Sign-On 2.0 feature is enabled by default.
For points 5 and 6 I have added some explanation for the metadata without making it sound like a task topic
[x] You've missed backticks on newSP in the final sentence (needs to be highlighted like defaultSP is.
[x] The sentence "You can disable the defaultSP..." still has the problem described in my point 7 above; ie don't use the name of the thing as part of the sentence; in this case it's not clear what a "defaultSP instance is" - it should probably say "You can disable the default service provider instance by..."?
Excellent, thanks.
Where you have "This default configuration..." sentence, can you reword to be more explicit eg "When you enable the SAML Web Single Sign-on feature, it is automatically configured and activates two endpoints."
Other than that, looks good. Thanks. If you can tweak that wording, I'll sign it off.
@lauracowen I made the update
Looks good, thanks.
As I said on Slack, I’ll sign it off but would you mind tweaking the last bit of that sentence you just changed? to “…and activates the following endpoints:“? Mainly because I put a period instead of a colon (and you were right originally that it should be a colon).
@lauracowen Thanks Laura. Added the colon.
@dmuelle I worked on your peer review.
@chirp1 updated draft link https://draft-openlibertyio.mybluemix.net/docs/20.0.0.9/reference/feature/samlWeb-2.0.html
New link for the draft https://draft-openlibertyio.mybluemix.net/docs/20.0.0.10/reference/feature/samlWeb-2.0.html
@chirp1 New edits and updates
"One way to obscure the meaning of a sentence is to string together lots of words, phrases, or clauses joined by and or or, which are called coordinate conjunctions." https://learning.oreilly.com/library/view/developing-quality-technical/9780133119046/ch06.html
Changed "The AssertionConsumerService URL endpoint on the service provider point of contact server receives assertions." to "The AssertionConsumerService URL endpoint on the service provider point of contact server receives assertions from the identity provider.", for clarity.
Changed the Service Provider and Identity Provider mentions to lower case throughout the topic for consistency. "Use terms consistently
If you decide to use a term that your audience is not familiar with, carefully choose it, introduce it, and use it consistently. If your field or industry uses two or more terms for the same thing, pick one and use it consistently. In some types of writing, you are encouraged to use different words for the same thing for variety. In technical information, however, using more than one term for the same thing causes confusion and can lead to inaccurate translations. And using two terms can suggest that the two terms refer to different things."-Chapter 6 Clarity https://learning.oreilly.com/library/view/developing-quality-technical/9780133119046/ch06.html
Added "endpoint" to "The service provider metadata URL", for consistency.
Changed "You can use a browser to download the metadata for the service provider with this URL." to "You can download the metadata for the service provider on a browser with the service provider metadata URL." for conciseness and clarity.
@chirp1 Updated draft link https://draft-openlibertyio.mybluemix.net/docs/20.0.0.11/reference/feature/samlWeb-2.0.html
@chirp1 20.12 updated draft link https://draft-openlibertyio.mybluemix.net/docs/20.0.0.12/reference/feature/samlWeb-2.0.html.
@ManasiGandhi Hi Manasi, The update looks good. I have a few comments:
@chirp1 I worked on your review,
[x] Rewrite "You can download the metadata for the service provider on a browser with the service provider metadata URL.". "on a browser" is a little odd. The original sentence in the document that you drew from was clearer.
[x] For the "Custom configuration" section, rewrite the first sentence to explain the point of the two statements in the example. Mention the new service provider in this sentence instead of mentioning it first in the second sentence.
[x] Also, for the "Custom configuration" section, rewrite the sentence that follows the example. Say what each statement does. Don't use "Thus, you can ". The phrase is similar to "you can" and is more appropriate for the first sentence. I added the info at the beginning of the section.
Hi Manasi, In looking over your configuration examples further, I spot a few other things to comment on: I believe you drew your updates mainly from this topic: https://www.ibm.com/support/knowledgecenter/SSEQTP_liberty/com.ibm.websphere.wlp.doc/ae/twlp_config_saml_web_sso.html Generally, your configuration examples for this topic could have more of the bits of concrete information that are in the topic that you drew from.
I worked on Karen's edit review for this issue. Waiting on the changes to show after the builds issue is resolved.
https://www.openliberty.io/docs/ref/feature/#samlWeb-2.0.html