search

agsh / onvif

ONVIF node.js implementation
http://agsh.github.io/onvif/
MIT License
692 stars 234 forks source link

support https protocol #194

Closed ManjunathaN closed 3 years ago

ManjunathaN commented 3 years ago

Related PR - https://github.com/agsh/onvif/pull/194

RogerHardiman commented 3 years ago

This is great. many thanks. I am wondering if using 'isSecure' (which is more of a 'get' status function) is the right option name, and it would be better called 'useSecure' instead.

coveralls commented 3 years ago

Coverage Status

Coverage decreased (-0.1%) to 87.601% when pulling 7bc082e93e60ab30b72535d13ea9c4d14b646580 on ManjunathaN:master into 67976400e4ecb7e3336f0287d1d4b174171ab344 on agsh:master.

ManjunathaN commented 3 years ago

This is great. many thanks. I am wondering if using 'isSecure' (which is more of a 'get' status function) is the right option name, and it would be better called 'useSecure' instead.

Thanks. Updated the fieldname.

chriswiggins commented 3 years ago

Thanks for the PR! Can you also please include an option to pass secureOptions (options) to the HTTPS client? Most of these cameras will have self-signed certs that I believe node will reject

ManjunathaN commented 3 years ago

Thanks for the PR! Can you also please include an option to pass secureOptions (options) to the HTTPS client? Most of these cameras will have self-signed certs that I believe node will reject

Thanks for the comments. I've modified the code.

chriswiggins commented 3 years ago

Awesome @ManjunathaN ! How about an extra-for-experts and creating a https mock server that we can use to test connectivity? I don't think it needs to do the full test suite but just an initial connection would be good

RogerHardiman commented 3 years ago

Hi @ManjunathaN Thanks for the changes. I've merged them into master. I had one extra piece of review feedback, in addition to Chris's request to add https to Mock Server for automated testing. In the Environment Variables, have IS_SECURE. I asked you to change options.isSecure to options.useSecure which you did. Thank you. Should we change the Environment Variable to match as well? I know it is just a comment, but it would match up with the options name.

If you want some help on the Mock server, you can run npm run test

It starts an ONVIF Server on a local port an then gets the ONVIF Library to connect to it. It uses ONVIF Discovery so does work better on a network with no other ONVIF devices connected (as the discovery phase will detect other cameras on your LAN)

ManjunathaN commented 3 years ago

Awesome @ManjunathaN ! How about an extra-for-experts and creating a https mock server that we can use to test connectivity? I don't think it needs to do the full test suite but just an initial connection would be good

@chriswiggins Sorry missed this message. I'll try to create a new PR for the same.

ManjunathaN commented 3 years ago

Hi @ManjunathaN Thanks for the changes. I've merged them into master. I had one extra piece of review feedback, in addition to Chris's request to add https to Mock Server for automated testing. In the Environment Variables, have IS_SECURE. I asked you to change options.isSecure to options.useSecure which you did. Thank you. Should we change the Environment Variable to match as well? I know it is just a comment, but it would match up with the options name.

If you want some help on the Mock server, you can run npm run test

It starts an ONVIF Server on a local port an then gets the ONVIF Library to connect to it. It uses ONVIF Discovery so does work better on a network with no other ONVIF devices connected (as the discovery phase will detect other cameras on your LAN)

Thanks @RogerHardiman. Yes, having an env variable HTTPS_MODE or IS_SECURE should do the job. I'll try to check this out over the weekend.