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.

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

feat: Added AUTH packet support for enhanced authentication. #852

Open
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

ting-ms
Copy link

@ting-ms ting-ms commented Apr 28, 2024

Type of change

New feature (non-breaking change which adds functionality)

Checklist:

  • Formatted with cargo fmt
  • Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.

Resolves #840

@ting-ms ting-ms marked this pull request as ready for review May 6, 2024 02:59
options.keep_alive = Duration::from_secs(keep_alive as u64);
}
network.set_max_outgoing_size(props.max_packet_size);
loop {
Copy link
Member

Choose a reason for hiding this comment

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

There are two ways to do this, one as has been already implemented in this PR, the other would involve improving the visibility into the whole process, updating the event log and returning events as they occur.
Example log of events

  1. Outgoing Connect packet
  2. Incoming/Outgoing Auth packets for as long as the challenge/response cycle goes on
  3. Incoming Connack packet

This would require a refactor of how the code is currently structured, let me delve deeper into this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any update on involve improving the visibility into the whole process, updating the event log and returning events as they occur?

Copy link
Member

@de-sh de-sh left a comment

Choose a reason for hiding this comment

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

Would be wonderful if you could provide us with guidance on setting up a test with a broker that supports auth packets, thank you!

@ting-ms
Copy link
Author

ting-ms commented May 30, 2024

Would be wonderful if you could provide us with guidance on setting up a test with a broker that supports auth packets, thank you!

Please reference the following docs for the testing guidance:
RumqttcAuthTestWithEMQX.pdf
RumqttcAuthTestWithEventGrid.pdf

@ting-ms
Copy link
Author

ting-ms commented May 30, 2024

Attached updated design doc.
RumqttcEnhancedAuhtenticationInterfaceDesign.pdf

@coveralls
Copy link

coveralls commented Jun 17, 2024

Pull Request Test Coverage Report for Build 9540617300

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 79 of 161 (49.07%) changed or added relevant lines in 6 files are covered.
  • 3 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.9%) to 37.026%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rumqttc/src/v5/mqttbytes/v5/mod.rs 0 3 0.0%
rumqttc/src/v5/mod.rs 4 9 44.44%
rumqttc/src/v5/mqttbytes/v5/auth.rs 32 39 82.05%
rumqttc/src/v5/client.rs 27 39 69.23%
rumqttc/src/v5/eventloop.rs 8 31 25.81%
rumqttc/src/v5/state.rs 8 40 20.0%
Files with Coverage Reduction New Missed Lines %
rumqttc/src/v5/state.rs 1 64.42%
rumqttc/src/v5/mqttbytes/v5/mod.rs 1 44.44%
rumqttc/src/v5/mqttbytes/v5/auth.rs 1 60.0%
Totals Coverage Status
Change from base Build 9290350918: 0.9%
Covered Lines: 6166
Relevant Lines: 16653

💛 - Coveralls

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.

rumqttc - AUTH packet support
4 participants