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

Improve emqx_hooks and credentials #2309

Merged
merged 9 commits into from Mar 16, 2019

Conversation

@terry-xiaoyu
Copy link
Contributor

terry-xiaoyu commented Mar 13, 2019

  1. Modify the return modes of emqx hooks.

Change the return value of hook functions to:

  • stop: stop the hook chain and return ok
  • _Any: continue the hook chain

The return value of emqx_hooks:run/2 is changed to:

  • ok

And the return value of emqx_hooks:run/3:

  • Acc
  1. Treat password as a member of credentials.

Password should be wrapped in the credentials data-structure, as the
username/password pair together consists of an authentication method.
There can be some methods using some other credential data (e.g.
a JWT token), and these credential data should also be wrapped in the
the credentials data-structure.

An event client.authenticate is triggered when an user logs in:

emqx_hooks:run_fold('client.authenticate', [], Credentials)

A default callback that deny/allow any user (according to the
allow_anonymous config) should be appended to the end of the
callback chain.

The credentails is passed through all of the callbacks, and
can be changed over in this process.

@terry-xiaoyu terry-xiaoyu marked this pull request as ready for review Mar 15, 2019
1. Modify the return modes of emqx hooks.

Change the return value of hook functions to:
- ok: stop the hook chain and return ok
- {error, Reason}: stop the hook chain and return error
- continue: continue the hook chain

And the return value of emqx_hooks:run/2 is changed to:
- ok
- {error, Reason}

And the return value of emqx_hooks:run/3:
- {ok, Acc}
- {error, Reason, Acc}

2. Treat password as a member of credentials.

Password should be wrapped in the `credentials` data-structure, as the
username/password pair together consists of an authentication method.
There can be some methods using some other credential data (e.g.
a JWT token), and these credential data should also be wrapped in the
the `credentials` data-structure.

An event `client.authenticate` is triggered when an user logs in:
```erlang
emqx_hooks:run('client.authenticate', [], Credentials)
```

A `default callback` that deny/allow any user (according to the
`allow_anonymous` config) should be appended to the end of the
callback chain.

The `credentails` is passed through all of the callbacks, and
can be changed over in this process.
@gilbertwong96 gilbertwong96 force-pushed the auth_using_hooks branch from d614b55 to dc10a16 Mar 15, 2019
etc/emqx.conf Outdated Show resolved Hide resolved
etc/emqx.conf Outdated Show resolved Hide resolved
priv/emqx.schema Outdated Show resolved Hide resolved
src/emqx_access_control.erl Show resolved Hide resolved
src/emqx_hooks.erl Show resolved Hide resolved
src/emqx_protocol.erl Show resolved Hide resolved
src/emqx_protocol.erl Outdated Show resolved Hide resolved
src/emqx_protocol.erl Outdated Show resolved Hide resolved
src/emqx_session.erl Outdated Show resolved Hide resolved
test/emqx_ct_broker_helpers.erl Outdated Show resolved Hide resolved
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 15, 2019

Pull Request Test Coverage Report for Build 5036

  • 75 of 110 (68.18%) changed or added relevant lines in 11 files are covered.
  • 86 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-1.6%) to 68.32%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/emqx_broker.erl 3 5 60.0%
src/emqx_hooks.erl 7 9 77.78%
src/emqx_access_control.erl 14 17 82.35%
src/emqx_psk.erl 0 4 0.0%
src/emqx_session.erl 14 18 77.78%
src/emqx_mod_acl_internal.erl 11 17 64.71%
src/emqx_protocol.erl 22 28 78.57%
src/emqx_reason_codes.erl 0 8 0.0%
Files with Coverage Reduction New Missed Lines %
src/emqx_sup.erl 1 58.82%
src/emqx_broker.erl 1 64.15%
src/emqx_pqueue.erl 1 76.24%
src/emqx_protocol.erl 2 78.35%
src/emqx_session.erl 3 69.28%
src/emqx_client.erl 7 49.62%
src/emqx_access_rule.erl 20 46.15%
src/emqx_acl_cache.erl 51 39.08%
Totals Coverage Status
Change from base Build 5008: -1.6%
Covered Lines: 3360
Relevant Lines: 4918

💛 - Coveralls
@terry-xiaoyu terry-xiaoyu force-pushed the auth_using_hooks branch from 94c1ed3 to d57a57f Mar 16, 2019
@terry-xiaoyu terry-xiaoyu merged commit 02fe856 into develop Mar 16, 2019
2 of 3 checks passed
2 of 3 checks passed
coverage/coveralls Coverage decreased (-0.07%) to 68.402%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@terry-xiaoyu terry-xiaoyu deleted the auth_using_hooks branch Mar 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.