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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
