From a4c6293b3315e3b07ee15ccf5adc3bd13ecc047e Mon Sep 17 00:00:00 2001 From: OleksandrZharov Date: Tue, 27 Sep 2022 12:42:49 +0200 Subject: [PATCH] Document and validate web-api constructs We ditch immutables.org and use interface/builder/record to provide minimal API footprint change. Any violations are flagged by builder setter methods. This forces a rather more thorough change in APIs, but the result is more worth the churn. The urlPatterns are check for compliance with Java Servlet Specification, version 3.1. JIRA: AAA-233 Change-Id: If65aa0fac7ee7040e89d926bf115b4f124c5b4e2 Signed-off-by: OleksandrZharov Signed-off-by: Ivan Hrasko Signed-off-by: Robert Varga --- .../shiro/web/env/ShiroWebContextSecurer.java | 18 +- .../aaa/shiro/web/env/WebInitializer.java | 19 +- web/api/pom.xml | 10 +- .../opendaylight/aaa/web/FilterDetails.java | 180 ++++++++++- .../opendaylight/aaa/web/ResourceDetails.java | 96 +++++- .../opendaylight/aaa/web/ServletDetails.java | 179 ++++++++++- .../org/opendaylight/aaa/web/ServletSpec.java | 103 +++++++ .../org/opendaylight/aaa/web/WebContext.java | 284 +++++++++++++----- .../aaa/web/WebContextSecurer.java | 28 +- .../org/opendaylight/aaa/web/WebServer.java | 8 +- .../aaa/web/FilterDetailsTest.java | 77 +++-- .../aaa/web/ServletDetailsTest.java | 66 ++-- .../aaa/web/WebContextApiTest.java | 51 ++-- .../aaa/web/jetty/JettyWebServer.java | 13 +- .../aaa/web/test/AbstractWebServerTest.java | 27 +- .../aaa/web/osgi/WhiteboardWebServer.java | 6 +- 16 files changed, 908 insertions(+), 257 deletions(-) create mode 100644 web/api/src/main/java/org/opendaylight/aaa/web/ServletSpec.java diff --git a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/ShiroWebContextSecurer.java b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/ShiroWebContextSecurer.java index 3e0cf68b2..7e8f14bd9 100644 --- a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/ShiroWebContextSecurer.java +++ b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/ShiroWebContextSecurer.java @@ -9,11 +9,11 @@ package org.opendaylight.aaa.shiro.web.env; import static java.util.Objects.requireNonNull; +import java.util.Arrays; import org.apache.shiro.web.env.WebEnvironment; import org.opendaylight.aaa.shiro.filters.AAAShiroFilter; import org.opendaylight.aaa.web.FilterDetails; import org.opendaylight.aaa.web.WebContext; -import org.opendaylight.aaa.web.WebContextBuilder; import org.opendaylight.aaa.web.WebContextSecurer; /** @@ -29,14 +29,14 @@ public class ShiroWebContextSecurer implements WebContextSecurer { } @Override - public void requireAuthentication(final WebContextBuilder webContextBuilder, final boolean asyncSupported, + public void requireAuthentication(final WebContext.Builder webContextBuilder, final boolean asyncSupported, final String... urlPatterns) { - webContextBuilder - // AAA filter in front of these REST web services as well as for moon endpoints - .addFilter(FilterDetails.builder() - .filter(new AAAShiroFilter(webEnvironment)) - .addUrlPatterns(urlPatterns) - .asyncSupported(asyncSupported) - .build()); + // AAA filter in front of these REST web services as well as for moon endpoints + final var filterBuilder = FilterDetails.builder() + .filter(new AAAShiroFilter(webEnvironment)) + .asyncSupported(asyncSupported); + Arrays.stream(urlPatterns).forEach(filterBuilder::addUrlPattern); + + webContextBuilder.addFilter(filterBuilder.build()); } } diff --git a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/WebInitializer.java b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/WebInitializer.java index de82f6169..ff32ba47d 100644 --- a/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/WebInitializer.java +++ b/aaa-shiro/impl/src/main/java/org/opendaylight/aaa/shiro/web/env/WebInitializer.java @@ -19,7 +19,6 @@ import org.opendaylight.aaa.shiro.idm.IdmLightApplication; import org.opendaylight.aaa.web.FilterDetails; import org.opendaylight.aaa.web.ServletDetails; import org.opendaylight.aaa.web.WebContext; -import org.opendaylight.aaa.web.WebContextBuilder; import org.opendaylight.aaa.web.WebContextSecurer; import org.opendaylight.aaa.web.WebServer; import org.opendaylight.aaa.web.servlet.ServletSupport; @@ -42,15 +41,21 @@ public class WebInitializer { final WebContextSecurer webContextSecurer, final ServletSupport servletSupport, final CustomFilterAdapterConfiguration customFilterAdapterConfig) throws ServletException { - WebContextBuilder webContextBuilder = WebContext.builder().contextPath("auth").supportsSessions(true) + final var webContextBuilder = WebContext.builder() + .contextPath("/auth") + .supportsSessions(true) - .addServlet(ServletDetails.builder().servlet(servletSupport.createHttpServletBuilder( - new IdmLightApplication(iidMStore, claimCache)).build()) - .addUrlPattern("/*").build()) + .addServlet(ServletDetails.builder() + .servlet(servletSupport.createHttpServletBuilder(new IdmLightApplication(iidMStore, claimCache)) + .build()) + .addUrlPattern("/*") + .build()) // Allows user to add javax.servlet.Filter(s) in front of REST services - .addFilter(FilterDetails.builder().filter(new CustomFilterAdapter(customFilterAdapterConfig)) - .addUrlPattern("/*").build()); + .addFilter(FilterDetails.builder() + .filter(new CustomFilterAdapter(customFilterAdapterConfig)) + .addUrlPattern("/*") + .build()); webContextSecurer.requireAuthentication(webContextBuilder, "/*", "/moon/*"); diff --git a/web/api/pom.xml b/web/api/pom.xml index 3dc710e44..5a4b265a0 100644 --- a/web/api/pom.xml +++ b/web/api/pom.xml @@ -22,15 +22,13 @@ bundle - - javax.servlet - javax.servlet-api + com.google.guava + guava - org.immutables - value - annotations + javax.servlet + javax.servlet-api org.opendaylight.yangtools diff --git a/web/api/src/main/java/org/opendaylight/aaa/web/FilterDetails.java b/web/api/src/main/java/org/opendaylight/aaa/web/FilterDetails.java index a5d848ea9..d9ed02604 100644 --- a/web/api/src/main/java/org/opendaylight/aaa/web/FilterDetails.java +++ b/web/api/src/main/java/org/opendaylight/aaa/web/FilterDetails.java @@ -7,37 +7,187 @@ */ package org.opendaylight.aaa.web; +import static java.util.Objects.requireNonNull; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import java.util.List; import java.util.Map; import javax.servlet.Filter; -import org.immutables.value.Value; -import org.immutables.value.Value.Default; +import org.eclipse.jdt.annotation.NonNull; /** * Details about a {@link Filter}. * * @author Michael Vorburger.ch */ -@Value.Immutable -@Value.Style(visibility = Value.Style.ImplementationVisibility.PRIVATE, depluralize = true) public interface FilterDetails { + /** + * Get a {@link Filter} instance. + * + * @return {@link Filter} instance + */ + @NonNull Filter filter(); - static FilterDetailsBuilder builder() { - return new FilterDetailsBuilder(); - } + /** + * Get Filter's name. + * + * @return {@link String} + */ + @NonNull String name(); + + /** + * Get list of Filter URL patterns. These patterns control where filter is applied. + * + *

+ * Restrictions to URLs and how it should look like are next: + *

    + *
  • A string beginning with a ‘ / ’ character and ending with a ‘ /*’ suffix is used for path mapping.
    • + *
  • A string beginning with a ‘ *. ’ prefix is used as an extension mapping.
    • + *
  • The empty string ("") is a special URL pattern that exactly maps to the application's context root, i.e., + * requests of the form {@code http://host:port/context-root/}. In this case the path info is ’ / ’ and the + * servlet path and context path is empty string (““).
    • + *
  • A string containing only the ’ / ’ character indicates the "default" servlet of the application. In this + * case the servlet path is the request URI minus the context path and the path info is null.
    • + *
  • All other strings are used for exact matches only.
    • + *
+ * + * @return {@link List} of Filter URL patterns + * @see "Java Servlet Specification Version 3.1, Section 12.2 Specification of Mappings" + */ + @NonNull List urlPatterns(); - Filter filter(); + /** + * Get Filter initial parameters. + * + * @return {@link Map} that contains initial parameters + */ + @NonNull Map initParams(); - @Default default String name() { - return filter().getClass().getName(); + /** + * Get indication whether {@link #filter()} supports asynchronous processing. + * + * @return {@code true} if the filter supports asynchronous processing + * @see "Java Servlet Specification Version 3.1, Section 2.3.3.3 Asynchronous Processing" + */ + boolean asyncSupported(); + + /** + * Create a builder for {@link FilterDetails}. + * + * @return {@link Builder} builder instance + */ + static @NonNull Builder builder() { + return new Builder(); } - List urlPatterns(); + /** + * Builds instances of type {@link FilterDetails FilterDetails}. Initialize attributes and then invoke the + * {@link #build()} method to create an immutable instance. + * + *

{@code FilterDetails.Builder} is not thread-safe and generally should not be stored in a field or + * collection, but instead used immediately to create instances. + */ + final class Builder { + private record ImmutableFilterDetails(Filter filter, String name, ImmutableList urlPatterns, + ImmutableMap initParams, boolean asyncSupported) implements FilterDetails { + ImmutableFilterDetails { + if (urlPatterns.isEmpty()) { + throw new IllegalStateException("No urlPattern specified"); + } + } + } - Map initParams(); + private final ImmutableMap.Builder initParams = ImmutableMap.builder(); + private final ImmutableList.Builder urlPatterns = ImmutableList.builder(); + private Filter filter; + private String name; + private boolean asyncSupported; - @Default default Boolean getAsyncSupported() { - return false; - } + private Builder() { + // Hidden on purpose + } + + /** + * Initializes the value for the {@link FilterDetails#filter() filter} attribute. + * + * @param filter The value for filter + * @return {@code this} builder for use in a chained invocation + */ + @SuppressWarnings("checkstyle:hiddenField") + public @NonNull Builder filter(final Filter filter) { + this.filter = requireNonNull(filter); + return this; + } + /** + * Initializes the value for the {@link FilterDetails#name() name} attribute. + * + *

If not set, this attribute will have a value corresponding to {@code filter().getClass().getName()}. + * + * + * @param name The value for name + * @return {@code this} builder for use in a chained invocation + * @throws NullPointerException if {code name} is {@code null} + */ + @SuppressWarnings("checkstyle:hiddenField") + public @NonNull Builder name(final String name) { + this.name = requireNonNull(name); + return this; + } + + /** + * Adds one element to {@link FilterDetails#urlPatterns() urlPatterns} list. + * + * @param urlPattern A urlPatterns element + * @return {@code this} builder for use in a chained invocation + * @throws NullPointerException if {code urlPattern} is {@code null} + * @throws IllegalArgumentException if {@code urlPattern} does not meet specification criteria + */ + public @NonNull Builder addUrlPattern(final String urlPattern) { + urlPatterns.add(ServletSpec.requireMappingSpec(urlPattern)); + return this; + } + + /** + * Put one entry to the {@link FilterDetails#initParams() initParams} map. + * + * @param key The key in the initParams map + * @param value The associated value in the initParams map + * @return {@code this} builder for use in a chained invocation + * @throws NullPointerException if any argument is {@code null} + */ + public @NonNull Builder putInitParam(final String key, final String value) { + initParams.put(key, value); + return this; + } + + /** + * Initializes the value for the {@link FilterDetails#asyncSupported() asyncSupported} attribute. + * + *

If not set, this attribute will have a default value of {@code false}. + * + * @param asyncSupported The value for asyncSupported + * @return {@code this} builder for use in a chained invocation + */ + @SuppressWarnings("checkstyle:hiddenField") + public @NonNull Builder asyncSupported(final boolean asyncSupported) { + this.asyncSupported = asyncSupported; + return this; + } + + /** + * Builds a new {@link FilterDetails FilterDetails}. + * + * @return An immutable instance of FilterDetails + * @throws IllegalStateException if any required attributes are missing + */ + public @NonNull FilterDetails build() { + if (filter == null) { + throw new IllegalStateException("No filter specified"); + } + return new ImmutableFilterDetails(filter, name != null ? name : filter.getClass().getName(), + urlPatterns.build(), initParams.build(), asyncSupported); + } + } } diff --git a/web/api/src/main/java/org/opendaylight/aaa/web/ResourceDetails.java b/web/api/src/main/java/org/opendaylight/aaa/web/ResourceDetails.java index 0d1a9e1a6..1c12844d4 100644 --- a/web/api/src/main/java/org/opendaylight/aaa/web/ResourceDetails.java +++ b/web/api/src/main/java/org/opendaylight/aaa/web/ResourceDetails.java @@ -7,32 +7,104 @@ */ package org.opendaylight.aaa.web; -import org.immutables.value.Value; -import org.immutables.value.Value.Default; +import static java.util.Objects.requireNonNull; + +import org.eclipse.jdt.annotation.NonNull; /** * Details about a resource registration. * * @author Thomas Pantelis */ -@Value.Immutable -@Value.Style(visibility = Value.Style.ImplementationVisibility.PRIVATE, depluralize = true) public interface ResourceDetails { - - static ResourceDetailsBuilder builder() { - return new ResourceDetailsBuilder(); - } - /** + * Get resource base name. + * + *

* The base name of the resources that will be registered, typically a directory in the bundle/jar where "/" * is used to denote the root. + * + * @return {@link String} base name */ - String name(); + @NonNull String name(); /** + * Get resource mapped alias. + * + *

* The name in the URI namespace to which the resources are mapped. This defaults to the {@link #name()}. + * + * @return {@link String} mapped alias + */ + @NonNull String alias(); + + /** + * Create builder for {@code ResourceDetails}. + * + * @return {@link Builder} builder instance + */ + static @NonNull Builder builder() { + return new Builder(); + } + + /** + * Builds instances of type {@link ResourceDetails ResourceDetails}. Initialize attributes and then invoke the + * {@link #build()} method to create an immutable instance. + * + *

{@code ResourceDetails.Builder} is not thread-safe and generally should not be stored in a field or + * collection, but instead used immediately to create instances. */ - @Default default String alias() { - return name(); + final class Builder { + private record ImmutableResourceDetails(String name, String alias) implements ResourceDetails { + // Not much else here + } + + private String name; + private String alias; + + private Builder() { + // Hidden on purpose + } + + /** + * Initializes the value for the {@link ResourceDetails#name() name} attribute. + * + * @param name The value for name + * @return {@code this} builder for use in a chained invocation + * @throws NullPointerException if {code name} is {@code null} + */ + @SuppressWarnings("checkstyle:hiddenField") + public @NonNull Builder name(final String name) { + this.name = requireNonNull(name); + return this; + } + + /** + * Initializes the value for the {@link ResourceDetails#alias() alias} attribute. + * + *

If not set, this attribute will have the same as {@link ResourceDetails#name() name}. + * + * @param alias The value for alias + * @return {@code this} builder for use in a chained invocation + * @throws NullPointerException if {code alias} is {@code null} + */ + @SuppressWarnings("checkstyle:hiddenField") + public @NonNull Builder alias(final String alias) { + this.alias = requireNonNull(alias); + return this; + } + + /** + * Builds a new {@link ResourceDetails ResourceDetails}. + * + * @return An immutable instance of ResourceDetails + * @throws IllegalStateException if any required attributes are missing + */ + public @NonNull ResourceDetails build() { + if (name == null) { + throw new IllegalStateException("name not specified"); + } + return new ImmutableResourceDetails(name, alias == null ? name : alias); + } } } diff --git a/web/api/src/main/java/org/opendaylight/aaa/web/ServletDetails.java b/web/api/src/main/java/org/opendaylight/aaa/web/ServletDetails.java index 2f3aa0cf8..a6180a6ab 100644 --- a/web/api/src/main/java/org/opendaylight/aaa/web/ServletDetails.java +++ b/web/api/src/main/java/org/opendaylight/aaa/web/ServletDetails.java @@ -7,36 +7,187 @@ */ package org.opendaylight.aaa.web; +import static java.util.Objects.requireNonNull; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import java.util.List; import java.util.Map; import javax.servlet.Servlet; -import org.immutables.value.Value; -import org.immutables.value.Value.Default; +import org.eclipse.jdt.annotation.NonNull; /** * Details about a {@link Servlet}. * * @author Michael Vorburger.ch */ -@Value.Immutable -@Value.Style(visibility = Value.Style.ImplementationVisibility.PRIVATE, depluralize = true) public interface ServletDetails { + /** + * Get a {@link Servlet} instance. + * + * @return {@link Servlet} instance + */ + @NonNull Servlet servlet(); - static ServletDetailsBuilder builder() { - return new ServletDetailsBuilder(); - } + /** + * Get Servlet's name. + * + * @return {@link String} servlet name + */ + @NonNull String name(); + + /** + * Get list of servlet URL patterns. These patterns control how you access a servlet. + * + *

+ * Restrictions to URLs and how it should look like are next: + *

    + *
  • A string beginning with a ‘ / ’ character and ending with a ‘ /*’ suffix is used for path mapping.
    • + *
  • A string beginning with a ‘ *. ’ prefix is used as an extension mapping.
    • + *
  • The empty string ("") is a special URL pattern that exactly maps to the application's context root, i.e., + * requests of the form {@code http://host:port/context-root}. In this case the path info is ’ / ’ and the + * servlet path and context path is empty string (““).
    • + *
  • A string containing only the ’ / ’ character indicates the "default" servlet of the application. In this + * case the servlet path is the request URI minus the context path and the path info is null.
    • + *
  • All other strings are used for exact matches only.
    • + *
+ * + * @return {@link List} of Servlet URL patterns + * @see "Java Servlet Specification Version 3.1, Section 12.2 Specification of Mappings" + */ + @NonNull List urlPatterns(); - Servlet servlet(); + /** + * Get Servlet initial parameters. + * + * @return {@link Map} that contains initial parameters + */ + @NonNull Map initParams(); - @Default default String name() { - return servlet().getClass().getName(); + /** + * Get indication whether {@link #servlet()} supports asynchronous processing. + * + * @return {@code true} if the filter supports asynchronous processing + * @see "Java Servlet Specification Version 3.1, Section 2.3.3.3 Asynchronous Processing" + */ + boolean asyncSupported(); + + /** + * Create a builder for {@link ServletDetails}. + * + * @return {@link Builder} builder instance + */ + static @NonNull Builder builder() { + return new Builder(); } - List urlPatterns(); + /** + * Builds instances of type {@link ServletDetails ServletDetails}. Initialize attributes and then invoke the + * {@link #build()} method to create an immutable instance. + * + *

{@code ServletDetails.Builder} is not thread-safe and generally should not be stored in a field or + * collection, but instead used immediately to create instances. + */ + final class Builder { + private record ImmutableServletDetails(Servlet servlet, String name, ImmutableList urlPatterns, + ImmutableMap initParams, boolean asyncSupported) implements ServletDetails { + ImmutableServletDetails { + if (urlPatterns.isEmpty()) { + throw new IllegalStateException("No urlPattern specified"); + } + } + } + + private final ImmutableMap.Builder initParams = ImmutableMap.builder(); + private final ImmutableList.Builder urlPatterns = ImmutableList.builder(); + private Servlet servlet; + private String name; + private boolean asyncSupported; + + private Builder() { + // Hidden on purpose + } + + /** + * Initializes the value for the {@link ServletDetails#servlet() servlet} attribute. + * + * @param servlet The value for servlet + * @return {@code this} builder for use in a chained invocation + */ + @SuppressWarnings("checkstyle:hiddenField") + public @NonNull Builder servlet(final Servlet servlet) { + this.servlet = requireNonNull(servlet); + return this; + } + + /** + * Initializes the value for the {@link ServletDetails#name() name} attribute. + * + *

If not set, this attribute will have a value corresponding to {@code servlet().getClass().getName()}. + * + * + * @param name The value for name + * @return {@code this} builder for use in a chained invocation + * @throws NullPointerException if {code name} is {@code null} + */ + @SuppressWarnings("checkstyle:hiddenField") + public @NonNull Builder name(final String name) { + this.name = requireNonNull(name); + return this; + } + + /** + * Adds one element to {@link ServletDetails#urlPatterns() urlPatterns} list. + * + * @param urlPattern A urlPatterns element + * @return {@code this} builder for use in a chained invocation + * @throws NullPointerException if {code urlPattern} is {@code null} + * @throws IllegalArgumentException if {@code urlPattern} does not meet specification criteria + */ + public @NonNull Builder addUrlPattern(final String urlPattern) { + urlPatterns.add(ServletSpec.requireMappingSpec(urlPattern)); + return this; + } + + /** + * Put one entry to the {@link ServletDetails#initParams() initParams} map. + * + * @param key The key in the initParams map + * @param value The associated value in the initParams map + * @return {@code this} builder for use in a chained invocation + * @throws NullPointerException if any argument is {@code null} + */ + public @NonNull Builder putInitParam(final String key, final String value) { + initParams.put(key, value); + return this; + } - Map initParams(); + /** + * Initializes the value for the {@link ServletDetails#asyncSupported() asyncSupported} attribute. + * + *

If not set, this attribute will have a default value of {@code false}. + * + * @param asyncSupported The value for asyncSupported + * @return {@code this} builder for use in a chained invocation + */ + @SuppressWarnings("checkstyle:hiddenField") + public @NonNull Builder asyncSupported(final boolean asyncSupported) { + this.asyncSupported = asyncSupported; + return this; + } - @Default default Boolean getAsyncSupported() { - return false; + /** + * Builds a new {@link ServletDetails ServletDetails}. + * + * @return An immutable instance of ServletDetails + * @throws IllegalStateException if any required attributes are missing + */ + public @NonNull ServletDetails build() { + if (servlet == null) { + throw new IllegalStateException("No servlet specified"); + } + return new ImmutableServletDetails(servlet, name != null ? name : servlet.getClass().getName(), + urlPatterns.build(), initParams.build(), asyncSupported); + } } } diff --git a/web/api/src/main/java/org/opendaylight/aaa/web/ServletSpec.java b/web/api/src/main/java/org/opendaylight/aaa/web/ServletSpec.java new file mode 100644 index 000000000..d48bfabe9 --- /dev/null +++ b/web/api/src/main/java/org/opendaylight/aaa/web/ServletSpec.java @@ -0,0 +1,103 @@ +/* + * Copyright (c) 2022 PANTHEON.tech, s.r.o. and others. All rights reserved. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v1.0 which accompanies this distribution, + * and is available at http://www.eclipse.org/legal/epl-v10.html + */ +package org.opendaylight.aaa.web; + +import static com.google.common.base.Preconditions.checkArgument; + +import org.eclipse.jdt.annotation.NonNull; + +/** + * Utility methods for dealing with aspects of Java Servlet Specification. We currently support + * + * version 3.1. + */ +final class ServletSpec { + private ServletSpec() { + // utility class + } + + /** + * Verify that the specified string is a valid Context Path as defined in Section 3.5. + * + * @param str String to check + * @return The string + * @throws IllegalArgumentException if {@code str} is not a valid context path + * @throws NullPointerException if {@code str} is {@code null} + */ + static @NonNull String requireContextPath(final String str) { + // We do not allow this: + // If this context is the “default” context rooted at the base of the + // Web server’s URL name space, this path will be an empty string. + checkArgument(!str.isEmpty(), "Context path is empty"); + + // Otherwise, if the + // context is not rooted at the root of the server’s name space, the path starts with a + // character but does not end with a / character. + checkArgument(str.charAt(0) == '/', "Context path '%s' does not start with '/'", str); + checkArgument(str.charAt(str.length() - 1) != '/', "Context path '%s' ends with '/'", str); + + // TODO: validate according to https://www.rfc-editor.org/rfc/rfc3986#section-3.3 + + return str; + } + + /** + * Verify that the specified string is a valid Specification of Mapping as defined in Section 12.2. + * + * @param str String to check + * @return The string + * @throws IllegalArgumentException if {@code str} is not a valid mapping specification + * @throws NullPointerException if {@code str} is {@code null} + */ + static @NonNull String requireMappingSpec(final String str) { + // Bullet 3: + // The empty string ("") is a special URL pattern that exactly maps to the + // application's context root, i.e., requests of the form http://host:port//. In this case the path info is ’ / ’ and the servlet path and context path is + // empty string (““). + if (str.isEmpty()) { + return ""; + } + + final char firstChar = str.charAt(0); + final int len = str.length(); + if (firstChar == '/') { + // Bullet 4: + // A string containing only the ’ / ’ character indicates the "default" servlet of the + // application. In this case the servlet path is the request URI minus the context path + // and the path info is null. + // otherwise ... + if (len != 1) { + // ... more checks starting at the second character + final int star = str.indexOf('*', 1); + checkArgument( + // Bullet 5: + // All other strings are used for exact matches only. + star == -1 + // or Bullet 1: + // A string beginning with a ‘ / ’ character and ending with a ‘ /*’ suffix is used for + // path mapping. + || star == len - 1 && str.charAt(star - 1) == '/', + // ... otherwise it is a '*' in an exact path + "Prefix-based spec '%s' with a '*' at offset %s", str, star); + } + } else { + // Bullet 2: + // A string beginning with a ‘ *. ’ prefix is used as an extension mapping + checkArgument(firstChar == '.' && len > 1 && str.charAt(1) == '*', + "Spec '%s' is neither prefix-based nor suffix-based", str); + + final int slash = str.indexOf('/', 2); + checkArgument(slash == -1, "Suffix-based spec '%s' with a '/' at offset %s", str, slash); + final int star = str.indexOf('*', 2); + checkArgument(star == -1, "Suffix-based spec '%s' with a '*' at offset %s", str, star); + } + + return str; + } +} diff --git a/web/api/src/main/java/org/opendaylight/aaa/web/WebContext.java b/web/api/src/main/java/org/opendaylight/aaa/web/WebContext.java index a8ca1a096..76928a025 100644 --- a/web/api/src/main/java/org/opendaylight/aaa/web/WebContext.java +++ b/web/api/src/main/java/org/opendaylight/aaa/web/WebContext.java @@ -7,137 +7,257 @@ */ package org.opendaylight.aaa.web; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import java.util.List; import java.util.Map; import javax.servlet.ServletContainerInitializer; import javax.servlet.ServletContext; import javax.servlet.ServletContextListener; import javax.servlet.ServletRegistration; -import org.immutables.value.Value; -import org.immutables.value.Value.Default; +import org.eclipse.jdt.annotation.NonNull; /** * Web Context with URL prefix. AKA Web App or Servlet context. * *

- * Its {@link WebContextBuilder} allows programmatic web component registration - * (as opposed to declarative e.g. via web.xml, OSGi HTTP Whiteboard blueprint - * integration, CXF BP etc.) + * Its {@link WebContext.Builder} allows programmatic web component registration (as opposed to declarative e.g. via + * web.xml, OSGi HTTP Whiteboard blueprint integration, CXF BP etc.) * *

* This is preferable because: *

    - *
  • using code instead of hiding class names in XML enables tools such as - * e.g. BND (in the maven-bundle-plugin) to correctly figure dependencies e.g. - * for OSGi Import-Package headers; - * - *
  • explicit passing of web components instances, instead of providing class - * names in XML files and letting a web container create the new instances using - * the default constructor, solves a pesky dependency injection (DI) related - * problem which typically leads to weird hoops in code through - * static etc. that can be avoided using this; - * - *
  • tests can more easily programmatically instantiate web components. + *
  • using code instead of hiding class names in XML enables tools such as e.g. BND (in the maven-bundle-plugin) to + * correctly figure dependencies e.g. for OSGi Import-Package headers;
    • + *
  • explicit passing of web components instances, instead of providing class names in XML files and letting a web + * container create the new instances using the default constructor, solves a pesky dependency injection (DI) + * related problem which typically leads to weird hoops in code through {@code static} etc. that can be avoided + * using this;
    • + *
  • tests can more easily programmatically instantiate web components.
    • *
* *

- * This, not surprisingly, looks somewhat like a Servlet (3.x) - * {@link ServletContext}, which also allows programmatic dynamic registration - * e.g. via {@link ServletRegistration}; however in practice direct use of that - * API has been found to be problematic under OSGi, because it is intended for - * JSE and does - * not easily appear to permit dynamic registration at any time (only during - * Servlet container initialization time by - * {@link ServletContainerInitializer}), and is generally less clear to use than - * this simple API which intentionally maps directly to what one would have - * declared in a web.xml file. This API is also slightly more focused and drops - * a number of concepts that API has which we do not want to support here - * (including e.g. security, roles, multipart etc.) + * This, not surprisingly, looks somewhat like a Servlet (3.x+) {@link ServletContext}, which also allows programmatic + * dynamic registration e.g. via {@link ServletRegistration}; however in practice direct use of that API has been found + * to be problematic under OSGi, because it is intended for JSE and + * does not easily appear to permit dynamic registration + * at any time (only during Servlet container initialization time by {@link ServletContainerInitializer}), and is + * generally less clear to use than this simple API which intentionally maps directly to what one would have declared in + * a web.xml file. This API is also slightly more focused and drops a number of concepts that API has which we do not + * want to support here (including e.g. security, roles, multipart etc.) * *

- * It also looks somewhat similar to the OSGi HttpService, but we want to avoid - * any org.osgi dependency (both API and impl) here, and that API is also less - * clear (and uses an ancient (!) {@link java.util.Dictionary} in its method - * signature), and -most importantly- simply does not support Filters and Listeners, only - * Servlets. The Pax Web API does extend the base OSGi API and adds supports for - * Filters, Listeners and context parameters, but is still OSGi specific, - * whereas this offers a much simpler standalone API without OSGi dependency. - * (The Pax Web API also has confusing signatures in its registerFilter() methods, - * where one can easily confuse which String[] is the urlPatterns; - * which we had initially done accidentally; and left AAA broken.) + * It also looks somewhat similar to the OSGi HttpService, but we want to avoid any org.osgi dependency (both API and + * impl) here, and that API is also less clear (and uses an ancient (!) {@link java.util.Dictionary} in its method + * signature), and -most importantly- simply does not support Filters and Listeners, only Servlets. The Pax Web API does + * extend the base OSGi API and adds supports for Filters, Listeners and context parameters, but is still OSGi specific, + * whereas this offers a much simpler standalone API without OSGi dependency. (The Pax Web API also has confusing + * signatures in its registerFilter() methods, where one can easily confuse which String[] is the urlPatterns; which we + * had initially done accidentally; and left AAA broken.) * *

- * This is immutable, with a Builder, because contrary to a declarative approach - * in a file such as web.xml, the registration order very much matters (e.g. an - * context parameter added after a Servlet registration would not be seen by that - * Servlet; or a Filter added to protect a Servlet might not yet be active - * for an instant if the registerServlet is before the registerFilter). - * Therefore, this API enforces atomicity and lets clients first register - * everything on the Builder, and only then use - * {@link WebServer#registerWebContext(WebContext)}. + * This is immutable, with a Builder, because contrary to a declarative approach in a file such as web.xml, the + * registration order very much matters (e.g. an context parameter added after a Servlet registration would not be seen + * by that Servlet; or a Filter added to protect a Servlet might not yet be active for an instant if the registerServlet + * is before the registerFilter). Therefore, this API enforces atomicity and lets clients first register everything on + * the Builder, and only then use {@link WebServer#registerWebContext(WebContext)}. * * @author Michael Vorburger.ch */ -@Value.Immutable -@Value.Style(visibility = Value.Style.ImplementationVisibility.PRIVATE, depluralize = true) -public abstract class WebContext { - - public static WebContextBuilder builder() { - return new WebContextBuilder(); - } - +public interface WebContext { /** - * Path which will be used as URL prefix to all registered servlets and filters. + * Get path which will be used as URL prefix to all registered servlets and filters. Guaranteed to be non-empty + * + * @return {@link String} path + * @see "Java Servlet Specification Version 3.1, Section 3.5 Request Path Elements" */ - public abstract String contextPath(); + @NonNull String contextPath(); /** - * Flag whether this context supports web sessions, defaults to true. + * Get flag value whether this context supports web sessions. + * + * @return boolean flag value */ - @Default - public boolean supportsSessions() { - return true; - } + boolean supportsSessions(); /** - * Servlets. + * Get list of servlets. + * + * @return {@link List} list of {@link ServletDetails} */ - public abstract List servlets(); + @NonNull List servlets(); /** - * Filters. + * Get list of filters. + * + * @return {@link List} list of {@link FilterDetails} */ - public abstract List filters(); + @NonNull List filters(); /** - * Listeners. + * Get list of servlet context listeners. + * + * @return {@link List} list of {@link ServletContextListener} */ - public abstract List listeners(); + @NonNull List listeners(); /** - * Registers resources (eg html files) that can be accessed via the URI namespace. + * Get lis of resources (e.g. html files) that can be accessed via the URI namespace. + * + * @return {@link List} list of {@link ResourceDetails} */ - public abstract List resources(); + @NonNull List resources(); /** - * Context params. These are the {@link ServletContext}s initial parameters; contrary to individual + * Get map of context params. + * + *

+ * These are the {@link ServletContext}s initial parameters; contrary to individual * {@link ServletDetails#initParams()} and {@link FilterDetails#initParams()}. While a ServletContext accepts * any Object as a parameter, that is not accepted in all implementations. Most notably OSGi HTTP Whiteboard * specification allows only String values, hence we are enforcing that. + * + * @return {@link Map} context parameters map */ - public abstract Map contextParams(); + @NonNull Map contextParams(); - @Value.Check - protected void check() { - servlets().forEach(servlet -> { - if (servlet.urlPatterns().isEmpty()) { - throw new IllegalArgumentException("Servlet has no URL: " + servlet.name()); - } - }); - filters().forEach(filter -> { - if (filter.urlPatterns().isEmpty()) { - throw new IllegalArgumentException("Filter has no URL: " + filter.name()); + /** + * Create builder for {@code WebContext}. + * + * @return {@link Builder} builder instance + */ + static @NonNull Builder builder() { + return new Builder(); + } + + /** + * Builds instances of type {@link WebContext WebContext}. Initialize attributes and then invoke the + * {@link #build()} method to create an immutable instance. + * + *

{@code WebContext.Builder} is not thread-safe and generally should not be stored in a field or + * collection, but instead used immediately to create instances. + */ + final class Builder { + private record ImmutableWebContext(String contextPath, ImmutableList servlets, + ImmutableList filters, ImmutableList listeners, + ImmutableList resources, ImmutableMap contextParams, + boolean supportsSessions) implements WebContext { + // Not much else here + } + + private final ImmutableMap.Builder contextParams = ImmutableMap.builder(); + private final ImmutableList.Builder servlets = ImmutableList.builder(); + private final ImmutableList.Builder filters = ImmutableList.builder(); + private final ImmutableList.Builder listeners = ImmutableList.builder(); + private final ImmutableList.Builder resources = ImmutableList.builder(); + private String contextPath; + private boolean supportsSessions = true; + + private Builder() { + // Hidden on purpose + } + + /** + * Initializes the value for the {@link WebContext#contextPath() contextPath} attribute. As per Servlet + * + * @param contextPath The value for contextPath + * @return {@code this} builder for use in a chained invocation + * @throws IllegalArgumentException if {@code contextPath} does not meet specification criteria + * @throws NullPointerException if {code contextPath} is {@code null} + */ + @SuppressWarnings("checkstyle:hiddenField") + public @NonNull Builder contextPath(final String contextPath) { + this.contextPath = ServletSpec.requireContextPath(contextPath); + return this; + } + + /** + * Adds one element to {@link WebContext#servlets() servlets} list. + * + * @param servlet A servlets element + * @return {@code this} builder for use in a chained invocation + * @throws NullPointerException if {code servlet} is {@code null} + */ + public @NonNull Builder addServlet(final ServletDetails servlet) { + servlets.add(servlet); + return this; + } + + /** + * Adds one element to {@link WebContext#filters() filters} list. + * + * @param filter A filters element + * @return {@code this} builder for use in a chained invocation + * @throws NullPointerException if {code filter} is {@code null} + */ + public @NonNull Builder addFilter(final FilterDetails filter) { + filters.add(filter); + return this; + } + + /** + * Adds one element to {@link WebContext#listeners() listeners} list. + * + * @param listener A listeners element + * @return {@code this} builder for use in a chained invocation + * @throws NullPointerException if {code listener} is {@code null} + */ + public @NonNull Builder addListener(final ServletContextListener listener) { + listeners.add(listener); + return this; + } + + /** + * Adds one element to {@link WebContext#resources() resources} list. + * + * @param resource A resources element + * @return {@code this} builder for use in a chained invocation + * @throws NullPointerException if {code resource} is {@code null} + */ + public @NonNull Builder addResource(final ResourceDetails resource) { + resources.add(resource); + return this; + } + + /** + * Put one entry to the {@link WebContext#contextParams() contextParams} map. + * + * @param key The key in the contextParams map + * @param value The associated value in the contextParams map + * @return {@code this} builder for use in a chained invocation + * @throws NullPointerException if any argument is {@code null} + */ + public @NonNull Builder putContextParam(final String key, final String value) { + contextParams.put(key, value); + return this; + } + + /** + * Initializes the value for the {@link WebContext#supportsSessions() supportsSessions} attribute. + * + *

If not set, this attribute will have a default value of {@code true}. + * + * @param supportsSessions The value for supportsSessions + * @return {@code this} builder for use in a chained invocation + */ + @SuppressWarnings("checkstyle:hiddenField") + public Builder supportsSessions(final boolean supportsSessions) { + this.supportsSessions = supportsSessions; + return this; + } + + /** + * Builds a new {@link WebContext WebContext}. + * + * @return An immutable instance of WebContext + * @throws IllegalStateException if any required attributes are missing + */ + public @NonNull WebContext build() { + if (contextPath == null) { + throw new IllegalStateException("No contextPath specified"); } - }); + return new ImmutableWebContext(contextPath, servlets.build(), filters.build(), listeners.build(), + resources.build(), contextParams.build(), supportsSessions); + } } } diff --git a/web/api/src/main/java/org/opendaylight/aaa/web/WebContextSecurer.java b/web/api/src/main/java/org/opendaylight/aaa/web/WebContextSecurer.java index 2b7f6e7e7..fac18b871 100644 --- a/web/api/src/main/java/org/opendaylight/aaa/web/WebContextSecurer.java +++ b/web/api/src/main/java/org/opendaylight/aaa/web/WebContextSecurer.java @@ -8,13 +8,16 @@ package org.opendaylight.aaa.web; /** - * Secures a {@link WebContextBuilder}. + * Secures a {@link WebContext.Builder}. * * @author Michael Vorburger.ch */ public interface WebContextSecurer { /** - * Configures the WebContext in an implementation specific manner so that it requires authentication to access the + * Configure the WebContext to require auth for specified URLs. + * + *

+ * Configure the WebContext so that it requires authentication to access the * given URL Patterns. Typically, this will be done by adding a {@code javax.servlet.Filter} (or several, and * whatever else they need). * @@ -22,25 +25,36 @@ public interface WebContextSecurer { * @param asyncSupported true if asynchronous communication should also be supported * @param urlPatterns URL patterns that require authentication */ - void requireAuthentication(WebContextBuilder webContextBuilder, boolean asyncSupported, String... urlPatterns); + void requireAuthentication(WebContext.Builder webContextBuilder, boolean asyncSupported, String... urlPatterns); /** - * Configures the WebContext in an implementation specific manner so that it requires authentication to access the + * Configure the WebContext to require auth for specified URLs. + * + *

+ * Configures the WebContext so that it requires authentication to access the * given URL Patterns. Typically, this will be done by adding a {@code javax.servlet.Filter} (or several, and * whatever else they need). * *

* This method is equivalent to {@code requireAuthentication(webContextBuilder, false, urlPatterns}. + * + * @param webContextBuilder builder to secure + * @param urlPatterns URL patterns that require authentication */ - default void requireAuthentication(final WebContextBuilder webContextBuilder, final String... urlPatterns) { + default void requireAuthentication(final WebContext.Builder webContextBuilder, final String... urlPatterns) { requireAuthentication(webContextBuilder, false, urlPatterns); } /** + * Configure the WebContext to require auth all URLs. + * + *

* Configures the WebContext so that all its URL patterns ({@code/**}) require authentication. - * @see #requireAuthentication(WebContextBuilder, String...) + * + * @param webContextBuilder builder to secure + * @see #requireAuthentication(WebContext.Builder, String...) */ - default void requireAuthentication(final WebContextBuilder webContextBuilder) { + default void requireAuthentication(final WebContext.Builder webContextBuilder) { requireAuthentication(webContextBuilder, "/*"); } } diff --git a/web/api/src/main/java/org/opendaylight/aaa/web/WebServer.java b/web/api/src/main/java/org/opendaylight/aaa/web/WebServer.java index c7964381f..c08e07927 100644 --- a/web/api/src/main/java/org/opendaylight/aaa/web/WebServer.java +++ b/web/api/src/main/java/org/opendaylight/aaa/web/WebServer.java @@ -32,9 +32,11 @@ public interface WebServer { Registration registerWebContext(WebContext webContext) throws ServletException; /** - * Base URL of this web server, without any contexts. In production, this would - * likely be HTTPS with a well known hostname and fixed port configured e.g. in - * a Karaf etc/ configuration file. In tests, this would be typically be HTTP on + * Get base URL of this web server, without any contexts. + * + *

+ * In production, this would likely be HTTPS with a well known hostname and fixed port configured. + * For example, in Karaf etc/ configuration file. In tests, this would be typically be HTTP on * localhost and an arbitrarily chosen port. * * @return base URL, with http[s] prefix and port, NOT ending in slash diff --git a/web/api/src/test/java/org/opendaylight/aaa/web/FilterDetailsTest.java b/web/api/src/test/java/org/opendaylight/aaa/web/FilterDetailsTest.java index 2fd6c20da..6f66b4a1a 100644 --- a/web/api/src/test/java/org/opendaylight/aaa/web/FilterDetailsTest.java +++ b/web/api/src/test/java/org/opendaylight/aaa/web/FilterDetailsTest.java @@ -7,6 +7,7 @@ */ package org.opendaylight.aaa.web; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; @@ -18,47 +19,77 @@ import org.junit.Test; public class FilterDetailsTest { @Test public void testDefaultValue() { - FilterDetails filterDetails = FilterDetails.builder() - .filter(mock(Filter.class)) - .addUrlPattern("test") - .addUrlPattern("another") - .name("custom") - .putInitParam("key", "value") - .build(); + var filterDetails = FilterDetails.builder() + .filter(mock(Filter.class)) + .addUrlPattern("/test") + .addUrlPattern("/another") + .name("custom") + .putInitParam("key", "value") + .build(); - assertFalse(filterDetails.getAsyncSupported()); + assertFalse(filterDetails.asyncSupported()); } @Test public void testAsyncFalse() { - FilterDetails filterDetails = FilterDetails.builder() + var filterDetails = FilterDetails.builder() .filter(mock(Filter.class)) - .addUrlPattern("test") - .addUrlPattern("another") + .addUrlPattern("/test") + .addUrlPattern("/another") .name("custom") .putInitParam("key", "value") .asyncSupported(false) .build(); - assertFalse(filterDetails.getAsyncSupported()); + assertFalse(filterDetails.asyncSupported()); } @Test public void testAsyncTrue() { - FilterDetails filterDetails = FilterDetails.builder() - .filter(mock(Filter.class)) - .addUrlPattern("test") - .addUrlPattern("another") - .name("custom") - .putInitParam("key", "value") - .asyncSupported(true) - .build(); + var filterDetails = FilterDetails.builder() + .filter(mock(Filter.class)) + .addUrlPattern("/test") + .addUrlPattern("/another") + .name("custom") + .putInitParam("key", "value") + .asyncSupported(true) + .build(); + + assertTrue(filterDetails.asyncSupported()); + } - assertTrue(filterDetails.getAsyncSupported()); + @Test + public void testEmptyBuilderException() { + final var builder = FilterDetails.builder(); + final var ex = assertThrows(IllegalStateException.class, builder::build); + assertEquals("No filter specified", ex.getMessage()); + } + + @Test + public void testBadFilterWithoutAnyURL() { + final var builder = FilterDetails.builder().filter(mock(Filter.class)); + final var ex = assertThrows(IllegalStateException.class, builder::build); + assertEquals("No urlPattern specified", ex.getMessage()); + } + + @Test + public void testNotPrefixNorSuffixPatternException() { + final var builder = FilterDetails.builder(); + final var ex = assertThrows(IllegalArgumentException.class, () -> builder.addUrlPattern("test")); + assertEquals("Spec 'test' is neither prefix-based nor suffix-based", ex.getMessage()); + } + + @Test + public void testIllegalPrefixPatternException() { + final var builder = FilterDetails.builder(); + final var ex = assertThrows(IllegalArgumentException.class, () -> builder.addUrlPattern("/*test")); + assertEquals("Prefix-based spec '/*test' with a '*' at offset 1", ex.getMessage()); } @Test - public void testException() { - assertThrows(IllegalStateException.class, () -> FilterDetails.builder().build()); + public void testIllegalSuffixPatternException() { + final var builder = FilterDetails.builder(); + final var ex = assertThrows(IllegalArgumentException.class, () -> builder.addUrlPattern("*./test")); + assertEquals("Spec '*./test' is neither prefix-based nor suffix-based", ex.getMessage()); } } diff --git a/web/api/src/test/java/org/opendaylight/aaa/web/ServletDetailsTest.java b/web/api/src/test/java/org/opendaylight/aaa/web/ServletDetailsTest.java index a57a6c97a..bb8278ca9 100644 --- a/web/api/src/test/java/org/opendaylight/aaa/web/ServletDetailsTest.java +++ b/web/api/src/test/java/org/opendaylight/aaa/web/ServletDetailsTest.java @@ -7,6 +7,7 @@ */ package org.opendaylight.aaa.web; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; @@ -18,47 +19,56 @@ import org.junit.Test; public class ServletDetailsTest { @Test public void testDefaultValue() { - ServletDetails servletDetails = ServletDetails.builder() - .servlet(mock(Servlet.class)) - .addUrlPattern("test") - .addUrlPattern("another") - .name("custom") - .putInitParam("key", "value") - .build(); + final var servletDetails = ServletDetails.builder() + .servlet(mock(Servlet.class)) + .addUrlPattern("/test") + .addUrlPattern("/another") + .name("custom") + .putInitParam("key", "value") + .build(); - assertFalse(servletDetails.getAsyncSupported()); + assertFalse(servletDetails.asyncSupported()); } @Test public void testAsyncFalse() { - ServletDetails servletDetails = ServletDetails.builder() - .servlet(mock(Servlet.class)) - .addUrlPattern("test") - .addUrlPattern("another") - .name("custom") - .putInitParam("key", "value") - .asyncSupported(false) - .build(); + final var servletDetails = ServletDetails.builder() + .servlet(mock(Servlet.class)) + .addUrlPattern("/test") + .addUrlPattern("/another") + .name("custom") + .putInitParam("key", "value") + .asyncSupported(false) + .build(); - assertFalse(servletDetails.getAsyncSupported()); + assertFalse(servletDetails.asyncSupported()); } @Test public void testAsyncTrue() { - ServletDetails servletDetails = ServletDetails.builder() - .servlet(mock(Servlet.class)) - .addUrlPattern("test") - .addUrlPattern("another") - .name("custom") - .putInitParam("key", "value") - .asyncSupported(true) - .build(); + final var servletDetails = ServletDetails.builder() + .servlet(mock(Servlet.class)) + .addUrlPattern("/test") + .addUrlPattern("/another") + .name("custom") + .putInitParam("key", "value") + .asyncSupported(true) + .build(); - assertTrue(servletDetails.getAsyncSupported()); + assertTrue(servletDetails.asyncSupported()); } @Test - public void testException() { - assertThrows(IllegalStateException.class, () -> ServletDetails.builder().build()); + public void testEmptyBuilderException() { + final var builder = ServletDetails.builder(); + final var ex = assertThrows(IllegalStateException.class, builder::build); + assertEquals("No servlet specified", ex.getMessage()); + } + + @Test + public void testBadServletWithoutAnyURL() { + final var builder = ServletDetails.builder().servlet(mock(Servlet.class)); + final var ex = assertThrows(IllegalStateException.class, builder::build); + assertEquals("No urlPattern specified", ex.getMessage()); } } diff --git a/web/api/src/test/java/org/opendaylight/aaa/web/WebContextApiTest.java b/web/api/src/test/java/org/opendaylight/aaa/web/WebContextApiTest.java index 12ae81413..f86217b58 100644 --- a/web/api/src/test/java/org/opendaylight/aaa/web/WebContextApiTest.java +++ b/web/api/src/test/java/org/opendaylight/aaa/web/WebContextApiTest.java @@ -19,7 +19,6 @@ import java.util.Map; import javax.servlet.Filter; import javax.servlet.Servlet; import javax.servlet.ServletContextListener; -import org.junit.Ignore; import org.junit.Test; /** @@ -32,20 +31,20 @@ import org.junit.Test; public class WebContextApiTest { @Test public void testEmptyBuilder() { - final WebContextBuilder builder = WebContext.builder(); - assertThrows(IllegalStateException.class, () -> builder.build()); + final var builder = WebContext.builder(); + assertThrows(IllegalStateException.class, builder::build); } @Test public void testMinimalBuilder() { - assertTrue(WebContext.builder().contextPath("test").build().supportsSessions()); - assertEquals("test", WebContext.builder().contextPath("test").supportsSessions(false).build().contextPath()); + assertTrue(WebContext.builder().contextPath("/test").build().supportsSessions()); + assertEquals("/test", WebContext.builder().contextPath("/test").supportsSessions(false).build().contextPath()); } @Test public void testAddSimpleServlet() { - WebContext webContext = WebContext.builder().contextPath("test") - .addServlet(ServletDetails.builder().servlet(mock(Servlet.class)).addUrlPattern("test").build()) + WebContext webContext = WebContext.builder().contextPath("/test") + .addServlet(ServletDetails.builder().servlet(mock(Servlet.class)).addUrlPattern("/test").build()) .build(); assertThat(webContext.servlets(), hasSize(1)); ServletDetails firstServletDetail = webContext.servlets().get(0); @@ -55,50 +54,42 @@ public class WebContextApiTest { @Test public void testAddFullServlet() { - WebContext.builder().contextPath("test").addServlet(ServletDetails.builder().servlet(mock(Servlet.class)) - .addUrlPattern("test").addUrlPattern("another").name("custom").putInitParam("key", "value").build()) + WebContext.builder().contextPath("/test").addServlet(ServletDetails.builder().servlet(mock(Servlet.class)) + .addUrlPattern("/test").addUrlPattern("/another").name("custom").putInitParam("key", "value").build()) .build(); } @Test public void testAddFilter() { - WebContext.builder().contextPath("test") - .addFilter(FilterDetails.builder().filter(mock(Filter.class)).addUrlPattern("test").build()).build(); + WebContext.builder().contextPath("/test") + .addFilter(FilterDetails.builder().filter(mock(Filter.class)).addUrlPattern("/test").build()).build(); } @Test public void testAddListener() { - assertThat(WebContext.builder().contextPath("test").addListener(mock(ServletContextListener.class)).build() + assertThat(WebContext.builder().contextPath("/test").addListener(mock(ServletContextListener.class)).build() .listeners(), hasSize(1)); } @Test public void testContextParam() { assertEquals(Map.of("key", "value"), - WebContext.builder().contextPath("test").putContextParam("key", "value").build().contextParams()); + WebContext.builder().contextPath("/test").putContextParam("key", "value").build().contextParams()); } @Test - @Ignore public void testBadContextPath() { - // FIXME: this is completely broken usage -- which call is expected to raise the exception?! - assertThrows(IllegalArgumentException.class, () -> WebContext.builder().contextPath("test/sub").build()); - assertThrows(IllegalArgumentException.class, () -> WebContext.builder().contextPath("test space").build()); - assertThrows(IllegalArgumentException.class, () -> WebContext.builder().contextPath("/test").build()); - assertThrows(IllegalArgumentException.class, () -> WebContext.builder().contextPath("test/").build()); + assertBadContextPath("Context path is empty", ""); + assertBadContextPath("Context path 'test/sub' does not start with '/'", "test/sub"); + assertBadContextPath("Context path 'test space' does not start with '/'", "test space"); + assertBadContextPath("Context path 'test/' does not start with '/'", "test/"); + assertBadContextPath("Context path '/test/' ends with '/'", "/test/"); } - @Test - public void testBadServletWithoutAnyURL() { - final WebContextBuilder builder = WebContext.builder().contextPath("test") - .addServlet(ServletDetails.builder().servlet(mock(Servlet.class)).build()); - assertThrows(IllegalArgumentException.class, () -> builder.build()); + private static void assertBadContextPath(final String expectedMessage, final String contextPath) { + final var builder = WebContext.builder(); + final var ex = assertThrows(IllegalArgumentException.class, () -> builder.contextPath(contextPath)); + assertEquals(expectedMessage, ex.getMessage()); } - @Test - public void testBadFilterWithoutAnyURL() { - final WebContextBuilder builder = WebContext.builder().contextPath("test") - .addFilter(FilterDetails.builder().filter(mock(Filter.class)).build()); - assertThrows(IllegalArgumentException.class, () -> builder.build()); - } } diff --git a/web/impl-jetty/src/main/java/org/opendaylight/aaa/web/jetty/JettyWebServer.java b/web/impl-jetty/src/main/java/org/opendaylight/aaa/web/jetty/JettyWebServer.java index bd811c265..8a3df0f9f 100644 --- a/web/impl-jetty/src/main/java/org/opendaylight/aaa/web/jetty/JettyWebServer.java +++ b/web/impl-jetty/src/main/java/org/opendaylight/aaa/web/jetty/JettyWebServer.java @@ -92,8 +92,7 @@ public class JettyWebServer implements WebServer { @Override public synchronized Registration registerWebContext(final WebContext webContext) throws ServletException { - String contextPathWithSlashPrefix = ensureAbsolutePath(webContext.contextPath()); - ServletContextHandler handler = new ServletContextHandler(contextHandlerCollection, contextPathWithSlashPrefix, + ServletContextHandler handler = new ServletContextHandler(contextHandlerCollection, webContext.contextPath(), webContext.supportsSessions() ? ServletContextHandler.SESSIONS : ServletContextHandler.NO_SESSIONS); // The order in which we do things here must be the same as @@ -111,7 +110,7 @@ public class JettyWebServer implements WebServer { FilterHolder filterHolder = new FilterHolder(filter.filter()); filterHolder.setInitParameters(filter.initParams()); filter.urlPatterns().forEach( - urlPattern -> handler.addFilter(filterHolder, ensureAbsolutePath(urlPattern), + urlPattern -> handler.addFilter(filterHolder, urlPattern, EnumSet.allOf(DispatcherType.class)) ); }); @@ -120,11 +119,11 @@ public class JettyWebServer implements WebServer { webContext.servlets().forEach(servlet -> { ServletHolder servletHolder = new ServletHolder(servlet.name(), servlet.servlet()); servletHolder.setInitParameters(servlet.initParams()); - servletHolder.setAsyncSupported(servlet.getAsyncSupported()); + servletHolder.setAsyncSupported(servlet.asyncSupported()); // AKA 1 servletHolder.setInitOrder(1); servlet.urlPatterns().forEach( - urlPattern -> handler.addServlet(servletHolder, ensureAbsolutePath(urlPattern)) + urlPattern -> handler.addServlet(servletHolder, urlPattern) ); }); @@ -155,8 +154,4 @@ public class JettyWebServer implements WebServer { } contextHandlerCollection.removeHandler(handler); } - - private static String ensureAbsolutePath(final String path) { - return path.startsWith("/") ? path : "/" + path; - } } diff --git a/web/impl-jetty/src/test/java/org/opendaylight/aaa/web/test/AbstractWebServerTest.java b/web/impl-jetty/src/test/java/org/opendaylight/aaa/web/test/AbstractWebServerTest.java index 56426de3b..374663684 100644 --- a/web/impl-jetty/src/test/java/org/opendaylight/aaa/web/test/AbstractWebServerTest.java +++ b/web/impl-jetty/src/test/java/org/opendaylight/aaa/web/test/AbstractWebServerTest.java @@ -9,6 +9,7 @@ package org.opendaylight.aaa.web.test; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import com.google.common.io.CharStreams; @@ -44,15 +45,12 @@ public abstract class AbstractWebServerTest { } @Test - public void testAddAfterStartWithoutSlashOnContext() throws ServletException, IOException { - // NB subtle difference to previous test: contextPath("test1") instead of /test1 with slash! - var webContext = WebContext.builder() - .contextPath("test1") - .addServlet(ServletDetails.builder().addUrlPattern("/*").name("Test").servlet(new TestServlet()).build()) - .build(); - try (var webContextRegistration = getWebServer().registerWebContext(webContext)) { - checkTestServlet(getWebServer().getBaseURL() + "/test1"); - } + public void testAddAfterStartWithoutSlashOnServlet() throws ServletException, IOException { + // NB subtle difference to testAddAfterStart() test: addUrlPattern("*") instead of /* with slash! + var builder = WebContext.builder() + .contextPath("/test1"); + assertThrows(IllegalArgumentException.class, () -> builder + .addServlet(ServletDetails.builder().addUrlPattern("*").name("Test").servlet(new TestServlet()).build())); } @Test @@ -68,6 +66,17 @@ public abstract class AbstractWebServerTest { } } + @Test + public void testAddFilterWithoutSlash() throws Exception { + // NB subtle difference to previous test: addUrlPattern("*") instead of /* with slash! + var testFilter = new TestFilter(); + var builder = WebContext.builder() + .contextPath("/testingFilters") + .putContextParam("testParam1", "avalue"); + assertThrows(IllegalArgumentException.class, () -> builder + .addFilter(FilterDetails.builder().addUrlPattern("*").name("Test").filter(testFilter).build())); + } + @Test public void testRegisterListener() throws Exception { var testListener = new TestListener(); diff --git a/web/impl-osgi/src/main/java/org/opendaylight/aaa/web/osgi/WhiteboardWebServer.java b/web/impl-osgi/src/main/java/org/opendaylight/aaa/web/osgi/WhiteboardWebServer.java index c353c453e..6a2b16b6a 100644 --- a/web/impl-osgi/src/main/java/org/opendaylight/aaa/web/osgi/WhiteboardWebServer.java +++ b/web/impl-osgi/src/main/java/org/opendaylight/aaa/web/osgi/WhiteboardWebServer.java @@ -106,7 +106,7 @@ public final class WhiteboardWebServer implements WebServer { // The order in which we set things up here matters... // 1. ServletContextHelper, to which all others are bound to - final var contextPath = absolutePath(webContext.contextPath()); + final var contextPath = webContext.contextPath(); // TODO: can we create a better name? final var contextName = contextPath + ".id"; @@ -178,7 +178,7 @@ public final class WhiteboardWebServer implements WebServer { private static Map filterProperties(final String contextSelect, final FilterDetails filter) { final var builder = ImmutableMap.builder() .put(HttpWhiteboardConstants.HTTP_WHITEBOARD_CONTEXT_SELECT, contextSelect) - .put(HttpWhiteboardConstants.HTTP_WHITEBOARD_FILTER_ASYNC_SUPPORTED, filter.getAsyncSupported()) + .put(HttpWhiteboardConstants.HTTP_WHITEBOARD_FILTER_ASYNC_SUPPORTED, filter.asyncSupported()) .put(HttpWhiteboardConstants.HTTP_WHITEBOARD_FILTER_NAME, filter.name()) .put(HttpWhiteboardConstants.HTTP_WHITEBOARD_FILTER_PATTERN, absolutePatterns(filter.urlPatterns())); @@ -201,7 +201,7 @@ public final class WhiteboardWebServer implements WebServer { private static Map servletProperties(final String contextSelect, final ServletDetails servlet) { final var builder = ImmutableMap.builder() .put(HttpWhiteboardConstants.HTTP_WHITEBOARD_CONTEXT_SELECT, contextSelect) - .put(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_ASYNC_SUPPORTED, servlet.getAsyncSupported()) + .put(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_ASYNC_SUPPORTED, servlet.asyncSupported()) .put(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_NAME, servlet.name()) .put(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_PATTERN, absolutePatterns(servlet.urlPatterns())); -- 2.36.6