[Auth jass] Additional login option with API Token ID - #3815
Conversation
Refer to: openhab#3813 This commit comprises a few changes: 1) Login screen - password field can be emptied but username must be generated API token 2) Some housekeeping to Jaas code to remove unnecessary codes. Signed-off-by: Felix Lo <lsafelix75@gmail.com>
|
@davidgraeff pls take a look. thanks |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/x-openhab-token-use-for-mainui-authentication/149548/4 |
|
@J-N-K appreciate if you can take a look. Tq |
That's exactly the case. Sorry for the noise. |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
There was a problem hiding this comment.
Pull Request Overview
This PR adds the ability to authenticate using API tokens as an alternative to username/password authentication in openHAB's JAAS authentication system. The implementation allows users to enter API tokens in the username field while leaving the password field empty.
- Modifies login flow to support API token authentication when password field is empty
- Implements new JAAS login modules specifically for API token handling
- Updates HTML form to make password field optional
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| AbstractAuthPageServlet.java | Adds conditional authentication logic to handle API tokens when password is empty |
| authorize.html | Removes required attribute from password field to support API token login |
| ManagedUserLoginModule.java | Updates login module to properly set user principals with roles |
| JaasAuthenticationProvider.java | Implements dual authentication paths for username/password and API token credentials |
| ApiTokenLoginModule.java | New login module dedicated to API token authentication |
| ApiTokenLoginConfiguration.java | Configuration class for API token login module |
| UsernamePasswordCredentials credentials = new UsernamePasswordCredentials(username, password); | ||
| Authentication auth = authProvider.authenticate(credentials); | ||
| Authentication auth; | ||
| if (!password.isEmpty()) { |
There was a problem hiding this comment.
The authentication logic relies solely on an empty password to determine API token authentication. This could allow unintended authentication attempts if a user accidentally submits with an empty password. Consider adding explicit validation for API token format (e.g., checking for 'oh.' prefix) to ensure the username field actually contains a valid API token.
| new ApiTokenLoginConfiguration()); | ||
| loginContext.login(); | ||
|
|
||
| return getAuthentication("", loginContext.getSubject()); |
There was a problem hiding this comment.
Passing an empty string as the username parameter to getAuthentication() for API token authentication could lead to incorrect authentication results. The method expects a valid username, and while it handles blank usernames by extracting from principals, this creates unnecessary complexity and potential for errors.
| return getAuthentication("", loginContext.getSubject()); | |
| String username = extractUsernameFromSubject(loginContext.getSubject()); | |
| return getAuthentication(username, loginContext.getSubject()); |
| GenericUser user = (GenericUser) principals.iterator().next(); | ||
| String[] roles = new String[user.getRoles().size()]; | ||
|
|
There was a problem hiding this comment.
This code performs an unsafe cast without checking if the principal is actually a GenericUser instance. If the principals set contains other types of Principal objects, this will throw a ClassCastException at runtime.
| GenericUser user = (GenericUser) principals.iterator().next(); | |
| String[] roles = new String[user.getRoles().size()]; | |
| GenericUser user = null; | |
| for (Principal principal : principals) { | |
| if (principal instanceof GenericUser) { | |
| user = (GenericUser) principal; | |
| break; | |
| } | |
| } | |
| if (user == null) { | |
| throw new CredentialException("No GenericUser found in principals"); | |
| } | |
| String[] roles = new String[user.getRoles().size()]; |
| private CallbackHandler callbackHandler; | ||
|
|
||
| @Override | ||
| public void initialize(Subject subject, CallbackHandler callbackHandler, Map<String, ?> sharedState, | ||
| Map<String, ?> options) { | ||
| this.subject = subject; | ||
| this.callbackHandler = callbackHandler; |
There was a problem hiding this comment.
The callbackHandler field is assigned in the initialize method but never used throughout the class. This unused field should be removed to improve code clarity.
| private CallbackHandler callbackHandler; | |
| @Override | |
| public void initialize(Subject subject, CallbackHandler callbackHandler, Map<String, ?> sharedState, | |
| Map<String, ?> options) { | |
| this.subject = subject; | |
| this.callbackHandler = callbackHandler; | |
| @Override | |
| public void initialize(Subject subject, CallbackHandler callbackHandler, Map<String, ?> sharedState, | |
| Map<String, ?> options) { | |
| this.subject = subject; |
|
|
||
| @Override | ||
| public boolean abort() throws LoginException { | ||
| return false; |
There was a problem hiding this comment.
The abort() method should clean up any state that was set during the login process. Since this module adds principals to the subject during login(), the abort() method should remove them if login was not committed.
| return false; | |
| if (this.subject != null) { | |
| this.subject.getPrincipals().removeIf(principal -> principal instanceof GenericUser); | |
| } | |
| return true; |
|
|
||
| @Override | ||
| public boolean logout() throws LoginException { | ||
| return false; |
There was a problem hiding this comment.
The logout() method should remove the principals that were added during login from the subject. Currently returning false means the logout was not successful, but no cleanup is performed.
| return false; | |
| try { | |
| // Remove all principals added by this LoginModule | |
| this.subject.getPrincipals().removeIf(principal -> principal instanceof GenericUser); | |
| return true; | |
| } catch (Exception e) { | |
| logger.error("Error during logout", e); | |
| throw new LoginException("Logout failed: " + e.getMessage()); | |
| } |
|
Hi @lsafelix75, Do you use jaas and how? If you use it we probably do not want to remove support for it (#5753). Perhaps you can also update this PR if you want to continue with it. |
Refer to: #3813 This commit comprises a few changes:
Signed-off-by: Felix Lo lsafelix75@gmail.com