[GitHub] [nifi] mark-weghorst commented on a change in pull request #4508: NIFI-6576 add basic auth to confluent schema registry service

[GitBox <gi...@apache.org>]

mark-weghorst commented on a change in pull request #4508:
URL: https://github.com/apache/nifi/pull/4508#discussion_r482575747



##########
File path: nifi-nar-bundles/nifi-confluent-platform-bundle/nifi-confluent-schema-registry-service/src/main/java/org/apache/nifi/confluent/schemaregistry/client/RestSchemaRegistryClient.java
##########
@@ -68,14 +69,21 @@
     private static final String SCHEMA_REGISTRY_CONTENT_TYPE = "application/vnd.schemaregistry.v1+json";
 
 
-    public RestSchemaRegistryClient(final List<String> baseUrls, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
+    public RestSchemaRegistryClient(final List<String> baseUrls, final String authType, final String authUser,
+                                    final String authPass, final int timeoutMillis, final SSLContext sslContext, final ComponentLog logger) {
         this.baseUrls = new ArrayList<>(baseUrls);
 
         final ClientConfig clientConfig = new ClientConfig();
         clientConfig.property(ClientProperties.CONNECT_TIMEOUT, timeoutMillis);
         clientConfig.property(ClientProperties.READ_TIMEOUT, timeoutMillis);
         client = WebUtils.createClient(clientConfig, sslContext);
 
+        if (!authUser.isEmpty() && !authType.isEmpty()) {
+            if (authType.equals("BASIC")) {
+                client.register(HttpAuthenticationFeature.basic(authUser, authPass));

Review comment:
       You raise an interesting question, AFAIK basic HTTP authentication requires a username, but will work with a password that is an empty string.  
   
   A quick search found bugs in other projects where they are failing to support null passwords.  **_From my point of view the question is really whether we should allow empty passwords or not._**  
   
   In your review of the original PR in April, you suggested that  customValidate for the controller be enhanced to throw an error if auth type was basic and either username or password were unset.  
   
   I've done as  you requested in that orignal PR, and the UX will display a validation error with a meaningful message that both username and password are required when auth type is basic.  
   
   Bottom line here is I'll bend to your judgement on this, let me know what you think we should do 




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org