Bugfix: Client-id is set to late on enhanced authentication flow

[mths1]

Bugfix: Client-id is set to late on enhanced authentication flow on_auth_m5). It expects an atom (e.g. success) and not a number.

Proposed Changes

Fixes #1780 : The documentation states that reason_code shall be a number (e.g.), but it actually expects an atom (e.g. success instead of 0)

Fixes #2026: This is more tricky. The client_id is only retrieved and checked (or calculated) after the on_auth_m5, within check_connect / check_client_id. This breaks check_connect/check_client_id apart and moves the client_id check before the on_auth_m5. That way the client_id will be available to the hook. This, of course, changes the behaviour a little bit.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

• Bugfix (non-breaking change which fixes issue #XXXX)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation (correction or otherwise)
• Cosmetics (whitespace, styles...)
• DevOps (Build scripts, pipelines...)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

• I have read the CODE_OF_CONDUCT.md document
• All tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if needed)
• Any dependent changes have been merged and published in related repositories
• I have updated changelog (At the bottom of the release version)
• I have squashed all my commits into one before merging

Further Comments

If this is a relatively large or complex change, kick off the discussion by
explaining why you chose the solution you did and what alternatives you
considered, etc.

[ioolkos]

@mths1 ouh, nice! I almost forgot about that one.

This, of course, changes the behaviour a little bit.

In what way? I don't immediately see a behaviour change in the FSM, except the reshuffling and the 'invalid' user thing?

[ioolkos]

This weirdly breaks the vmq_webhooks_SUITE, so there is actually a behaviour change of some sort.

[mths1]

This was triggered because reason_code is now "success" and not 0. I am not a big fan of magic numbers, so I thought we want to continue with success instead of 0 (which is a breaking change). Nontheless, I have the feeling that this code path is largely unused.

Currently, for backwards-compatability I still call on_auth_m5 even if the client_id is invalid. I'd propose to not call on_auth_m5 and just fail in such cases (which would also be a breaking change)

Furthermore, I suspect that there are more problems waiting to be discovered in that on_auth_m5 flow.

[ioolkos]

@mths1 yes, on_auth_m5 is largely unused for the simple reason that it enables users to implement their own enhanced auth flows. Which is something users don't often need or don't want to do. It is still an important feature, though, so I'm glad to battletest this more! And even if it's only for potential internal plugins.
We can certainly go for explicit response codes ('success'). Not yet sure about invalid client_ids, I guess it depends what the enduser wants to do in the hook implementation (let's say log that event).
No worries about breaking changes here, in any case.

[mths1]

It seems to be such an obscure feature that I did not find an ready-to-use example somewhere on the web to test against... mosquitto-pub can send authentication method and data, but cannot simulate the whole flow...