Most of Second PR by lyzhang27 · Pull Request #2 · GJWT/java-jwt

lyzhang27 · 2018-05-02T21:05:38Z

No description provided.

manu-sinha · 2018-05-02T21:30:42Z

lib/src/main/java/com/auth0/msg/AbstractMessage.java

@@ -0,0 +1,208 @@
+package com.auth0.msg;


You haven't refactored for package name?

manu-sinha · 2018-05-07T05:32:41Z

lib/src/main/java/com/auth0/msg/AbstractMessage.java

+import java.io.IOException;
+import java.net.MalformedURLException;
+import java.nio.charset.StandardCharsets;
+import java.util.*;


Please add sepcific import.

manu-sinha

Javadocs are missing from many classes/methods
Lot of methods are empty

manu-sinha · 2018-05-07T05:33:08Z

lib/src/main/java/com/auth0/msg/AbstractMessage.java

+    private String input;
+    private Error error = null;
+    private boolean verified = false;
+    ObjectMapper mapper = new ObjectMapper();


does this need to be protected?

manu-sinha · 2018-05-07T05:34:32Z

lib/src/main/java/com/auth0/msg/AbstractMessage.java

+     * @throws InvalidClaimsException
+     */
+    public String toJwt(Key key, Algorithm algorithm) throws InvalidClaimsException, SerializationException {
+        return null;


implementation?

manu-sinha · 2018-05-07T05:35:27Z

lib/src/main/java/com/auth0/msg/AbstractMessage.java

+            }
+        }
+        if (!errors.isEmpty()) {
+            String aggregateError = "";


please reuse errorSB instead of aggregateError.

manu-sinha · 2018-05-07T05:36:56Z

lib/src/main/java/com/auth0/msg/AbstractMessage.java

+     * verify that the required claims are present
+     * @return whether the verification passed
+     */
+    protected boolean verify() {


The method signature should tell that this method can throw InvalidClaimsException so that caller can appropriately handle failed validation.

manu-sinha · 2018-05-07T05:37:38Z

lib/src/main/java/com/auth0/msg/AbstractMessage.java

+
+        if (errorSB.length() != 0) {
+            errors.add("Message is missing required claims:" + errorSB.toString());
+            return false;


This doesn't help - you are adding errors but not returning.

Modified to put error messages into the InvalidClaimException thrown at the end which should be handled upstream and also update the instance of Error object

manu-sinha · 2018-05-07T05:47:22Z

lib/src/main/java/com/auth0/msg/ECKeyDefinition.java

+public class ECKeyDefinition extends KeyDefinition {
+    private String crv;
+
+    public ECKeyDefinition(KeyType type, KeyUseCase useCase, String crv) {


Please explain each of the parameters.

manu-sinha · 2018-05-07T05:49:22Z

lib/src/main/java/com/auth0/msg/InvalidClaimsException.java

+/**
+ * A runtime exception that is thrown when there is an invalid claim in a Message object type
+ */
+public class InvalidClaimsException extends RuntimeException {


Extending RuntimeException is not right. You should extend Exception. This should be a checked exception so that caller can handle and recover from this.

manu-sinha · 2018-05-07T05:49:43Z

lib/src/main/java/com/auth0/msg/JsonResponseDescriptor.java

+
+import java.util.List;
+import java.util.Map;
+


Please provide Javadoc

manu-sinha · 2018-05-07T05:50:45Z

lib/src/main/java/com/auth0/msg/JsonResponseDescriptor.java

+
+    @Override
+    public boolean hasError() {
+        return false;


why are you overriding hasError and always returning false? What happens when verify fails? If this is a special scenario please provide javadoc to explain that.

manu-sinha · 2018-05-07T05:51:38Z

lib/src/main/java/com/auth0/msg/Message.java

@@ -0,0 +1,87 @@
+package com.auth0.msg;


In abstract message, i have suggested some changes to methods, please make the same changes to interface.

Yexuan Leo Zhang added 4 commits April 10, 2018 16:16

OICMSG interfaces, abstract class, enums

70ec60f

Respond to comments from PR review

dc83e42

Dependencies for OICService

7c7bc11

Portion of milestone 2 with AbstractMessage functionality

9e5aedb

lyzhang27 requested a review from manu-sinha May 2, 2018 21:05

manu-sinha reviewed May 2, 2018

View reviewed changes

Portion of milestone 2 with verify functionality

7631253

manu-sinha reviewed May 7, 2018

View reviewed changes

manu-sinha requested changes May 7, 2018

View reviewed changes

Yexuan Leo Zhang added 4 commits May 8, 2018 03:53

Updated verify logic

9e2da6b

Respond to PR feedback

2bff0b3

Make changes to note TODO items and jwt functionality

8e4e020

Include incomplete keyjar and keybundle funcationality

aa876ae


		import java.util.List;
		import java.util.Map;

Conversation

lyzhang27 commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manu-sinha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants