Value injector for RESTEasy #31

yegorius · 2017-11-16T00:17:09Z

What do you think?

victornoel · 2017-11-16T07:40:37Z

Oh great! I'm going to review this soon!

victornoel · 2017-11-16T07:42:05Z

You should already try to enable the tests that involve the annotations, it's easy, there is just some empty overriding test method to remove normally.

victornoel

Thank, there are some points that should be improved but it's great already :)
Could you:

enable the tests (remove the empty overriding tests in RestEasyUndertowServletTest and enable the feature in RestEasyUndertowServletRule)
find out why some tests do not pass (actually, if you run all the tests of RestEasyUndertowServletTest some tests do not pass, but if you execute them one by one, they do... there is something fishy with state that is not cleaned as expected or maybe it is resteasy...
Add some very basic documentation (in the same way as the jersey feature)

victornoel · 2017-11-19T10:06:22Z

src/main/java/org/pac4j/jax/rs/resteasy/Pac4JProfileInjectorFactory.java

+				);
+			} else {
+				return new Pac4JValueInjector(
+						(req, resp) -> new ProfileManager(new J2EContext(req, resp)).get(profile.readFromSession()).orElse(null)


In the jersey implementation, I preferred to throw an new WebApplicationException(401) instead of returning null.
The rational is that if you want to retrieve a profile that can be potentially not there (i.e. you are not authenticated), then you use the Optional.
If not (i.e., you are expecting to be authenticated, then the profile shouldn't be null, then we should't allow for such situation, especially because it introduces a security risks.

Actually one of the test relies on this behaviour ;)

See https://github.com/pac4j/jax-rs-pac4j/blob/master/src/main/java/org/pac4j/jax/rs/jersey/features/Pac4JValueFactoryProvider.java#L246

victornoel · 2017-11-19T10:09:28Z

src/main/java/org/pac4j/jax/rs/resteasy/Pac4JProfileInjectorFactory.java

+			this.provider = provider;
+		}
+
+		public Object inject(HttpRequest request, HttpResponse response) {


Add an @Override annotation here.

victornoel · 2017-11-19T10:09:32Z

src/main/java/org/pac4j/jax/rs/resteasy/Pac4JProfileInjectorFactory.java

+			return inject();
+		}
+
+		public Object inject() {


Add an @Override annotation here.

victornoel · 2017-11-19T10:10:37Z

src/main/java/org/pac4j/jax/rs/pac4j/JaxRsProfileManager.java

@@ -131,6 +131,10 @@ public PrincipalImpl(LinkedHashMap<String, CommonProfile> profiles) {
            this.roles = Collections.unmodifiableSet(rs);
        }

+        public CommonProfile getProfile() {


This is not related to this PR, right? I would prefer you remove it then :)

victornoel · 2017-11-19T10:11:34Z

src/main/java/org/pac4j/jax/rs/resteasy/Pac4JProfileInjectorFactory.java

@@ -0,0 +1,79 @@
+package org.pac4j.jax.rs.resteasy;


Move this class into org.pac4j.jax.rs.resteasy.features, it would make more sense with the organization of the rest of this library :)

victornoel · 2017-11-19T10:29:04Z

There are also some formatting problems... some tabs crept up :)

yegorius · 2017-11-20T08:09:20Z

I am investigating issues with tests right now.

yegorius · 2017-11-20T18:02:13Z

The only test which fails for me is testInject(). And it only fails when it is being run with other tests. If I launch this single test, then it passes.
I get a weird error message from Undertow: UT005014: Failed to parse HTTP request.
Which happens because this is exactly(!) what it receives on the wire (I have copied the complete byte buffer from the Undertow request parser):

User-Agent: Java/1.8.0_152-release
Host: localhost:24257
Accept: text/html, image/gif, image/jpeg, *; q=.2, */*; q=.2
Connection: close
Cookie: JSESSIONID=5icpg-Zdu3SOLgo2QXDn_s2X_zzO00y5auNK9hqu

At this moment, I have no clue.
I have added a delay in the very beginning of testInject() for 5000ms and all tests pass now, but this is obviously not a solution, it's more of a hint for us.

victornoel · 2017-11-21T07:49:23Z

Thanks for the exploration.
I suppose it is undertow's closing that is not properly synchronized with the execution of tests or something like that… I say that because I've noticed harmless (until now...) undertow errors in the logs at the end of the tests before.
I will try to find the time to fix this next week-end, but if you feel like it, don't hesitate to continue exploring and maybe finding the solution ;)

We can proceed without it. To be fixed later.

yegorius · 2017-11-21T13:23:04Z

I have ignored the failing test so that the build is successful now. I have also updated the documentation.
Would you mind merging this PR in this state and moving the test problem to a separate GitHub issue?

victornoel · 2017-11-21T13:55:38Z

yes, ok, let's do that

victornoel · 2017-11-21T13:57:25Z

Thanks again @yegorius :)

We should do a release soon then! Maybe 2.1.0 @leleuj, what do you think?

leleuj · 2017-11-21T16:10:51Z

Yes, it makes sense. It'a noticeable update. Starting...

leleuj · 2017-11-21T16:12:52Z

@victornoel The version is still 2.0.3, I think it should be 2.1.0 as it's a backward compatible change (not a bug fix), isn't it?

victornoel · 2017-11-21T16:21:04Z

yes, 2.1.0 is a good number! (that's what I was saying in my comment above ; )

leleuj · 2017-11-21T17:42:58Z

Yes, right. I just performed the release, it will show up in the Maven central repository in a few hours...

Value injector for RESTEasy

56189bf

victornoel self-requested a review November 16, 2017 07:40

victornoel self-assigned this Nov 16, 2017

victornoel requested changes Nov 19, 2017

View reviewed changes

Code review rework

e98ab34

victornoel approved these changes Nov 20, 2017

View reviewed changes

yegorius added 2 commits November 21, 2017 12:01

Ignore bad test.

c6bf72c

We can proceed without it. To be fixed later.

Add instructions for RestEasy Pac4JProfile injection

bad5c01

victornoel merged commit 7eb19c5 into pac4j:master Nov 21, 2017

victornoel mentioned this pull request Nov 21, 2017

Value injector for RESTEasy #6

Closed

victornoel added this to the 2.1.0 milestone Nov 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Value injector for RESTEasy #31

Value injector for RESTEasy #31

yegorius commented Nov 16, 2017

victornoel commented Nov 16, 2017

victornoel commented Nov 16, 2017

victornoel left a comment

victornoel Nov 19, 2017

victornoel Nov 19, 2017

yegorius Nov 19, 2017

victornoel Nov 19, 2017

yegorius Nov 19, 2017

victornoel Nov 19, 2017

yegorius Nov 19, 2017

victornoel Nov 19, 2017

yegorius Nov 19, 2017

victornoel commented Nov 19, 2017

yegorius commented Nov 20, 2017

yegorius commented Nov 20, 2017

victornoel commented Nov 21, 2017

yegorius commented Nov 21, 2017

victornoel commented Nov 21, 2017

victornoel commented Nov 21, 2017

leleuj commented Nov 21, 2017

leleuj commented Nov 21, 2017

victornoel commented Nov 21, 2017

leleuj commented Nov 21, 2017

Value injector for RESTEasy #31

Value injector for RESTEasy #31

Conversation

yegorius commented Nov 16, 2017

victornoel commented Nov 16, 2017

victornoel commented Nov 16, 2017

victornoel left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victornoel commented Nov 19, 2017

yegorius commented Nov 20, 2017

yegorius commented Nov 20, 2017

victornoel commented Nov 21, 2017

yegorius commented Nov 21, 2017

victornoel commented Nov 21, 2017

victornoel commented Nov 21, 2017

leleuj commented Nov 21, 2017

leleuj commented Nov 21, 2017

victornoel commented Nov 21, 2017

leleuj commented Nov 21, 2017