Example config for MP JWT feature (MSSEC16)

lauracowen commented 5 years ago

https://www.openliberty.io/docs/ref/feature/#mpJwt-1.1.html

Add one or more realistic config examples to the generated feature topic: https://www.openliberty.io/docs/ref/feature/#mpJwt-1.1.html (see JDBC feature for the format). Use example from MP JWT guide (https://openliberty.io/guides/microprofile-jwt.html#securing-back-end-services-with-microprofile-jwt) and get reviewed to check it's good enough.

Also the example config in the blog post here: https://github.com/OpenLiberty/blogs/blob/master/publish/2019-08-29-securing-microservices-social-login-jwt.adoc

Charlotte-Holt commented 4 years ago

Hi Manasi, good job getting the includes in there so that this is building properly! 👍

Peer review feedback:

The examples.adoc file needs to have Examples as an H2 title (see the JDBC file or the Social Media Login file)
- Instead of "The mpJwt feature", I'd use plain text for the feature name (from our stylistic guidelines)
- I think that the example(s) need to be framed in a more helpful way. There could be a better lead-in to the example that explains what it does and when you would use it. Maybe you can work with an SME to develop another example of a common use case for this feature (if you can't find any more good examples from the KC). I also like how in the JDBC example, the different examples are separated with descriptive H3 headings so that someone reading the documentation can easily pick out which example they might be looking for. Have you identified an SME to work with?

lauracowen commented 4 years ago

Can you add the "Examples" H2 heading at the top of the include file so that your examples have a section heading? This isn't auto-generated sadly.
You don't need the sentence starting "The mpjwt feature adds the libraries...". Can you remove it. In theory, you could put that in a separate description.adoc file and include that too (and it'd display near the top of the topic) but I don't think it adds much information to what's already there.
In the config example, can you include only the <mpJwt....../> section. Keep examples as tight and minimal as possible. The keyStore section might be important actually, so keep that too. But remove everything else.
Also make sure with the SME (Chunlong Liang will be able to help) that all the attributes in those elements are essential for the most basic, usable configuration that can be set up for MPJWT.
Then in the intro to that example, explain why each attribute/element has been set in the example config. Take a look at the Social Media Login and Java Database Connectivity examples to get an idea of the type of explanation needed.

ManasiGandhi commented 4 years ago

Bruce's edit comments(from Slack) "Should omit since don’t have the rest of the file or the matching tag. Don’t neet to mention maven or the keystore - not everybody uses maven (containers, for example, use Docker images) MicrProfile --> MicroProfile"

ManasiGandhi commented 4 years ago

@brutif Can you review the draft for the updates https://draft-openlibertyio.mybluemix.net/docs/ref/feature/#mpJwt-1.1.html?

brutif commented 4 years ago

looks fine.

ManasiGandhi commented 4 years ago

@lauracowen Can you review this feature https://draft-openlibertyio.mybluemix.net/docs/ref/feature/#mpJwt-1.1.html?

lauracowen commented 4 years ago

I think the first example needs to be much simpler. Not everything shown is necessary for a basic configuration. Take a look at the blog post https://openliberty.io/blog/2019/08/29/securing-microservices-social-login-jwt.html and find the server.xml example for "The microservice server configuration" (there are two examples in the post). I think all you need are the mpJwt and keyStore lines. Everything else is just stuff you'd put in the app anyway for other purposes. Can you use those two lines instead of the ones you've currently got?
Doing that also resolves my second point which was that in examples of server configuration like this we need to replace the variables with hardcoded values so it's clearer what the values are likely to be.The guides externalise a lot of the attribute values as variables that are resolved by the Maven build (anything with a value in the format ${...}). Examples without proper values aren't that useful in this context though. So if you use the example from Bruce's blog post, it will remove that problem.
I think there needs to be a little more explanation of what's going on. I think replace that lead-in sentence with something more explanatory such as: "The following example shows that the service requires requests to be authenticated using JWTs. The mpJwt element defines the....(check what these attributes' values define) and the keyStore element defines the keystore that will be used by the JWT:" or something like that.
Don't put triangular brackets from around the element; just use the form "the mpJwt element" as it's easier to read/skim.
Phrasing "requests that target the myApp application are processed by the mp-jwt feature:" -> "requests to the myApp application are authorized using JWT:" (more concrete about what it means for the app; rather than just describing the technology).

Thanks

brutif commented 4 years ago

looks good, you might consider. where to retrieve the public key from to validate the JWT --> where to obtain the public key used to validate the JWT.

brutif commented 4 years ago

latest edit looks fine.

lauracowen commented 4 years ago

Looks good. Thank you :)

ManasiGandhi commented 4 years ago

@chirp1 Can you review this draft https://draft-openlibertyio.mybluemix.net/docs/ref/feature/#mpJwt-1.1.html?

ManasiGandhi commented 4 years ago

@Charlotte-Holt Can you review this draft https://draft-openlibertyio.mybluemix.net/docs/ref/feature/#mpJwt-1.1.html?

Charlotte-Holt commented 4 years ago

Peer review feedback:

When you say, "The JwksUri attribute...", I think it should be, "The jwksUri attribute...". Since the actual element doesn't seem to be capitalized in the example?
Similar thing with keystore. In the config example, the attribute is keyStore. Check which is correct, and maybe make them consistent.
Acrolinx is flagging that "trust store" should be "truststore". Check on that in this context.
I might spell out what JWKS stands for the first time you use it
In the second example, consider whether to monospace "myApp" in the description. It reads funny to me not monospaced, but this could've been intentional. I don't think it's necessarily incorrect.

Other than that, looks good!

ManasiGandhi commented 4 years ago

@Charlotte-Holt , I worked on your edit suggestions

[x] When you say, "The JwksUri attribute...", I think it should be, "The jwksUri attribute...". Since the actual element doesn't seem to be capitalized in the example?

[x] Similar thing with keystore. In the config example, the attribute is keyStore. Check which is correct, and maybe make them consistent.
[x] Acrolinx is flagging that "trust store" should be "truststore". Check on that in this context.
[x] I might spell out what JWKS stands for the first time you use it
[x] In the second example, consider whether to monospace "myApp" in the description. It reads funny to me not monospaced, but this could've been intentional. I don't think it's necessarily incorrect.

lauracowen commented 4 years ago

Thanks. Signing off.

lauracowen commented 4 years ago

Hi Manasi,

I just happened to look for this topic to show as an example to someone else but I don't see any examples in this topic: https://draft-openlibertyio.mybluemix.net/docs/20.0.0.9/reference/feature/mpJwt-1.1.html Am I missing something or has something built incorrectly?

lauracowen commented 4 years ago

I tracked down the problem. The include statements that pull in the examples.adoc file are not in the MP JWT 1.1 version of the feature doc, only in the MP JWT 1.0 version. This is probably (I'm guessing?) because the new gendoc version was added after you started work on this? Whether or not that's what happened here, it will keep happening with this and other features. We could do with some automation to handle this when the gendoc are created/updated. cc @Charlotte-Holt @dmuelle

lauracowen commented 4 years ago

Thanks for fixing. Signing off.

ManasiGandhi commented 4 years ago

New link of the draft https://draft-openlibertyio.mybluemix.net/docs/20.0.0.10/reference/feature/mpJwt-1.1.html

ManasiGandhi commented 4 years ago

@chirp1 New edits and updates

Changed , "The jwksUri attribute tells the mpjwt element from where to obtain the public key to validate the JWT.", to, "The jwksUri attribute points the mpjwt element towards the public key to validate the JSON Web Token." for clarity

"A concise, powerful verb more clearly conveys your intended meaning and promotes a more active style." https://learning.oreilly.com/library/view/developing-quality-technical/9780133119046/ch06.html

Changed "The security configuration takes effect when the conditions in the filter are met.", to "The security configuration works when the conditions in the filter are met." for conciseness and clarity.

"Watch for nominalizations that use weak verbs such as be, have, perform, make, and give. " https://learning.oreilly.com/library/view/developing-quality-technical/9780133119046/ch06.html

Changed JWT to JSON Web Token throughout the topic.

ManasiGandhi commented 4 years ago

@chirp1 Updated draft link https://draft-openlibertyio.mybluemix.net/docs/20.0.0.11/reference/feature/jwt-1.0.html.

chirp1 commented 4 years ago

Hi Manasi,

Some of the updates that I list might be in development code. If they are, and you can't make the changes, then open a defect for development so that they make the changes. Add to your issue a link to the defect. Review with me the changes that you are considering putting into the defect before you put them in so that we agree on those changes.

Change "This feature allows runtime..." to "This feature allows the runtime...:
Review "JWT tokens". Does the wording seem right to you?
Change "...declaration into your ..." to "...declaration to your...". <= Change "into" to "in".
For the section titled "Examples", do the number of examples in the section match the singular/plural of the title?
"ID" in "The ID attribute...." does not match the example.
Run Acrolinx. I see at least one issue for you to fix.
Change "URL http://example.com " to "http://example.com URL ".
For "that identifies who issued the JSON Web Token.", investigate the use of "who", and change accordingly.`
For "The expiry attribute indicates", I think you meant a different attribute.

Also, your comments on Sep 23 seem to be for another topic as i don't see the changes that you made in this JSON Web Token 1.0 topic. We generally write out a term on first occurrence, for example: JSON Web Token (JWT), and then use the acronym afterwards in the topic.

ManasiGandhi commented 4 years ago

@chirp1 I made updates per your edit suggestions.

Here's the link https://draft-openlibertyio.mybluemix.net/docs/20.0.0.12/reference/feature/jwt-1.0.html

Opened an issue for the first three suggestions https://github.com/OpenLiberty/docs/issues/3000

 Change "This feature allows runtime..." to "This feature allows the runtime...:
 Review "JWT tokens". Does the wording seem right to you?
 Change "...declaration into your ..." to "...declaration to your...". <= Change "into" to "in".

[x] For the section titled "Examples", do the number of examples in the section match the singular/plural of the title?
[x] "ID" in "The ID attribute...." does not match the example.
[x] Run Acrolinx. I see at least one issue for you to fix.
[x] Change "URL http://example.com " to "http://example.com URL ".
[x] For "that identifies who issued the JSON Web Token.", investigate the use of "who", and change accordingly.` (referred to this link, https://tools.ietf.org/html/rfc7519)
[x] For "The expiry attribute indicates", I think you meant a different attribute.

chirp1 commented 4 years ago

Hi Manasi, I think you meant to give me the link to the MP JWT topic: https://draft-openlibertyio.mybluemix.net/docs/20.0.0.11/reference/feature/mpJwt-1.1.html instead of the link to the JWT topic at https://draft-openlibertyio.mybluemix.net/docs/20.0.0.12/reference/feature/jwt-1.0.html This link to the JWT topic goes instead with this issue https://github.com/OpenLiberty/docs/issues/636. So, my comments that I gave you previously in this issue go with issue #636. I added a note to #636, indicating that the comments are instead in this issue.

I'll review the MP JWT topic.

chirp1 commented 3 years ago

Hi Manasi, Here are my comments:

For "The following example shows that the service needs request authentication with a JSON Web Token (JWT). ", I don't know what "the service needs request authentication " means. Maybe "needs" is a word that was inadvertently left in the sentence. Rewrite the sentence so that it is clear.
For "....points the mpjwt element towards the public key.." Rewrite the sentence to explain what you mean. Are you saying that the public key is at https://localhost:19443/jwt/ibm/api/myBuilder/jwk?
For the sentence that starts with "The keyStore element defines the key and truststore where the public key is stored, " I think that the sentence is too complex and that some of your what you are trying to say is unclear. I don't understand what "to validate the JSON Web Token when the JSON Web Key Sets(JWKS) are not used: " as it applies to the first part of the sentence. I think that some part of the first sentence is used to validate the JSON Web Token, but I don't know what that part is. Break the sentence into two sentences and clearly state what is used to validate the JSON Web Token.
For "JSON Web Key Sets(JWKS) ", I don't think that ""JSON Web Key Sets" is correct. Investigate and change as appropriate.
Add a space between "Sets" and "(JWKS)".
The title of "Specify login with MicroProfile JWT for a subset of applications, URLs, browsers, or IP addresses" is awkward. Rewrite. I would avoid "subset of applications, URLs, browsers, or IP addresses" and "Specify" in the title.
For "..myApp application are authorized with a JSON Web Token ", I don't think "authorized" is the right word.
I think that in the authentication filter example that the "myAuthFilter" ID is supposed to be mentioned in the mp-jwt statement.

ManasiGandhi commented 3 years ago

I worked on Karen's review for this issue and waiting on the changes to show after the builds are resolved.

ManasiGandhi commented 3 years ago

@chirp1 I moved this issue to your column for an editorial review.

https://draft-openlibertyio.mybluemix.net/docs/21.0.0.1/reference/feature/mpJwt-1.1.html

dmuelle commented 2 years ago

https://openliberty.io/docs/latest/reference/feature/mpJwt-2.0.html

OpenLiberty / docs

Example config for MP JWT feature (MSSEC16) #571