kevdoran commented on code in PR #10554:
URL: https://github.com/apache/nifi/pull/10554#discussion_r2611770316
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/security/authentication/IdentityFilter.java:
##########
@@ -76,13 +80,24 @@ public void doFilter(ServletRequest servletRequest,
ServletResponse servletRespo
try {
AuthenticationRequest authenticationRequest =
identityProvider.extractCredentials((HttpServletRequest) servletRequest);
if (authenticationRequest != null) {
- Authentication authentication = new
AuthenticationRequestToken(authenticationRequest, identityProvider.getClass(),
servletRequest.getRemoteAddr());
+ Authentication authentication = new AuthenticationRequestToken(
+ authenticationRequest,
+ identityProvider.getClass(),
+ servletRequest.getRemoteAddr());
logger.debug("Adding credentials claim to SecurityContext to
be authenticated. Credentials extracted by {}: {}",
identityProvider.getClass().getSimpleName(),
authenticationRequest);
-
SecurityContextHolder.getContext().setAuthentication(authentication);
- // This filter's job, which is merely to search for and
extract an identity claim, is done.
- // The actual authentication of the identity claim will be
handled by a corresponding IdentityAuthenticationProvider
+ if (authenticationManager != null) {
+ try {
+ Authentication authenticated =
authenticationManager.authenticate(authentication);
+
SecurityContextHolder.getContext().setAuthentication(authenticated);
+ } catch (AuthenticationException ex) {
+ logger.debug("Authentication failed in IdentityFilter
for provider {}: {}", identityProvider.getClass().getSimpleName(),
ex.getMessage());
+ throw ex;
+ }
+ } else {
+
SecurityContextHolder.getContext().setAuthentication(authentication);
+ }
Review Comment:
@pvillard31 @exceptionfactory After some more research, it looks like the
Spring Security `AuthenticationProvider` is not deprecated, it has only been
discussed: https://github.com/spring-projects/spring-security/issues/11428
The `AuthenticationManager` we use in NiFi Registry the delegates to one or
more AuthenticationProviders is still valid, but it is no longer being invoked
implicitly. @pvillard31's approach to invoke it directly from the NiFi Registry
`IdentityFilter` is correct. I proposed some small changes here:
https://github.com/pvillard31/nifi/pull/9
Note, one important functional change that I made is not to rethrow the
authentication exception in the IdentityFilter. This will result in HTTP 500
errors due to unmapped exception handing in the servlet. I tested this my
passing an invalid bearer token. My code will allow the request to continue
without an authenticated authentication object in the ServletContext, resulting
in our configured http401AuthenticationEntryPoint returning a 401 response.
I think this is good for Spring Security 7 for as long as we need it to
function this way. I think at this point, the IdentityProvider extension point
could be removed, along with some of the included providers (Kerberos, etc) and
we could simplify Registry by allowing for configuring OIDC (or similar) to
integrate with an external IdentityProvider without allowing for extensibility
for a custom identity provider. But I think that should be out of scope for now
as it requires its own discussion.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
