kevdoran commented on a change in pull request #194: NIFIREG-212 Separating
proxy into Read, Write, and Delete so some pro…
URL: https://github.com/apache/nifi-registry/pull/194#discussion_r289536898
##########
File path:
nifi-registry-core/nifi-registry-security-api/src/main/java/org/apache/nifi/registry/security/authentication/AuthenticationRequest.java
##########
@@ -17,17 +17,20 @@
package org.apache.nifi.registry.security.authentication;
import java.io.Serializable;
+import java.util.Objects;
public class AuthenticationRequest implements Serializable {
private String username;
private Object credentials;
private Object details;
+ private String httpMethod;
- public AuthenticationRequest(String username, Object credentials, Object
details) {
+ public AuthenticationRequest(String username, Object credentials, Object
details, String httpMethod) {
Review comment:
This works, but I'd prefer not to add the HttpMethod info to all instances
of AuthenticationRequest as it is only every really used by the
X509IdentityProvider.
The `Object details` argument of this constructor is designed to be a
generic way that additional details relevant to the AuthenticationRequest can
be passed from the `IdentityProvider.extractCredentials` method to the
IdentityProvider implementation later for authentication. `details` can be any
Object type, it is only later read by the same `IdentityProvider` instance or
the AuthenticationProvider for that same IdentityProvider. In most cases, it is
unused.
Currently, for the `X509IdentityProvider`, details is set to the String
value of the Proxied Entities Chain Header, as that was the only relevant
"detail" at the time. I would suggest just creating a new wrapper object such
as X509AuthenticationRequestDetails that can hold the proxied entities chain
and the HTTP method. That can be created and set in
`X509IdentityProvider.extractCredentials`, and later both values can be
extracted in `X509IdentityAuthenticationProvider.buildAuthenticatedToken`,
This is a small nitpick, but I'd prefer not to change AuthenticationRequest
if necessary as it is part of the extension point API for third-parties, so
changing this could break and third-party impls out there. I know it would also
be possible to keep the current 3-arg constructor and add a 4-arg constructor,
but that looses some of the benefits of this implementation such as ensuring
non-null, and also adds complexity to an extension point that most people won't
care about for authentication purposes. Encasing the http method in details
prevents any other IdentityProviders (ours or third-parties) from needing
updating, but still lets us do our authorization-as-part-of-authentication
check for proxies.
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
