Skip to content

Fix: Enforce bot-type check on generateToken endpoint#27078

Merged
mohityadav766 merged 2 commits into1.12.5from
fix/generate-token-bot-guard-1.12.5
Apr 6, 2026
Merged

Fix: Enforce bot-type check on generateToken endpoint#27078
mohityadav766 merged 2 commits into1.12.5from
fix/generate-token-bot-guard-1.12.5

Conversation

@aji-aju
Copy link
Copy Markdown
Collaborator

@aji-aju aji-aju commented Apr 6, 2026

Summary

Context

Test plan

  • put_generateToken_bot_user_200_ok — existing test, still passes
  • put_generateToken_non_bot_user_400 — new test, verifies non-bot users are rejected
  • Manual testing via Swagger — confirmed 400 for regular user, 200 for bot user

🤖 Generated with Claude Code

The generateToken endpoint was intended for bot users only but the
guard was accidentally removed in #8617 (Nov 2022). This restores the
original check, rejecting non-bot users with a 400 error.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@aji-aju aji-aju self-assigned this Apr 6, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 6, 2026

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Apr 6, 2026

Code Review 👍 Approved with suggestions 0 resolved / 1 findings

Adds bot-type validation to the generateToken endpoint to prevent unauthorized token generation. The implementation is sound, but the test expects BAD_REQUEST while passing OK status to TestUtils—consider aligning the expected status code with the actual validation behavior.

💡 Quality: Test passes OK to put() but expects BAD_REQUEST from response

📄 openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/UserResourceTest.java:1517

The test passes OK (200) as the expected status to TestUtils.put(), but the endpoint actually returns 400. This works by accident: put() sees a non-success status, throws HttpResponseException, and assertResponseContains() catches it and checks for BAD_REQUEST. However, the OK parameter is misleading and doesn't match the actual expectation. Other error-path tests in this codebase typically pass the correct expected error status to avoid confusion.

Suggested fix
Replace `OK` with `BAD_REQUEST`:
            TestUtils.put(
                getResource(String.format("users/generateToken/%s", user.getId())),
                new GenerateTokenRequest().withJWTTokenExpiry(JWTTokenExpiry.Seven),
                BAD_REQUEST,
                ADMIN_AUTH_HEADERS),
🤖 Prompt for agents
Code Review: Adds bot-type validation to the generateToken endpoint to prevent unauthorized token generation. The implementation is sound, but the test expects BAD_REQUEST while passing OK status to TestUtils—consider aligning the expected status code with the actual validation behavior.

1. 💡 Quality: Test passes OK to put() but expects BAD_REQUEST from response
   Files: openmetadata-service/src/test/java/org/openmetadata/service/resources/teams/UserResourceTest.java:1517

   The test passes `OK` (200) as the expected status to `TestUtils.put()`, but the endpoint actually returns 400. This works by accident: `put()` sees a non-success status, throws `HttpResponseException`, and `assertResponseContains()` catches it and checks for `BAD_REQUEST`. However, the `OK` parameter is misleading and doesn't match the actual expectation. Other error-path tests in this codebase typically pass the correct expected error status to avoid confusion.

   Suggested fix:
   Replace `OK` with `BAD_REQUEST`:
               TestUtils.put(
                   getResource(String.format("users/generateToken/%s", user.getId())),
                   new GenerateTokenRequest().withJWTTokenExpiry(JWTTokenExpiry.Seven),
                   BAD_REQUEST,
                   ADMIN_AUTH_HEADERS),

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

TestUtils.put(
getResource(String.format("users/generateToken/%s", user.getId())),
new GenerateTokenRequest().withJWTTokenExpiry(JWTTokenExpiry.Seven),
OK,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Quality: Test passes OK to put() but expects BAD_REQUEST from response

The test passes OK (200) as the expected status to TestUtils.put(), but the endpoint actually returns 400. This works by accident: put() sees a non-success status, throws HttpResponseException, and assertResponseContains() catches it and checks for BAD_REQUEST. However, the OK parameter is misleading and doesn't match the actual expectation. Other error-path tests in this codebase typically pass the correct expected error status to avoid confusion.

Suggested fix:

Replace `OK` with `BAD_REQUEST`:
            TestUtils.put(
                getResource(String.format("users/generateToken/%s", user.getId())),
                new GenerateTokenRequest().withJWTTokenExpiry(JWTTokenExpiry.Seven),
                BAD_REQUEST,
                ADMIN_AUTH_HEADERS),

Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion

@mohityadav766 mohityadav766 merged commit 0b7425f into 1.12.5 Apr 6, 2026
15 of 42 checks passed
@mohityadav766 mohityadav766 deleted the fix/generate-token-bot-guard-1.12.5 branch April 6, 2026 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants