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

support https protocol #194

Merged
merged 7 commits into from
Jun 11, 2021
Merged

support https protocol #194

merged 7 commits into from
Jun 11, 2021

Conversation

ManjunathaN
Copy link
Contributor

No description provided.

@ManjunathaN
Copy link
Contributor Author

Related PR - #194

@RogerHardiman
Copy link
Collaborator

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
Copy link

coveralls commented Jun 8, 2021

Coverage Status

Coverage decreased (-0.1%) to 87.601% when pulling 7bc082e on ManjunathaN:master into 6797640 on agsh:master.

@ManjunathaN
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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 RogerHardiman merged commit 0b47cea into agsh:master Jun 11, 2021
@RogerHardiman
Copy link
Collaborator

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
Copy link
Contributor Author

ManjunathaN commented Jun 11, 2021

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants