Update playground app to support ID Token encryption #2313

vihanga-liyanage · 2018-02-13T13:44:45Z

Proposed changes in this pull request

Main changes proposed are the UI and the backend servlets that can decrypt JWE id tokens. User can provide the private key of the client and the decrypted id token header and claims data will be displayed.

In reference to wso2/product-is#2336

When should this PR be merged

No dependencies. We can merge this ASAP.

Follow up actions

N/A

Checklist (for reviewing)

General

Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
Is the PR labeled correctly?

Functionality

Are all requirements met? Compare implemented functionality with the requirements specification.
Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?

Security

Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
Are all UI and API inputs run through forms or serializers?
Are all external inputs validated and sanitized appropriately?
Does all branching logic have a default case?
Does this solution handle outliers and edge cases gracefully?
Are all external communications secured and restricted to SSL?

Documentation

Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes should be documented.
Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented.
Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

mefarazath · 2018-02-14T07:31:01Z

...les/oauth2/playground2/src/main/org/wso2/sample/identity/oauth2/IDTokenDecrypterServlet.java

+
+        ServletOutputStream out = response.getOutputStream();
+
+        if (idToken.equals("") || privateKeyString.equals("")) {


When the id_token is empty this will return "Client private key cannot be empty!".
Better to do separate checks or change the error message to say "Client private key and id_token cannot be empty!"

I'll do separate checks.

mefarazath · 2018-02-14T07:33:09Z

...les/oauth2/playground2/src/main/org/wso2/sample/identity/oauth2/IDTokenDecrypterServlet.java

+                    json.put(entry.getKey(), entry.getValue());
+                }
+
+                out.print(json.toString());


Here we are sending the payload of the JWT.
What about the JWT header? Shouldn't we display it as well?

I guess we can. But the header comes as an object. We'll have to build a JSON with info we need to display.

mefarazath · 2018-02-14T07:34:56Z

modules/samples/oauth2/playground2/src/main/webapp/oauth2.jsp

+                        try {
+                            name = SignedJWT.parse(idToken).getJWTClaimsSet().getSubject();
+                        } catch (Exception e) {
+                            //ignore


Shouldn't we at least log the exception?

This was already there before me. What should be the best way? Can we log them since this is a jsp?

denuwanthi · 2018-10-25T05:57:46Z

...les/oauth2/playground2/src/main/org/wso2/sample/identity/oauth2/IDTokenDecrypterServlet.java

+
+        jwt.decrypt(decrypter);
+
+        return jwt;


In this method, actually what is happening is returning a decrypted jwt.
But the return type is set to 'EncryptedJWT'. What is actually happening(decrypting) and the retrun value(Encrypted) seems contradictory.
Can you please explain this behavior? or can we fix it properly to match the flow.

modules/samples/oauth2/playground2/src/main/webapp/oauth2.jsp

denuwanthi · 2018-10-25T06:05:32Z

modules/samples/oauth2/playground2/src/main/webapp/oauth2.jsp

@@ -307,24 +459,24 @@

                        <tr>
                            <td><label>Client Id : </label></td>
-                            <td><input type="text" id="consumerKey" name="consumerKey" value="<%=consumerKey%>" style="width:350px"></td>
+                            <td><input type="text" id="consumerKey" name="consumerKey" value="<%=consumerKey%>" style="width:450px"></td>


shall we use Encode.forHtmlAttribute(consumerKey)?

denuwanthi · 2018-10-25T06:07:00Z

modules/samples/oauth2/playground2/src/main/webapp/oauth2.jsp

@@ -440,21 +592,21 @@
                        </tr>
                        <tr>
                            <td>Callback URL :</td>
-                            <td><input type="text" id="callbackurl" name="callbackurl" value="<%=callbackUrl%>" style="width:350px"></td>
+                            <td><input type="text" id="callbackurl" name="callbackurl" value="<%=callbackUrl%>" style="width:450px"></td>


lets use Encode.forHtmlAttribute(callbackUrl) to prevent possible XSS attacks

denuwanthi · 2018-10-25T06:07:38Z

modules/samples/oauth2/playground2/src/main/webapp/oauth2.jsp

                        </tr>
                        <tr>
                            <td>Access Token Endpoint :</td>
-                            <td><input type="text" id="accessEndpoint" name="accessEndpoint" value="<%=accessTokenEndpoint%>" style="width:350px"></td>
+                            <td><input type="text" id="accessEndpoint" name="accessEndpoint" value="<%=accessTokenEndpoint%>" style="width:450px"></td>


shall we use Encode.forHtmlAttribute()?

denuwanthi · 2018-10-25T06:08:18Z

modules/samples/oauth2/playground2/src/main/webapp/oauth2.jsp

                        </tr>
                        <tr>
                            <td><label>Client Secret : </label></td>
-                            <td><input type="password" id="consumerSecret" name="consumerSecret" value="<%=consumerSecret%>" style="width:350px">
+                            <td><input type="password" id="consumerSecret" name="consumerSecret" value="<%=consumerSecret%>" style="width:450px">


Lets enncode the input to prevent security threats.

mefarazath reviewed Feb 14, 2018

View reviewed changes

Update playground app to support ID Token encryption

05723b7

darshanasbg approved these changes Feb 15, 2018

View reviewed changes

darshanasbg merged commit 90a60ec into wso2:5.5.x Feb 15, 2018

denuwanthi reviewed Oct 25, 2018

View reviewed changes

modules/samples/oauth2/playground2/src/main/webapp/oauth2.jsp Show resolved Hide resolved

denuwanthi reviewed Oct 25, 2018

View reviewed changes

denuwanthi requested a review from emswbandara October 25, 2018 13:12

vihanga-liyanage mentioned this pull request Oct 28, 2018

Fix Review comments #3874

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update playground app to support ID Token encryption #2313

Update playground app to support ID Token encryption #2313

vihanga-liyanage commented Feb 13, 2018 •

edited

Loading

mefarazath Feb 14, 2018

vihanga-liyanage Feb 14, 2018

mefarazath Feb 14, 2018

vihanga-liyanage Feb 14, 2018

mefarazath Feb 14, 2018

vihanga-liyanage Feb 14, 2018

denuwanthi Oct 25, 2018

denuwanthi Oct 25, 2018

denuwanthi Oct 25, 2018

denuwanthi Oct 25, 2018 •

edited

Loading

denuwanthi Oct 25, 2018


		ServletOutputStream out = response.getOutputStream();

		if (idToken.equals("") \|\| privateKeyString.equals("")) {

Update playground app to support ID Token encryption #2313

Update playground app to support ID Token encryption #2313

Conversation

vihanga-liyanage commented Feb 13, 2018 • edited Loading

Proposed changes in this pull request

When should this PR be merged

Follow up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

Security

Documentation

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

denuwanthi Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vihanga-liyanage commented Feb 13, 2018 •

edited

Loading

denuwanthi Oct 25, 2018 •

edited

Loading