-
Notifications
You must be signed in to change notification settings - Fork 120
Enable multiple authentication methods (External + Basic Auth) #785
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,28 +145,39 @@ private LbFormAuthManager getFormAuthManager(HaGatewayConfiguration configuratio | |
| private ChainedAuthFilter getAuthenticationFilters(AuthenticationConfiguration config, Authorizer authorizer) | ||
| { | ||
| ImmutableList.Builder<ContainerRequestFilter> authFilters = ImmutableList.builder(); | ||
| String defaultType = config.getDefaultType(); | ||
| if (oauthManager != null) { | ||
| authFilters.add(new LbFilter( | ||
| new LbAuthenticator(oauthManager, authorizationManager), | ||
| authorizer, | ||
| "Bearer", | ||
| new LbUnauthorizedHandler(defaultType))); | ||
| List<String> authMethods = config.getDefaultTypes(); | ||
| if (authMethods == null || authMethods.isEmpty()) { | ||
| return new ChainedAuthFilter(authFilters.build()); | ||
| } | ||
|
|
||
| if (formAuthManager != null) { | ||
| authFilters.add(new LbFilter( | ||
| new FormAuthenticator(formAuthManager, authorizationManager), | ||
| authorizer, | ||
| "Bearer", | ||
| new LbUnauthorizedHandler(defaultType))); | ||
| for (String authMethod : authMethods) { | ||
| switch (authMethod) { | ||
| case "oauth" -> { | ||
| if (oauthManager != null) { | ||
| authFilters.add(new LbFilter( | ||
| new LbAuthenticator(oauthManager, authorizationManager), | ||
| authorizer, | ||
| "Bearer", | ||
| new LbUnauthorizedHandler("oauth"))); | ||
| } | ||
| } | ||
| case "form" -> { | ||
| if (formAuthManager != null) { | ||
| authFilters.add(new LbFilter( | ||
| new FormAuthenticator(formAuthManager, authorizationManager), | ||
| authorizer, | ||
| "Bearer", | ||
| new LbUnauthorizedHandler("form"))); | ||
|
|
||
| authFilters.add(new BasicAuthFilter( | ||
| new ApiAuthenticator(formAuthManager, authorizationManager), | ||
| authorizer, | ||
| new LbUnauthorizedHandler(defaultType))); | ||
| authFilters.add(new BasicAuthFilter( | ||
| new ApiAuthenticator(formAuthManager, authorizationManager), | ||
| authorizer, | ||
| new LbUnauthorizedHandler("form"))); | ||
| } | ||
| } | ||
| default -> {} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(HaGatewayProviderModule.class);Place this line with other field declarations in the class. |
||
| } | ||
| } | ||
|
|
||
| return new ChainedAuthFilter(authFilters.build()); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,11 +24,13 @@ public class AuthorizationManager | |
| { | ||
| private final Map<String, UserConfiguration> presetUsers; | ||
| private final LbLdapClient lbLdapClient; | ||
| private final AuthorizationConfiguration authorizationConfiguration; | ||
|
|
||
| public AuthorizationManager(AuthorizationConfiguration configuration, | ||
| Map<String, UserConfiguration> presetUsers) | ||
| { | ||
| this.presetUsers = presetUsers; | ||
| this.authorizationConfiguration = configuration; | ||
| if (configuration != null && configuration.getLdapConfigPath() != null) { | ||
| lbLdapClient = new LbLdapClient(LdapConfiguration.load(configuration.getLdapConfigPath())); | ||
| } | ||
|
|
@@ -49,6 +51,13 @@ public Optional<String> getPrivileges(String username) | |
| else if (lbLdapClient != null) { | ||
| privs = lbLdapClient.getMemberOf(username); | ||
| } | ||
| return Optional.ofNullable(privs); | ||
|
|
||
| if (privs == null || privs.trim().isEmpty()) { | ||
| if (authorizationConfiguration != null && authorizationConfiguration.getDefaultPrivilege() != null) { | ||
|
Comment on lines
+55
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚨 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. |
||
| return Optional.of(authorizationConfiguration.getDefaultPrivilege()); | ||
| } | ||
| return Optional.empty(); // No default privilege if not configured | ||
| } | ||
| return Optional.of(privs); | ||
| } | ||
| } | ||
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.
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.