NIFI-9065 Add support for OAuth2AccessTokenProvider in InvokeHTTP. #5319

tpalfy · 2021-08-19T14:55:39Z

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
Have you written or updated unit tests to verify your changes?
Have you verified that the full build is successful on JDK 8?
Have you verified that the full build is successful on JDK 11?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.

pvillard31 · 2021-08-20T11:55:51Z

Hey @MikeThomsen - IIRC you contributed the existing controller service in the first place, you may want to have a look at this pull request.

exceptionfactory

Thanks for contributing this feature @tpalfy. The general capability looks like a helpful improvement to InvokeHTTP, but there are several concerns with the implementation as it stands.

At a high level, removing the existing OAuth2AccessTokenProvider interface and implementation seems like a problem for a minor version update. Although there do not appear to be any other direct references to the current implementation, removing the existing interface and API would break others who have developed custom components. The current interface appears to have some issues, so introducing a new interface would be one way to provide backward compatibility. In that case, the existing interface and implementation should be marked as deprecated.

I also noted a handful of concerns on the new implementation related to properties and exception handling.

exceptionfactory · 2021-08-20T13:26:21Z

...e/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java

@@ -1112,6 +1121,11 @@ private void setHeaderProperties(final ProcessContext context, final Request.Bui
            requestBuilder.addHeader("Date", RFC_2616_DATE_TIME.format(universalCoordinatedTimeNow));
        }

+        if (context.getProperty(OAUTH2_ACCESS_TOKEN_PROVIDER).isSet()) {
+            OAuth2AccessTokenProvider oauth2AccessTokenProvider = context.getProperty(OAUTH2_ACCESS_TOKEN_PROVIDER).asControllerService(OAuth2AccessTokenProvider.class);
+            requestBuilder.addHeader("Authorization", "Bearer " + oauth2AccessTokenProvider.getAccessToken());


Setting the Authorization header will conflict with the Username and Password properties used for configuring Basic or Digest authentication. One way to address this is introducing more logic in customValidate() to ensure that those properties are not set when OAuth2 Access Token Provider is set.

exceptionfactory · 2021-08-20T13:27:48Z

...rd-services/nifi-oauth2-provider-api/src/main/java/org/apache/nifi/oauth2/AccessDetails.java

    private String accessToken;
    private String refreshToken;
    private String tokenType;
-    private Integer expires;
-    private String scope;
+    private Integer expiresIn;


This should be changed to an int to avoid unexpected NullPointerExceptions. Changing this to java.time.Duration would be a helpful improvement to avoid potential ambiguity.

exceptionfactory · 2021-08-20T13:28:16Z

...rd-services/nifi-oauth2-provider-api/src/main/java/org/apache/nifi/oauth2/AccessDetails.java


-    private Long fetchTime;
+    private final Long fetchTime;


This should be changed to a long to avoid unexpected NullPointerExceptions in isExpired(). It would probably be better to represent that as a java.time.Instant.

exceptionfactory · 2021-08-20T13:32:17Z

...-provider-service/src/main/java/org/apache/nifi/oauth2/PasswordBasedOauth2TokenProvider.java

+@Tags({"oauth2", "provider", "authorization", "access token", "http" })
+@CapabilityDescription("Provides OAuth 2.0 access tokens that can be used as Bearer authorization header in HTTP requests." +
+    " Uses Resource Owner Password Credentials Grant.")
+public class PasswordBasedOauth2TokenProvider extends AbstractControllerService implements OAuth2AccessTokenProvider {


The implementation includes both Password and Client Secret properties, are both of those necessary in all use cases? The previous implementation had a separate method using only the Client Credentials, so that should also be implemented for completeness.

I omitted the support for the Client Credentials Grant type from this service because to my understanding it has very limited usage that is not really applicable in a NiFi context.

To my understanding OAuth works like this:
Users can do certain things in the system, clients by themselves can do basically nothing. Nothing.
They merely serve as conduits for users. Like proxies, whatever they try to do - it must be initiated by a user.

The only exception when they need to access resources specifically meant for them as clients - and them only.
For example they might be able to change their secret. Or maybe their registration has an expiry and they can query the expiration date. And that's what the Client Credentials Grant type for - you have no user involved, you only need the Client ID and the Client Secret.

I investigated a bit more and there might be scenarios where resources are basically public but only for registered entities. Which can include both users and clients.
In such a scenario using the Client Credentials Grant flow is a valid use-case.
I'll add it to the new service as well.

exceptionfactory · 2021-08-20T13:33:59Z

...nifi-oauth2-provider-api/src/main/java/org/apache/nifi/oauth2/OAuth2AccessTokenProvider.java

+    /**
+     * @return A valid OAuth2 access token
+     */
+    String getAccessToken();


Is there a particular reason for changing the interface to return a String as opposed to an object of some kind?

This is a published API, so I'm not going to back this change without a good reason.

I thought simply accessing the token itself is enough but I guess it makes sense to be able to access the corresponding metadata. I'll change it to return AccessDetails.

exceptionfactory · 2021-08-20T13:38:19Z

...-provider-service/src/main/java/org/apache/nifi/oauth2/PasswordBasedOauth2TokenProvider.java

+            } catch (Exception e) {
+                getLogger().info("Couldn't refresh access token, probably refresh token has expired." +
+                    " Getting new token using credentials.");
+                acquireAuthorizationDetails();


This method could also throw any number of RuntimeExceptions, are expecting the caller to implement optional handling of RuntimeExceptions?

I don't see this any more special than other methods (which "could also throw any number of RuntimeExceptions"). The framework-provided default error-handling should suffice in my opinion.

As mentioned in the other comment, breaking out exception handling between issues with HTTP responses, versus other types of issues would help determine whether to call acquireAuthorizationDetails(). At that point, I agree that framework-level handling is sufficient for exception processing.

I tested all error scenarios and in all cases I see the issue properly reported.
The information provided is clear and lacks no detail.
Unless presented a concrete detailed and reproducible example that would prove otherwise I consider this level of error handling sufficient.

Thanks for the response, I added a comment with some additional details and one potential suggestion.

exceptionfactory · 2021-08-20T13:39:22Z

...-provider-service/src/main/java/org/apache/nifi/oauth2/PasswordBasedOauth2TokenProvider.java

+        .name("client-secret")
+        .displayName("Client secret")
+        .dependsOn(CLIENT_ID)
+        .required(true)


This property is required, but it appears to be ignored if Client ID is not specified.

Yes that's exactly what I wanted to achieve. It depends on CLIENT_ID. If that is set, this must be set as well. If CLIENT_ID is not set, this can be ignored.

Thanks for clarifying, I missed the dependsOn() attribute in the initial review.

exceptionfactory · 2021-08-20T13:40:06Z

...-provider-service/src/main/java/org/apache/nifi/oauth2/PasswordBasedOauth2TokenProvider.java

+
+            return accessDetails;
+        } catch (IOException e) {
+            throw new ProcessException(e);


It seems like another more specific exception could be better here, or perhaps an UncheckedIOException. In either case, a message should be included for troubleshooting. In this case, the IOException could be coming from the HTTP call itself, or the JSON parsing. Recommend breaking out those operations into separate methods for clearer error handling.

I'm not sure I see the value in making this more complex.
Whatever the issue was it will get reported properly and no information will be lost. I think we can rely on the default exception handling of the framework.

For clarification, I am not recommending making this more complex, but just changing the wrapping exception. Since this is catching IOException, wrapping it in an UncheckedIOException, with a message, seems better.

exceptionfactory · 2021-08-20T13:40:24Z

...-provider-service/src/main/java/org/apache/nifi/oauth2/PasswordBasedOauth2TokenProvider.java

+
+            ObjectMapper mapper = new ObjectMapper()
+                .configure(com.fasterxml.jackson.databind.DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
+                .setPropertyNamingStrategy(com.fasterxml.jackson.databind.PropertyNamingStrategies.SNAKE_CASE);


Is there a reason for including the full class in these two lines?

IntelliJ did this way for some reason. Didn't notice, I'll fix it.

exceptionfactory · 2021-08-20T13:40:50Z

...-provider-service/src/main/java/org/apache/nifi/oauth2/PasswordBasedOauth2TokenProvider.java

+                throw new ProcessException(String.format("Got HTTP %d during oauth2 request.", response.code()));
+            }
+
+            ObjectMapper mapper = new ObjectMapper()


The ObjectMapper instance can be reused, instead of being created in every method invocation.

MikeThomsen · 2021-08-24T18:30:50Z

@tpalfy what was the reason behind all but scrapping and redoing the existing controller service?

MikeThomsen

The idea seems simple, but I'm not tracking why this ticket went from adding to InvokeHttp and turned into a rewrite of the service.

MikeThomsen · 2021-08-24T18:20:23Z

...e/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java

@@ -1112,6 +1121,11 @@ private void setHeaderProperties(final ProcessContext context, final Request.Bui
            requestBuilder.addHeader("Date", RFC_2616_DATE_TIME.format(universalCoordinatedTimeNow));
        }

+        if (context.getProperty(OAUTH2_ACCESS_TOKEN_PROVIDER).isSet()) {
+            OAuth2AccessTokenProvider oauth2AccessTokenProvider = context.getProperty(OAUTH2_ACCESS_TOKEN_PROVIDER).asControllerService(OAuth2AccessTokenProvider.class);


This should be done once with @OnScheduled.

MikeThomsen · 2021-08-24T18:24:47Z

...nifi-oauth2-provider-api/src/main/java/org/apache/nifi/oauth2/OAuth2AccessTokenProvider.java

+    /**
+     * @return A valid OAuth2 access token
+     */
+    String getAccessToken();


This is a published API, so I'm not going to back this change without a good reason.

tpalfy · 2021-08-25T13:01:24Z

@tpalfy what was the reason behind all but scrapping and redoing the existing controller service?

@MikeThomsen
For backward compatibility's sake I'll put back the previous interface and implementation. I'll follow the suggestion by @exceptionfactory and mark them deprecated however.

My main issue with the previous solution is that it leaves too many responsibilities in the hands of the caller - i.e. the processor. This makes it harder to integrate as those processors need to take care of getting username, password, client id and client secret configurations themselves (not to mention validation) and pass that to the controller service.
Yet the url is separated from the rest of the configuration and tied to the controller service.
This is inconvenient from a user experience point of view as well because new/modified client configurations need to be amended across multiple components.

I would be curious how the previous solution was tested in a real-life environment.

MikeThomsen · 2021-08-25T16:04:39Z

I would be curious how the previous solution was tested in a real-life environment.

It started as a controller service for a private bundle I have been maintaining for a client. It's not the only component we have on site that uses it. We tend to start testing with password flows first and then move to client credentials as we move toward production.

The logic behind the implementation is that the controller service is meant to be dumb with regard to the specific credentials and provide functionality to a particular process with its own configured credentials.

One major concern I have is with the naming convention. Password* implies a particular OAuth2 strategy, but the service supports both password and client credential flows.

exceptionfactory · 2021-08-25T16:14:48Z

One major concern I have is with the naming convention. Password* implies a particular OAuth2 strategy, but the service supports both password and client credential flows.

I agree with the concern on naming, it seems like the name should be changed, or there should be different implementations.

exceptionfactory · 2021-08-30T16:34:46Z

...provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java

+            } catch (Exception e) {
+                getLogger().info("Couldn't refresh access token, probably refresh token has expired." +
+                    " Getting new token.");
+                acquireAccessDetails();
+            }


What about the case of an IOException thrown from refreshAccessDetails()? Whether wrapped in a ProcessException or UncheckedIOException, this will hit this catch block. In that case, the failure is not due to an expired refresh token, but a network issue like a SocketTimeoutException.

At minimum, the log message is not indicative of that type of failure. It also seems like making another call to the service with acquireAuthorizationDetails() is unlikely to succeed. One option is to catch and throw IOExceptions.

Suggested change

} catch (Exception e) {

getLogger().info("Couldn't refresh access token, probably refresh token has expired." +

" Getting new token.");

acquireAccessDetails();

}

} catch (final IOException ioe) {

throw new UncheckedIOException("Refresh Access Token Failed", ioe);

} catch (final Exception e) {

getLogger().info("Refresh Access Token Failed [{}]: New Token request started", e.getMessage());

acquireAccessDetails();

}

We might save a round to the access server with this approach but that's about the only advantage. However a simple retry (in this case, try to get a whole new access token) might even provide a solution for a momentary connection issue. Overall I still think the current implementation is more optimal.

As for troubleshooting I stand by my previous opinion: I tested all error scenarios and in all cases I see the issue properly reported. The information provided is clear and lacks no detail.
Unless presented a concrete detailed and reproducible example that would prove otherwise I consider this level of error handling sufficient.

If the initial request for a token fails, it is important to know the reason, and OAuth2 responses provide specific details for the failure. Another problem with the current implementation is that the exception details are not logged.

@MikeThomsen can probably provide more detail on the reason that the current implementation separates the initial token request and the refresh token request. Although there is more work on the caller, these are two different concepts that can fail for different reasons.

Although the general functionality would be an improvement to InvokeHTTP, I am -1 on the current approach. If token refresh remains handled implicitly in the Controller Service, the failure conditions need to be handled more clearly.

If the initial request for a token fails, it is important to know the reason, and OAuth2 responses provide specific details for the failure.

I agree. I tested this scenario and - as I said previously: I see the issue properly reported. The information provided is clear and lacks no detail.

Another problem with the current implementation is that the exception details are not logged.

Okay, I'll add the exception to the logger call.

If you still think you can present a concrete example where you can show that a certain issue can't be investigated efficiently or a case is not handled properly please give me the details how to reproduce it and I'd be very happy to take a look.

exceptionfactory

Thanks for following up @tpalfy. On further review, I noted a couple issues with OkHttp Response and ResponseBody objects not being closed according to their documentation. I also highlighted the potential for the service to maintain a reference to an invalid refresh token if the refreshAccessToken() request fails. For these reasons, I also recommend adding a unit test method that exercises some exception conditions.

...provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java

exceptionfactory · 2021-09-01T20:24:38Z

...provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java

+        this.accessDetails = getAccessDetails(refreshRequest);
+    }
+
+    private AccessToken getAccessDetails(Request newRequest) {


Suggested change

private AccessToken getAccessDetails(Request newRequest) {

private AccessToken getAccessDetails(final Request newRequest) {

This is a matter of taste, I'm against the excessive use of the final keyword.
Making local variables final usually just makes the code less readable and much more painful to debug for no real benefit.

Usage of final is not consistent across the application, but it does provide an important indication that the variable reference cannot be changed in subsequent lines, so it is more than a stylistic question. I recommend it, but it isn't required.

exceptionfactory · 2021-09-01T20:28:35Z

...provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java

+                getLogger().error(String.format("Bad response from the server during oauth2 request:\n%s", responseBody));
+                throw new ProcessException(String.format("Got HTTP %d during oauth2 request.", response.code()));


The newline character is platform-specific, recommend the following adjustments:

Suggested change

getLogger().error(String.format("Bad response from the server during oauth2 request:\n%s", responseBody));

throw new ProcessException(String.format("Got HTTP %d during oauth2 request.", response.code()));

getLogger().error("OAuth2 Request Failed [HTTP {}] {}", response.code(), responseBody);

throw new ProcessException(String.format("OAuth2 Request Failed [HTTP %d]", response.code()));

...provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java

exceptionfactory · 2021-09-01T20:31:59Z

...provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java

+    }
+
+    private void acquireAccessDetails() {
+        getLogger().debug("Getting a new access token.");


Although logging is not consistent across the application, recommend removing the period . character from the message:

Suggested change

getLogger().debug("Getting a new access token.");

getLogger().debug("Getting a new access token");

exceptionfactory · 2021-09-01T20:42:21Z

...provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java

+                try {
+                    refreshAccessDetails();
+                } catch (Exception e) {
+                    getLogger().info("Couldn't refresh access token.", e);


If this generalized exception handling and retry remains, should this be logged as warning?

Suggested change

getLogger().info("Couldn't refresh access token.", e);

getLogger().warn("Refresh Access Token Failed", e);

It's better to keep this as info.
If the subsequent new token acquisition succeeds we don't want to leave a warning that basically immediately became outdated.

exceptionfactory · 2021-09-01T20:48:55Z

...provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java

+                try {
+                    refreshAccessDetails();
+                } catch (Exception e) {
+                    getLogger().info("Couldn't refresh access token.", e);


If the refresh request throws an Exception, the object still retains a reference to the expired Access Token. If the subsequent call to acquireAccessDetails() also throws an Exception, it looks like the class will retain a reference to a refreshToken that is potentially invalid, does that sounds possible? One option would be setting accessDetails = null in this catch block, which would cause subsequent calls to getAccessDetails() to call acquireAccessDetails() instead of going down this path again.

@exceptionfactory , although I don't have a strong opinion on this particular point my concern with nulling the accessDetails is that it might be a bit confusing to the reader and we don't save too much here:

if there'll be multiple calls to this method and the refresh-acquire pair fails then there's some bigger problem -> the "extra" refresh try doesn't make too much difference.

But if the subsequent call succeeds at acquireAccessDetails then we only have one unnecessary refresh call.

Compared to how confusing the accessDetails = null could be it might not be worth it.
WDYT?

I'd rather leave out this type of micromanagement code. The benefit doesn't translate to practical advantage in real-life situations. Also, retrying the refresh after the new token acquisition failed very well may make sense and could succeed. Maybe it fails more often than not but the point is, it doesn't really matter.

I agree that setting accessDetails to null could be confusing. That is part of the reason for recommend more discrete exception handling. Here is the potential scenario:

Initial acquireAccessDetails() succeeds and includes a refresh token

Access Token expires, leading to the service calling refreshAccessDetails()

The call to refreshAccessDetails() fails due to an IOException

The call to acquireAccessDetails() fails due to an IOException

As long there are continuing connection issues, the service will continue calling both refreshAccessDetails() and acquireAccessDetails() on every getAccessDetails() invocation. The same thing would happen in the case of both methods throwing some type of RuntimeException related to a bad HTTP response. Logs might be enough to identify the problem, but the main concern is the state of the access details. Given that this new service implementation intends to hide the complexities of the state from the caller, it seems important to have the implementation itself as clear as possible.

I was thinking a bit more about this.
I understand the reason of setting the accessDetails to null. What if the acquireAccessDetails resets it at the beginning? Logically it belongs there and it's cleaner for the reader that after the acquireAccessDetails it either has a value or is null due to an exception.

Thanks @adenes, I was thinking about that approach as well. Setting accessDetails to null in acquireAccessDetails does seem like the logical location to address this concern.

Would it be possible to move past theoretical potential scenarios and consider reproducible issues?

I understand how the mentioned scenario would work. I thought about it already a long time ago.
Yes, it's possible that there would be multiple unsuccessful calls to the access server.
Yes, if the access server is unaccessible, we would have 2 unsuccessful calls at each attempt.

Where I disagree is that this is actually a problem. Would we be missing any important detail to be able to solve the issue? Would there be a scenario where this implementation would not work but another implementation would? I see no proof for any of those to be true.

As for setting acquireAccessDetails to null, again I see no case where this would be better than leaving it as it is. In fact I'd argue that in case we would like to do a memory dump it's better to keep the last working accessDetails all the time.

Thanks for the reply @tpalfy. Although runtime issues can create some strange edge cases, I can appreciate not wanting to get stuck on theoretical concerns.

In order to move this forward, creating unit tests that exercise exception conditions would satisfy my concerns. It would be very helpful to have one unit test that throws some type of IOException, and another that throws an exception as the result of a non-success HTTP status. Having those two tests would exercise this block of code and cover the bases.

With the addition of unit tests, and if @MikeThomsen signs off on this refactored approach, I can support these changes.

Expecting better test coverage is fair enough. Going to add ones that cover the various error cases.

To be honest previously I thought about adding unit tests but the non-error cases would rely so much on mocks that it would no longer ensure proper functionality in a real-life environment. So I settled for manual testing. But for the error-cases it makes sense to add some unit tests to cover the error handling.

...provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java

exceptionfactory

Thanks for including the additional unit tests @tpalfy! Using assertThrows() follows the current JUnit recommendation for expecting exception cases.

Although less of a concern, checking for the exact message in the log seems a bit brittle. Adjusting the log verification to look for a keyword, such as refresh, would be helpful.

The main concern is the use of assertThrows() instead of a try-catch with fail(). Otherwise, the general test methods look clear enough.

The mocked approach works as implemented, although if you are interested, the OkHttp mockwebserver library used for testing InvokeHTTP is another option.

exceptionfactory · 2021-09-06T18:35:20Z

...ider-service/src/test/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProviderTest.java

+        try {
+            testSubject.getAccessDetails();
+            fail();
+        } catch (ProcessException e) {
+            // THEN
+            checkLoggedDebugWhenRefreshThrowsIOException();
+
+            checkLoggedRefreshError(new ProcessException("OAuth2 access token request failed [HTTP 500]"));
+
+            checkedLoggedErrorWhenRefreshReturnsBadHTTPResponse(expectedLoggedInfo);
+
+            checkError(new ProcessException("OAuth2 access token request failed [HTTP 503]"), e);
+        }


With recent work underway to refactor unit tests, it would be helpful to implement this test using assertThrows().

The Junit5 assertThrow support is kind of up to par to the current JavaSE solution, although even that is debatable. It is fairly new and hasn't been proven to be obviously superior yet.

The Junit4 approach however has objective shortcomings.
Switching to Junit5 is out of scope of this change.

Leaving the tests as they are is the most optimal in my opinion.

JUnit 4.13 includes assertThrows(), which is already used in other places throughout the system. Using assertThrows with the current JUnit 4 test methods should be a straightforward adjustment, and will also streamline future migration to JUnit 5.

exceptionfactory

Thanks for updating the unit tests @tpalfy. This looks closer to completion, although I think it would also be useful to add a unit test to InvokeHTTP that exercises the OAuth2 service. Otherwise, the failure unit tests do a good job of exercising the various exception conditions.

@MikeThomsen do you have any additional feedback on the current implementation?

exceptionfactory · 2021-09-08T14:20:46Z

...provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java

+        .name("ssl-context")
+        .displayName("SSL Context")


Other components such as InvokeHTTP and ListenHTTP use SSL Context Service as the property name, so recommend adjusting the naming for consistency:

Suggested change

.name("ssl-context")

.displayName("SSL Context")

.name("ssl-context-service")

.displayName("SSL Context Service")

exceptionfactory · 2021-09-08T14:24:16Z

...e/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java

+            results.add(new ValidationResult.Builder()
+                .subject("Authorization properties")
+                .valid(false)
+                .explanation("Can't use username+password and OAuth2 authorization at the same time")


It would be helpful to reference to actual property names in the message, similar to formatted message in customValidate() on the service, as opposed to just username+password.

I disagree. There are 3 fairly long property names that could be referenced here ('Basic Authentication Username', 'Basic Authentication Password' and 'OAuth2 Access Token provider') and putting them all in a fairly convoluted manner in the explanation would make it too bloated and harder to actually interpret the issue.

On the other hand having the explanation message be something like "Can't use Basic- and OAuth2 authorization at the same time" would be a bit too lacking and would require just a bit too much extra effort to figure out what exactly the issue is.

I think the current message gives just the right amount of detail.

Although it seems helpful to call out the actual property name, I would be happy with something shorter that removes the + character. Here is one suggestion:

Suggested change

.explanation("Can't use username+password and OAuth2 authorization at the same time")

.explanation("OAuth2 Authorization cannot be configured together with Username and Password properties")

...e/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java

exceptionfactory · 2021-09-08T15:14:59Z

...provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java

+        final List<ValidationResult> validationResults = new ArrayList<>(super.customValidate(validationContext));
+
+        if (
+            validationContext.getProperty(GRANT_TYPE).getValue().equals(CLIENT_CREDENTIALS_GRANT_TYPE.getValue())


There do not appear to be any unit tests that exercise this validation.

exceptionfactory · 2021-09-17T14:59:42Z

Thanks for working the feedback @tpalfy, the latest version looks good. I ran through some basic runtime testing and was able to observe the error reporting.

@MikeThomsen and @adenes, do either of you have any additional feedback?

MikeThomsen · 2021-09-17T23:58:47Z

@exceptionfactory I'll try to take a look this weekend.

OlivierBondu · 2021-12-07T10:15:50Z

Hi,
we would be very interested in this feature as well... Any chance you had the time to review it ?
Unfortunately I don't have the java skills to help out :(
Cheers,
Olivier

pvillard31 · 2021-12-07T10:42:15Z

@tpalfy - can you rebase against latest?
I think this would be a nice feature. @MikeThomsen @exceptionfactory - did we clear all previous concerns?

tpalfy · 2021-12-07T18:28:23Z

@exceptionfactory @MikeThomsen @pvillard31 @OlivierBondu
Rebased to main.

exceptionfactory · 2021-12-08T18:06:48Z

Thanks for the updates @tpalfy, the latest version looks good. Given that this deprecates the current API and @MikeThomsen previously expressed some concern of the changes, will wait for additional approval.

@MikeThomsen What do you think of the current approach?

OlivierBondu · 2022-01-04T13:06:13Z

Hi all,

Happy new year to all ! Best wishes, good health, etc. 😄

Did you had any chance to move this PR forward ? Or would you have a rough time estimate for the feature ? Either would definitely be of help !

Thanks

exceptionfactory · 2022-01-04T14:13:14Z

@MikeThomsen do you have any remaining concerns or does this look good to you?

MikeThomsen

Overall looks ready to merge, but one point about validation needs to be discussed.

MikeThomsen · 2022-01-05T16:20:54Z

...provider-service/src/main/java/org/apache/nifi/oauth2/StandardOauth2AccessTokenProvider.java

+        .displayName("Username")
+        .description("Username on the service that is being accessed.")
+        .dependsOn(GRANT_TYPE, RESOURCE_OWNER_PASSWORD_CREDENTIALS_GRANT_TYPE)
+        .required(true)


I'm not sure about the validation/required field settings here. I'd feel better if username, password, client_id and client_secret were validated in a custom validator that checks to ensure the right values, lengths, etc. are there. I've experienced enough weirdness with nifi-mock on this to be a little wary here.

I think the current design is adequate.
We have username, password, client id and client secret of which the former and latter two are tied and we have no pre-existing knowledge about them regarding "right values, lenghts, etc."
The .required and .dependsOn settings properly cover all available scenarios.

pvillard31 · 2022-01-17T17:42:50Z

@tpalfy - can you rebase against main?

Unless there is something else to be discussed here (@MikeThomsen @exceptionfactory), I'm happy to get this merged for inclusion in NiFi 1.16

…e OAuth2-related implementations had to be adjusted.

@onschedule

…ils renamed back to AccessToken. Minor refactor in AccessToken for robustness and code quality. OAuth2AccessTokenProvider.getAccessDetails returns a full AccessToken object instead of a String. In PasswordBasedOauth2TokenProvider json mapper for AccessToken made constant. In InvokeHTTP added custom validation to prevend clashing of basic/digest and oauth2 authorization. Access token provider service is "warmed up" at @onschedule (retrieves a valid token that it then caches).

…roller service (which was renamed from 'PasswordBasedOauth2TokenProvider' to 'StandardOauth2AccessTokenProvider'). Cached access token is cleared when service is disabled (to force a new retrieval in case configuration has changed).

…en fails.

…fresh token. Adding exception details to the log when refreshing access token fails.

… Fixed NPE in StandardOauth2AccessTokenProviderTest.

…ccessTokenProvider.

…by using 'assertThrows' to check error handling.

…eckLoggedDebugWhenRefreshThrowsIOException' to 'checkLoggedDebugWhenRefreshFails'.

…cessTokenProviderTest.

exceptionfactory

The current version looks good to me, thanks for the work on this @tpalfy! I defer to @pvillard31 or @MikeThomsen for merging.

pvillard31

Merging, please file follow-up JIRAs if needed/required.
Thanks all for working on this very useful improvement!

MikeThomsen · 2022-02-01T17:42:32Z

@pvillard31 going to be copying this over to our client's network to start some live testing against real OAuth2-driven services.

lawrencegrass · 2022-03-14T20:38:22Z

...dard-services/nifi-oauth2-provider-api/src/main/java/org/apache/nifi/oauth2/AccessToken.java

        return fetchTime;
    }

    public boolean isExpired() {
-        return System.currentTimeMillis() >= ( fetchTime + (expires * 1000) );
+        boolean expired = Duration.between(Instant.now(), fetchTime.plusSeconds(expiresIn - EXPIRY_MARGIN)).isNegative();


expiresIn needs to be multiplied by 1000, or EXPIRY_MARGIN needs to be set to 5. Per OAuth standards the expires_in is in seconds. I've confirmed (using keycloak) that setting my token expiration to 5 minutes gets converted to 300 seconds and then on this step has 5000 subtracted from it. Which of course expires my non expired token.

Thanks for raising this concern @lawrencegrass. This pull request is already closed and merged. Can you summarize the problem and create a new issue in the Apache NiFi Jira project?

https://issues.apache.org/jira/projects/NIFI/issues

Created JIRA.
https://issues.apache.org/jira/browse/NIFI-9797

Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com> This closes apache#5319.

exceptionfactory requested changes Aug 20, 2021

View reviewed changes

MikeThomsen reviewed Aug 24, 2021

View reviewed changes

exceptionfactory reviewed Aug 30, 2021

View reviewed changes

exceptionfactory requested changes Sep 1, 2021

View reviewed changes

exceptionfactory requested changes Sep 6, 2021

View reviewed changes

exceptionfactory reviewed Sep 8, 2021

View reviewed changes

exceptionfactory requested review from MikeThomsen and adenes November 15, 2021 14:25

tpalfy force-pushed the NIFI-9065 branch from 88a1382 to caaa1e1 Compare December 7, 2021 18:27

MikeThomsen reviewed Jan 5, 2022

View reviewed changes

tpalfy added 6 commits January 17, 2022 19:30

NIFI-9065 Add support for OAuth2AccessTokenProvider in InvokeHTTP. Th…

cf64b70

…e OAuth2-related implementations had to be adjusted.

NIFI-9065 Minor error message tweaking when refreshing the access tok…

2b0c5e0

…en fails.

NIFI-9065 Only attempting to refresh access token if we received a re…

8351820

…fresh token. Adding exception details to the log when refreshing access token fails.

NIFI-9065 Minor logging changes in StandardOauth2AccessTokenProvider.…

32aa69a

… Fixed NPE in StandardOauth2AccessTokenProviderTest.

tpalfy added 6 commits January 17, 2022 19:31

NIFI-9065 Added unit tests to cover error-handling in StandardOauth2A…

1c81de8

…ccessTokenProvider.

NIFI-9065 Minor improvement in StandardOauth2AccessTokenProviderTest …

f8c3bd6

…by using 'assertThrows' to check error handling.

NIFI-9065 In StandardOauth2AccessTokenProviderTest renamed method 'ch…

22c7728

…eckLoggedDebugWhenRefreshThrowsIOException' to 'checkLoggedDebugWhenRefreshFails'.

NIFI-9065 Minor refactor (renamed some variables) in StandardOauth2Ac…

1ff138e

…cessTokenProviderTest.

NIFI-9065 More unit tests, minor fixes.

722233d

NIFI-9065 Minor change in validation error message.

c299248

exceptionfactory approved these changes Jan 17, 2022

View reviewed changes

NIFI-9065 Adjust after rebasing to main.

43e2328

tpalfy force-pushed the NIFI-9065 branch from caaa1e1 to 43e2328 Compare January 18, 2022 15:07

pvillard31 approved these changes Jan 19, 2022

View reviewed changes

asfgit closed this in aa61494 Jan 19, 2022

lawrencegrass reviewed Mar 14, 2022

View reviewed changes

krisztina-zsihovszki pushed a commit to krisztina-zsihovszki/nifi that referenced this pull request Jun 28, 2022

NIFI-9065 Add support for OAuth2AccessTokenProvider in InvokeHTTP

98968ee

Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com> This closes apache#5319.

Lehel44 pushed a commit to Lehel44/nifi that referenced this pull request Jul 1, 2022

NIFI-9065 Add support for OAuth2AccessTokenProvider in InvokeHTTP

8554347

Signed-off-by: Pierre Villard <pierre.villard.fr@gmail.com> This closes apache#5319.

	private AccessToken getAccessDetails(Request newRequest) {
	private AccessToken getAccessDetails(final Request newRequest) {

		getLogger().error(String.format("Bad response from the server during oauth2 request:\n%s", responseBody));
		throw new ProcessException(String.format("Got HTTP %d during oauth2 request.", response.code()));

	getLogger().debug("Getting a new access token.");
	getLogger().debug("Getting a new access token");

	getLogger().info("Couldn't refresh access token.", e);
	getLogger().warn("Refresh Access Token Failed", e);

	.explanation("Can't use username+password and OAuth2 authorization at the same time")
	.explanation("OAuth2 Authorization cannot be configured together with Username and Password properties")

NIFI-9065 Add support for OAuth2AccessTokenProvider in InvokeHTTP. #5319

NIFI-9065 Add support for OAuth2AccessTokenProvider in InvokeHTTP. #5319

Conversation

tpalfy commented Aug 19, 2021 • edited Loading

For all changes:

For code changes:

For documentation related changes:

Note:

pvillard31 commented Aug 20, 2021

exceptionfactory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpalfy Aug 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeThomsen commented Aug 24, 2021

MikeThomsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpalfy commented Aug 25, 2021

MikeThomsen commented Aug 25, 2021

exceptionfactory commented Aug 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exceptionfactory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exceptionfactory Sep 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exceptionfactory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exceptionfactory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpalfy commented Aug 19, 2021 •

edited

Loading

tpalfy Aug 25, 2021 •

edited

Loading

exceptionfactory Sep 6, 2021 •

edited

Loading