You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/08/13 17:56:52 UTC
[GitHub] [nifi-registry] thenatog opened a new pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
thenatog opened a new pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296
NIFIREG-313 - Added Login and Logout filters. Added unit tests.
NIFIREG-313 - Added changes from NiFi PR #4344. Fixing tests.
NIFIREG-313 - alopresto provided a patch to fix failing tests as a result of the NiFiRegistryProperties mocking not working the same as NiFi and other minor issues.
NIFIREG-313 - Added the OIDC identity provider to the security config.
NIFIREG-313 - Testing javascript to make call to exchange OIDC cookie for JWT. I believe some parts in the NiFiRegistrySecurityConfig are not required, similar to how Kerberos auth works.
NIFIREG-313 - OIDC Authentication works correctly. Need to adjust javascript to allow either Kerberos or OIDC - currently only allows OIDC.
NIFIREG-313 - Updated ticketExchange function to call both Kerberos and OIDC endpoints to retrieve a JWT.
NIFIREG-313 - User can hit the LOGIN button to log in with OIDC, or it will redirect to the default login dialog for basic credential login.
NIFIREG-313 - Removing unused imports
NIFIREG-313 - OIDC login can only be attempted if OIDC is configured.
NIFIREG-313 - Fixed ticketExchange() javascript to work with x509 certs. Removed unnecessary code from LogoutFilter.
Thank you for submitting a contribution to Apache NiFi Registry.
Please provide a short description of the PR here:
#### Description of PR
_Enables X functionality; fixes bug NIFIREG-YYYY._
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 **NIFIREG-XXXX** where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically `main`)?
- [x] 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:
- [x] Have you ensured that the full suite of tests is executed via `mvn -Pcontrib-check clean install` at the root `nifi-registry` folder?
- [x] Have you written or updated unit tests to verify your changes?
- [x] 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](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the `LICENSE` file, including the main `LICENSE` file under `nifi-registry-assembly`?
- [ ] If applicable, have you updated the `NOTICE` file, including the main `NOTICE` file found under `nifi-registry-assembly`?
### 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.
----------------------------------------------------------------
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
[GitHub] [nifi-registry] thenatog commented on pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
thenatog commented on pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#issuecomment-673631378
This change matches, as close as possible, the OIDC authentication code in NiFi. When opening the NiFi Registry UI with OIDC configured, there should be a Login button in the top right corner. Once clicked, NiFi Registry should redirect you to the configured OIDC provider login page, you can enter your credentials, and the provider should redirect back to NiFi Registry and show your logged in user in the top right. Hitting log out will log out your user from NiFi Registry and remove the user's JWT.
I have tested:
- Using X509 without any other authentication mechanism enabled.
- Using OIDC authentication provider (Google Suite). Logging in/logging out is working.
- Using LDAP through the NiFi Registry basic credentials dialog.
I think the reviewer should test using Kerberos to ensure everything is still fine there - I do not have a good set up to test that.
----------------------------------------------------------------
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
[GitHub] [nifi-registry] pvillard31 commented on pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#issuecomment-691457645
----------------------------------------------------------------
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
[GitHub] [nifi-registry] bbende commented on pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
bbende commented on pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#issuecomment-691097051
----------------------------------------------------------------
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
[GitHub] [nifi-registry] bbende merged pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
bbende merged pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296
----------------------------------------------------------------
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
[GitHub] [nifi-registry] thenatog commented on pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
thenatog commented on pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#issuecomment-691313090
----------------------------------------------------------------
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
[GitHub] [nifi-registry] bbende commented on a change in pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
bbende commented on a change in pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#discussion_r475666718
##########
File path: nifi-registry-core/nifi-registry-properties/src/main/java/org/apache/nifi/registry/properties/NiFiRegistryProperties.java
##########
@@ -87,6 +92,19 @@
public static final String KERBEROS_SERVICE_PRINCIPAL = "nifi.registry.kerberos.service.principal";
public static final String KERBEROS_SERVICE_KEYTAB_LOCATION = "nifi.registry.kerberos.service.keytab.location";
+ // OIDC properties
+ public static final String SECURITY_USER_OIDC_DISCOVERY_URL = "nifi.registry.security.user.oidc.discovery.url";
+ public static final String SECURITY_USER_OIDC_CONNECT_TIMEOUT = "nifi.registry.security.user.oidc.connect.timeout";
+ public static final String SECURITY_USER_OIDC_READ_TIMEOUT = "nifi.registry.security.user.oidc.read.timeout";
+ public static final String SECURITY_USER_OIDC_CLIENT_ID = "nifi.registry.security.user.oidc.client.id";
+ public static final String SECURITY_USER_OIDC_CLIENT_SECRET = "nifi.registry.security.user.oidc.client.secret";
+ public static final String SECURITY_USER_OIDC_PREFERRED_JWSALGORITHM = "nifi.registry.security.user.oidc.preferred.jwsalgorithm";
+ public static final String SECURITY_USER_OIDC_ADDITIONAL_SCOPES = "nifi.registry.security.user.oidc.additional.scopes";
+ public static final String SECURITY_USER_OIDC_CLAIM_IDENTIFYING_USER = "nifi.registry.security.user.oidc.claim.identifying.user";
Review comment:
The default nifi-registry.properties file needs to be updated to include these
----------------------------------------------------------------
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
[GitHub] [nifi-registry] thenatog commented on a change in pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
thenatog commented on a change in pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#discussion_r470725839
##########
File path: nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/security/NiFiRegistrySecurityConfig.java
##########
@@ -88,7 +88,8 @@ public NiFiRegistrySecurityConfig() {
@Override
public void configure(WebSecurity webSecurity) throws Exception {
// allow any client to access the endpoint for logging in to generate an access token
- webSecurity.ignoring().antMatchers( "/access/token/**");
+ webSecurity.ignoring().antMatchers("/access/config", "/access/token", "/access/kerberos",
+ "/access/oidc/exchange", "/access/oidc/callback", "/access/oidc/request", "/access/token/identity-provider");
Review comment:
Yeah good point, I'll verify all of these patterns as some appear different to NiFi. We want this list to be concise.
----------------------------------------------------------------
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
[GitHub] [nifi-registry] thenatog commented on pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
thenatog commented on pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#issuecomment-691313090
Added the OIDC properties and updated the NOTICE file. I'm noticing issues with the checks failing - I'm not seeing this happening locally. It also appears to be happening on Kevin's PR so perhaps these failures are unrelated to my PR.
----------------------------------------------------------------
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
[GitHub] [nifi-registry] bbende commented on pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
bbende commented on pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#issuecomment-692066511
+1 latest updates look good, going to merge
----------------------------------------------------------------
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
[GitHub] [nifi-registry] pvillard31 commented on pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#issuecomment-691457645
----------------------------------------------------------------
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
[GitHub] [nifi-registry] thenatog commented on a change in pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
thenatog commented on a change in pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#discussion_r470722627
##########
File path: nifi-registry-core/pom.xml
##########
@@ -143,6 +143,17 @@
<artifactId>jersey-media-multipart</artifactId>
<version>${jersey.server.version}</version>
</dependency>
+ <!-- open id connect - override transitive dependency version ranges -->
+ <dependency>
+ <groupId>com.nimbusds</groupId>
+ <artifactId>oauth2-oidc-sdk</artifactId>
+ <version>6.16.2</version>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ <version>28.0-jre</version>
+ </dependency>
Review comment:
I'd say yes. Are there any issues with adding this dependency to the core pom? I believe this is used already in NiFi.
Guava provides a cache used by the OIDC service.
----------------------------------------------------------------
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
[GitHub] [nifi-registry] bbende commented on a change in pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
bbende commented on a change in pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#discussion_r470210095
##########
File path: nifi-registry-core/nifi-registry-web-ui/src/main/java/org/apache/nifi/registry/web/filter/LogoutFilter.java
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.registry.web.filter;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletContext;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * Filter for determining appropriate logout location.
+ */
+public class LogoutFilter implements Filter {
+
+ private ServletContext servletContext;
+
+ @Override
+ public void init(FilterConfig filterConfig) throws ServletException {
+ servletContext = filterConfig.getServletContext();
+ }
+
+ @Override
+ public void doFilter(ServletRequest request, ServletResponse response, FilterChain filterChain) throws IOException, ServletException {
+ final boolean supportsOidc = Boolean.parseBoolean(servletContext.getInitParameter("oidc-supported"));
+
+ if (supportsOidc) {
+ final ServletContext apiContext = servletContext.getContext("/nifi-api");
Review comment:
Should this be nifi-registry-api?
##########
File path: nifi-registry-core/nifi-registry-properties/src/main/java/org/apache/nifi/registry/properties/NiFiRegistryProperties.java
##########
@@ -87,6 +92,19 @@
public static final String KERBEROS_SERVICE_PRINCIPAL = "nifi.registry.kerberos.service.principal";
public static final String KERBEROS_SERVICE_KEYTAB_LOCATION = "nifi.registry.kerberos.service.keytab.location";
+ // OIDC properties
+ public static final String SECURITY_USER_OIDC_DISCOVERY_URL = "nifi.registry.security.user.oidc.discovery.url";
+ public static final String SECURITY_USER_OIDC_CONNECT_TIMEOUT = "nifi.registry.security.user.oidc.connect.timeout";
+ public static final String SECURITY_USER_OIDC_READ_TIMEOUT = "nifi.registry.security.user.oidc.read.timeout";
+ public static final String SECURITY_USER_OIDC_CLIENT_ID = "nifi.registry.security.user.oidc.client.id";
+ public static final String SECURITY_USER_OIDC_CLIENT_SECRET = "nifi.registry.security.user.oidc.client.secret";
+ public static final String SECURITY_USER_OIDC_PREFERRED_JWSALGORITHM = "nifi.registry.security.user.oidc.preferred.jwsalgorithm";
+ public static final String SECURITY_USER_OIDC_ADDITIONAL_SCOPES = "nifi.registry.security.user.oidc.additional.scopes";
+ public static final String SECURITY_USER_OIDC_CLAIM_IDENTIFYING_USER = "nifi.registry.security.user.oidc.claim.identifying.user";
+
+ // Apache Knox
+ public static final String SECURITY_USER_KNOX_URL = "nifi.registry.security.user.knox.url";
Review comment:
I think this is can be removed since we don't have knox support yet
##########
File path: nifi-registry-core/nifi-registry-properties/src/main/java/org/apache/nifi/registry/properties/NiFiRegistryProperties.java
##########
@@ -336,4 +364,119 @@ private File getPropertyAsFile(String propertyKey, String defaultFileLocation) {
}
}
+ /**
+ * Returns true if the login identity provider has been configured.
+ *
+ * @return true if the login identity provider has been configured
+ */
+ public boolean isLoginIdentityProviderEnabled() {
+ return !StringUtils.isBlank(getProperty(NiFiRegistryProperties.SECURITY_IDENTITY_PROVIDER));
+ }
+
+ /**
+ * Returns whether an OpenId Connect (OIDC) URL is set.
+ *
+ * @return whether an OpenId Connect URL is set
+ */
+ public boolean isOidcEnabled() {
+ return !StringUtils.isBlank(getOidcDiscoveryUrl());
+ }
+
+ /**
+ * Returns whether Knox SSO is enabled.
+ *
+ * @return whether Knox SSO is enabled
+ */
+ public boolean isKnoxSsoEnabled() {
+ return !StringUtils.isBlank(getKnoxUrl());
+ }
+
+ /**
+ * Returns the Knox URL.
+ *
+ * @return Knox URL
+ */
+ public String getKnoxUrl() {
+ return getProperty(SECURITY_USER_KNOX_URL);
+ }
Review comment:
Same with these two properties
##########
File path: nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/security/authentication/LoginFilter.java
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.registry.web.security.authentication;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletContext;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import java.io.IOException;
+
+/**
+ * Filter for determining appropriate login location.
+ */
+public class LoginFilter implements Filter {
Review comment:
Is this filter used? I think the one in web-ui is being used
##########
File path: nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/security/authentication/LogoutFilter.java
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.registry.web.security.authentication;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletContext;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+
+/**
+ * Filter for determining appropriate logout location.
+ */
+public class LogoutFilter implements Filter {
Review comment:
Same as previous comment
##########
File path: nifi-registry-core/pom.xml
##########
@@ -143,6 +143,17 @@
<artifactId>jersey-media-multipart</artifactId>
<version>${jersey.server.version}</version>
</dependency>
+ <!-- open id connect - override transitive dependency version ranges -->
+ <dependency>
+ <groupId>com.nimbusds</groupId>
+ <artifactId>oauth2-oidc-sdk</artifactId>
+ <version>6.16.2</version>
+ </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava</artifactId>
+ <version>28.0-jre</version>
+ </dependency>
Review comment:
Does anything need to be added to the LICENSE/NOTICE for these? I haven't checked, just wondering
##########
File path: nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/security/NiFiRegistrySecurityConfig.java
##########
@@ -88,7 +88,8 @@ public NiFiRegistrySecurityConfig() {
@Override
public void configure(WebSecurity webSecurity) throws Exception {
// allow any client to access the endpoint for logging in to generate an access token
- webSecurity.ignoring().antMatchers( "/access/token/**");
+ webSecurity.ignoring().antMatchers("/access/config", "/access/token", "/access/kerberos",
+ "/access/oidc/exchange", "/access/oidc/callback", "/access/oidc/request", "/access/token/identity-provider");
Review comment:
I'm not sure we have "/access/config", I only see "/config" which maps the ConfigResource and returns information about which type of user-group-provider and authorizer are available, which could probably go either way on whether that is exposed to unauthenticated users. I don't think it is needed by anything until after you login, but its also not exposing anything important.
----------------------------------------------------------------
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
[GitHub] [nifi-registry] pvillard31 commented on pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
pvillard31 commented on pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#issuecomment-691457645
Fixing the build issue in #303.
----------------------------------------------------------------
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
[GitHub] [nifi-registry] thenatog commented on pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
thenatog commented on pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#issuecomment-691313090
----------------------------------------------------------------
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
[GitHub] [nifi-registry] bbende commented on pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
bbende commented on pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#issuecomment-691097051
Updates look good, still two remaining comments...
1) The default nifi-registry.properties should be updated to include the OIDC properties
https://github.com/apache/nifi-registry/blob/main/nifi-registry-core/nifi-registry-resources/src/main/resources/conf/nifi-registry.properties
2) The main LICENSE and/or NOTICE files in nifi-registry-assembly should be updated to account for any new dependencies. It looks like NiFi's NOTICE file lists Guava and Nimbus, so those should be added to the NOTICE here since most likely they aren't there yet.
----------------------------------------------------------------
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
[GitHub] [nifi-registry] bbende commented on pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
bbende commented on pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#issuecomment-691097051
----------------------------------------------------------------
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
[GitHub] [nifi-registry] thenatog edited a comment on pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
thenatog edited a comment on pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#issuecomment-673631378
This change matches, as close as possible, the OIDC authentication code in NiFi. When opening the NiFi Registry UI with OIDC configured, there should be a Login button in the top right corner. Once clicked, NiFi Registry should redirect you to the configured OIDC provider login page, you can enter your credentials, and the provider should redirect back to NiFi Registry and show your logged in user in the top right. Hitting log out will log out your user from NiFi Registry and remove the user's JWT.
I have tested:
- Using X509 without any other authentication mechanism enabled.
- Using OIDC authentication provider (Google Suite). Logging in/logging out is working.
- Using LDAP through the NiFi Registry basic credentials dialog.
I think the reviewer should test using Kerberos to ensure everything is still fine there - I do not have a good set up to test that.
Thanks to others in the community for assistance with this PR. Also, thanks to @scottyaslan for assistance with the javascript :)
----------------------------------------------------------------
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
[GitHub] [nifi-registry] bbende commented on pull request #296: NIFIREG-313 - Add OpenId Connect support for authenticating users
Posted by GitBox <gi...@apache.org>.
bbende commented on pull request #296:
URL: https://github.com/apache/nifi-registry/pull/296#issuecomment-691097051
Updates look good, still two remaining comments...
1) The default nifi-registry.properties should be updated to include the OIDC properties
https://github.com/apache/nifi-registry/blob/main/nifi-registry-core/nifi-registry-resources/src/main/resources/conf/nifi-registry.properties
2) The main LICENSE and/or NOTICE files in nifi-registry-assembly should be updated to account for any new dependencies. It looks like NiFi's NOTICE file lists Guava and Nimbus, so those should be added to the NOTICE here since most likely they aren't there yet.
----------------------------------------------------------------
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