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.

Sign up for GitHub

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

Jump to bottom

Patch limited access #457

Merged
merged 6 commits into from
Sep 29, 2016
Merged

Patch limited access #457

merged 6 commits into from
Sep 29, 2016

Conversation

Jopyth
Copy link
Contributor

@Jopyth Jopyth commented Sep 29, 2016 •
edited
Loading

Solves #456 as discussed there.

Features:

  • Warning message of default behaviour is logged, if ipWhitelist value is not specified in the config
  • If you try access from a blacklisted IP you will see how to change it
  • It logs if anyone tries to access your mirror, who is not allowed

From my PC:
screenshot from 2016-09-29 17 10 32

From my phone:
screenshot_2016-09-29-17-10-56

MichMich
MichMich reviewed Sep 29, 2016
View reviewed changes
js/server.js

var Server = function(config, callback) {
console.log("Starting server op port " + config.port + " ... ");

server.listen(config.port);
if (config.ipWhitelist === undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The defaults for the config can be set in: https://github.com/MichMich/MagicMirror/blob/develop/js/defaults.js that might be a cleaner option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that makes sense. However can we still display the warning message then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just check if it's set to a certain value, and then show a message ...

@MichMich
Copy link
Collaborator

Awesome work! I've added a comment about the default settings. You might want to change/look into that. Also, is it possible for you to add information about this feature to the readme and add it to your PR?

@MichMich
Copy link
Collaborator

MichMich commented Sep 29, 2016 •
edited
Loading

Also, it seems like travis bot isn't happy:

/home/travis/build/MichMich/MagicMirror/js/server.js
  13:24  error  Strings must use doublequote  quotes
  25:52  error  Strings must use doublequote  quotes

You might want to check the code using grunt.

@Jopyth
Copy link
Contributor Author

Jopyth commented Sep 29, 2016 •
edited
Loading

Will take care of this. We can probably do it without the warning message. If someone wants to access the mirror from outside, the error message is pretty clear on how to fix it.

@Jopyth Jopyth force-pushed the patch-limited-access branch from fbb5fa9 to 99cf1f4 Compare September 29, 2016 15:42
@Jopyth Jopyth force-pushed the patch-limited-access branch from 99cf1f4 to b583140 Compare September 29, 2016 15:44
@MichMich MichMich merged commit 501c89a into MagicMirrorOrg:develop Sep 29, 2016
@MichMich
Copy link
Collaborator

Awesome work! Thanks!

@MichMich
Copy link
Collaborator

MichMich commented Oct 1, 2016

I just tried installing develop on my real mirror, and get Access denied to IP address: ::1 with the default config. Any idea?

@MichMich
Copy link
Collaborator

MichMich commented Oct 1, 2016

Never mind, fixed: b267491

@Jopyth
Copy link
Contributor Author

Jopyth commented Oct 1, 2016

I had ::1 at first (since that is wikipedia said would be the loopback address), but my IPv6 somehow displayed ::ffff:127.0.0.1, so I changed it to that instead. Lets hope this works for everyone now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichMich MichMich MichMich left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Jopyth @MichMich