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

Add auth for mongoose_domain_handler #3160

Merged
merged 5 commits into from
Jun 23, 2021
Merged

Add auth for mongoose_domain_handler #3160

merged 5 commits into from
Jun 23, 2021

Conversation

arcusfelis
Copy link
Contributor

This PR addresses #

Proposed changes include:

  • auth
  • tests

@codecov
Copy link

codecov bot commented Jun 21, 2021

Codecov Report

Merging #3160 (f45ced8) into master (0d82dc9) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3160      +/-   ##
==========================================
- Coverage   79.89%   79.88%   -0.01%     
==========================================
  Files         396      396              
  Lines       32335    32356      +21     
==========================================
+ Hits        25834    25849      +15     
- Misses       6501     6507       +6     
Impacted Files Coverage Δ
src/config/mongoose_config_spec.erl 98.52% <100.00%> (+0.03%) ⬆️
src/domain/mongoose_domain_handler.erl 98.71% <100.00%> (+0.28%) ⬆️
src/mam/mod_mam_rdbms_async_pool_writer.erl 62.96% <0.00%> (-4.63%) ⬇️
src/mongoose_tcp_listener.erl 76.59% <0.00%> (-4.26%) ⬇️
...bal_distrib/mod_global_distrib_hosts_refresher.erl 73.58% <0.00%> (-3.78%) ⬇️
src/cassandra/mongoose_cassandra_worker.erl 70.75% <0.00%> (-2.36%) ⬇️
src/ejabberd_s2s_out.erl 60.82% <0.00%> (-1.14%) ⬇️
src/muc_light/mod_muc_light_utils.erl 90.00% <0.00%> (-1.00%) ⬇️
...c/global_distrib/mod_global_distrib_server_mgr.erl 76.27% <0.00%> (-0.57%) ⬇️
src/ejabberd_c2s.erl 89.21% <0.00%> (-0.08%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d82dc9...f45ced8. Read the comment docs.

Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

The code looks good in general. I have some minor comments.

src/domain/mongoose_domain_handler.erl Show resolved Hide resolved
Comment on lines 65 to 66
username = "admin"
password = "secret"
Copy link
Member

@chrzaszcz chrzaszcz Jun 21, 2021

Choose a reason for hiding this comment

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

I am not sure if it's good to set up such defaults in the config file... they aren't more secure than having no auth. Maybe we could leave out these two options? This way the user would have to consciously set something here. This would be also more in line with the docs, which document these two fields as empty by default. I know that this is not equal to them being empty in the default config file... but in this case I think they can be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we have a way to dynamically start cowboy handlers?

@chrzaszcz
Copy link
Member

One more thing, it would be good to add a test case to config_parser_SUITE similar to listen_http_handlers_api, called e.g. listen_http_handlers_domain.

Use start_listener in the service_domain_db_SUITE
Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

Looks good! I have two minor remarks.

big_tests/tests/service_domain_db_SUITE.erl Outdated Show resolved Hide resolved
big_tests/tests/service_domain_db_SUITE.erl Outdated Show resolved Hide resolved
Copy link
Member

@chrzaszcz chrzaszcz left a comment

Choose a reason for hiding this comment

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

Great!

@chrzaszcz chrzaszcz merged commit 449fbb5 into master Jun 23, 2021
@chrzaszcz chrzaszcz deleted the mu-domain-rest-auth branch June 23, 2021 11:37
@Premwoik Premwoik added this to the 5.0.0 milestone Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants