Skip to content

NIFI-4890 Refactor OIDC with support for Refresh Tokens#7013

Merged
mcgilman merged 3 commits intoapache:mainfrom
exceptionfactory:NIFI-4890
Mar 28, 2023
Merged

NIFI-4890 Refactor OIDC with support for Refresh Tokens#7013
mcgilman merged 3 commits intoapache:mainfrom
exceptionfactory:NIFI-4890

Conversation

@exceptionfactory
Copy link
Contributor

Summary

NIFI-4890 Refactors the NiFi OpenID Connect integration using Spring Security 5 with support for extended application sessions using Refresh Tokens.

The new implementation replaces custom REST Resources with Spring Security filters to maintain existing support for the Authorization Code Grant Flow. The new implementation also maintains support for OpenID Connect RP-Initiated Logout 1.0 when the OpenID Connect Provider supports ending sessions.

As part of supporting session continuation with Refresh Tokens, the new implementation supports OAuth 2.0 Token Revocation. The updated Administrator's Guide documentation describes the Refresh Token handling. As noted in the documentation, application use of Refresh Tokens is conditional on the Authorization Server returning a Refresh Token during authentication. OIDC support follows the same pattern as other authentication strategies when the Authorization Server does not provide a Refresh Token.

The implementation introduces a new property named nifi.security.user.oidc.token.refresh.window to control the duration of time between when the application Bearer Token expires and when the application should attempt to renew access using the Refresh Token.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 11
    • JDK 17

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

@exceptionfactory exceptionfactory force-pushed the NIFI-4890 branch 2 times, most recently from e401560 to e97add4 Compare March 6, 2023 16:37
@mcgilman mcgilman self-requested a review March 9, 2023 21:06
Copy link
Contributor

@emiliosetiadarma emiliosetiadarma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to test with Keycloak and Google as the providers and it works as expected!

@mcgilman
Copy link
Contributor

Reviewing...

- Implemented OIDC Authorization Code Grant Flow using Spring Security Filters
- Implemented OIDC RP-Initiated Logout 1.0
- Implemented OAuth2 Token Revocation RFC 7009 for Refresh Tokens
- Added OIDC Bearer Token Refresh Filter for updating application Bearer Tokens from Refresh Token exchanges
- Added configurable Token Refresh Window to application properties
- Removed original implementation and supporting classes
@exceptionfactory
Copy link
Contributor Author

Thanks for the testing @emiliosetiadarma!

After some discussion with @mcgilman, I pushed an update to change the source of initial application Bearer Token expiration.

The previous implementation derived the application Bearer Token expiration from the ID Token, but the update changes the approach to derive the expiration from the Access Token. This strategy aligns both initial expiration and refreshed expiration to derive from the Access Token expiration.

Some Identity Providers return the same expiration value for both the ID Token and the Access Token, so the end result will not change for those providers. Changing the source of the application Bearer Token expiration to the Access Token expiration provides a consistent approach, and the updated section of the Administrator's Guide reflects these changes. Some Identity Providers make the Access Token expiration configurable, so this also aligns with expected integration behavior.

I also rebased the pull request from the current main branch.

Copy link
Contributor

@greyp9 greyp9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested happy path using [1]. Only behavioral difference I noticed was an update to the needed "Sign-out redirect URI" specified in the Okta configuration.

[1] https://exceptionfactory.com/posts/2022/12/21/integrating-apache-nifi-with-okta-oidc-authentication/

@mtien-apache
Copy link
Contributor

Reviewing.

@exceptionfactory
Copy link
Contributor Author

Tested happy path using [1]. Only behavioral difference I noticed was an update to the needed "Sign-out redirect URI" specified in the Okta configuration.

[1] https://exceptionfactory.com/posts/2022/12/21/integrating-apache-nifi-with-okta-oidc-authentication/

Thanks for testing @greyp9. The updated OpenID Connect section of the Administrator's Guide notes the logout destination path, but it is worth calling out the change as a migration guide note when this is ready to go. The updated logout destination removes the relative path elements in favor for declaring the direct /nifi/logout-complete path.

Copy link
Contributor

@mtien-apache mtien-apache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 Thanks for the PR, @exceptionfactory! 🙌🏼 Tested login and logout with Google, Okta, and Azure and they all work well. One thing to note is with Azure logout, I don't get re-directed back to NiFi or login even though I added a post logout redirect URI to my Azure ID Provider. I'm still able to log out completely. After you and I looked at this, it seems like it could be an issue on Azure's side. Thanks for also walking through the reasons why and how we are saving the ID/access/refresh tokens.

Copy link
Contributor

@mcgilman mcgilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @exceptionfactory! Nice work and thanks for the update following our initial discussion.

@exceptionfactory
Copy link
Contributor Author

Thanks for the feedback and testing @mtien-apache and @mcgilman! I pushed one more update correcting some spelling and naming issues.

@mcgilman mcgilman merged commit 26400fc into apache:main Mar 28, 2023
exceptionfactory added a commit that referenced this pull request Mar 28, 2023
* NIFI-4890 Refactored OIDC with support for Refresh Tokens

- Implemented OIDC Authorization Code Grant Flow using Spring Security Filters
- Implemented OIDC RP-Initiated Logout 1.0
- Implemented OAuth2 Token Revocation RFC 7009 for Refresh Tokens
- Added OIDC Bearer Token Refresh Filter for updating application Bearer Tokens from Refresh Token exchanges
- Added configurable Token Refresh Window to application properties
- Removed original implementation and supporting classes

* NIFI-4890 Set Bearer Token expiration based on Access Token

* NIFI-4890 Corrected spelling and naming issues based on feedback

This closes #7013
r-vandenbos pushed a commit to r-vandenbos/nifi that referenced this pull request Apr 11, 2023
* NIFI-4890 Refactored OIDC with support for Refresh Tokens

- Implemented OIDC Authorization Code Grant Flow using Spring Security Filters
- Implemented OIDC RP-Initiated Logout 1.0
- Implemented OAuth2 Token Revocation RFC 7009 for Refresh Tokens
- Added OIDC Bearer Token Refresh Filter for updating application Bearer Tokens from Refresh Token exchanges
- Added configurable Token Refresh Window to application properties
- Removed original implementation and supporting classes

* NIFI-4890 Set Bearer Token expiration based on Access Token

* NIFI-4890 Corrected spelling and naming issues based on feedback

This closes apache#7013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants