-
Notifications
You must be signed in to change notification settings - Fork 28
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.
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
Enable test mode on API gateway #389
Conversation
@@ -21,20 +21,20 @@ gateways: | |||
topic: input-topic | |||
parameters: | |||
- sessionId | |||
produceOptions: | |||
produce-options: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this reads like a breaking change and we have stuff in production, please don't do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no it's not, it's an alias and they both work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work but:
- I don't think we need a separate class hierarchy, we can make jwt and http regular authentication providers
- I don't think we need the ability to override the values (if we need it then we have to configure it statically somewhere, not let the client override security related pieces of information)
- I don't think we need to have a different admin-credentials parameter, it is enough to have "credentials"
My idea is that if you enable the system authentication then you gateway accepts also the credentials for the system provider and run the authentication flow.
The question maybe is "how do the gateway know that the client is authenticating with one provider, like Google, or another provider, like the System provider ?"
if that's the problem maybe it is good to have "admin-credentials", but I won't create different class hierarchies for the "System Gateway Authentication" and the regular Gateway Authentication
import java.util.Map; | ||
import lombok.SneakyThrows; | ||
|
||
public class JwtAdminAuthenticationProvider implements GatewayAdminAuthenticationProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why only "admin", we can make it a regular implementation
import lombok.extern.slf4j.Slf4j; | ||
|
||
@Slf4j | ||
public class HttpAdminAuthenticationProvider implements GatewayAdminAuthenticationProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why only "admin", we can make it a regular implementation
langstream-api-gateway-auth/pom.xml
Outdated
@@ -30,5 +30,7 @@ | |||
<modules> | |||
<module>langstream-google-api-gateway-auth</module> | |||
<module>langstream-github-api-gateway-auth</module> | |||
<module>langstream-jwt-gateway-admin-auth</module> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would drop "admin" from everywhere
} | ||
|
||
final GatewayAdminAuthenticationProvider gatewayAdminAuthenticationProvider = | ||
adminAuthProviders.get(provider); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we shouldn't let the client decide which provider,
there will be only one configured, there is no need to have more
Map<String, String> pathVars, | ||
Map<String, String> queryString, | ||
Map<String, String> httpHeaders) { | ||
Map<String, String> options = new HashMap<>(); | ||
Map<String, String> userParameters = new HashMap<>(); | ||
|
||
final String credentials = queryString.remove("credentials"); | ||
final String adminCredentials = queryString.remove("admin-credentials"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is need to have a different parameter
|
||
for (Map.Entry<String, String> entry : queryString.entrySet()) { | ||
if (entry.getKey().startsWith("admin-credentials-input-")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we need this thing, it seems like a security hole, what's the use case ?
@@ -42,4 +42,26 @@ public static GatewayAuthenticationProvider loadProvider( | |||
store.initialize(configuration); | |||
return store; | |||
} | |||
|
|||
public static GatewayAdminAuthenticationProvider loadAdminProvider( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to have a different mechanism
@@ -67,6 +67,21 @@ public class ChatGatewayCmd extends BaseGatewayCmd { | |||
description = "Connect timeout for WebSocket connections in seconds.") | |||
private long connectTimeoutSeconds = 0; | |||
|
|||
@CommandLine.Option( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Changes:
allow-test-mode
to theauthentication
gateway field. By default is true.There are two new auth implementations:
They can be used also by the gateway for production authentications.
test-credentials
param instead ofcredentials
to enter the test mode.Client connect string example with token:
HTTP
JWT