Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

addding interface for AuthenticationHandlerProvider #217

Merged
merged 12 commits into from
Feb 4, 2025
Merged

addding interface for AuthenticationHandlerProvider #217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
75c2a1b
addding interface for AuthenticationHandlerProvider
alekh2 Jan 27, 2025
a9cb298
adding org field in the UserContext interface
alekh2 Jan 27, 2025
9391259
updating interface definition of spi
alekh2 Jan 28, 2025
772776c
adding default methods to the interface
alekh2 Jan 28, 2025
f43c912
adding default methods to the interface
alekh2 Jan 28, 2025
433b0d3
Removing TODO and adding the content to the interface description
alekh2 Jan 28, 2025
fa8f531
rename method name from handle to authenticate
alekh2 Jan 28, 2025
2e3980f
reoving configresolver from the AuthenticationProvider interface
alekh2 Feb 3, 2025
36e447d
removing getOrg method from UserContext interface
alekh2 Feb 4, 2025
0166dcb
reducing number of param to authenticate method
alekh2 Feb 4, 2025
c312cda
reducing number of param to authenticate method
alekh2 Feb 4, 2025
30cb0b1
RequestContext's fields renames
gauravAshok Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion entities/src/main/java/com/flipkart/varadhi/entities/auth/UserContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,19 @@
package com.flipkart.varadhi.entities.auth;

import java.util.Map;

/*
* UserContext represents the authenticated user. It is used to pass the user information across the system.
* This can be enhanced to include more details like context around the authentication.
*/

public interface UserContext {

String getSubject();

boolean isExpired();

// TODO: enhance it to include more details like context around the authentication
default Map<String, String> getAttributes() {
return Map.of();
}
}
14 changes: 14 additions & 0 deletions entities/src/main/java/com/flipkart/varadhi/entities/utils/RequestContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package com.flipkart.varadhi.entities.utils;

import lombok.Data;

import java.net.URI;
import java.util.Map;

@Data
public class RequestContext {
Comment on lines +8 to +9
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider making the class immutable and adding documentation.

The class holds request context that should be immutable after creation to prevent unintended modifications during request processing.

Apply this diff:

+/**
+ * Represents the context of an HTTP request, providing access to URI, parameters,
+ * headers, and additional context information.
+ * <p>
+ * This class is immutable and thread-safe.
+ */
-@Data
+@Value
 public class RequestContext {

Note: @Value is Lombok's immutable variant of @Data that makes all fields private and final.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Data
public class RequestContext {
/**
* Represents the context of an HTTP request, providing access to URI, parameters,
* headers, and additional context information.
* <p>
* This class is immutable and thread-safe.
*/
@Value
public class RequestContext {

private URI uri;
private Map<String, String> params;
private Map<String, String> headers;
private Map<String, Object> context;
Comment on lines +10 to +13
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve field declarations for better type safety and immutability.

The current implementation has several potential issues:

  1. Mutable maps could be modified after creation
  2. Raw Object type in context map is not type-safe
  3. HTTP headers should be case-insensitive

Apply this diff:

-    private URI uri;
-    private Map<String, String> params;
-    private Map<String, String> headers;
-    private Map<String, Object> context;
+    private final URI uri;
+    private final Map<String, String> params;
+    private final TreeMap<String, String> headers;  // Use TreeMap with String.CASE_INSENSITIVE_ORDER
+    private final Map<String, RequestAttribute<?>> context;
+
+    public RequestContext(URI uri,
+                         Map<String, String> params,
+                         Map<String, String> headers,
+                         Map<String, RequestAttribute<?>> context) {
+        this.uri = uri;
+        this.params = Map.copyOf(params);  // Create immutable copy
+        this.headers = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
+        this.headers.putAll(headers);
+        this.context = Map.copyOf(context);
+    }
+
+    /**
+     * Type-safe container for request context attributes.
+     * @param <T> The type of the attribute value
+     */
+    public interface RequestAttribute<T> {
+        String getKey();
+        Class<T> getType();
+        T getValue();
+    }

This improves the implementation by:

  1. Making fields final and creating immutable copies of maps
  2. Using case-insensitive TreeMap for headers
  3. Adding type-safe RequestAttribute interface
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private URI uri;
private Map<String, String> params;
private Map<String, String> headers;
private Map<String, Object> context;
private final URI uri;
private final Map<String, String> params;
private final TreeMap<String, String> headers; // Use TreeMap with String.CASE_INSENSITIVE_ORDER
private final Map<String, RequestAttribute<?>> context;
public RequestContext(URI uri,
Map<String, String> params,
Map<String, String> headers,
Map<String, RequestAttribute<?>> context) {
this.uri = uri;
this.params = Map.copyOf(params); // Create immutable copy
this.headers = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
this.headers.putAll(headers);
this.context = Map.copyOf(context);
}
/**
* Type-safe container for request context attributes.
* @param <T> The type of the attribute value
*/
public interface RequestAttribute<T> {
String getKey();
Class<T> getType();
T getValue();
}

}
12 changes: 12 additions & 0 deletions spi/src/main/java/com/flipkart/varadhi/spi/authn/AuthenticationOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.flipkart.varadhi.spi.authn;

import com.flipkart.varadhi.spi.ConfigFile;
import lombok.Data;

@Data
public class AuthenticationOptions {
private String providerClassName;

@ConfigFile
private String configFile;
}
Comment on lines +7 to +12
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add documentation and proper encapsulation.

The class lacks essential documentation and proper encapsulation. As this is part of the SPI (Service Provider Interface), clear documentation is crucial for implementers.

Consider applying these improvements:

 package com.flipkart.varadhi.spi.authn;

 import com.flipkart.varadhi.spi.ConfigFile;

+/**
+ * Configuration options for authentication providers.
+ * This class holds the necessary configuration parameters required to initialize
+ * and configure an authentication provider implementation.
+ */
 public class AuthenticationOptions {
+    /**
+     * The fully qualified class name of the authentication provider implementation.
+     */
     private String providerClassName;

+    /**
+     * Path to the configuration file containing provider-specific settings.
+     */
     @ConfigFile
     private String configFile;

+    public String getProviderClassName() {
+        return providerClassName;
+    }
+
+    public void setProviderClassName(String providerClassName) {
+        this.providerClassName = providerClassName;
+    }
+
+    public String getConfigFile() {
+        return configFile;
+    }
+
+    public void setConfigFile(String configFile) {
+        this.configFile = configFile;
+    }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public class AuthenticationOptions {
private String providerClassName;
@ConfigFile
private String configFile;
}
package com.flipkart.varadhi.spi.authn;
import com.flipkart.varadhi.spi.ConfigFile;
/**
* Configuration options for authentication providers.
* This class holds the necessary configuration parameters required to initialize
* and configure an authentication provider implementation.
*/
public class AuthenticationOptions {
/**
* The fully qualified class name of the authentication provider implementation.
*/
private String providerClassName;
/**
* Path to the configuration file containing provider-specific settings.
*/
@ConfigFile
private String configFile;
public String getProviderClassName() {
return providerClassName;
}
public void setProviderClassName(String providerClassName) {
this.providerClassName = providerClassName;
}
public String getConfigFile() {
return configFile;
}
public void setConfigFile(String configFile) {
this.configFile = configFile;
}
}

11 changes: 11 additions & 0 deletions spi/src/main/java/com/flipkart/varadhi/spi/authn/AuthenticationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.flipkart.varadhi.spi.authn;

import com.flipkart.varadhi.entities.Org;
import com.flipkart.varadhi.entities.auth.UserContext;
import com.flipkart.varadhi.entities.utils.RequestContext;
import io.vertx.core.Future;

public interface AuthenticationProvider {
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add interface documentation.

Please add Javadoc to describe:

  • The purpose and responsibility of this authentication provider
  • The lifecycle management expectations
  • Thread-safety guarantees
  • Error handling contract
+/**
+ * Service provider interface for implementing authentication mechanisms.
+ * Implementations must be thread-safe and handle concurrent requests.
+ * <p>
+ * The lifecycle of an AuthenticationProvider:
+ * 1. Instance created
+ * 2. init() called once
+ * 3. handle() called multiple times concurrently
+ * </p>
+ */
 public interface AuthenticationProvider {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public interface AuthenticationProvider {
/**
* Service provider interface for implementing authentication mechanisms.
* Implementations must be thread-safe and handle concurrent requests.
* <p>
* The lifecycle of an AuthenticationProvider:
* 1. Instance created
* 2. init() called once
* 3. handle() called multiple times concurrently
* </p>
*/
public interface AuthenticationProvider {

Future<Boolean> init(AuthenticationOptions authenticationOptions);
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance init method contract.

The boolean return type doesn't provide enough context for initialization failures. Consider:

  1. Adding method documentation
  2. Using a more descriptive return type
+    /**
+     * Initializes the authentication provider with the given options.
+     * This method is called once during provider initialization.
+     *
+     * @param authenticationOptions configuration options for the provider
+     * @return Future that completes with initialization result
+     * @throws IllegalArgumentException if options are invalid
+     */
-    Future<Boolean> init(AuthenticationOptions authenticationOptions);
+    Future<Void> init(AuthenticationOptions authenticationOptions);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Future<Boolean> init(AuthenticationOptions authenticationOptions);
/**
* Initializes the authentication provider with the given options.
* This method is called once during provider initialization.
*
* @param authenticationOptions configuration options for the provider
* @return Future that completes with initialization result
* @throws IllegalArgumentException if options are invalid
*/
Future<Void> init(AuthenticationOptions authenticationOptions);

Future<UserContext> authenticate(Org org, RequestContext requestContext);
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve authenticate method contract.

Add method documentation specifying:

  • Parameter validation requirements
  • Error handling expectations
  • Thread-safety guarantees

Apply this diff:

+    /**
+     * Authenticates a request in the context of an organization.
+     *
+     * @param org organization context for the authentication
+     * @param requestContext context containing request details
+     * @return Future that completes with UserContext on successful authentication
+     * @throws IllegalArgumentException if org or requestContext is null
+     * @throws AuthenticationException if authentication fails
+     */
     Future<UserContext> authenticate(Org org, RequestContext requestContext);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Future<UserContext> authenticate(Org org, RequestContext requestContext);
/**
* Authenticates a request in the context of an organization.
*
* @param org organization context for the authentication
* @param requestContext context containing request details
* @return Future that completes with UserContext on successful authentication
* @throws IllegalArgumentException if org or requestContext is null
* @throws AuthenticationException if authentication fails
*/
Future<UserContext> authenticate(Org org, RequestContext requestContext);

}
1 change: 1 addition & 0 deletions spi/src/main/java/module-info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@

exports com.flipkart.varadhi.spi.services;
exports com.flipkart.varadhi.spi.db;
exports com.flipkart.varadhi.spi.authn;
}
Loading