You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by jasonrogena <gi...@git.apache.org> on 2018/07/17 13:19:16 UTC
[GitHub] nifi pull request #2901: NIFI-4246 - Client Credentials Grant based OAuth2 C...
GitHub user jasonrogena opened a pull request:
https://github.com/apache/nifi/pull/2901
NIFI-4246 - Client Credentials Grant based OAuth2 Controller Service
Duplicate PR to https://github.com/apache/nifi/pull/2085 (created by @jdye64). Created a new PR since the remote originally used no longer exists.
We're 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:
- [x] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [x] 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](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] 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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/onaio/nifi NIFI-4246
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2901.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2901
----
commit 8442f87bb0528ed25ce4d79ce7e7f1d7bf94c1e9
Author: Jeremy Dyer <je...@...>
Date: 2017-07-25T14:07:24Z
NIFI-4246 - Client Credentials Grant based OAuth2 Controller Service
commit 137158dbd5d5f8fafaba10c3bb50342299b600dd
Author: Jeremy Dyer <je...@...>
Date: 2017-10-06T20:49:35Z
Rebasing and upping to version 1.5.0-SNAPSHOT from 1.4.0-SNAPSHOT
commit 5601c1cac771ce923db2bd1b54b368b83e5cee7d
Author: Jeremy Dyer <je...@...>
Date: 2017-10-08T18:02:44Z
version introduced
commit 2f2b6b10a19d6a133645d490f612c48ea6abeb7a
Author: Jeremy Dyer <je...@...>
Date: 2018-01-08T16:59:59Z
changes suggested by Andy
commit 5df8791a9de55bf6725d47ce02848d4f51608bf9
Author: Jeremy Dyer <je...@...>
Date: 2018-01-09T19:02:54Z
updated version of jersey-client and jersey-server for OAuth components.
commit 51c185668cd7f2a11e81d439b486846346bde9a1
Author: Jeremy Dyer <jd...@...>
Date: 2018-03-05T19:35:23Z
Latest updated
commit 1823242ca889d335c7cf8b7f654ab8c8c256d05f
Author: Jeremy Dyer <jd...@...>
Date: 2018-03-07T01:21:43Z
build issues
commit 6a80063313f9c8e03a1f42ae84813cabd5fc8349
Author: Andy LoPresto <al...@...>
Date: 2018-03-14T20:29:00Z
NIFI-4246 Resolved merge conflicts in InvokeHTTP and various poms.
Removed empty authorize.jsp.
commit badedabecdcac9830c4c7473347a7e0489e8c070
Author: Jeremy Dyer <jd...@...>
Date: 2018-03-18T22:41:08Z
Require HTTPS for authentication URL, removed unused Java properties from inner static class, and other changes
commit 3aa32a7dc7d0fd1a210b58f84d7fa75f9cfa7e8a
Author: Jeremy Dyer <jd...@...>
Date: 2018-03-18T22:58:47Z
Support expression language for CLIENT_ID and CLIENT_SECRET
commit ed2983e4b8fcfd6f9eb989f52696d078bfd7c88d
Author: Jeremy Dyer <jd...@...>
Date: 2018-03-19T12:59:48Z
build enhancements
commit 73410d0f3544ebc5c3892c37920f62876e4e37c2
Author: Jason Rogena <ja...@...>
Date: 2018-07-04T15:49:07Z
Merge branch 'master' into NIFI-4246
Signed-off-by: Jason Rogena <ja...@gmail.com>
commit 2305ced6e61ddbaeeb51558fa290cc71657fc608
Author: Jason Rogena <ja...@...>
Date: 2018-07-17T12:24:09Z
Add OAuth2 Controller Service Test
Add test for the OAuth2 Controller Service test.
Signed-off-by: Jason Rogena <ja...@gmail.com>
----
---
[GitHub] nifi issue #2901: NIFI-4246 - Client Credentials Grant based OAuth2 Controll...
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/2901
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).
---
[GitHub] nifi issue #2901: NIFI-4246 - Client Credentials Grant based OAuth2 Controll...
Posted by markAcomm <gi...@git.apache.org>.
Github user markAcomm commented on the issue:
https://github.com/apache/nifi/pull/2901
@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 ?
---