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

Changed User.create signature to accept a hash, added a couple of utilit... #820

Merged
merged 8 commits into from
Jan 23, 2014
Merged

Conversation

akhoury
Copy link
Member

@akhoury akhoury commented Jan 20, 2014

...y functions as well, also added a new hook filter:user.create .

EDIT: also added a new config use_proxy - I wasn't sure where exactly you would prefer that config to live, feel free to either move it or suggest a better place (db maybe?) - but it does require a restart, so I figured the config.json is not a bad idea.

@akhoury
Copy link
Member Author

akhoury commented Jan 20, 2014

oops - this commit's message should read: " password should NOT be passed ... "

@julianlam
Copy link
Member

Awesome -- we're going to merge this in after 0.3.0 lands, since it does not fix any pressing issues.

@psychobunny
Copy link
Contributor

ping (just a reminder)

@@ -35,7 +35,7 @@ var user = require('./user'),
});
}

bcrypt.compare(password, userData.password, function(err, res) {
bcrypt.compare(password, userData.password || '', function(err, res) {
Copy link
Member

Choose a reason for hiding this comment

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

What is the || '' for? If their pwd is undefined they should just be rejected?

Copy link
Member Author

Choose a reason for hiding this comment

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

I remember hitting this statement with a NULL password somehow, if the pwd is null, bcrypt.compare crashed NodeBB.. but now that I look at it again, I think that was caused by a bug that I introduced then fixed with the before-last commit.

I'll take that out

@julianlam julianlam merged commit 659817b into NodeBB:master Jan 23, 2014
@julianlam
Copy link
Member

Am I right in assuming that trust proxy being enabled just means that req.ip is accessible if connections to NodeBB are proxied? Didn't know that was a problem, so nice catch 👍

@julianlam
Copy link
Member

Food for thought, it is possible that "trust proxy" could be inferred from the existing config values:

if (port !== '80' && port !== '8080' && use_port === false)

Let me know if I'm wrong 😄

@akhoury
Copy link
Member Author

akhoury commented Jan 23, 2014

actually i think you're right. but why 8080 is a condition?

@julianlam
Copy link
Member

8080 is "HTTP Alternate", I assume because it's > 1024, so it is not restricted like low-number ports are. I've noticed that when I try to connect to an IP with port 8080, my browser sometimes just transparently redirects it to 80.

Perhaps I'm wrong, so I suppose let's take out the 8080 conditional for now... could we actually remove the use_proxy option now?

@akhoury
Copy link
Member Author

akhoury commented Jan 23, 2014

Oh ok, i think it's safe to take out the use_proxy option, and probably the 8080 condition too.

I know I wasn't in love with adding another option in config.json. if you don't beat me to it, I'll clean it up tonight (with the prompt question and the block of comments)

@akhoury
Copy link
Member Author

akhoury commented Jan 23, 2014

oh what about 443

@julianlam
Copy link
Member

You know, I'd prefer if people didn't simply set up NodeBB with port 80 or 443 and run it as root, but what can you do?

We should add the 443 there as well.

No rush -- I'll wait for your PR tonight.

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