Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PLUGINAPI-42 Migrate javax -> jakarta (backward compatibility) #92

Closed
wants to merge 6 commits into from
Closed

PLUGINAPI-42 Migrate javax -> jakarta (backward compatibility) #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
98c911e
PLUGINAPI-42 Create new custom types for http request and response
ericg138 Apr 12, 2023
afdd169
PLUGINAPI-42 Use new types for identity providers, deprecate javax me…
ericg138 Apr 13, 2023
945a314
PLUGINAPI-42 Introduce HttpFilter class and deprecate ServletFilter w…
jacek-poreda-sonarsource Apr 13, 2023
f5e6e85
PLUGINAPI-42 Add new methods required by filters in SonarQube
jacek-poreda-sonarsource Apr 14, 2023
8c10020
PLUGINAPI-42 Deprecate javax.servlet usage in org.sonar.api.security …
antoine-vinot-sonarsource Apr 13, 2023
3e275a1
PLUGINAPI-42 Add unit tests
ericg138 Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion plugin-api/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ dependencies {
implementation project(':check-api')

compileOnly libs.jsr305
compileOnly libs.servlet.api
compileOnly libs.javax.servlet.api
compileOnly libs.jakarta.servlet.api

testImplementation libs.junit4
testImplementation libs.junit5
Expand Down
31 changes: 31 additions & 0 deletions plugin-api/src/main/java/org/sonar/api/security/Authenticator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import javax.servlet.http.HttpServletRequest;
import org.sonar.api.ExtensionPoint;
import org.sonar.api.server.ServerSide;
import org.sonar.api.server.http.HttpRequest;
import org.sonar.api.server.http.JavaxHttpRequest;

import static java.util.Objects.requireNonNull;

Expand All @@ -43,13 +45,31 @@ public abstract class Authenticator {
public static final class Context {
private String username;
private String password;
/**
* @deprecated since 9.16 use {@link #httpRequest} instead
*/
@Deprecated(since = "9.16", forRemoval = true)
private HttpServletRequest request;
private HttpRequest httpRequest;

/**
* @deprecated since 9.16 use {@link #Context(String, String, HttpRequest)} instead
*/
@Deprecated(since = "9.16", forRemoval = true)
public Context(@Nullable String username, @Nullable String password, HttpServletRequest request) {
requireNonNull(request);
this.request = request;
this.username = username;
this.password = password;
this.httpRequest = new JavaxHttpRequest(request);
}

public Context(@Nullable String username, @Nullable String password, HttpRequest httpRequest) {
requireNonNull(httpRequest);
this.httpRequest = httpRequest;
this.username = username;
this.password = password;
this.request = (HttpServletRequest) httpRequest.getRawRequest();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'd probably be better for products to pass both HttpRequest and HttpServletRequest.

}

/**
Expand All @@ -66,8 +86,19 @@ public String getPassword() {
return password;
}

/**
* @deprecated since 9.16. Use {@link #getHttpRequest()} instead.
*/
@Deprecated(since = "9.16", forRemoval = true)
public HttpServletRequest getRequest() {
return request;
}

/**
* @since 9.16
*/
public HttpRequest getHttpRequest() {
return httpRequest;
}
}
}
29 changes: 29 additions & 0 deletions plugin-api/src/main/java/org/sonar/api/security/ExternalGroupsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import java.util.Collection;
import javax.annotation.CheckForNull;
import javax.servlet.http.HttpServletRequest;
import org.sonar.api.server.http.HttpRequest;
import org.sonar.api.server.http.JavaxHttpRequest;

/**
* Note that prefix "do" for names of methods is reserved for future enhancements, thus should not be used in subclasses.
Expand All @@ -45,19 +47,46 @@ public Collection<String> doGetGroups(Context context) {

public static final class Context {
private String username;
/**
* @deprecated since 9.16 use {@link #httpRequest} instead
*/
@Deprecated(since = "9.16", forRemoval = true)
private HttpServletRequest request;
private HttpRequest httpRequest;

/**
* @deprecated since 9.16 use {@link #Context(String, HttpRequest)} instead
*/
@Deprecated(since = "9.16", forRemoval = true)
public Context(String username, HttpServletRequest request) {
this.username = username;
this.request = request;
this.httpRequest = new JavaxHttpRequest(request);
}

public Context(String username, HttpRequest httpRequest) {
this.username = username;
this.httpRequest = httpRequest;
this.request = (HttpServletRequest) httpRequest.getRawRequest();
}

public String getUsername() {
return username;
}

/**
* @deprecated since 9.16. Use {@link #getHttpRequest()} instead.
*/
@Deprecated(since = "9.16", forRemoval = true)
public HttpServletRequest getRequest() {
return request;
}

/**
* @since 9.16
*/
public HttpRequest getHttpRequest() {
return httpRequest;
}
}
}
29 changes: 29 additions & 0 deletions plugin-api/src/main/java/org/sonar/api/security/ExternalUsersProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@

import javax.annotation.Nullable;
import javax.servlet.http.HttpServletRequest;
import org.sonar.api.server.http.HttpRequest;
import org.sonar.api.server.http.JavaxHttpRequest;

/**
* Note that prefix "do" for names of methods is reserved for future enhancements, thus should not be used in subclasses.
Expand All @@ -42,19 +44,46 @@ public UserDetails doGetUserDetails(Context context) {

public static final class Context {
private String username;
/**
* @deprecated since 9.16 use {@link #httpRequest} instead
*/
@Deprecated(since = "9.16", forRemoval = true)
private HttpServletRequest request;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a private field, not really an API that needs to be deprecated. It's not like we can replace its use.

private HttpRequest httpRequest;

/**
* @deprecated since 9.16 use {@link #Context(String, HttpRequest)} instead
*/
@Deprecated(since = "9.16", forRemoval = true)
public Context(@Nullable String username, HttpServletRequest request) {
this.username = username;
this.request = request;
this.httpRequest = new JavaxHttpRequest(request);
}

public Context(@Nullable String username, HttpRequest httpRequest) {
this.username = username;
this.httpRequest = httpRequest;
this.request = (HttpServletRequest) httpRequest.getRawRequest();
Copy link
Collaborator

@dbmeneses dbmeneses Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would imply that the Object returned by getRawRequest is of type HttpServletRequest, which defeats the purpose of a framework agnostic API.

I'd suggest we add the constructor Context(String, HttpRequest, HttpServletContext) instead. It's up to the products using this context to still provide the HttpServletContext during the deprecation period (which in practice will force them to still use the old servlet framework).

Note that the constructors of this class should only be used by the products implementing the API in production code, so compliance with the deprecation policy is not critical (plugins might use it in tests though, but that's only broken once they upgrade the API). We could add a javadoc comment warning that it's not intended for plugins to instantiate this class, except in tests.
So that means that at the end of the deprecation policy, we could immediately replace this constructor with Context(username, HttpRequest) and completely get rid of the dependency to servlet in the API. Following the same idea, we could perhaps drop the constructor Context(String, HttpServletRequest) now.

If we wanted to be super strict about the deprecation policy, the only solutions would be to go through 2 deprecation periods, or to introduce a completely new interface and deprecate this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that make sense

}

public String getUsername() {
return username;
}

/**
* @deprecated since 9.16. Use {@link #getHttpRequest()} instead.
*/
@Deprecated(since = "9.16", forRemoval = true)
public HttpServletRequest getRequest() {
return request;
}

/**
* @since 9.16
*/
public HttpRequest getHttpRequest() {
return httpRequest;
}
}
}
22 changes: 22 additions & 0 deletions plugin-api/src/main/java/org/sonar/api/server/authentication/BaseIdentityProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import org.sonar.api.server.http.HttpRequest;
import org.sonar.api.server.http.HttpResponse;

/**
* @since 5.4
Expand All @@ -35,16 +37,36 @@ public interface BaseIdentityProvider extends IdentityProvider {

interface Context {

/**
* Get the received HTTP request.
*
* @since 9.16
*/
HttpRequest getHttpRequest();

/**
* Get the HTTP response to send.
*
* @since 9.16
*/
HttpResponse getHttpResponse();

/**
* Get the received HTTP request.
* Note - {@code getRequest().getSession()} must not be used in order to support
* future clustering of web servers without stateful server sessions.
*
* @deprecated since 9.16. Use {@link #getHttpRequest()} instead.
*/
@Deprecated(since = "9.16", forRemoval = true)
HttpServletRequest getRequest();

/**
* Get the HTTP response to send
*
* @deprecated since 9.16. Use {@link #getHttpResponse()} instead.
*/
@Deprecated(since = "9.16", forRemoval = true)
HttpServletResponse getResponse();

/**
Expand Down
22 changes: 22 additions & 0 deletions plugin-api/src/main/java/org/sonar/api/server/authentication/OAuth2IdentityProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import org.sonar.api.server.http.HttpRequest;
import org.sonar.api.server.http.HttpResponse;

/**
* @since 5.4
Expand All @@ -45,16 +47,36 @@ interface OAuth2Context {
*/
String getCallbackUrl();

/**
* Get the received HTTP request.
*
* @since 9.16
*/
HttpRequest getHttpRequest();

/**
* Get the HTTP response to send.
*
* @since 9.16
*/
HttpResponse getHttpResponse();

/**
* Get the received HTTP request.
* Note - {@code getRequest().getSession()} must not be used in order to support
* future clustering of web servers without stateful server sessions.
*
* @deprecated since 9.16. Use {@link #getHttpRequest()} instead.
*/
@Deprecated(since = "9.16", forRemoval = true)
HttpServletRequest getRequest();

/**
* Get the HTTP response to send
*
* @deprecated since 9.16. Use {@link #getHttpResponse()} instead.
*/
@Deprecated(since = "9.16", forRemoval = true)
HttpServletResponse getResponse();
}

Expand Down
107 changes: 107 additions & 0 deletions plugin-api/src/main/java/org/sonar/api/server/http/HttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
/*
* Sonar Plugin API
* Copyright (C) 2009-2023 SonarSource SA
* mailto:info AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
package org.sonar.api.server.http;

import java.util.Enumeration;

/**
* Framework-agnostic definition of an HTTP request.
*
* @since 9.16
*/
public interface HttpRequest {

/**
* Returns the port number to which the request was sent.
*/
int getServerPort();

/**
* Returns a boolean indicating whether this request was made using a secure channel, such as HTTPS.
*/
boolean isSecure();

/**
* Returns the name of the scheme used to make this request, for example, http, https, or ftp.
*/
String getScheme();

/**
* Returns the host name of the server to which the request was sent.
*/
String getServerName();

/**
* Returns the URL the client used to make the request. The returned URL contains a protocol, server name, port number, and server path,
* but it does not include query string parameters.
*/
String getRequestURL();

/**
* Returns the part of this request's URL from the protocol name up to the query string in the first line of the HTTP request.
*/
String getRequestURI();

/**
* Returns the query string that is contained in the request URL after the path. This method returns null if the URL does not have a
* query string.
*/
String getQueryString();

/**
* Returns the portion of the request URI that indicates the context of the request. The context path always comes first
* in a request URI. The path starts with a "/" character but does not end with a "/" character. For servlets in the
* default (root) context, this method returns "". The container does not decode this string.
*/
String getContextPath();

/**
* Returns the value of a request parameter as a String, or null if the parameter does not exist.
* You should only use this method when you are sure the parameter has only one value. If the parameter might have more than one value,
* use {@link #getParameterValues}.
*/
String getParameter(String name);

/**
* Returns an array containing all the values the given request parameter has, or null if the parameter does not exist.
*/
String[] getParameterValues(String name);

/**
* Returns the value of the specified request header as a String. If the request did not include a header of the specified name, this
* method returns null. If there are multiple headers with the same name, this method returns the first head in the request.
*/
String getHeader(String name);

/**
* Returns all the values of the specified request header as an Enumeration of String objects.
*/
Enumeration<String> getHeaders(String name);

/**
* Returns the name of the HTTP method with which this request was made, for example, GET, POST, or PUT.
*/
String getMethod();

/**
* Returns the raw request object from the Servlet API that matches this request.
*/
Object getRawRequest();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to defeat the purpose of having our own objects that represent HTTP requests and responses.

If the goal is for this to be used only by the products implementing the API, we should cast classes instead, since they are in control of the implementations at runtime.

For example, if a given version of SonarQube is creating filter contexts with HttpRequest of type JakartaHttpRequest, it's save for its filters to cast HttpRequest to JakartaHttpRequest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so we wanted to return "raw" requests objects as we wanted to avoid creating more classes that mimic javax.servlet.* or jakarta.servlet.* classes, like for example Cookies. Though maybe indeed you are right as it defeats the purpose and with a little more effort and we could get rid of all classes instead of taking shortcuts.

Also, SAML library which we use requires us to pass javax.servlet.HttpRequest, see: org.sonar.auth.saml.SamlAuthenticator#initSamlAuth(java.lang.String, javax.servlet.http.HttpServletRequest, javax.servlet.http.HttpServletResponse). We saw that they are working on changing it, though it might take time for them: SAML-Toolkits/java-saml#395, SAML-Toolkits/java-saml#115.

}
Loading