[SHIRO-889] Provide Jakarta jar modules #522

fpapon · 2022-11-02T18:43:15Z

No description provided.

bmarwell · 2022-11-02T21:38:03Z

Maybe we can user undertow?
Here's a basic setup without deployment and empty instance manager.
All you need to do: configure the .war and stop the server again later.

Switched to non-TLS for test purposes.

fpapon/shiro@SHIRO-889...apache:shiro:SHIRO-889_undertow

WDYT?

fpapon · 2022-11-03T07:22:00Z

Maybe we can user undertow? Here's a basic setup without deployment and empty instance manager. All you need to do: configure the .war and stop the server again later.

Switched to non-TLS for test purposes.

fpapon/shiro@SHIRO-889...apache:shiro:SHIRO-889_undertow

WDYT?

Thank you! I will take a look :)

.../web-jakarta/src/test/java/org/apache/shiro/test/web/jakarta/JakartaAbstractContainerIT.java

bmarwell · 2022-11-03T21:01:48Z

.../web-jakarta/src/test/java/org/apache/shiro/test/web/jakarta/JakartaAbstractContainerIT.java

+
+    // Dealing with a keystore is NOT fun, it's easier to script one with the keytool
+    // see src/main/resources/createKeyStore.sh for more info
+    private static File setupKeyStore() {


This can be removed, as this test now uses http which is sufficient

samples/web-jakarta/src/test/java/org/apache/shiro/test/web/jakarta/WebContainerTest.java

samples/web-jakarta/src/test/resources/createKeyStore.sh

samples/web-jakarta/src/test/resources/test-keystore.pem

lprimak · 2022-11-06T05:33:47Z

Question: why is there separate packages to be renamed in the shade plugin ?
Can’t it just be done in a single javax->jakarta rename and not individual packages such as JaxRS etc?

bmarwell

Great! Just some nits and some imports, then it is close-to-done (imho)!

bmarwell · 2022-11-08T18:31:53Z

samples/web-jakarta/src/test/java/org/apache/shiro/test/web/jakarta/WebContainerTest.java

+import org.junit.Test;
+import org.junit.jupiter.api.Assertions;


You are mixing junit4 and junit-jupiter assertions here. Please completely switch to -jupiter for all new tests.

... or maybe even assertj.

assertj is not needed :)

bmarwell · 2022-11-08T18:38:13Z

samples/web-jakarta/src/test/java/org/apache/shiro/test/web/jakarta/WebContainerTest.java

+        final Client client = ClientBuilder.newClient();
+
+        try {
+            final Response loginPage = client.target(getBaseUri())


response needs to be close()d. However, you might be able to use auto-closeable. https://jakarta.ee/specifications/platform/8/apidocs/javax/ws/rs/core/response#close--

client is closed in finally so I don't think we need to close the response but I can close it also in finally.

bmarwell · 2022-11-08T18:38:28Z

samples/web-jakarta/src/test/java/org/apache/shiro/test/web/jakarta/WebContainerTest.java

+            final Cookie jsessionid = new Cookie("JSESSIONID",loginPage.getMetadata().get("Set-Cookie").get(0).toString().split(";")[0].split("=")[1]);
+            Assertions.assertTrue(loginPage.readEntity(String.class).contains("loginform"));
+
+            final Response loginAction = client.target(getBaseUri())


Same. Needs close().

bmarwell · 2022-11-08T18:39:55Z

samples/web-jakarta/src/main/webapp/style.css

+    color: black;
+    background: #eee;
+    border: thick black solid;
+}


line feed missing at end of file

weird because I copy/paste the web module sample without modification...

bmarwell · 2022-11-08T18:40:08Z

samples/web-jakarta/src/main/webapp/index.jsp

+  --%>
+
+<%-- Forward the user to the home page --%>
+<jsp:forward page="home.jsp"/>


line feed missing at EOF

bmarwell · 2022-11-08T18:40:15Z

samples/web-jakarta/src/main/webapp/include.jsp

+<%@ page import="org.apache.shiro.SecurityUtils" %>
+<%@ taglib prefix="c" uri="http://java.sun.com/jsp/jstl/core" %>
+<%@ taglib prefix="fmt" uri="http://java.sun.com/jsp/jstl/fmt" %>
+<%@ taglib prefix="shiro" uri="http://shiro.apache.org/tags" %>


bmarwell · 2022-11-08T18:40:24Z

samples/web-jakarta/src/main/webapp/account/index.jsp

+<p><a href="<c:url value="/logout"/>" onclick="document.getElementById('logout_form').submit();return false;">Log out.</a></p>
+<form id="logout_form" action="<c:url value="/logout"/>" method="post"></form>
+</body>
+</html>


fpapon · 2022-11-11T16:48:30Z

@bmarwell changes done

bdemers

I'm +0.5, I worry a little about the jakarta classifier, as the exclude * will drop all of the dependencies (so the user would probably need to add the other bits like shiro-core to their pom)
But... that's not the end of the world, and other projects are doing the same thing (e.g. meecrowave)
I think getting it in as is and getting feedback from interested parties makes it worth merging now.
(given other projects are doing something similar, I'm hesitant to try to suggest an over-engineered solution 🤣 :)

TL;DR - my opinion is to merge it

fpapon · 2022-11-18T07:00:56Z

I'm +0.5, I worry a little about the jakarta classifier, as the exclude * will drop all of the dependencies (so the user would probably need to add the other bits like shiro-core to their pom) But... that's not the end of the world, and other projects are doing the same thing (e.g. meecrowave) I think getting it in as is and getting feedback from interested parties makes it worth merging now. (given other projects are doing something similar, I'm hesitant to try to suggest an over-engineered solution rofl :)

TL;DR - my opinion is to merge it

Hi, yes but honestly I don't know how to do it differently without a lot of code...
I will add a page on the Shiro website to well document the usage of the jakarta classifier.

garydgregory · 2022-11-18T13:45:40Z

Releasing takes time and effort, which will happen, just not right now.

bdemers · 2022-11-18T15:07:14Z

without a lot of code...

@fpapon exactly my concern too! Let’s move forward with this PR 🚢

fpapon · 2022-11-18T16:15:30Z

Thanks, now I will prepare a PR for 1.11.x

lucasdillmann · 2023-01-05T21:02:06Z

Hello everyone.

First of all, a huge thanks on the work to provide the Jakarta JARs.

Looking in the 2.0.0 nightly snapshots, I was able to find the artifacts over the shiro-web and shiro-spring modules, but I couldn't find them in the shiro-spring-boot, shiro-spring-boot-starter and shiro-spring-boot-web-started.

Isn't needed to create the jakarta variant JAR/POM on the Spring Boot modules as well?

lprimak · 2023-01-05T21:10:31Z

@lucasdillmann I don't think Shiro spring is compatible with Spring 6 or SpringBoot 3 quite yet. Not sure if those modules can be currently done. I am not a Spring user so I can't comment further.

lucasdillmann · 2023-01-05T21:19:52Z

@lprimak Is there any way I could help you guys in order to check/test if it's compatible?

For example, if I create a test project with some basic scenarios as a POC (using a Shiro fork just to change to the Jakarta APIs on the boot modules), will it be useful in any way?

lprimak · 2023-01-05T21:58:06Z

If it's only that Jakarta relocations are needed, you can try to add the shade plugin in your fork the same way other modules are done. That would be very helpful and appreciated!
The issue is that I personally can't test it because I am not a spring user.
Thank you!

lucasdillmann · 2023-01-05T22:18:35Z

I'll do it and get back here with the results soon. As far I've seen in the source code, the changes come down to one single import change in this file (apart from the dependency graph between the modules).

lucasdillmann · 2023-01-06T02:49:19Z

@lprimak I finished the tests here using Spring Boot 3.0.1, and everything seems to be working just fine.

The source code of the test project is available here. I didn't uplod the changes in the Shiro source, but it is just the inclusion of the maven-shade-plugin in the shiro-spring-boot module.

In short, these are the (exploratory) tests I've made:

Check if subject is authenticated or not using both the annotations (RequiresUser, RequiresGuest and RequiresAuthentication) and programmatic code.
Check if subject has a permission or not, also using the RequiresPermissions annotation and the Subject#checkPermission method.
Check if subject has a role or not, also using the RequiresRoles annotation and Subject#checkRole
Login and logout procedures

The only "problem" I've found is in the dependency management: If you check the build.gradle file here you will see that I needed to explicitly include every module with the jakarta variant and exclude the regular one. This happens because of the POM file contents, which bring the regular JAR as transitive.

I don't know if this is expected or not and, if it isn't, if it should be changed or not.

lprimak · 2023-01-06T03:00:13Z

Can you please submit a PR for this? Sounds like a simple fix. Yes, the dependency management issue is expected.

lucasdillmann · 2023-01-06T03:12:34Z

Sure. PR code is #632.

fpapon force-pushed the SHIRO-889 branch from 51bc457 to 2b40d6f Compare November 2, 2022 19:09

fpapon force-pushed the SHIRO-889 branch 2 times, most recently from 434b15f to 5d7e615 Compare November 3, 2022 19:35

bmarwell requested changes Nov 3, 2022

View reviewed changes

fpapon force-pushed the SHIRO-889 branch from 5d7e615 to c41aa6e Compare November 4, 2022 16:22

fpapon requested review from bmarwell and bdemers November 4, 2022 16:58

bmarwell requested changes Nov 8, 2022

View reviewed changes

[SHIRO-889] Provide Jakarta jar modules

9635f01

fpapon force-pushed the SHIRO-889 branch from c41aa6e to 9635f01 Compare November 11, 2022 16:45

fpapon requested a review from bmarwell November 11, 2022 16:48

bdemers approved these changes Nov 17, 2022

View reviewed changes

bmarwell approved these changes Nov 18, 2022

View reviewed changes

fpapon merged commit b6b5849 into apache:main Nov 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SHIRO-889] Provide Jakarta jar modules #522

[SHIRO-889] Provide Jakarta jar modules #522

fpapon commented Nov 2, 2022

bmarwell commented Nov 2, 2022

fpapon commented Nov 3, 2022

bmarwell Nov 3, 2022

lprimak commented Nov 6, 2022

bmarwell left a comment

bmarwell Nov 8, 2022

bmarwell Nov 8, 2022

fpapon Nov 9, 2022

bmarwell Nov 8, 2022

fpapon Nov 9, 2022

bmarwell Nov 8, 2022

bmarwell Nov 8, 2022

fpapon Nov 9, 2022

bmarwell Nov 8, 2022

bmarwell Nov 8, 2022

bmarwell Nov 8, 2022

fpapon commented Nov 11, 2022

bdemers left a comment

fpapon commented Nov 18, 2022

garydgregory commented Nov 18, 2022

bdemers commented Nov 18, 2022

fpapon commented Nov 18, 2022

lucasdillmann commented Jan 5, 2023

lprimak commented Jan 5, 2023 •

edited

Loading

lucasdillmann commented Jan 5, 2023 •

edited

Loading

lprimak commented Jan 5, 2023

lucasdillmann commented Jan 5, 2023

lucasdillmann commented Jan 6, 2023

lprimak commented Jan 6, 2023

lucasdillmann commented Jan 6, 2023

		import org.junit.Test;
		import org.junit.jupiter.api.Assertions;

[SHIRO-889] Provide Jakarta jar modules #522

[SHIRO-889] Provide Jakarta jar modules #522

Conversation

fpapon commented Nov 2, 2022

bmarwell commented Nov 2, 2022

fpapon commented Nov 3, 2022

Choose a reason for hiding this comment

lprimak commented Nov 6, 2022

bmarwell 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpapon commented Nov 11, 2022

bdemers left a comment

Choose a reason for hiding this comment

fpapon commented Nov 18, 2022

garydgregory commented Nov 18, 2022

bdemers commented Nov 18, 2022

fpapon commented Nov 18, 2022

lucasdillmann commented Jan 5, 2023

lprimak commented Jan 5, 2023 • edited Loading

lucasdillmann commented Jan 5, 2023 • edited Loading

lprimak commented Jan 5, 2023

lucasdillmann commented Jan 5, 2023

lucasdillmann commented Jan 6, 2023

lprimak commented Jan 6, 2023

lucasdillmann commented Jan 6, 2023

lprimak commented Jan 5, 2023 •

edited

Loading

lucasdillmann commented Jan 5, 2023 •

edited

Loading