You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@jena.apache.org by "TelicentPaul (via GitHub)" <gi...@apache.org> on 2024/02/26 16:11:06 UTC
[PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
TelicentPaul opened a new pull request, #2298:
URL: https://github.com/apache/jena/pull/2298
GitHub issue resolved #2297
Pull request Description:
Enabling the potential passing of a configuration file containing the desired CORS configuration so that we have a greater choice than present - none or default.
Also some tidying up of spellings, some better error checks and improved testing of the existing Cross Origin Filter class and all of the exceptional cases for Fuseki Main's command-line arg processing.
----
- [x] Tests are included.
- [x] Documentation change and updates are provided for the [Apache Jena website](https://github.com/apache/jena-site/)
- [x] Commits have been squashed to remove intermediate development commit messages.
- [x] Key commit messages start with the issue number (GH-xxxx, or if in JIRA, JENA-xxxx)
By submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the [Contributor's Agreement](https://www.apache.org/licenses/contributor-agreements.html).
----
See the [Apache Jena "Contributing" guide](https://github.com/apache/jena/blob/main/CONTRIBUTING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "afs (via GitHub)" <gi...@apache.org>.
afs commented on code in PR #2298:
URL: https://github.com/apache/jena/pull/2298#discussion_r1506080339
##########
jena-fuseki2/jena-fuseki-main/src/main/java/org/apache/jena/fuseki/main/FusekiServer.java:
##########
@@ -34,6 +35,7 @@
import jakarta.servlet.http.HttpServletResponse;
import org.apache.jena.atlas.json.JSON;
import org.apache.jena.atlas.json.JsonObject;
+import org.apache.jena.atlas.json.JsonValue;
Review Comment:
Unused import?
##########
jena-cmds/src/main/java/org/apache/jena/cmd/CommandLineBase.java:
##########
@@ -69,7 +69,15 @@ private List<String> processArgv(String[] argv) {
boolean positional = false;
+ if (null == argv) {
+ return argList;
+ }
+
Review Comment:
How can argv be null?
Similarly, below anArgv being null.
##########
jena-fuseki2/jena-fuseki-main/src/main/java/org/apache/jena/fuseki/main/FusekiServer.java:
##########
@@ -1383,12 +1399,12 @@ private static void prepareDataServices(DataAccessPointRegistry dapRegistry, Ope
}
/**
- * Given a ServletContextHandler, set the servlet attributes for
- * {@link DataAccessPointRegistry} and {@link OperationRegistry}.
+ * Given a ServletContextHandler, set the servlet attributes for {@link DataAccessPointRegistry} and
+ * {@link OperationRegistry}.
*/
private static void applyDatabaseSetup(ServletContextHandler handler,
- DataAccessPointRegistry dapRegistry,
- OperationRegistry operationReg) {
+ DataAccessPointRegistry dapRegistry,
+ OperationRegistry operationReg) {
Review Comment:
This is a layout improvement.
##########
jena-fuseki2/jena-fuseki-main/src/main/java/org/apache/jena/fuseki/main/FusekiServer.java:
##########
@@ -1498,93 +1526,106 @@ private void validate() {
}
/**
- * Set up the ServletContextHandler if there is a securityHandler or password file
- * Return true if there is a security handler built for this server (not externally provided).
+ * Set up the ServletContextHandler if there is a securityHandler or password file Return true if there is a
+ * security handler built for this server (not externally provided).
*/
private boolean applySecurityHandler(ServletContextHandler cxt) {
- if ( securityHandler == null && passwordFile != null )
+ if (securityHandler == null && passwordFile != null) {
securityHandler = buildSecurityHandler();
+ }
// -- Access control
- if ( securityHandler == null )
+ if (securityHandler == null) {
return false;
+ }
cxt.setSecurityHandler(securityHandler);
- if ( ! ( securityHandler instanceof ConstraintSecurityHandler ) ) {
+ if (!(securityHandler instanceof ConstraintSecurityHandler)) {
// Externally provided security handler.
return false;
}
- ConstraintSecurityHandler csh = (ConstraintSecurityHandler)securityHandler;
- if ( hasServerWideAuth() )
+ ConstraintSecurityHandler csh = (ConstraintSecurityHandler) securityHandler;
+ if (hasServerWideAuth()) {
JettySecurityLib.addPathConstraint(csh, "/*");
+ }
return true;
}
- /** Look in a DataAccessPointRegistry for datasets and endpoints with authentication policies.*/
+ /**
+ * Look in a DataAccessPointRegistry for datasets and endpoints with authentication policies.
+ */
private void applyAccessControl(ServletContextHandler cxt, DataAccessPointRegistry dapRegistry) {
- ConstraintSecurityHandler csh = (ConstraintSecurityHandler)(cxt.getSecurityHandler());
- if ( csh == null )
- return ;
+ ConstraintSecurityHandler csh = (ConstraintSecurityHandler) (cxt.getSecurityHandler());
+ if (csh == null) {
+ return;
+ }
// Look for datasets and endpoints that need login and add a path constraint.
- dapRegistry.forEach((name, dap)-> {
- if ( ! authAny(dap.getDataService().authPolicy()) ) {
+ dapRegistry.forEach((name, dap) -> {
+ if (!authAny(dap.getDataService().authPolicy())) {
// Dataset wide.
JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name));
- JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name)+"/*");
- }
- else {
+ JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name) + "/*");
+ } else {
// Check endpoints.
- dap.getDataService().forEachEndpoint(ep->{
- if ( ! authAny(ep.getAuthPolicy()) ) {
+ dap.getDataService().forEachEndpoint(ep -> {
+ if (!authAny(ep.getAuthPolicy())) {
// Unnamed - unfortunately this then applies to all operations on the dataset.
- if ( ep.getName().isEmpty() ) {
+ if (ep.getName().isEmpty()) {
JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name));
- JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name)+"/*");
+ JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name) + "/*");
} else {
// Named service.
- JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name)+"/"+ep.getName());
- if ( Fuseki.GSP_DIRECT_NAMING )
- JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name)+"/"+ep.getName()+"/*");
+ JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(
+ name) + "/" + ep.getName());
+ if (Fuseki.GSP_DIRECT_NAMING) {
+ JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(
+ name) + "/" + ep.getName() + "/*");
+ }
}
}
});
}
});
}
- /** Build a ServletContextHandler */
+ /**
+ * Build a ServletContextHandler
+ */
private ServletContextHandler buildServletContext(String contextPath) {
- if ( contextPath == null || contextPath.isEmpty() )
+ if (contextPath == null || contextPath.isEmpty()) {
contextPath = "/";
- else if ( !contextPath.startsWith("/") )
+ } else if (!contextPath.startsWith("/")) {
contextPath = "/" + contextPath;
+ }
ServletContextHandler context = new ServletContextHandler();
context.setDisplayName(Fuseki.servletRequestLogName);
// Also set on the server which handles request that don't dispatch.
context.setErrorHandler(errorHandler);
context.setContextPath(contextPath);
// SPARQL Update by HTML - not the best way but.
- context.setMaxFormContentSize(1024*1024);
+ context.setMaxFormContentSize(1024 * 1024);
// securityHandler done in buildAccessControl
return context;
}
- /** Add servlets and servlet filters, including the {@link FusekiFilter} */
+ /**
+ * Add servlets and servlet filters, including the {@link FusekiFilter}
+ */
private void servletsAndFilters(ServletContextHandler context) {
// First in chain. CORS.
// Preflight to set to respond without passing on OPTIONS.
- // Otherwise passes on to the next filter.
- if ( corsInitParams != null ) {
+ // Other-wise passes on to the next filter.
Review Comment:
"Otherwise"
##########
jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/servlets/CrossOriginFilter.java:
##########
@@ -274,7 +274,7 @@ private boolean generateAllowedOrigins(Set<String> allowedOriginStore, List<Patt
String[] allowedOrigins = StringUtil.csvSplit(allowedOriginsConfig);
for (String allowedOrigin : allowedOrigins)
{
- if (allowedOrigin.length() > 0)
+ if (!allowedOrigin.isEmpty())
Review Comment:
This file is an exact copy of the code from Jetty (see the top of this file).
I prefer to keep the code exactly as it came from Jetty in case we need to do a compare.
##########
jena-fuseki2/jena-fuseki-main/src/main/java/org/apache/jena/fuseki/main/FusekiServer.java:
##########
@@ -1366,14 +1381,15 @@ private ServletContextHandler buildFusekiServerContext() {
private static void prepareDataServices(DataAccessPointRegistry dapRegistry, OperationRegistry operationReg) {
dapRegistry.forEach((name, dap) -> {
// Override for graph-level access control.
- if ( DataAccessCtl.isAccessControlled(dap.getDataService().getDataset()) ) {
- dap.getDataService().forEachEndpoint(ep->
- FusekiLib.modifyForAccessCtl(ep, DataAccessCtl.requestUserServlet));
+ if (DataAccessCtl.isAccessControlled(dap.getDataService().getDataset())) {
+ dap.getDataService().forEachEndpoint(ep ->
+ FusekiLib.modifyForAccessCtl(ep,
+ DataAccessCtl.requestUserServlet));
Review Comment:
This reformatting has done wrong!
##########
jena-fuseki2/jena-fuseki-main/src/main/java/org/apache/jena/fuseki/main/FusekiServer.java:
##########
@@ -1498,93 +1526,106 @@ private void validate() {
}
/**
- * Set up the ServletContextHandler if there is a securityHandler or password file
- * Return true if there is a security handler built for this server (not externally provided).
+ * Set up the ServletContextHandler if there is a securityHandler or password file Return true if there is a
+ * security handler built for this server (not externally provided).
*/
private boolean applySecurityHandler(ServletContextHandler cxt) {
- if ( securityHandler == null && passwordFile != null )
+ if (securityHandler == null && passwordFile != null) {
securityHandler = buildSecurityHandler();
+ }
// -- Access control
- if ( securityHandler == null )
+ if (securityHandler == null) {
return false;
+ }
cxt.setSecurityHandler(securityHandler);
- if ( ! ( securityHandler instanceof ConstraintSecurityHandler ) ) {
+ if (!(securityHandler instanceof ConstraintSecurityHandler)) {
// Externally provided security handler.
return false;
}
- ConstraintSecurityHandler csh = (ConstraintSecurityHandler)securityHandler;
- if ( hasServerWideAuth() )
+ ConstraintSecurityHandler csh = (ConstraintSecurityHandler) securityHandler;
+ if (hasServerWideAuth()) {
JettySecurityLib.addPathConstraint(csh, "/*");
+ }
return true;
}
- /** Look in a DataAccessPointRegistry for datasets and endpoints with authentication policies.*/
+ /**
+ * Look in a DataAccessPointRegistry for datasets and endpoints with authentication policies.
+ */
private void applyAccessControl(ServletContextHandler cxt, DataAccessPointRegistry dapRegistry) {
- ConstraintSecurityHandler csh = (ConstraintSecurityHandler)(cxt.getSecurityHandler());
- if ( csh == null )
- return ;
+ ConstraintSecurityHandler csh = (ConstraintSecurityHandler) (cxt.getSecurityHandler());
+ if (csh == null) {
+ return;
+ }
// Look for datasets and endpoints that need login and add a path constraint.
- dapRegistry.forEach((name, dap)-> {
- if ( ! authAny(dap.getDataService().authPolicy()) ) {
+ dapRegistry.forEach((name, dap) -> {
+ if (!authAny(dap.getDataService().authPolicy())) {
// Dataset wide.
JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name));
- JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name)+"/*");
- }
- else {
+ JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name) + "/*");
+ } else {
// Check endpoints.
- dap.getDataService().forEachEndpoint(ep->{
- if ( ! authAny(ep.getAuthPolicy()) ) {
+ dap.getDataService().forEachEndpoint(ep -> {
+ if (!authAny(ep.getAuthPolicy())) {
// Unnamed - unfortunately this then applies to all operations on the dataset.
- if ( ep.getName().isEmpty() ) {
+ if (ep.getName().isEmpty()) {
JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name));
- JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name)+"/*");
+ JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name) + "/*");
} else {
// Named service.
- JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name)+"/"+ep.getName());
- if ( Fuseki.GSP_DIRECT_NAMING )
- JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(name)+"/"+ep.getName()+"/*");
+ JettySecurityLib.addPathConstraint(csh, DataAccessPoint.canonical(
Review Comment:
Formatting gone wrong.
##########
jena-fuseki2/jena-fuseki-main/src/main/java/org/apache/jena/fuseki/main/cmds/FusekiMain.java:
##########
@@ -96,8 +96,7 @@ public class FusekiMain extends CmdARQ {
// Static files. URLs are affected by argPathBase
private static ArgDecl argBase = new ArgDecl(ArgDecl.HasValue, "base", "files");
- // This is now a no-op - CORS is included unless "--no-cors" is used.
- private static ArgDecl argCORS = new ArgDecl(ArgDecl.NoValue, "withCORS", "cors", "CORS");
+ private static ArgDecl argCORS = new ArgDecl(ArgDecl.HasValue, "withCORS", "cors", "CORS", "cors-config");
Review Comment:
This is significant. The arg (no value) was left for compatibility. Changing to requiring an argument is a signifcant step. (But I don't see a better argument name.)
Is the format documented?
##########
jena-fuseki2/jena-fuseki-main/testing/Config/cors.properties:
##########
@@ -0,0 +1,5 @@
+allowedOrigins=*
+allowedMethods=GET,POST,DELETE,PUT,HEAD,OPTIONS,PATCH
+allowedHeaders=X-Requested-With, Content-Type, Accept, Origin, Last-Modified, Authorization, Security-Label
+exposedHeaders=Cache-Control, Content-Language, Content-Length, Content-Type, Expires, Last-Modified, Pragma
+chainPreflight=false
Review Comment:
Minor - add newline.
##########
jena-fuseki2/jena-fuseki-main/src/test/java/org/apache/jena/fuseki/main/TestFusekiMainCmdArguments.java:
##########
@@ -0,0 +1,373 @@
+/*
Review Comment:
Add to TS_FusekiMain.
##########
jena-fuseki2/jena-fuseki-main/src/main/java/org/apache/jena/fuseki/main/FusekiServer.java:
##########
@@ -1699,19 +1765,38 @@ private Server jettyServer(ServletContextHandler handler, String jettyServerConf
return server;
}
- /** Jetty server with https */
- private static Server jettyServerHttps(ServletContextHandler handler, int httpPort, int httpsPort, int minThreads, int maxThreads, String keystore, String certPassword) {
- return JettyHttps.jettyServerHttps(handler, keystore, certPassword, httpPort, httpsPort, minThreads, maxThreads);
+ /**
+ * Jetty server with https
+ */
+ private static Server jettyServerHttps(ServletContextHandler handler, int httpPort, int httpsPort,
+ int minThreads, int maxThreads, String keystore, String certPassword) {
+ return JettyHttps.jettyServerHttps(handler, keystore, certPassword, httpPort, httpsPort, minThreads,
+ maxThreads);
}
- /** Restrict connectors to localhost */
+ /**
+ * Restrict connectors to localhost
+ */
private static void applyLocalhost(Server server) {
Connector[] connectors = server.getConnectors();
- for ( int i = 0; i < connectors.length; i++ ) {
- if ( connectors[i] instanceof ServerConnector serverConnector) {
+ for (int i = 0; i < connectors.length; i++) {
+ if (connectors[i] instanceof ServerConnector serverConnector) {
serverConnector.setHost("localhost");
}
}
}
+
+ private static Map<String, String> parseCORSConfigFile(String filename) {
+ try {
+ Properties properties = loadFromFile(filename);
+ Map<String, String> map = new HashMap<>(properties.size());
+ for (Map.Entry<Object, Object> entry : properties.entrySet()) {
+ map.put((String) entry.getKey(), (String) entry.getValue());
+ }
+ return map;
+ } catch (Exception ex) {
+ throw new FusekiConfigException("Failed to read the CORS config file: "+ex.getMessage());
Review Comment:
Probably, yes.
I don't see any source of exceptions except loadFromFile and after a quick look at the JDK properties loading code, it isn't strong on errors.
##########
jena-fuseki2/jena-fuseki-core/src/test/java/org/apache/jena/fuseki/servlets/TestCrossOriginFilter.java:
##########
@@ -0,0 +1,446 @@
+/*
Review Comment:
Add to TS_FusekiCore
##########
jena-fuseki2/jena-fuseki-main/testing/Config/cors.properties:
##########
@@ -0,0 +1,5 @@
+allowedOrigins=*
+allowedMethods=GET,POST,DELETE,PUT,HEAD,OPTIONS,PATCH
+allowedHeaders=X-Requested-With, Content-Type, Accept, Origin, Last-Modified, Authorization, Security-Label
Review Comment:
Remove "Security-Label".
Maybe we need an "and also" list that adds headers to the default set "X-Requested-With", "Content-Type", "Accept", "Origin".
##########
jena-fuseki2/jena-fuseki-main/testing/Config/invalid.ttl:
##########
@@ -0,0 +1,2 @@
+# An empty file for testing error conditions
+Rubbish entry that is not going to work
Review Comment:
Minor - add newline.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "afs (via GitHub)" <gi...@apache.org>.
afs merged PR #2298:
URL: https://github.com/apache/jena/pull/2298
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "TelicentPaul (via GitHub)" <gi...@apache.org>.
TelicentPaul commented on code in PR #2298:
URL: https://github.com/apache/jena/pull/2298#discussion_r1507463507
##########
jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/servlets/CrossOriginFilter.java:
##########
@@ -274,7 +274,7 @@ private boolean generateAllowedOrigins(Set<String> allowedOriginStore, List<Patt
String[] allowedOrigins = StringUtil.csvSplit(allowedOriginsConfig);
for (String allowedOrigin : allowedOrigins)
{
- if (allowedOrigin.length() > 0)
+ if (!allowedOrigin.isEmpty())
Review Comment:
I'll revert this and any other changes I deem non-essential to the underlying problem
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "TelicentPaul (via GitHub)" <gi...@apache.org>.
TelicentPaul commented on code in PR #2298:
URL: https://github.com/apache/jena/pull/2298#discussion_r1507465967
##########
jena-fuseki2/jena-fuseki-main/testing/Config/invalid.ttl:
##########
@@ -0,0 +1,2 @@
+# An empty file for testing error conditions
+Rubbish entry that is not going to work
Review Comment:
Sure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "afs (via GitHub)" <gi...@apache.org>.
afs commented on code in PR #2298:
URL: https://github.com/apache/jena/pull/2298#discussion_r1507480283
##########
jena-fuseki2/jena-fuseki-main/src/main/java/org/apache/jena/fuseki/main/FusekiServer.java:
##########
@@ -1383,12 +1399,12 @@ private static void prepareDataServices(DataAccessPointRegistry dapRegistry, Ope
}
/**
- * Given a ServletContextHandler, set the servlet attributes for
- * {@link DataAccessPointRegistry} and {@link OperationRegistry}.
+ * Given a ServletContextHandler, set the servlet attributes for {@link DataAccessPointRegistry} and
+ * {@link OperationRegistry}.
*/
private static void applyDatabaseSetup(ServletContextHandler handler,
- DataAccessPointRegistry dapRegistry,
- OperationRegistry operationReg) {
+ DataAccessPointRegistry dapRegistry,
+ OperationRegistry operationReg) {
Review Comment:
That one specifically is a fault in the current codebase. Looks like a method rename in initial development.
The others (and ones that seem to have disappeared from an earlier state) are just "different format" and will lead to thrashing back and forth.
The Jena codebase has [a loose style](https://github.com/apache/jena/blob/main/CONTRIBUTING.md#code). There is a balance between excessive clearing up (and history disruption) and converting some of the more unusual styles to more modern forms.
For me, the git history is what matters so cleanup as separate commits to functional changes works when the cleanup is in the same area.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "afs (via GitHub)" <gi...@apache.org>.
afs commented on PR #2298:
URL: https://github.com/apache/jena/pull/2298#issuecomment-1969182245
Could you rebase this against current main please?
The build does not work - it runs into a bug in a dependency that's now been upgraded to a working version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "afs (via GitHub)" <gi...@apache.org>.
afs commented on code in PR #2298:
URL: https://github.com/apache/jena/pull/2298#discussion_r1507436836
##########
jena-cmds/src/main/java/org/apache/jena/cmd/CommandLineBase.java:
##########
@@ -69,7 +69,15 @@ private List<String> processArgv(String[] argv) {
boolean positional = false;
+ if (null == argv) {
+ return argList;
+ }
+
Review Comment:
Which test(s)? It is not clear to me that returning is the correct way to handle what is an error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "TelicentPaul (via GitHub)" <gi...@apache.org>.
TelicentPaul commented on code in PR #2298:
URL: https://github.com/apache/jena/pull/2298#discussion_r1507517495
##########
jena-fuseki2/jena-fuseki-main/src/test/java/org/apache/jena/fuseki/main/TestFusekiMainCmdArguments.java:
##########
@@ -0,0 +1,373 @@
+/*
Review Comment:
ok
##########
jena-fuseki2/jena-fuseki-core/src/test/java/org/apache/jena/fuseki/servlets/TestCrossOriginFilter.java:
##########
@@ -0,0 +1,446 @@
+/*
Review Comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "afs (via GitHub)" <gi...@apache.org>.
afs commented on PR #2298:
URL: https://github.com/apache/jena/pull/2298#issuecomment-1976334346
> - [x] Documentation change and updates are provided for the [Apache Jena website](https://github.com/apache/jena-site/)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "TelicentPaul (via GitHub)" <gi...@apache.org>.
TelicentPaul commented on code in PR #2298:
URL: https://github.com/apache/jena/pull/2298#discussion_r1507464274
##########
jena-fuseki2/jena-fuseki-main/src/main/java/org/apache/jena/fuseki/main/FusekiServer.java:
##########
@@ -34,6 +35,7 @@
import jakarta.servlet.http.HttpServletResponse;
import org.apache.jena.atlas.json.JSON;
import org.apache.jena.atlas.json.JsonObject;
+import org.apache.jena.atlas.json.JsonValue;
Review Comment:
From the earlier iteration a JSON file was being processed for the configuration. Easily removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "TelicentPaul (via GitHub)" <gi...@apache.org>.
TelicentPaul commented on code in PR #2298:
URL: https://github.com/apache/jena/pull/2298#discussion_r1507465037
##########
jena-fuseki2/jena-fuseki-main/src/main/java/org/apache/jena/fuseki/main/FusekiServer.java:
##########
@@ -1366,14 +1381,15 @@ private ServletContextHandler buildFusekiServerContext() {
private static void prepareDataServices(DataAccessPointRegistry dapRegistry, OperationRegistry operationReg) {
dapRegistry.forEach((name, dap) -> {
// Override for graph-level access control.
- if ( DataAccessCtl.isAccessControlled(dap.getDataService().getDataset()) ) {
- dap.getDataService().forEachEndpoint(ep->
- FusekiLib.modifyForAccessCtl(ep, DataAccessCtl.requestUserServlet));
+ if (DataAccessCtl.isAccessControlled(dap.getDataService().getDataset())) {
+ dap.getDataService().forEachEndpoint(ep ->
+ FusekiLib.modifyForAccessCtl(ep,
+ DataAccessCtl.requestUserServlet));
Review Comment:
A side-effect of the rebase. Intellij's formatting seems to have taken real issue with what was there. I'm going to remove most of those as they're not tied to the change
##########
jena-fuseki2/jena-fuseki-main/src/main/java/org/apache/jena/fuseki/main/FusekiServer.java:
##########
@@ -1383,12 +1399,12 @@ private static void prepareDataServices(DataAccessPointRegistry dapRegistry, Ope
}
/**
- * Given a ServletContextHandler, set the servlet attributes for
- * {@link DataAccessPointRegistry} and {@link OperationRegistry}.
+ * Given a ServletContextHandler, set the servlet attributes for {@link DataAccessPointRegistry} and
+ * {@link OperationRegistry}.
*/
private static void applyDatabaseSetup(ServletContextHandler handler,
- DataAccessPointRegistry dapRegistry,
- OperationRegistry operationReg) {
+ DataAccessPointRegistry dapRegistry,
+ OperationRegistry operationReg) {
Review Comment:
Again, I'll remove it. Better for you to to have a small "fix formatting and layout" PR outside of this than muddy the waters here.
##########
jena-fuseki2/jena-fuseki-main/testing/Config/cors.properties:
##########
@@ -0,0 +1,5 @@
+allowedOrigins=*
+allowedMethods=GET,POST,DELETE,PUT,HEAD,OPTIONS,PATCH
+allowedHeaders=X-Requested-With, Content-Type, Accept, Origin, Last-Modified, Authorization, Security-Label
+exposedHeaders=Cache-Control, Content-Language, Content-Length, Content-Type, Expires, Last-Modified, Pragma
+chainPreflight=false
Review Comment:
Sure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "kinow (via GitHub)" <gi...@apache.org>.
kinow commented on code in PR #2298:
URL: https://github.com/apache/jena/pull/2298#discussion_r1506250456
##########
jena-fuseki2/jena-fuseki-main/src/main/java/org/apache/jena/fuseki/main/FusekiServer.java:
##########
@@ -1699,19 +1765,38 @@ private Server jettyServer(ServletContextHandler handler, String jettyServerConf
return server;
}
- /** Jetty server with https */
- private static Server jettyServerHttps(ServletContextHandler handler, int httpPort, int httpsPort, int minThreads, int maxThreads, String keystore, String certPassword) {
- return JettyHttps.jettyServerHttps(handler, keystore, certPassword, httpPort, httpsPort, minThreads, maxThreads);
+ /**
+ * Jetty server with https
+ */
+ private static Server jettyServerHttps(ServletContextHandler handler, int httpPort, int httpsPort,
+ int minThreads, int maxThreads, String keystore, String certPassword) {
+ return JettyHttps.jettyServerHttps(handler, keystore, certPassword, httpPort, httpsPort, minThreads,
+ maxThreads);
}
- /** Restrict connectors to localhost */
+ /**
+ * Restrict connectors to localhost
+ */
private static void applyLocalhost(Server server) {
Connector[] connectors = server.getConnectors();
- for ( int i = 0; i < connectors.length; i++ ) {
- if ( connectors[i] instanceof ServerConnector serverConnector) {
+ for (int i = 0; i < connectors.length; i++) {
+ if (connectors[i] instanceof ServerConnector serverConnector) {
serverConnector.setHost("localhost");
}
}
}
+
+ private static Map<String, String> parseCORSConfigFile(String filename) {
+ try {
+ Properties properties = loadFromFile(filename);
+ Map<String, String> map = new HashMap<>(properties.size());
+ for (Map.Entry<Object, Object> entry : properties.entrySet()) {
+ map.put((String) entry.getKey(), (String) entry.getValue());
+ }
+ return map;
+ } catch (Exception ex) {
+ throw new FusekiConfigException("Failed to read the CORS config file: "+ex.getMessage());
Review Comment:
Maybe include the original exception in the stack trace?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "rvesse (via GitHub)" <gi...@apache.org>.
rvesse commented on code in PR #2298:
URL: https://github.com/apache/jena/pull/2298#discussion_r1505622316
##########
jena-fuseki2/jena-fuseki-main/src/main/java/org/apache/jena/fuseki/main/FusekiServer.java:
##########
@@ -1704,5 +1713,28 @@ private static void applyLocalhost(Server server) {
}
}
}
+
+ private static Map<String,String> parseCORSConfigFile(String filename) {
+ try {
Review Comment:
Is using a JSON file overkill here? Wondering if we might not be better just taking in a Java Properties file as then the whole method reduces to:
```java
try (FileInputStream input = new FileInputStream(filename)) {
Properties properties = new Properties();
properties.load(input);
// Convert Properties into Map
return map;
}
```
##########
jena-fuseki2/jena-fuseki-main/testing/Config/cors_config.json:
##########
Review Comment:
Per my other comment this could just be a Java properties file i.e.
```
allowedOrigins=*
allowedMethods=GET,POST,DELETE,PUT,HEAD,OPTIONS,PATH
# etc
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "TelicentPaul (via GitHub)" <gi...@apache.org>.
TelicentPaul commented on code in PR #2298:
URL: https://github.com/apache/jena/pull/2298#discussion_r1506035733
##########
jena-fuseki2/jena-fuseki-main/src/main/java/org/apache/jena/fuseki/main/FusekiServer.java:
##########
@@ -1704,5 +1713,28 @@ private static void applyLocalhost(Server server) {
}
}
}
+
+ private static Map<String,String> parseCORSConfigFile(String filename) {
+ try {
Review Comment:
No reason - happy to change.
##########
jena-fuseki2/jena-fuseki-main/testing/Config/cors_config.json:
##########
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "rvesse (via GitHub)" <gi...@apache.org>.
rvesse commented on PR #2298:
URL: https://github.com/apache/jena/pull/2298#issuecomment-1969103225
@kinow Could you take a look over this PR please, I know you do a lot of front end stuff atop Fuseki so figure this is probably useful to your long term and worth double checking it would address any CORS configuration needs you've encountered previously
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "TelicentPaul (via GitHub)" <gi...@apache.org>.
TelicentPaul commented on code in PR #2298:
URL: https://github.com/apache/jena/pull/2298#discussion_r1507433392
##########
jena-cmds/src/main/java/org/apache/jena/cmd/CommandLineBase.java:
##########
@@ -69,7 +69,15 @@ private List<String> processArgv(String[] argv) {
boolean positional = false;
+ if (null == argv) {
+ return argList;
+ }
+
Review Comment:
It's admittedly contrived but I was able to with the test cases I had.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "TelicentPaul (via GitHub)" <gi...@apache.org>.
TelicentPaul commented on code in PR #2298:
URL: https://github.com/apache/jena/pull/2298#discussion_r1507460063
##########
jena-cmds/src/main/java/org/apache/jena/cmd/CommandLineBase.java:
##########
@@ -69,7 +69,15 @@ private List<String> processArgv(String[] argv) {
boolean positional = false;
+ if (null == argv) {
+ return argList;
+ }
+
Review Comment:
test_error_noArguments_null
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "TelicentPaul (via GitHub)" <gi...@apache.org>.
TelicentPaul commented on PR #2298:
URL: https://github.com/apache/jena/pull/2298#issuecomment-1969213969
I'm assuming that's the Maven Javadoc plugin, yeah? I'd noticed problems with it and had commented it out locally as it bore no relation to this work. Rebasing shouldn't be a problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org
Re: [PR] GH-2297: Add CORS configuration file as command line parameter to Fuseki Main [jena]
Posted by "TelicentPaul (via GitHub)" <gi...@apache.org>.
TelicentPaul commented on PR #2298:
URL: https://github.com/apache/jena/pull/2298#issuecomment-1976133569
@afs I've changed the header in tests for "Custom-Header".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org