Bugfix: Client-id is set to late on enhanced authentication flow - #2123
Bugfix: Client-id is set to late on enhanced authentication flow#2123mths1 wants to merge 1 commit into
Conversation
|
@mths1 ouh, nice! I almost forgot about that one.
In what way? I don't immediately see a behaviour change in the FSM, except the reshuffling and the 'invalid' user thing? |
|
This weirdly breaks the |
|
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. |
…auth_m5). It expects an atom (e.g. success) and not a number.
3276d69 to
d8b5d95
Compare
|
@mths1 yes, |
|
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... |
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
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creatingthe 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.
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.