Conversation
The SQL authentication provider discards every column of the authentication query except the password, forcing a second query to load user data such as its id right after authentication. Add a SqlAuthentication.create overload taking a mapping function that receives the authenticated row and returns a JSON object to merge into the user attributes. The password remains expected in the first column and options stay data only, following the guidance in the issue. Fixes eclipse-vertx#694
489f880 to
7ee6b4c
Compare
tsegismont
left a comment
There was a problem hiding this comment.
Thank you @jnbdz
In addition to the comments inline, I think we should put the additional information into the user principal, not the attributes.
In vertx-auth, principal() holds identity data (who the user is) while attributes() holds
authentication metadata (decoded tokens, expiration timestamps, claims).
Every credential-based auth provider in the project (LDAP, htpasswd, htdigest, properties, OTP, WebAuthn, and SQL itself) puts all user data into principal() and leaves attributes() empty.
Only JWT and OAuth2 use attributes(), and exclusively for decoded token structures and timing metadata (accessToken, idToken, exp, iat, nbf, rootClaim, etc.).
Database columns like email or display_name describe the user's identity, they belong in principal().
Merging them into attributes() is inconsistent with the rest of the codebase
and risks colliding with framework-managed keys that control User.expired(), User.subject(), User.get(), and authorization provider lookups.
| * @param attributeMapper maps the authenticated row to extra user attributes, may return {@code null} | ||
| * @return the auth provider | ||
| */ | ||
| static SqlAuthentication create(SqlClient client, SqlAuthenticationOptions options, Function<Row, JsonObject> attributeMapper) { |
There was a problem hiding this comment.
Please add @GenIgnore(GenIgnore.PERMITTED_TYPE)
| } | ||
|
|
||
| /** | ||
| * Create a JDBC auth provider implementation that enriches the authenticated user with |
There was a problem hiding this comment.
| * Create a JDBC auth provider implementation that enriches the authenticated user with | |
| * Create a SQL auth provider implementation that enriches the authenticated user with |
Could you please also fix the other mentions of JDBC?
| * The authentication query is expected to return the password in the first column, any other | ||
| * column is available to the given {@code attributeMapper}. The JSON object returned by the |
There was a problem hiding this comment.
Can you please reword this part to make it clear that all columns returned by the authentication query are present? Including the hashed password in the first column. And that the user provided function should handle this information with care
| // metadata "amr" | ||
| user.principal().put("amr", Collections.singletonList("pwd")); | ||
| if (attributeMapper != null) { | ||
| JsonObject attributes = attributeMapper.apply(row); |
There was a problem hiding this comment.
The mapper call should be wrapped with try/catch, log failures and return a failed future Failure in authentication
The SQL authentication provider discards every column of the authentication query except the password (
row.getString(0)), so callers that need more user data right after login — typically the user id for a session or token — have to run a second query for the row they just fetched.Following the guidance in #694, this adds a
SqlAuthentication.create(client, options, attributeMapper)overload where the mapper is aFunction<Row, JsonObject>living on the auth type (options stay data-only). The mapper receives the authenticated row and the returned JSON object is merged into the authenticatedUserattributes:Behavior notes:
createoverloads are unchangednullmapper result is ignoredIncludes a test in
MySQLTest(the DDL gains a nullableemailcolumn), an example and documentation.Fixes #694