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.

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

NIFI-4246 - Client Credentials Grant based OAuth2 Controller Service #2901

Closed
wants to merge 15 commits into from

Conversation

jasonrogena
Copy link

@jasonrogena jasonrogena commented Jul 17, 2018

Duplicate PR to #2085 (created by @jdye64). Created a new PR since the remote originally used no longer exists.

We'll be focusing on adding the changes requested by @alopresto in the original pull request:

  • It looks like somehow an empty file was added at nifi/nifi-nar-bundles/nifi-update-attribute-bundle/nifi-update-attribute-ui/src/main/webapp/WEB-INF/jsp/authorize.jsp. I only noticed this because it failed the RAT check. I imagine this can be deleted?
  • An unused field remains in AbstractOAuthControllerService: protected long expireTimeSafetyNetSeconds = -1;
  • OAuth2ClientCredentialsGrantControllerService.CLIENT_ID and OAuth2ClientCredentialsGrantControllerService.CLIENT_SECRET don't support expression language (with the variable registry and NiFi Registry, these values will likely be populated externally)
  • The exception handling in OAuth2ClientCredentialsGrantControllerService#authenticate can be condensed
  • There are no unit or integration tests. I understand a real integration test with an external service is difficult, but unit tests for OAuth2ClientCredentialsGrantControllerService#authenticate() (via mocking OAuthHTTPConnectionClient#execute()) and AbstractOAuthControllerService#isOAuthTokenExpired() would be helpful
  • There is no update to the User Guide or Admin Guide describing this feature. At a minimum, the InvokeHTTP processor documentation should be updated

Thank you for submitting a contribution to Apache NiFi.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

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 master)?

  • Is your initial contribution a single, squashed commit?

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?
  • 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 travis-ci for build issues and submit an update to your PR as soon as possible.

@alopresto
Copy link
Contributor

Hi @jasonrogena . Thanks for submitting this. I'm sure @jdye64 appreciates it as well because I know he is busy but wants this feature incorporated. I am a little busy with some other things right now but I will definitely take a look at this.

A couple quick suggestions I know I will have:

  • Some of your files still reference NiFi 1.5.0-SNAPSHOT or 1.6.0. Others correctly reference 1.8.0-SNAPSHOT as the current development version. Please ensure that all development versions are 1.8.0-SNAPSHOT, and the latest released version at this time is 1.7.1.
  • Some files have the license text block after package declarations, imports, or XML opening tags. In each of these files, it needs to be the very first block of text. You can identify issues by running the maven build with -Pcontrib-check and it will highlight these issues for you. This will also run the NiFi checkstyle rules to help identify any code style issues. If you have issues with binary files being caught, you can exclude them in the pom.xml (search the project for <excludes combine.children="append"> in other pom.xml files for examples).

Update the nifi-oauth-api-nar to 1.8.0-SNAPSHOT on the nifi-assemble and
nifi-standard-bundle modules.

Signed-off-by: Jason Rogena <jasonrogena@gmail.com>
Signed-off-by: Jason Rogena <jasonrogena@gmail.com>
@markAcomm
Copy link

@jasonrogena,

Thanks for moving this forward. Good to see this PR is keeping up with 1.8

I noticed a small bug. In AbstractOAuthControllerService.java the property names look like they have a cut-and-paste error. Look for multiple properties with the line:
.Builder().name("JSON_response_access_token_name")
 
At lines 91, 103 and 114. I am pretty sure those need different names.
 
Also, kudos for adding the scope property as this is pretty critical to using this against Google APIs. I am not sure how all OAuth services work when accepting scope, but in the case of Google, would it be best to use the new StandardValidators.URI_LIST_VALIDATOR ?

@TranceMaster86
Copy link

When will it be fixed? We need that also

@MikeThomsen
Copy link
Contributor

@jasonrogena you have merge conflicts now. Please let me know if you're still interested in pushing this PR forward. If you're not or I don't get a reply back in a few days, I'll copy this branch and push it forward for you and @jdye64

@Xyrodileas
Copy link

Hello,
Really interested in this feature. Was any progress made related to the merge conflict ?

@MikeThomsen
Copy link
Contributor

Was a really a good PR, but unfortunately Oltu is in the Apache Attic so I don't think we can safely use it at this point for security reasons. Going to close now because of that. If any other committer or PMC member disagrees, feel free to reopen.

@Xyrodileas
Copy link

What is the security reason for not integrating this PR ? What could be done to integrate this feature ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants