Enable multiple authentication methods (External + Basic Auth) #785
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Andy Jiang.
|
Reviewer's GuideThis PR enhances authentication to support multiple fallback methods, adds a default-privilege mechanism for new users, refactors OIDC test setup into reusable utilities, and updates the UI and docs accordingly. Sequence diagram for authentication fallback logicsequenceDiagram
participant "Client"
participant "ChainedAuthFilter"
participant "LbFilter (OAuth)"
participant "LbFilter (Form)"
participant "BasicAuthFilter (Form API)"
Client->>ChainedAuthFilter: Send authentication request
ChainedAuthFilter->>"LbFilter (OAuth)": Try OAuth authentication
alt OAuth fails
ChainedAuthFilter->>"LbFilter (Form)": Try Form authentication
alt Form fails
ChainedAuthFilter->>"BasicAuthFilter (Form API)": Try Basic Auth (Form API)
end
end
Class diagram for updated AuthenticationConfiguration and AuthorizationConfigurationclassDiagram
class AuthenticationConfiguration {
+List<String> defaultTypes
+OAuthConfiguration oauth
+FormAuthConfiguration form
+getDefaultTypes()
+setDefaultTypes(List<String>)
}
class AuthorizationConfiguration {
+String admin
+String user
+String api
+String ldapConfigPath
+String defaultPrivilege
+getDefaultPrivilege()
+setDefaultPrivilege(String)
}
AuthenticationConfiguration --> OAuthConfiguration
AuthenticationConfiguration --> FormAuthConfiguration
Class diagram for updated AuthorizationManager privilege logicclassDiagram
class AuthorizationManager {
-Map<String, UserConfiguration> presetUsers
-LbLdapClient lbLdapClient
-AuthorizationConfiguration authorizationConfiguration
+getPrivileges(String username)
}
AuthorizationManager --> AuthorizationConfiguration
AuthorizationManager --> LbLdapClient
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `gateway-ha/src/main/java/io/trino/gateway/ha/module/HaGatewayProviderModule.java:178` </location>
<code_context>
+ new LbUnauthorizedHandler("form")));
+ }
+ }
+ default -> {}
+ }
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Default case in switch silently ignores unknown auth methods.
Logging a warning or error in the default case would help identify misconfigurations involving unsupported authentication methods.
Suggested implementation:
```java
for (String authMethod : authMethods) {
switch (authMethod) {
case "oauth" -> {
```
```java
default -> {
logger.warn("Unknown authentication method configured: {}", authMethod);
}
```
If the `logger` variable is not already defined in this file, you should add it at the top of the class:
```java
private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(HaGatewayProviderModule.class);
```
Place this line with other field declarations in the class.
</issue_to_address>
### Comment 2
<location> `gateway-ha/src/main/java/io/trino/gateway/ha/security/AuthorizationManager.java:55-56` </location>
<code_context>
}
- return Optional.ofNullable(privs);
+
+ if (privs == null || privs.trim().isEmpty()) {
+ if (authorizationConfiguration != null && authorizationConfiguration.getDefaultPrivilege() != null) {
+ return Optional.of(authorizationConfiguration.getDefaultPrivilege());
+ }
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Default privilege fallback may mask misconfigurations.
Log fallback events to alert administrators to missing privilege assignments.
Suggested implementation:
```java
this.presetUsers = presetUsers;
this.authorizationConfiguration = configuration;
if (configuration != null && configuration.getLdapConfigPath() != null) {
lbLdapClient = new LbLdapClient(LdapConfiguration.load(configuration.getLdapConfigPath()));
}
else if (lbLdapClient != null) {
privs = lbLdapClient.getMemberOf(username);
}
// Add logger for fallback events
private static final org.slf4j.Logger log = org.slf4j.LoggerFactory.getLogger(AuthorizationManager.class);
```
```java
if (privs == null || privs.trim().isEmpty()) {
if (authorizationConfiguration != null && authorizationConfiguration.getDefaultPrivilege() != null) {
log.warn("Privilege for user '{}' is missing or empty. Falling back to default privilege: '{}'. Please check privilege assignments.", username, authorizationConfiguration.getDefaultPrivilege());
return Optional.of(authorizationConfiguration.getDefaultPrivilege());
}
return Optional.empty(); // No default privilege if not configured
```
If the logger is already defined elsewhere in the class, do not redefine it. Place the logger definition at the top of the class if needed.
</issue_to_address>
### Comment 3
<location> `docs/security.md:39` </location>
<code_context>
`authentication:` section in the config file. The default authentication type is
-set using `defaultType: "form"` Following types of the authentications are
-supported.
+set using `defaultTypes: ["form"]`. The first authentication type in `defaultTypes` is prioritized and then falls back to following ones.
+Following types of the authentications are supported.
</code_context>
<issue_to_address>
**suggestion (typo):** Consider revising 'falls back to following ones' to 'falls back to the following ones' for grammatical correctness.
Adding 'the' before 'following ones' improves clarity and grammatical accuracy.
```suggestion
set using `defaultTypes: ["form"]`. The first authentication type in `defaultTypes` is prioritized and then falls back to the following ones.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| new LbUnauthorizedHandler("form"))); | ||
| } | ||
| } | ||
| default -> {} |
There was a problem hiding this comment.
suggestion (bug_risk): Default case in switch silently ignores unknown auth methods.
Logging a warning or error in the default case would help identify misconfigurations involving unsupported authentication methods.
Suggested implementation:
for (String authMethod : authMethods) {
switch (authMethod) {
case "oauth" -> { default -> {
logger.warn("Unknown authentication method configured: {}", authMethod);
}If the logger variable is not already defined in this file, you should add it at the top of the class:
private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(HaGatewayProviderModule.class);Place this line with other field declarations in the class.
Comment on lines
+55
to
+56
| if (privs == null || privs.trim().isEmpty()) { | ||
| if (authorizationConfiguration != null && authorizationConfiguration.getDefaultPrivilege() != null) { |
There was a problem hiding this comment.
🚨 suggestion (security): Default privilege fallback may mask misconfigurations.
Log fallback events to alert administrators to missing privilege assignments.
Suggested implementation:
this.presetUsers = presetUsers;
this.authorizationConfiguration = configuration;
if (configuration != null && configuration.getLdapConfigPath() != null) {
lbLdapClient = new LbLdapClient(LdapConfiguration.load(configuration.getLdapConfigPath()));
}
else if (lbLdapClient != null) {
privs = lbLdapClient.getMemberOf(username);
}
// Add logger for fallback events
private static final org.slf4j.Logger log = org.slf4j.LoggerFactory.getLogger(AuthorizationManager.class); if (privs == null || privs.trim().isEmpty()) {
if (authorizationConfiguration != null && authorizationConfiguration.getDefaultPrivilege() != null) {
log.warn("Privilege for user '{}' is missing or empty. Falling back to default privilege: '{}'. Please check privilege assignments.", username, authorizationConfiguration.getDefaultPrivilege());
return Optional.of(authorizationConfiguration.getDefaultPrivilege());
}
return Optional.empty(); // No default privilege if not configuredIf the logger is already defined elsewhere in the class, do not redefine it. Place the logger definition at the top of the class if needed.
| `authentication:` section in the config file. The default authentication type is | ||
| set using `defaultType: "form"` Following types of the authentications are | ||
| supported. | ||
| set using `defaultTypes: ["form"]`. The first authentication type in `defaultTypes` is prioritized and then falls back to following ones. |
There was a problem hiding this comment.
suggestion (typo): Consider revising 'falls back to following ones' to 'falls back to the following ones' for grammatical correctness.
Adding 'the' before 'following ones' improves clarity and grammatical accuracy.
Suggested change
| set using `defaultTypes: ["form"]`. The first authentication type in `defaultTypes` is prioritized and then falls back to following ones. | |
| set using `defaultTypes: ["form"]`. The first authentication type in `defaultTypes` is prioritized and then falls back to the following ones. |
AndyJiang99
force-pushed
the
andyjiang/dual-auth
branch
from
94425d1 to
06e7e0f
Compare
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Andy Jiang.
|
AndyJiang99
force-pushed
the
andyjiang/dual-auth
branch
from
06e7e0f to
b043ad5
Compare
AndyJiang99
force-pushed
the
andyjiang/dual-auth
branch
from
b043ad5 to
5b57d8f
Compare
AndyJiang99
force-pushed
the
andyjiang/dual-auth
branch
from
5b57d8f to
dd4e53e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
At LinkedIn, we need to be able to use multiple different authentication mechanisms, similar to the Trino Server itself, as we would like to:
This PR has the following changes:
defaultTypeauthentication to a list, to enable fallback authentications. This change allows for multiple authentication methods similar to trinodb/trino authentication: https://trino.io/docs/current/security/authentication-types.htmldefaultType, even if there are multiple authentication methods set.authorization, anddefaultPrivilegeis set, they will be given the permission of defined indefaultPrivilege. This resolves the issue for OAuth authentications for large enterprises using Trino Gateway as they cannot add every single user to haveUSERpermissions by default.This PR is related to : #616
Additional context and related issues
Configurations
UI View with defaultPrivilege
andjiangis nto in the presetUsers list, defaults over toUSERpermissions so can only view historyAPI Calls with OAuth enabled:
trino-adhoc1Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( x ) Release notes are required, with the following suggested text:
Summary by Sourcery
Enable support for multiple authentication methods in Trino Gateway HA by replacing the single defaultType with a prioritized list (defaultTypes), add fallbacks for BasicAuth API calls alongside OAuth/form login, and introduce a defaultPrivilege setting to automatically assign permissions to users not explicitly configured. Centralize OIDC test setup and add new tests for authorization and authentication fallback behaviors.
New Features:
Enhancements:
Build:
Documentation:
Tests: