You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/12/17 17:36:47 UTC
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6782: NIFI-10456: Add client authentication strategy option to OAuth2 provider
exceptionfactory commented on code in PR #6782:
URL: https://github.com/apache/nifi/pull/6782#discussion_r1051431949
##########
nifi-nar-bundles/nifi-standard-services/nifi-oauth2-provider-bundle/nifi-oauth2-provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java:
##########
@@ -181,6 +205,7 @@ public class StandardOauth2AccessTokenProvider extends AbstractControllerService
private volatile String authorizationServerUrl;
private volatile OkHttpClient httpClient;
+ private volatile String clientAuthenticationStrategy;
Review Comment:
Changing the `AllowableValue` to a specific `enum` named `ClientAuthenticationStrategy` will allow this property to be more strongly typed.
##########
nifi-nar-bundles/nifi-standard-services/nifi-oauth2-provider-bundle/nifi-oauth2-provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java:
##########
@@ -69,6 +71,27 @@ public class StandardOauth2AccessTokenProvider extends AbstractControllerService
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static AllowableValue BASIC_AUTHENTICATION = new AllowableValue(
+ "basic",
+ "Basic Authentication",
+ "Send client authentication as HTTP Basic authentication"
+ );
+
+ public static AllowableValue REQUEST_BODY = new AllowableValue(
+ "request_body",
+ "Request Body",
+ "Send client authentication in request body. Note: users should prefer Basic Authentication unless authorization server does not support it."
+ );
Review Comment:
Although this works, recommend creating an `enum` that implements the `DescribedValue` interface. See other examples such as `ContentEncodingStrategy` and `AuthorizationScheme` in other modules. The `getValue()` method should return `name()`, and allows for better value comparison in other methods.
##########
nifi-nar-bundles/nifi-standard-services/nifi-oauth2-provider-bundle/nifi-oauth2-provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java:
##########
@@ -69,6 +71,27 @@ public class StandardOauth2AccessTokenProvider extends AbstractControllerService
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static AllowableValue BASIC_AUTHENTICATION = new AllowableValue(
+ "basic",
+ "Basic Authentication",
+ "Send client authentication as HTTP Basic authentication"
+ );
+
+ public static AllowableValue REQUEST_BODY = new AllowableValue(
+ "request_body",
+ "Request Body",
+ "Send client authentication in request body. Note: users should prefer Basic Authentication unless authorization server does not support it."
+ );
+
+ public static final PropertyDescriptor CLIENT_AUTHENTICATION_STRATEGY = new PropertyDescriptor.Builder()
+ .name("client-authentication-strategy")
+ .displayName("Client Authentication Strategy")
+ .description("Authentication strategy to use for client authentication.")
Review Comment:
I agree with the suggested improvement, it is clearer/
##########
nifi-nar-bundles/nifi-standard-services/nifi-oauth2-provider-bundle/nifi-oauth2-provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java:
##########
@@ -69,6 +71,27 @@ public class StandardOauth2AccessTokenProvider extends AbstractControllerService
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static AllowableValue BASIC_AUTHENTICATION = new AllowableValue(
+ "basic",
+ "Basic Authentication",
+ "Send client authentication as HTTP Basic authentication"
Review Comment:
```suggestion
"Send client authentication using HTTP Basic authentication"
```
##########
nifi-nar-bundles/nifi-standard-services/nifi-oauth2-provider-bundle/nifi-oauth2-provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java:
##########
@@ -298,11 +324,15 @@ private void acquireAccessDetails() {
}
RequestBody acquireTokenRequestBody = acquireTokenBuilder.build();
-
- Request acquireTokenRequest = new Request.Builder()
+ Request.Builder acquireTokenRequestBuilder = new Request.Builder()
.url(authorizationServerUrl)
- .post(acquireTokenRequestBody)
- .build();
+ .post(acquireTokenRequestBody);
+
+ if (clientAuthenticationStrategy.equals(BASIC_AUTHENTICATION.getValue()) && clientId != null) {
+ acquireTokenRequestBuilder.addHeader("Authorization", Credentials.basic(clientId, clientSecret));
Review Comment:
Recommend defining `private static final String AUTHORIZATION_HEADER = "Authorization"` and reusing that value here and in the `refreshAccessDetails()` method.
##########
nifi-nar-bundles/nifi-standard-services/nifi-oauth2-provider-bundle/nifi-oauth2-provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java:
##########
@@ -69,6 +71,27 @@ public class StandardOauth2AccessTokenProvider extends AbstractControllerService
.expressionLanguageSupported(ExpressionLanguageScope.VARIABLE_REGISTRY)
.build();
+ public static AllowableValue BASIC_AUTHENTICATION = new AllowableValue(
+ "basic",
+ "Basic Authentication",
+ "Send client authentication as HTTP Basic authentication"
+ );
+
+ public static AllowableValue REQUEST_BODY = new AllowableValue(
+ "request_body",
+ "Request Body",
+ "Send client authentication in request body. Note: users should prefer Basic Authentication unless authorization server does not support it."
Review Comment:
Recommend referencing RFC 6749 Section 2.3.1:
```suggestion
"Send client authentication in request body. RFC 6749 Section 2.3.1 recommends Basic Authentication instead of request body."
```
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org