Skip to content

MINOR: Improve PlainSaslServer error message for empty tokens#6249

Merged
ijuma merged 3 commits intoapache:trunkfrom
ijuma:improve-plain-sasl-server-errors
Feb 12, 2019
Merged

MINOR: Improve PlainSaslServer error message for empty tokens#6249
ijuma merged 3 commits intoapache:trunkfrom
ijuma:improve-plain-sasl-server-errors

Conversation

@ijuma
Copy link
Copy Markdown
Member

@ijuma ijuma commented Feb 10, 2019

Empty username or password would result in the "expected 3 tokens"
error instead of "username not specified" or "password not specified".

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@ijuma ijuma requested a review from rajinisivaram February 10, 2019 09:34
@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Feb 10, 2019

I'm using assertThrows so this depends on #6248

@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Feb 10, 2019

cc @apovzner

Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram left a comment

Choose a reason for hiding this comment

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

@ijuma Thanks for the PR, looks good. We should probably change the exception from SaslException to SaslAuthenticationException for the missing username/password cases. I have opened a related PR to fix a regression: #6252.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we still need this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point. We can add code to split (and maybe rename it) to verify if two utf8nul are not present and return an error like this one. Would it be SaslException still?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this can also be SaslAuthenticationException since it is only returning error based on the information the client passed in, so it wont be leaking any server state.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sounds good.

@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Feb 11, 2019

Thanks for the review, I did wonder SaslException versus SaslAuthenticationException, but was not sure if it had been intentional. Will change.

Empty username or password would result in the "expected 3 tokens"
error instead of "username not specified" or "password not specified".
@ijuma ijuma force-pushed the improve-plain-sasl-server-errors branch from 1c566e2 to 65115c1 Compare February 12, 2019 14:40
Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram left a comment

Choose a reason for hiding this comment

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

@ijuma Thanks for the updates, LGTM

@ijuma
Copy link
Copy Markdown
Member Author

ijuma commented Feb 12, 2019

JDK 11 build passed, JDK 8 had an unrelated failure:

org.scalatest.junit.JUnitTestFailedError: Should have received an class org.apache.kafka.common.errors.GroupMaxSizeReachedException during the cluster roll
at org.scalatest.junit.AssertionsForJUnit$class.newAssertionFailedException(AssertionsForJUnit.scala:100)
at org.scalatest.junit.JUnitSuite.newAssertionFailedException(JUnitSuite.scala:71)
at org.scalatest.Assertions$class.fail(Assertions.scala:1089)
at org.scalatest.junit.JUnitSuite.fail(JUnitSuite.scala:71)
at kafka.api.ConsumerBounceTest.testRollingBrokerRestartsWithSmallerMaxGroupSizeConfigDisruptsBigGroup(ConsumerBounceTest.scala:376)

Merging to trunk.

@ijuma ijuma merged commit 6373551 into apache:trunk Feb 12, 2019
@ijuma ijuma deleted the improve-plain-sasl-server-errors branch February 12, 2019 19:58
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
…#6249)

Empty username or password would result in the "expected 3 tokens"
error instead of "username not specified" or "password not specified".

Reviewers: Rajini Sivaram <rajinisivaram@googlemail.com>
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