Move logic to reject certain endpoints when using OBO from authenticator to endpoint validator

[cwperks]

Description

This PR removes authorization logic from within OnBehalfOfAuthenticator. Currently, this authenticator has logic to reject a request for forbidden endpoints (i.e. obo token cannot be used to change a user's password or request new obo tokens).

While the logic in the authenticator works, it necessarily puts authorization logic where authentication takes place. The changes in this PR move that decision to after authentication is performed and mark it directly on the rest handler.

• Category

Refactoring

Issues Resolved

Related to discussion here: #5443 (comment)

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 7f671f2)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Remove OBO restriction logic from authenticator and add authenticatedBy tracking Relevant files: src/main/java/org/opensearch/security/user/User.java src/main/java/org/opensearch/security/auth/BackendRegistry.java src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java src/main/java/org/opensearch/security/securityconf/DynamicConfigModelV7.java src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java src/main/java/org/opensearch/security/util/AuthTokenUtils.java src/test/java/org/opensearch/security/authtoken/jwt/AuthTokenUtilsTest.java src/test/java/org/opensearch/security/http/OnBehalfOfAuthenticatorTest.java Sub-PR theme: Add OBO token restriction enforcement at endpoint level Relevant files: src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java src/main/java/org/opensearch/security/dlic/rest/api/AccountApiAction.java src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java

⚡ Recommended focus areas for review Mutable State The authenticatedBy field is mutable (has a setter) on what is described as an immutable User object. This could allow the authenticator type to be changed after authentication, potentially bypassing the OBO restriction checks in AccountApiAction and CreateOnBehalfOfTokenAction. public void setAuthenticatedBy(String authenticatedBy) { this.authenticatedBy = authenticatedBy; } Ordering Issue setAuthenticatedBy is called before the admin check. If the admin check fails and authentication is rejected, the user object still has authenticatedBy set. More importantly, verify that setAuthenticatedBy is called for all authentication paths (e.g., client cert auth, injected users) so the field is always populated when needed. authenticatedUser.setAuthenticatedBy(authDomain.getHttpAuthenticator().getType()); if (adminDns.isAdmin(authenticatedUser)) { Magic String The string "onbehalfof_jwt" is hardcoded in both AccountApiAction and CreateOnBehalfOfTokenAction. If the authenticator type string changes or a new token-based authenticator is added, these checks will silently fail. Consider extracting this to a shared constant. if ("onbehalfof_jwt".equals(user.getAuthenticatedBy())) { Magic String The string "onbehalfof_jwt" in isTokenAuthenticatedUser is duplicated from AccountApiAction. This should be a shared constant to ensure consistency and ease of maintenance. String authBy = user.getAuthenticatedBy(); return "onbehalfof_jwt".equals(authBy);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 7f671f2
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Extract duplicated magic string into a shared constant The magic string "onbehalfof_jwt" is duplicated in both CreateOnBehalfOfTokenAction and AccountApiAction. This creates a maintenance risk where changing the authenticator type string in one place may not be reflected in the other. Consider defining this as a shared constant. src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java [177-180] +public static final String OBO_AUTH_TYPE = "onbehalfof_jwt"; + private static boolean isTokenAuthenticatedUser(User user) { String authBy = user.getAuthenticatedBy(); - return "onbehalfof_jwt".equals(authBy); + return OBO_AUTH_TYPE.equals(authBy); } Suggestion importance[1-10]: 6 __ Why: The magic string "onbehalfof_jwt" is duplicated in both CreateOnBehalfOfTokenAction and AccountApiAction, creating a maintenance risk. Extracting it to a shared constant would improve maintainability and reduce the risk of inconsistency.	Low
	Reference shared constant instead of duplicating magic string The string "onbehalfof_jwt" is hardcoded here and also in CreateOnBehalfOfTokenAction. These should reference the same constant to avoid inconsistency if the authenticator type name ever changes. src/main/java/org/opensearch/security/dlic/rest/api/AccountApiAction.java [116-121] -if ("onbehalfof_jwt".equals(user.getAuthenticatedBy())) { +if (CreateOnBehalfOfTokenAction.OBO_AUTH_TYPE.equals(user.getAuthenticatedBy())) { return ValidationResult.error( RestStatus.FORBIDDEN, forbiddenMessage("Token-authenticated users cannot change passwords") ); } Suggestion importance[1-10]: 5 __ Why: This suggestion is valid and complements suggestion 2 - both files use the hardcoded string "onbehalfof_jwt" and should reference a shared constant. However, the improved_code creates a cross-module dependency that may not be ideal architecturally.	Low
Possible issue	Set authenticator type only after successful authentication The setAuthenticatedBy call is placed before the admin check, which means even users who fail authentication (admin users rejected via HTTP) will have their authenticatedBy field set. This field should only be set after the user has been successfully authenticated. Move the setAuthenticatedBy call to after all validation checks pass, or at least after the admin check. src/main/java/org/opensearch/security/auth/BackendRegistry.java [455-457] +if (adminDns.isAdmin(authenticatedUser)) { + log.error("Cannot authenticate user because admin user is not permitted to login via HTTP"); + auditLog.logFailedLogin(authenticatedUser.getName(), true, null, request); + request.queueForSending( + // ... rest of admin rejection logic +} authenticatedUser.setAuthenticatedBy(authDomain.getHttpAuthenticator().getType()); -if (adminDns.isAdmin(authenticatedUser)) { - Suggestion importance[1-10]: 5 __ Why: The setAuthenticatedBy call is placed before the admin check, meaning admin users who fail authentication will still have their authenticatedBy field set. While this is a transient field and the user object won't be used after rejection, moving it after validation would be cleaner. However, the practical impact is low since the user object is discarded on failure.	Low

---

Previous suggestions

Suggestions up to commit b1982e8

Category	Suggestion	Impact
General	Extract duplicated magic string to a shared constant The string "onbehalfof_jwt" is hardcoded in both AccountApiAction.java and CreateOnBehalfOfTokenAction.java. This creates a maintenance risk where the two checks could diverge. Extract this constant to a shared location (e.g., a constant in OnBehalfOfAuthenticator or a dedicated constants class) and reference it in both places. src/main/java/org/opensearch/security/dlic/rest/api/AccountApiAction.java [116] -if ("onbehalfof_jwt".equals(user.getAuthenticatedBy())) { +if (OnBehalfOfAuthenticator.OBO_AUTH_TYPE.equals(user.getAuthenticatedBy())) { Suggestion importance[1-10]: 6 __ Why: The hardcoded string "onbehalfof_jwt" appears in both AccountApiAction.java and CreateOnBehalfOfTokenAction.java, creating a maintenance risk. Extracting it to a shared constant is a valid improvement, though it references OnBehalfOfAuthenticator.OBO_AUTH_TYPE which may not exist yet in the codebase.	Low
	Clarify or broaden token type restriction check The isTokenAuthenticatedUser method only checks for "onbehalfof_jwt", but the comment in User.java mentions other token types like "apitoken". If other token-based authenticators should also be restricted, this check is incomplete. Consider broadening the check or documenting explicitly that only OBO tokens are restricted here. src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java [177-180] private static boolean isTokenAuthenticatedUser(User user) { String authBy = user.getAuthenticatedBy(); - return "onbehalfof_jwt".equals(authBy); + // Only OBO JWT tokens are restricted; extend this list if other token types should also be blocked + return OnBehalfOfAuthenticator.OBO_AUTH_TYPE.equals(authBy); } Suggestion importance[1-10]: 2 __ Why: The suggestion is primarily about adding a comment and referencing a constant that doesn't exist yet. The improved_code doesn't actually broaden the check and the score is low since it's essentially a documentation/comment suggestion combined with a refactoring that depends on an undefined constant.	Low
Possible issue	Avoid setting auth type before authentication completes The setAuthenticatedBy call is placed before the admin check, but it should also be set for successfully authenticated non-admin users. More critically, if authentication continues through multiple domains (e.g., continue is called later), the authenticatedBy value may be overwritten or set prematurely for a user that hasn't fully passed authentication yet. Ensure setAuthenticatedBy is only called after the user is confirmed as successfully authenticated. src/main/java/org/opensearch/security/auth/BackendRegistry.java [455-457] -authenticatedUser.setAuthenticatedBy(authDomain.getHttpAuthenticator().getType()); - if (adminDns.isAdmin(authenticatedUser)) { Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about setAuthenticatedBy being called before the admin check and potentially before full authentication is confirmed. However, the improved_code simply removes the setAuthenticatedBy call entirely without showing where it should be placed instead, making the suggestion incomplete.	Low

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 7f671f2.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java	152	medium	Removal of the centralized `isRequestAllowed` path-based guard that blocked OBO tokens from restricted endpoints at the authenticator level. The replacement shifts enforcement to individual action handlers (`CreateOnBehalfOfTokenAction`, `AccountApiAction`). Any restricted endpoint whose handler was not updated with a corresponding `authenticatedBy` check now silently accepts OBO tokens. The deleted `AuthTokenUtils` covered three endpoint patterns (obo/token, generateonbehalfoftoken, account); only two action-level checks appear in this diff, leaving the deprecated `generateonbehalfoftoken` endpoint's handler coverage unverifiable from this diff alone.
src/main/java/org/opensearch/security/user/User.java	106	medium	The new `authenticatedBy` field is declared `transient`, meaning it is never persisted or serialized. The entire OBO restriction mechanism introduced in this PR silently fails open if the field is not populated (null check returns false in both `isTokenAuthenticatedUser` and the AccountApiAction inline check). This creates a dependency on `BackendRegistry.setAuthenticatedBy` being called on every code path that authenticates OBO users; any path that bypasses BackendRegistry (e.g., internal thread-context injection, serialization/deserialization of the User object) would produce a user with a null `authenticatedBy`, allowing OBO tokens to perform restricted operations without any error or audit log entry.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 2 | Low: 0

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

Persistent review updated to latest commit 7f671f2

[codecov]

Codecov Report

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.81%. Comparing base (8208a94) to head (7f671f2).

Files with missing lines	Patch %	Lines
...y/action/onbehalf/CreateOnBehalfOfTokenAction.java	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6132      +/-   ##
==========================================
- Coverage   74.85%   74.81%   -0.04%     
==========================================
  Files         447      446       -1     
  Lines       28480    28476       -4     
  Branches     4331     4327       -4     
==========================================
- Hits        21318    21305      -13     
- Misses       5166     5178      +12     
+ Partials     1996     1993       -3     

Files with missing lines	Coverage Δ
.../org/opensearch/security/auth/BackendRegistry.java	80.05% <100.00%> (+0.05%)	⬆️
...g/opensearch/security/authtoken/jwt/JwtVendor.java	82.75% <100.00%> (ø)
...earch/security/dlic/rest/api/AccountApiAction.java	98.75% <100.00%> (+0.06%)	⬆️
...nsearch/security/http/OnBehalfOfAuthenticator.java	88.23% <ø> (-1.06%)	⬇️
...ch/security/securityconf/DynamicConfigModelV7.java	68.60% <100.00%> (ø)
...c/main/java/org/opensearch/security/user/User.java	83.00% <100.00%> (+0.52%)	⬆️
...y/action/onbehalf/CreateOnBehalfOfTokenAction.java	80.00% <83.33%> (+0.31%)	⬆️

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.