BasicAuth: Tutorial Refactoring #2735
Conversation
…ion workflow - Introduced `AlgoSalt` record for streamlined password hashing and salt handling. - Refactored `JdbcUserDataProvider` and `StaticUserDataProvider` to utilize enhanced hashing mechanisms. - Added comprehensive unit tests for new password hashing, salt extraction, and validation logic in `SecurityUtilsTest`. - Included a new tutorial class `BasicAuthenticationTutorialTest` for basic authentication scenarios with YAML configuration.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors Basic auth and user-data providers: adds BasicAuthenticationUtil and AlgoSalt, introduces User and AbstractUserDataProvider, converts Static/File/Jdbc providers to extend the abstract provider, changes BasicAuthenticationInterceptor flow (optional Authorization header removal), expands hashing API, and adds extensive tests plus a Basic Auth tutorial. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Interceptor as BasicAuthenticationInterceptor
participant Util as BasicAuthenticationUtil
participant Provider as UserDataProvider
participant RouterDB as Router/DB
Client->>Interceptor: HTTP request (Authorization header)
Interceptor->>Util: getCredentials(exc)
Util-->>Interceptor: BasicCredentials(username,password)
Interceptor->>Provider: verify({username,password})
Provider->>Provider: AlgoSalt.from(storedHash) / createPasswdCompatibleHash(AlgoSalt,password)
alt provider uses DB
Provider->>RouterDB: resolve DataSource / execute query
RouterDB-->>Provider: query result
end
Provider-->>Interceptor: verified attributes (password removed) or failure
alt verified
Interceptor->>Interceptor: optionally remove Authorization header
Interceptor-->>Client: forward request (next handler)
else failure
Interceptor-->>Client: 401 Unauthorized + WWW-Authenticate
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java (2)
102-121:⚠️ Potential issue | 🔴 CriticalResource leak: JDBC resources not properly managed with try-with-resources.
If an exception occurs after
getConnection()but before theclose()calls (e.g., duringprepareStatement(),executeQuery(), or result processing), theConnection,PreparedStatement, andResultSetwill not be closed, leading to resource leaks and potential connection pool exhaustion.🐛 Proposed fix using try-with-resources
- Map<String, String> result = null; - try { - var con = datasource.getConnection(); - var preparedStatement = con.prepareStatement(createGetUsersSql()); + Map<String, String> result = new HashMap<>(); + try (var con = datasource.getConnection(); + var preparedStatement = con.prepareStatement(createGetUsersSql())) { preparedStatement.setString(1, username); - ResultSet rs = preparedStatement.executeQuery(); - ResultSetMetaData rsmd = rs.getMetaData(); - - result = new HashMap<>(); - while (rs.next()) for (int i = 1; i <= rsmd.getColumnCount(); i++) - result.put(rsmd.getColumnName(i).toLowerCase(), rs.getObject(i).toString()); - - rs.close(); - preparedStatement.close(); - con.close(); - + try (ResultSet rs = preparedStatement.executeQuery()) { + ResultSetMetaData rsmd = rs.getMetaData(); + while (rs.next()) { + for (int i = 1; i <= rsmd.getColumnCount(); i++) { + Object value = rs.getObject(i); + if (value != null) { + result.put(rsmd.getColumnName(i).toLowerCase(), value.toString()); + } + } + } + } } catch (SQLException e) { log.error(e.getMessage(),e); }
112-113:⚠️ Potential issue | 🟠 MajorPotential
NullPointerExceptionwhen column value is null.
rs.getObject(i)can returnnullfor nullable database columns. Calling.toString()on a null value will throwNullPointerException.
🤖 Fix all issues with AI agents
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/authentication/BasicAuthenticationInterceptor.java`:
- Around line 129-132: setUsers currently does an unchecked cast to
StaticUserDataProvider which can throw ClassCastException; change it to mirror
getUsers by checking if userDataProvider instanceof StaticUserDataProvider first
and if so cast and call setUsers, otherwise create a new StaticUserDataProvider,
call setUsers on that new instance and replace the interceptor's provider via
setUserDataProvider(newProvider) (or alternatively throw a clear
IllegalStateException) so the behavior is safe and consistent with getUsers.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/FileUserDataProvider.java`:
- Around line 64-67: FileUserDataProvider currently splits userHash into
userHashSplit and indexes [1]/[2] which duplicates AlgoSalt parsing and can
throw ArrayIndexOutOfBoundsException; replace that manual parsing with
AlgoSalt.from(userHash) and pass the returned AlgoSalt into
createPasswdCompatibleHash(algosalt, postDataPassword), and add validation/error
handling for the case AlgoSalt.from(userHash) returns null or throws (log or
throw a clear authentication error) before assigning pw to avoid using malformed
hashes.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProvider.java`:
- Around line 77-82: AlgoSalt.from(String userHash) currently splits the input
and blindly accesses indices 1 and 2; add input validation to avoid
ArrayIndexOutOfBoundsException by first checking for null/empty userHash and
ensuring the split array length >= 3, and if the check fails throw an
IllegalArgumentException (or another appropriate checked/unchecked exception)
with a clear message including the bad input; keep the existing behavior when
valid and return new AlgoSalt(userHashSplit[1], userHashSplit[2]).
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProviderTest.java`:
- Around line 108-121: The test uses an undersized hash so isHashedPassword()
returns false; replace the user hash in StaticUserDataProviderTest (the User
created in provider.setUsers) with a properly formatted SHA-512 string
"$6$<salt>$<hash>" where the final hash part is at least 20 characters so
isHashedPassword() detects it, then update the assertion around
provider.verify(postData) to reflect the correct hashed-password behavior
(either assert successful verification or assert the specific exception you
expect from createPasswdCompatibleHash); reference User, isHashedPassword(),
createPasswdCompatibleHash(), and provider.verify() when making the change.
In `@distribution/tutorials/security/30-Basic-Authentication.yaml`:
- Around line 5-13: Update the curl examples to avoid inline credentials so
secret scanners don't flag them: replace "curl -u alice:qwertz localhost:2000"
with "curl -u alice localhost:2000" and "curl -u carol:abc123 localhost:2000"
with "curl -u carol localhost:2000" (or equivalent using --user) so curl will
prompt for the password interactively; keep the surrounding examples and
comments intact.
🧹 Nitpick comments (6)
distribution/tutorials/security/30-Basic-Authentication.yaml (1)
19-32: Add an explicit “demo only” note for plaintext credentials.A short warning helps prevent copy‑pasting plaintext passwords into real configs.
✏️ Suggested note
- basicAuthentication: # Static in-memory user configuration. # Alternatives: JDBC-, LDAP-, or FileUserDataProvider. + # NOTE: Plaintext passwords are for tutorial/demo use only. + # In production, prefer hashed passwords for all users. users:core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProvider.java (3)
51-52: Special "error" username handling appears to be test code in production.The hardcoded
"error"username that throwsRuntimeExceptionseems intended for testing error paths. Consider removing this from production code or documenting its purpose if it serves a legitimate use case.
70-72: Exception wrapping loses context.Wrapping the exception with only
e.getMessage()discards the original exception type and stack trace. Consider preserving the cause for better debugging.♻️ Proposed fix
} catch (Exception e) { - throw new RuntimeException(e.getMessage()); + throw new RuntimeException("Failed to hash password", e); }
162-169: Duplicate population ofusersByNamein bothsetUsers()andinit().Both methods populate
usersByNamefrom the users list. IfsetUsers()is called beforeinit(), the map will be populated twice with the same entries. While not incorrect, this is redundant. Consider havinginit()only populate ifusersByNameis empty, or document the expected usage pattern.Also applies to: 179-183
core/src/main/java/com/predic8/membrane/core/util/SecurityUtils.java (1)
17-18: Consider movingAlgoSalttoSecurityUtilsto avoid circular dependency.
SecurityUtilsimportsAlgoSaltfromStaticUserDataProvider, whileStaticUserDataProviderimports fromSecurityUtils. This bidirectional coupling can complicate maintenance. SinceAlgoSaltrepresents a password hashing concept (algorithm + salt), it would fit more naturally inSecurityUtils.core/src/test/java/com/predic8/membrane/core/util/SecurityUtilsTest.java (1)
1-16: Copyright notice should precede package statement.By Java convention, the copyright/license header should appear before the
packagestatement.♻️ Suggested reordering
+/* Copyright 2024 predic8 GmbH, www.predic8.com + + Licensed under the Apache License, Version 2.0 (the "License"); + ... + limitations under the License. */ + package com.predic8.membrane.core.util; - -/* Copyright 2024 predic8 GmbH, www.predic8.com -...
...ava/com/predic8/membrane/core/interceptor/authentication/BasicAuthenticationInterceptor.java
Show resolved
Hide resolved
.../java/com/predic8/membrane/core/interceptor/authentication/session/FileUserDataProvider.java
Outdated
Show resolved
Hide resolved
...ava/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProvider.java
Outdated
Show resolved
Hide resolved
...com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProviderTest.java
Show resolved
Hide resolved
- Introduced comprehensive unit tests for `FileUserDataProvider` covering initialization, malformed inputs, empty files, and user verification. - Refactored user splitting logic to limit splits, fixing issues with passwords containing colons. - Improved exception handling for invalid file paths and malformed user entries. - Documented current behavior for handling colon usage in hashed passwords.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java (2)
88-124:⚠️ Potential issue | 🔴 CriticalBind the username parameter before executing the query.
The prepared statement is created from SQL containing a
?placeholder, but the parameter is never bound. This will throw aSQLExceptionwhen the query executes.Fix: bind parameter before execute
try (var con = datasource.getConnection(); var preparedStatement = con.prepareStatement(createGetUsersSql())) { + preparedStatement.setString(1, username); try (var rs = preparedStatement.executeQuery()) { var rsmd = rs.getMetaData();
52-57:⚠️ Potential issue | 🟠 MajorImplement
sanitizeUserInputs()to prevent SQL injection via table and column identifiers.Table and column names are concatenated directly into SQL queries at lines 67-72 (
getCreateTableSql()) and 128 (createGetUsersSql()) without validation. Since these values are user-configurable via@MCAttributesetters (lines 146, 156, 166), malicious input can inject SQL. Though prepared statement parameters cannot be used for identifiers, validation is essential before interpolation.Add allow-list validation using a pattern like
^[A-Za-z0-9_]+$insanitizeUserInputs():Example implementation
private void sanitizeUserInputs() { // As we can't use prepared statement parameters for table- and columnnames so we have to sanitize the user input here. // After this method it is assumed that user input is save to use - - // TODO sanitize inputs here + if (tableName == null || userColumnName == null || passwordColumnName == null) + throw new IllegalArgumentException("Table and column names must be set."); + + String identifierPattern = "^[A-Za-z0-9_]+$"; + if (!tableName.matches(identifierPattern) + || !userColumnName.matches(identifierPattern) + || !passwordColumnName.matches(identifierPattern)) { + throw new IllegalArgumentException("Table/column names must be alphanumeric/underscore."); + } }
🤖 Fix all issues with AI agents
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/authentication/session/FileUserDataProviderTest.java`:
- Around line 291-304: The test testPasswordWithColons in
FileUserDataProviderTest currently documents the bug but doesn't assert
behavior; after provider.init(router) and obtaining FileUserDataProvider.User
user via provider.getUsersByName().get("user"), add an assertion that
user.getPassword() (or the appropriate field on FileUserDataProvider.User)
equals the entire htpasswd hash string "$6$salt$hash:with:colons" to ensure
colons in the hash are preserved.
🧹 Nitpick comments (3)
core/src/main/java/com/predic8/membrane/core/util/SecurityUtils.java (1)
58-65: TightenAlgoSalt.fromvalidation for malformed hashes.
Right now it accepts strings without a leading$or without a hash part, which can silently mis-parse into the wrong algo/salt. Consider aligning withisHashedPassword()’s expected 4‑part format.🔧 Suggested validation tightening
public record AlgoSalt(String algo, String salt) { public static AlgoSalt from(String userHash) { String[] userHashSplit = userHash.split(Pattern.quote("$")); - if (userHashSplit.length < 3) { - throw new IllegalArgumentException("Invalid hash format: %s at least 3 dollar separated parts required, got: %d".formatted(userHash, userHashSplit.length)); - } - return new AlgoSalt(userHashSplit[1], userHashSplit[2]); + if (userHashSplit.length != 4 || !userHashSplit[0].isEmpty() || userHashSplit[3].isEmpty()) { + throw new IllegalArgumentException( + "Invalid hash format: %s (expected $<algo>$<salt>$<hash>)".formatted(userHash) + ); + } + return new AlgoSalt(userHashSplit[1], userHashSplit[2]); } }core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProvider.java (2)
68-73: Improve exception handling and logging context.The exception handling could be more informative:
- The log message is empty, losing context about what failed
- Catching generic
Exceptionis overly broad- The re-thrown
RuntimeExceptionprovides no additional context♻️ Proposed improvement
if (userAttributes.getPassword() != null && isHashedPassword(userAttributes.getPassword())) { try { return createPasswdCompatibleHash(AlgoSalt.from(userAttributes.getPassword()), postDataPassword); - } catch (Exception e) { - log.debug("",e); - throw new RuntimeException(e); + } catch (IllegalArgumentException e) { + log.debug("Failed to hash password for user verification", e); + throw new RuntimeException("Password hashing failed", e); } }
173-181: Consider resolving the TODO to avoid redundant map population.The TODO correctly identifies that
usersByNamemay be populated twice ifsetUsers()is called beforeinit(). While not incorrect, this is inefficient and could be easily fixed.♻️ Option 1: Skip if already populated
`@Override` public void init(Router router) { + if (!usersByName.isEmpty()) { + return; + } for (User user : users) getUsersByName().put(user.getUsername(), user); }♻️ Option 2: Clear before populating (matches setUsers pattern)
`@Override` public void init(Router router) { + getUsersByName().clear(); for (User user : users) getUsersByName().put(user.getUsername(), user); }
...a/com/predic8/membrane/core/interceptor/authentication/session/FileUserDataProviderTest.java
Outdated
Show resolved
Hide resolved
…splitting, and add tests - Improved sanitization logic in `JdbcUserDataProvider` for table/column names using regex validation. - Fixed hash handling in `SecurityUtils` to enforce correct format and refine error messages. - Resolved colon splitting issue in `FileUserDataProviderTest` for hashed passwords. - Added extensive unit tests for `JdbcUserDataProvider` to verify initialization, input sanitization, and user verification scenarios. - Refactored `StaticUserDataProvider` to prevent redundant map population in `init` method.
…L handling in `JdbcUserDataProviderTest`.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java (1)
83-101:⚠️ Potential issue | 🟠 MajorFix exception that bypasses bean-factory fallback when registry lacks DataSource bean.
Calling
get()on an emptyOptional(returned bygetBean()) throwsNoSuchElementException, preventing the fallback lookup. Check for presence before callingget():🛠️ Suggested fix
if (router.getRegistry() != null) { - var ds = router.getRegistry().getBean(DataSource.class).get(); - if (ds == null) - throw new RuntimeException("No datasource found - specifiy a DataSource bean in your Membrane configuration"); - datasource = ds; - return; + var dsOpt = router.getRegistry().getBean(DataSource.class); + if (dsOpt.isPresent()) { + datasource = dsOpt.get(); + return; + } }
🤖 Fix all issues with AI agents
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/authentication/BasicAuthenticationInterceptor.java`:
- Around line 98-117: getAuthorizationHeaderDecoded currently assumes a non-null
Authorization header with a "Basic " prefix and will throw unchecked exceptions
for other schemes or malformed values; update it to first verify the header is
non-null and starts with the "Basic " scheme (case-insensitive) and that the
length is sufficient before trying to substring/decoding, and if the check fails
throw NoSuchElementException so callers like validUser() will treat it as
missing/invalid credentials; reference the methods hasNoAuthorizationHeader,
getAuthorizationHeaderDecoded and validUser to locate where to add the
scheme/length validation and exception throw.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProvider.java`:
- Around line 64-75: The getPassword method may attempt to hash a null incoming
password when stored userAttributes has a hashed password, causing
createPasswdCompatibleHash(...) to throw and return a 500; update getPassword to
first read postData.get("password") into postDataPassword and if
userAttributes.getPassword() is hashed, check postDataPassword for null/empty
and return null (or a sentinel that indicates auth failure) instead of calling
createPasswdCompatibleHash; keep the try/catch around
createPasswdCompatibleHash(AlgoSalt.from(...), postDataPassword) but only invoke
it when postDataPassword is non-null, and maintain existing logging/exception
behavior for true unexpected errors.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/authentication/session/FileUserDataProviderTest.java`:
- Around line 102-108: The test in FileUserDataProviderTest is duplicating the
valid htpasswd entry by combining Files.readString(htpasswdFile) with
StandardOpenOption.APPEND; instead of re-reading and re-appending the whole
file, append only the malformed lines to the existing file (e.g., call
Files.writeString on htpasswdFile with just the malformed_line_without_colon and
:nousername lines and StandardOpenOption.APPEND) so the valid entry is not
duplicated and the provider's duplicate-user behavior remains testable.
- Around line 118-121: The test testInitWithNonExistentFile uses a hardcoded
Unix path; make it OS-portable by creating a path inside a temporary directory
that is guaranteed not to exist and passing that to provider.setHtpasswdPath,
then assertThrows on provider.init(router); use Java's temporary folder or
Files.createTempDirectory/resolve to build the missing file path and delete it
if necessary so the test is deterministic (refer to testInitWithNonExistentFile,
provider.setHtpasswdPath(...), provider.init(router)).
- Around line 200-215: The verify() flow currently returns a User attributes map
that includes the password/hash (set via User.setPassword()), which
unnecessarily exposes sensitive data; modify the implementation so the map
returned by FileUserDataProvider.verify() (or User.getAttributes()) excludes the
"password" key—either by making User.getAttributes() return a copy with
"password" removed or by filtering/removing "password" just before verify()
returns—ensuring TokenProvider implementations (TOTPTokenProvider,
NumericTokenProvider, SMSTokenProvider) still receive needed custom attributes
like "secret", "sms", "token", and "username".
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProviderTest.java`:
- Around line 87-95: The test currently uses the router initialized in setUp()
which registers a DataSource, so provider.init(router) succeeds and verify(...)
fails for the wrong reason; change the test to use a router instance that does
not have any DataSource registered (e.g., create a fresh Router or clear the
datasource on the existing router) before calling provider.init(router) and then
assert that init (or init followed immediately by verify if you prefer) throws
RuntimeException; specifically update JdbcUserDataProviderTest to avoid using
the setUp() router, locate the provider.init(router) call and replace the router
with a router-without-datasource to trigger and assert the intended
RuntimeException.
...ava/com/predic8/membrane/core/interceptor/authentication/BasicAuthenticationInterceptor.java
Outdated
Show resolved
Hide resolved
...ava/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProvider.java
Outdated
Show resolved
Hide resolved
...a/com/predic8/membrane/core/interceptor/authentication/session/FileUserDataProviderTest.java
Show resolved
Hide resolved
...a/com/predic8/membrane/core/interceptor/authentication/session/FileUserDataProviderTest.java
Show resolved
Hide resolved
...a/com/predic8/membrane/core/interceptor/authentication/session/FileUserDataProviderTest.java
Show resolved
Hide resolved
...a/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProviderTest.java
Show resolved
Hide resolved
…mline user attribute filtering, and improve test coverage - Introduced `BasicAuthenticationUtil` for extracting and handling Basic authentication credentials. - Refactored `StaticUserDataProvider` to use a common `filterPassword` method for secure user attribute handling. - Enhanced `JdbcUserDataProvider` for safer bean initialization with null checks and better error messages. - Expanded test coverage with detailed tests for `BasicAuthenticationUtil` and user attribute filtering. - Improved robustness and maintainability of authentication components.
…entralize user attribute handling, and update tests
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProvider.java (1)
54-68:⚠️ Potential issue | 🟡 MinorRemove redundant getter method overrides.
getUsername(),getPassword(), andgetAttributes()are identical to the parentUserclass implementations and lack annotations. These getters can be safely removed.However, keep the setter method overrides (
setUsername()andsetPassword()) as they carry the@MCAttributeannotation required for configuration schema processing. The parent class methods lack these annotations, and the annotation framework only detects annotations on methods in the class being configured, not inherited methods.core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java (1)
114-148:⚠️ Potential issue | 🟡 MinorAvoid last-row override when multiple users share a username.
If the query returns multiple rows (no unique constraint on username), the current logic overwrites
resultand only validates the last row. Consider returning as soon as a matching row is found.🔧 Proposed fix (return on first matching row)
- Map<String, String> result = new HashMap<>(); + Map<String, String> result = new HashMap<>(); try (var con = datasource.getConnection(); var preparedStatement = con.prepareStatement(createGetUsersSql())) { preparedStatement.setString(1, username); try (var rs = preparedStatement.executeQuery()) { var rsmd = rs.getMetaData(); while (rs.next()) { + result.clear(); for (int i = 1; i <= rsmd.getColumnCount(); i++) { var value = rs.getObject(i); if (value != null) { result.put(rsmd.getColumnName(i).toLowerCase(), value.toString()); } } + if (username.equals(result.get(getUserColumnName().toLowerCase())) + && verifyPassword(password, result.get(getPasswordColumnName().toLowerCase()))) { + return result; + } } } } catch (SQLException e) { log.error(e.getMessage(), e); } - - - if (result != null && !result.isEmpty()) { - if (username.equals(result.get(getUserColumnName().toLowerCase())) && verifyPassword(password, result.get(getPasswordColumnName().toLowerCase()))) { - return result; - } - } - throw new NoSuchElementException();
🤖 Fix all issues with AI agents
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/AbstractUserDataProvider.java`:
- Around line 28-31: The filterPassword method in AbstractUserDataProvider
currently mutates the passed-in userAttributes (the User's internal attributes
map) by calling remove("password"); change it to make a defensive copy first
(e.g., new HashMap<>(userAttributes)), remove the "password" entry from the
copy, and return the copy so the original User attributes remain untouched;
update the filterPassword(Map<String,String> userAttributes) implementation to
return the modified copy rather than altering the input.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java`:
- Around line 93-109: In JdbcUserDataProvider (the block that searches for a
DataSource and throws a RuntimeException), fix the typo in the exception message
by changing "specifiy" to "specify" in the RuntimeException text so it reads
e.g. "No datasource found - specify a DataSource bean in your Membrane
configuration"; update the message where the RuntimeException is thrown.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/authentication/BasicAuthenticationInterceptorTest.java`:
- Around line 35-38: The test currently registers two UserConfig entries with
the same username ("admin") so StaticUserDataProvider will overwrite the first
entry; change the second UserConfig to a distinct username (e.g., "adminHashed")
and update any assertions or calls to accept()/hashedPassword() in
BasicAuthenticationInterceptorTest to use the new username when validating the
hashed-password scenario (also apply the same username-split fix to the other
occurrence around lines 50-58); this ensures one UserConfig tests plaintext
credentials and the other tests hashed credentials without collision.
🧹 Nitpick comments (8)
core/src/main/java/com/predic8/membrane/core/util/security/BasicAuthenticationUtil.java (1)
11-12: Consider derivingBASIC_PREFIX_LENGTHfromBASIC_PREFIX.The hardcoded value
6works correctly but could become a maintenance issue ifBASIC_PREFIXis ever modified.♻️ Suggested improvement
private static final String BASIC_PREFIX = "Basic "; - private static final int BASIC_PREFIX_LENGTH = 6; + private static final int BASIC_PREFIX_LENGTH = BASIC_PREFIX.length();core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/User.java (2)
33-35: Consider returning an unmodifiable view or defensive copy fromgetAttributes().Returning the internal mutable map allows external code to modify the user's attributes directly, which could lead to unexpected side effects. If this is intentional for framework integration, consider documenting this behavior.
♻️ Option 1: Return unmodifiable view (if mutation is not needed)
public Map<String, String> getAttributes() { - return attributes; + return Collections.unmodifiableMap(attributes); }♻️ Option 2: Document the intentional mutability
+ /** + * Returns the mutable attributes map. Modifications to the returned map + * will affect this User instance. + */ public Map<String, String> getAttributes() { return attributes; }
5-39: Consider addingequals(),hashCode(), andtoString()methods.The
Userclass is used in maps (usersByName) and may be compared or logged. Without these methods, identity-based comparisons and defaultObject.toString()will be used, which may not be the desired behavior.core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/AbstractUserDataProvider.java (2)
33-35: Exposing the internalusersByNamemap allows external modification.Returning the internal mutable map permits callers to add, remove, or modify users directly. If this is intentional for configuration purposes, consider documenting it; otherwise, return an unmodifiable view.
12-21: Consider using a more specific exception type for authentication failures.
NoSuchElementExceptionis typically used for iterator/collection operations. A custom exception orAuthenticationExceptionwould better communicate the authentication context and allow callers to handle auth failures distinctly from other errors.core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProvider.java (1)
40-40: Unused logger declaration.The
logfield is declared but never used in this class. Either remove it or add logging where appropriate.♻️ Remove unused logger
- private static final Logger log = LoggerFactory.getLogger(StaticUserDataProvider.class.getName());core/src/test/java/com/predic8/membrane/core/util/security/BasicAuthenticationUtilTest.java (1)
311-319: Consider overridingtoString()inBasicCredentialsto mask the password.The default record implementation exposes the password in plaintext via
toString(). While the current codebase doesn't log the credentials object, this is a defensive programming practice—if credentials are ever included in logs or exception traces in the future, the password would be exposed. A maskedtoString()(e.g.,BasicCredentials[username=alice, password=***]) prevents accidental exposure.core/src/test/java/com/predic8/membrane/core/interceptor/authentication/session/FileUserDataProviderTest.java (1)
55-58: Use a per-user deterministic salt to strengthen test coverage.A single fixed salt for all users can let per-user salt handling bugs slip through. Consider a deterministic per-user salt (no randomness) so multi-user tests exercise distinct salts.
♻️ Suggested change
- String salt = "saltsalt"; // 8 characters minimum for SHA-512 + // Deterministic per-user salt to exercise per-user hash handling + String salt = "salt%08x".formatted(username.hashCode()); String hash = createPasswdCompatibleHash(new AlgoSalt("6", salt), plainPassword);
...a/com/predic8/membrane/core/interceptor/authentication/session/AbstractUserDataProvider.java
Outdated
Show resolved
Hide resolved
.../java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java
Show resolved
Hide resolved
...com/predic8/membrane/core/interceptor/authentication/BasicAuthenticationInterceptorTest.java
Show resolved
Hide resolved
…ialization and error handling, and update tests
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java`:
- Around line 136-138: In JdbcUserDataProvider (the catch block that currently
does "catch (SQLException e) { log.error(...); }" and then results in a
NoSuchElementException), stop masking the SQL error: instead of
swallowing/logging and letting a NoSuchElementException be thrown, rethrow an
unchecked exception that preserves the SQLException as the cause (e.g., throw
new IllegalStateException("Database error while loading user", e) or a custom
DataAccessException). Update the catch in the method (the SQLException handler)
to log and then throw the new unchecked exception so callers can distinguish DB
errors from "user not found."
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/session/StaticUserDataProvider.java (1)
95-101: Inconsistent field access: directusersByNamevsgetUsersByName().Line 97 accesses
usersByNamedirectly while line 99 usesgetUsersByName(). For consistency withsetUsers()(lines 88-90) andFileUserDataProvider, prefer using the getter throughout.♻️ Proposed fix for consistency
`@Override` public void init(Router router) { - if (usersByName.isEmpty()) { + if (getUsersByName().isEmpty()) { for (var user : users) getUsersByName().put(user.getUsername(), user); } }
.../java/com/predic8/membrane/core/interceptor/authentication/session/JdbcUserDataProvider.java
Show resolved
Hide resolved
…er by default after authentication, add configurability, and update tests
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/ROADMAP.md`:
- Line 71: Update the sentence describing BasicAuthentication so it uses correct
subject-verb agreement and the proper HTTP header name: change
"BasicAuthentication interceptor remove the Authentication header from the
request." to a grammatically correct form such as "BasicAuthentication
interceptor removes the Authorization header from the request." — target the
phrase "BasicAuthentication interceptor" and the header mention "Authentication"
to replace with "Authorization".
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/authentication/BasicAuthenticationInterceptor.java (1)
143-158: Minor: Empty user list produces slightly awkward description.If
getUsers()returns an empty list, the method produces"Users<br/>Passwords are not shown."due to howsb.delete(sb.length() - 2, sb.length())removes": "instead of", ". Consider adding an empty-list check for a cleaner output.♻️ Optional improvement
if (userDataProvider instanceof StaticUserDataProvider) { - sb.append("Users: "); - for (User user : ((StaticUserDataProvider) userDataProvider).getUsers()) { - sb.append(escapeHtml4(user.getUsername())); - sb.append(", "); + var users = ((StaticUserDataProvider) userDataProvider).getUsers(); + if (users.isEmpty()) { + sb.append("No users configured."); + } else { + sb.append("Users: "); + for (User user : users) { + sb.append(escapeHtml4(user.getUsername())); + sb.append(", "); + } + sb.delete(sb.length() - 2, sb.length()); + sb.append("<br/>Passwords are not shown."); } - sb.delete(sb.length() - 2, sb.length()); - sb.append("<br/>Passwords are not shown."); }
|
This pull request needs "/ok-to-test" from an authorized committer. |
…logic and remove redundant `getUsersByName` calls
…to basic-auth-tutorial-refactor
|
/ok-to-test |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/authentication/BasicAuthenticationInterceptor.java`:
- Around line 144-158: The getLongDescription method in
BasicAuthenticationInterceptor appends "Users: " then always calls
sb.delete(...) which corrupts output when StaticUserDataProvider.getUsers() is
empty; update getLongDescription to check whether ((StaticUserDataProvider)
userDataProvider).getUsers() is non-empty (or track whether at least one
username was appended) before performing sb.delete(sb.length() - 2,
sb.length()), so the trailing comma removal only runs when a username was
actually appended; keep the rest of the logic (escapeHtml4(user.getUsername()),
appending "<br/>Passwords are not shown.") the same.
- Around line 94-101: In deny(Exchange exc) replace the call that overwrites the
response headers with one that appends the WWW-Authenticate header so the
Content-Type (and other headers) set by security(...).buildAndSetResponse(exc)
are preserved; specifically, after calling
security(...).buildAndSetResponse(exc) use the response's existing Header and
call its add(...) (or equivalent) to append "WWW-Authenticate" with the Basic
realm value instead of using setHeader(...).
- Around line 83-92: The validUser(Exchange exc) method currently only catches
NoSuchElementException from userDataProvider.verify(), but
BasicAuthenticationUtil.getCredentials(...) can throw IllegalArgumentException
for parsing errors causing 500s; update validUser to also catch
IllegalArgumentException (or catch Exception types covering parsing failures)
around the call to BasicAuthenticationUtil.getCredentials and treat those as
authentication failures (return false) so malformed/absent Authorization headers
produce 401s; keep the existing behavior for NoSuchElementException from
userDataProvider.verify() and ensure exc.setProperty(SECURITY_SCHEMES, ...) is
only executed when credentials parsing succeeds.
...ava/com/predic8/membrane/core/interceptor/authentication/BasicAuthenticationInterceptor.java
Show resolved
Hide resolved
...ava/com/predic8/membrane/core/interceptor/authentication/BasicAuthenticationInterceptor.java
Outdated
Show resolved
Hide resolved
...ava/com/predic8/membrane/core/interceptor/authentication/BasicAuthenticationInterceptor.java
Show resolved
Hide resolved
…r count display, and refine response headers for compliance
…r count display, and refine response headers for compliance
…to basic-auth-tutorial-refactor
…toString` and add assertion to verify it's not exposed
…to basic-auth-tutorial-refactor
…ility by using `var`, remove unnecessary password filtering, update test methods, and streamline imports.
| * @description The user's phone number | ||
| */ | ||
| @MCAttribute | ||
| public void setSms(String value) { |
There was a problem hiding this comment.
The sms attribute is still being used (SMSTokenProvider).
| * @description The user's shared TOTP secret (if used in combination with the {@link TOTPTokenProvider}). | ||
| */ | ||
| @MCAttribute | ||
| public void setSecret(String value) { |
There was a problem hiding this comment.
The secret attribute is still being used (TOTPTokenProvider).
| # Generate hashes with: | ||
| # openssl passwd -6 -salt k3jia "yourPassword" | ||
| # mkpasswd -m sha-512 -S k3jia "yourPassword" | ||
| password: "$6$k3jia$NlCL.JTXitlwZ4i67ELwWpqK4Co3a8xEsp5UBxlR2In7nJgKJFNywV4Bx6jB9Py5g69eNKyxveLIG3OMcyc8M1" |
There was a problem hiding this comment.
"If you see $6$ in a modern web database, it usually means the developers haven't updated their security stack in about a decade."
| @@ -1,44 +0,0 @@ | |||
| # yaml-language-server: $schema=https://www.membrane-api.io/v7.0.6.json | |||
2 participants
Summary by CodeRabbit
New Features
Improvements
Tests
Docs