Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Changed mqtt connection expired behavior #3529

Merged
merged 1 commit into from
Aug 16, 2023
Merged

Changed mqtt connection expired behavior #3529

merged 1 commit into from
Aug 16, 2023

Conversation

mattkaem
Copy link
Contributor

  • Changed the exception in case a mqtt connection is expired to an AuthorizationException to have the correct error code in the error message to the device, if it subscribes the error topic (500 -> 401).
  • Extended the list of terminal errors for mqtt and amqp messages with unauthorized error.

@mattkaem mattkaem requested a review from calohmn August 10, 2023 14:55
@mattkaem mattkaem requested a review from kaniyan as a code owner August 10, 2023 14:55
@mattkaem mattkaem self-assigned this Aug 10, 2023
@sonatype-lift
Copy link

sonatype-lift bot commented Aug 10, 2023

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

@sophokles73 sophokles73 added this to the 2.4.0 milestone Aug 14, 2023
sophokles73
sophokles73 reviewed Aug 14, 2023
View reviewed changes
Copy link
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

This PR seems to address a bug in the MQTT adapter, right? If so, could you please add a corresponding issue and refer to it in the commit message?

site/documentation/content/user-guide/mqtt-adapter.md Outdated
@@ -859,6 +859,7 @@ The error message's *code* field may contain the following HTTP status codes:
| Code | Description |
| :---- | :---------- |
| *400* | Bad Request, the request cannot be processed. A possible reason for this is an invalid *PUBLISH* topic. |
| *401* | Unauthorized, the device connection is not authorized (e.g. the connection expired). |
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't it the credentials (token) that expire?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you are right. I changed it.

...-base/src/main/java/org/eclipse/hono/adapter/mqtt/AbstractVertxBasedMqttProtocolAdapter.java Outdated
@@ -1318,7 +1319,7 @@ private boolean disconnectOnExpired() {

private Future<Void> checkExpiration(final MqttContext context) {
if (context.authenticatedDevice() != null && context.authenticatedDevice().expired()) {
return Future.failedFuture(new MqttConnectionException(MqttConnectReturnCode.CONNECTION_REFUSED_NOT_AUTHORIZED));
return Future.failedFuture(new AuthorizationException(context.tenant(), "Connection expired.", null));
Copy link
Contributor

Choose a reason for hiding this comment

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

Credentials/Token expired?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

adapter-base/src/main/java/org/eclipse/hono/adapter/AbstractProtocolAdapterBase.java Outdated
@@ -1062,6 +1062,7 @@ public static ConnectionAttemptOutcome getOutcome(final Throwable e) {
* <li>The adapter is disabled for the tenant that the client belongs to.</li>
* <li>The authenticated device or gateway is disabled or not registered.</li>
* <li>The tenant is disabled or does not exist.</li>
* <li>The authenticated device is not authorized anymore (e.g. the connection expired).</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

credentials/token expired?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

...-base/src/main/java/org/eclipse/hono/adapter/mqtt/AbstractVertxBasedMqttProtocolAdapter.java Outdated
@@ -1310,6 +1310,7 @@ private Future<Void> checkTopic(final MqttContext context) {

private boolean disconnectOnExpired() {
if (authenticatedDevice != null && authenticatedDevice.expired()) {
log.debug("Connection with device {} has expired - Disconnecting device.", authenticatedDevice.getDeviceId());
Copy link
Contributor

Choose a reason for hiding this comment

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

Credentials/Token has expired?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@mattkaem
[eclipse-hono#3530] Changed mqtt credentials expired behavior
f8ffa97 
* Changed the exception that is used in case a mqtt device tries to send a message with expired credentials to an AuthorizationException to have the correct error code in the error message to the device.
* Extended the list of terminal errors for mqtt and amqp messages with unauthorized error.

Signed-off-by: Matthias Kaemmer <[email protected]>
@sophokles73 sophokles73 linked an issue Aug 15, 2023 that may be closed by this pull request
MQTT device with expired credentials does not get disconnected on message send if error topic is subscribed #3530
Closed
@mattkaem mattkaem force-pushed the mqtt-connection-expired-behavior branch from 5f71a1d to f8ffa97 Compare August 15, 2023 07:19
sophokles73
sophokles73 approved these changes Aug 15, 2023
View reviewed changes
Copy link
Contributor

@sophokles73 sophokles73 left a comment

Choose a reason for hiding this comment

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

LGTM

Can you please also merge this fix to the 2.3.x branch? I have added the issue to the 2.3.2 board already ...

@mattkaem mattkaem merged commit b9c4f0c into eclipse-hono:master Aug 16, 2023
5 checks passed
@mattkaem mattkaem deleted the mqtt-connection-expired-behavior branch August 16, 2023 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sophokles73 sophokles73 sophokles73 approved these changes

@calohmn calohmn Awaiting requested review from calohmn

@kaniyan kaniyan Awaiting requested review from kaniyan kaniyan is a code owner

@dejanb dejanb Awaiting requested review from dejanb dejanb is a code owner

Assignees

@mattkaem mattkaem

Labels
MQTT Adapter
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.

MQTT device with expired credentials does not get disconnected on message send if error topic is subscribed
2 participants
@mattkaem @sophokles73