Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Payara-1554 Basic MP-JWT 1.0 implementation passing TCK basic and full #2226

Merged
merged 8 commits into from Dec 28, 2017
Merged

Payara-1554 Basic MP-JWT 1.0 implementation passing TCK basic and full #2226

merged 8 commits into from Dec 28, 2017

Conversation

arjantijms
Copy link
Contributor

PAYARA-1554 Basic MP-JWT 1.0 implementation passing TCK basic and full.

@arjantijms
Copy link
Contributor Author

jenkins test please

@arjantijms arjantijms added this to the Payara 4.181 milestone Dec 22, 2017
@payara-ci
Copy link
Contributor

Quick build and test failed!

@arjantijms
Copy link
Contributor Author

jenkins test please

@payara-ci
Copy link
Contributor

Quick build and test passed!

@@ -114,6 +114,12 @@

<jsftemplating.version>2.1.2</jsftemplating.version>
<uc-pkg-client.version>1.122-57.2889</uc-pkg-client.version>

<nimbus-jose-jwt.version>5.1</nimbus-jose-jwt.version>
<asm.version>5.0.4</asm.version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property is already defined in nucleus

<dependency>
<groupId>org.eclipse.microprofile.jwt</groupId>
<artifactId>microprofile-jwt-auth-api</artifactId>
<version>1.0</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Propertise please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, that's a merge or commit going wrong as the properties were being put there. I'll fix it.

<dependency>
<groupId>com.nimbusds</groupId>
<artifactId>nimbus-jose-jwt</artifactId>
<version>5.1</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Propertise please


Bean<?> bean = injectionPoint.getBean();

Class<?> scope = bean != null? injectionPoint.getBean().getScope() : null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ternary? (╯°□°)╯︵ ┻━┻
Ternary with no space before the question mark? ┻━┻ ︵ヽ(`Д´)ノ︵ ┻━┻

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Omg... how then?

*
* @author Arjan Tijms
*/
public class CdiExtension2 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yuck!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no words indeed to describe that. It's absolutely maddening, spend 2 days on it including debugging deep into the bowels of the sun.* classes. For some reason the @Observers annotations can't be processed, unless the code of the event observers is moved elsewhere.

converter = e -> ((JsonString) e).getString();
} else if (coreClass.equals(Set.class)) {
converter = e -> e instanceof JsonArray
? new HashSet<>(((JsonArray) e).getValuesAs(JsonString.class)).stream().map(t -> t.getString()).collect(toSet())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very "busy" statement - probably easier to read as a proper if/else

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, it's busyness comes from I developed against the Java EE 8 API first where it's much less code, then had to convert to EE 7 which Payara 4 is still using of course. Probably an entire separate method, which then can use the if/else is even better here.

return (T) singleton( ((JsonString) claimValue).getString());
} else {
return (T) new HashSet<>(((JsonArray) claimValue).getValuesAs(JsonString.class)).stream().map(t ->
t.getString()).collect(toSet());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holy awful indenting batman!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, used to be small single line on Java EE 8, looks like mess with EE 7 API.

Map<String, JsonValue> rawClaims =
new HashMap<>(
Json.createReader(new StringReader(signedJWT.getPayload().toString()))
.readObject());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be double indent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where? lines 96 and 97 have double indents, and on line 98 .readObject() is aligned with .createReader.


private boolean checkNotExpired(Map<String, JsonValue> presentedClaims) {
int currentTime = (int) (System.currentTimeMillis() / 1000);
long expiredTime = ((JsonNumber) presentedClaims.get(exp.name())).intValue();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why long? You've just done .intValue()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eagle eyes, sir

}
}
} catch (Exception e) {
// Ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to log at finer/finest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@arjantijms
Copy link
Contributor Author

jenkins test please

@payara-ci
Copy link
Contributor

Quick build and test passed!

@arjantijms
Copy link
Contributor Author

jenkins test please

@payara-ci
Copy link
Contributor

Quick build and test passed!

@smillidge smillidge merged commit 2a2d3d9 into payara:master Dec 28, 2017
michaelranaldo pushed a commit to michaelranaldo/Payara that referenced this pull request Jan 8, 2018
payara#2226)

* PAYARA-1554 Initial MP-JWT implementation. WIP!

* PAYARA-1554 Basic MP-JWT 1.0 implementation passing TCK basic and full.

* PAYARA-1554 Versions via placeholder and new deps added to license text

* PAYARA-1554 Added JCIP to ext modules

* Payara-1554 Addressing Andy's feedback

* PAYARA-1554 Renaming CdiExtension2 to a more semantic name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants