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

Password Protection #5103

Merged
merged 1 commit into from May 2, 2015

Conversation

@acburdine
Copy link
Member

commented Apr 4, 2015

"In which the hero of our novel, being so inclined as to enjoy the comforts of privacy, decided to put it upon himself to journey to the land of password protection, in that he might protect his writings from the eyes of the enemy."

This just needs a few more things (tests, rss/sitemap/robots.txt 404ing). And the password.hbs default file needs to be redone, but I think this is being taken care of in #5073?

@ErisDS

This comment has been minimized.

Copy link
Member

commented Apr 5, 2015

"In which the hero of our novel, being so inclined as to enjoy the comforts of privacy, decided to put it upon himself to journey to the land of password protection, in that he might protect his writings from the eyes of the enemy."

😂

classNames: ['settings-view-pass'],

beforeModel: function () {
var feature = this.controllerFor('feature'),

This comment has been minimized.

Copy link
@novaugust

novaugust Apr 6, 2015

Member

controllerFor is a deprecated macro for injecting a controller as a computed property, not for getting it in a method :)
you want something more like this:

feature: Ember.inject.controller(),
beforeModel: function () {
  var feature = this.get('feature');
  ....
}
@novaugust

This comment has been minimized.

Copy link
Member

commented Apr 6, 2015

Ping me if you want some help with the ember stuff

@@ -76,6 +80,14 @@ function sslForbiddenOrRedirect(opt) {
return response;
}

// because bcrypt isn't thenable I had to add in a callback argument

This comment has been minimized.

Copy link
@jaswilli

jaswilli Apr 7, 2015

Member

You can create a promise-aware method:

var bcryptCompare = Promise.promisify(bcrypt.compare);
bcryptCompare(a, b).then(function (match) {
    // ... 
});

@acburdine acburdine force-pushed the acburdine:passprotect branch from 731ee11 to daf9895 Apr 7, 2015

@@ -76,6 +81,16 @@ function sslForbiddenOrRedirect(opt) {
return response;
}

function verifySessionHash(hash) {
var bcryptCompare = Promise.promisify(bcrypt.compare);

This comment has been minimized.

Copy link
@jaswilli

jaswilli Apr 7, 2015

Member

I'd recommend moving the reference to the promisified method up to the top of the file with all the other module level declarations. There's no need to re-create it on every invocation of verifySessionHash.

var pass = response.settings[0].value;
return bcryptCompare(pass, hash).then(function (verified) {
return verified;
});

This comment has been minimized.

Copy link
@jaswilli

jaswilli Apr 7, 2015

Member

No need to use an explicit then in this case--you can simply return bcryptCompare(pass, hash); to the caller.

@acburdine acburdine force-pushed the acburdine:passprotect branch 2 times, most recently from c4e4d5e to f46766d Apr 8, 2015

usePassProtectUI: Ember.computed('controllers.feature.passProtectUI', function (key, value) {
// setter
if (arguments.length > 1) {
this.saveLabs('passProtectUI', value);

This comment has been minimized.

Copy link
@novaugust

novaugust Apr 8, 2015

Member

In a setter situation, you want to also return the value so that it can be cached. Just follow this line up with return value;

This comment has been minimized.

Copy link
@acburdine

acburdine Apr 8, 2015

Author Member

@novaugust I did that, but that didn't seem to fix the problem... The problem is occurring in either templates/settings/pass-protect.hbs or controllers/settings/pass-protect.js.

.then(this.transitionEditor())
.then(function () {
return feature.then(function () {
if (!feature.get('passProtectUI')) {

This comment has been minimized.

Copy link
@novaugust

novaugust Apr 8, 2015

Member

You're doing a get on the promise here, if that's what feature is. You probably wanted to take it as a param in your fulfilled function on the line above

This comment has been minimized.

Copy link
@novaugust

novaugust Apr 8, 2015

Member

It works regardless though, because magic o.O

This comment has been minimized.

Copy link
@novaugust

novaugust Apr 8, 2015

Member

Yeah, if you could just go ahead and ignore me, that'd be great.

@acburdine acburdine force-pushed the acburdine:passprotect branch 3 times, most recently from 42f91c8 to 2858491 Apr 9, 2015

@acburdine

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2015

@ErisDS Something happened with this PR and the Travis build status disappeared. Is there any way to reenable it?

@ErisDS

This comment has been minimized.

Copy link
Member

commented Apr 10, 2015

A force push sometimes kicks it back in, unfortunately it's a bug in Travis we run into from time to time.

@acburdine acburdine force-pushed the acburdine:passprotect branch 4 times, most recently from 295f7d2 to b182552 Apr 16, 2015

@ErisDS

This comment has been minimized.

Copy link
Member

commented Apr 21, 2015

A note on the config flag part of this - I think it is missing the addition of a line here which would pass the config through to the frontend? - https://github.com/TryGhost/Ghost/blob/master/core/server/api/configuration.js#L12

res.isPrivateBlog = true;
return session({
maxAge: utils.ONE_MONTH_MS,
keys: ['isPrivateBlog']

This comment has been minimized.

Copy link
@acburdine

acburdine Apr 22, 2015

Author Member

I'm not sure if this is the best thing to put for cookie session keys....
@ErisDS any better ideas on what to put for keys?
looking at the cookie-session documentation, I'm not sure what this does :/

@acburdine acburdine force-pushed the acburdine:passprotect branch 5 times, most recently from 48c2f4e to cbb6e61 Apr 22, 2015

@acburdine acburdine force-pushed the acburdine:passprotect branch from cbb6e61 to 9f05dfc Apr 27, 2015

@acburdine

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2015

@ErisDS This is finished, finally.
I had to change the bcryptjs module to use bcrypt instead because of issues with the salt version. I'm almost 100% certain the error was with the bcryptjs module and not the Ghost code.

@ErisDS

This comment has been minimized.

Copy link
Member

commented Apr 27, 2015

Hey, not sure what's going on with bcrypt.js, perhaps we need to update. We cannot use bcrypt in Ghost as it requires a binary and would make it almost impossible for many people to install the software.

@acburdine acburdine force-pushed the acburdine:passprotect branch from 9f05dfc to 0fc6d6d Apr 27, 2015

@acburdine

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2015

@ErisDS do you want me to write functional tests for this as well? or are just unit tests good?

@acburdine acburdine changed the title [WIP] Password Protection Password Protection Apr 28, 2015

@sebgie

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2015

Hey @acburdine,

I have taken this PR for a spin and I really like where this is going! Sorry it took me a bit. I would have some questions/suggestions:

  • Why are sitemap and rss returning a 404 error? It would be nice to access them using the cookie for testing purposes imo?
  • Enable the password protection interface is enabled by default. I think the default should be disabled for labs features?
  • From the original issue is seems that the input for a message that users see when they visit the private blog is missing.
  • I think the UI needs some love from @JohnONolan (http://note.io/1EvuMVi)
  • Future improvement: Should we add spam protection agains brute force attacks? (Forget that, didn't realize it is already there 🙈)

I also encountered the invalid salt error. It happens when the user doesn't have a cookie and bcrypt tries to verify a non existent hash. I'll add a comment where this happens in the code.

@acburdine

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2015

@sebgie In a conversation between @JohnONolan, @ErisDS and I on slack, we decided to not do the message (correct me if I'm wrong Hannah or John). In #5073, John's design doesn't have a message area. The UI stuff is also from #5073, but Hannah also said to make something for the password field for this PR, and then make another one improving the password page design later.

I'll take a look at the invalid salt thing later today.

@ErisDS

This comment has been minimized.

Copy link
Member

commented Apr 30, 2015

All correct, it's behind labs and fine for it to be ugly to start.

The RSS & Sitemaps thing was in the original spec because I felt trying to add the auth to those routes would add unnecessary complexity given that they wouldn't work in any useful way.

@sebgie

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2015

@acburdine thanks for clarifying! If you could fix the invalid salt error and set the default setting to disabled I think it's good to go 🎊.

@acburdine acburdine force-pushed the acburdine:passprotect branch 2 times, most recently from e93f414 to baa7a79 Apr 30, 2015

@acburdine

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2015

@sebgie fixed the errors

@ErisDS

This comment has been minimized.

Copy link
Member

commented Apr 30, 2015

I found a wee bug!

When you have this setup on a subdirectory, when you get sent to the private route, the redirect route doesn't contain the subdirectory - here's what happens if I hit the Ghost logo to visit my blog right after turning this on:

@acburdine acburdine force-pushed the acburdine:passprotect branch from baa7a79 to 548acf5 Apr 30, 2015

@acburdine

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2015

@ErisDS I fixed it, and the ?r= will still show up as /, but it's now run through a config.urlFor after successful authentication, so it redirects correctly

@acburdine acburdine force-pushed the acburdine:passprotect branch 2 times, most recently from b455858 to d802b52 Apr 30, 2015

added password protection
closes #4993
- brings password protection to the frontend of blogs
- adds testing for password protection
- upgrades bcrypt-js to 2.1.0

@acburdine acburdine force-pushed the acburdine:passprotect branch from d802b52 to 2865662 May 1, 2015

@ErisDS ErisDS closed this May 2, 2015

@ErisDS ErisDS reopened this May 2, 2015

ErisDS added a commit that referenced this pull request May 2, 2015

@ErisDS ErisDS merged commit 7fe63b2 into TryGhost:master May 2, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@ErisDS

This comment has been minimized.

Copy link
Member

commented May 2, 2015

Sorry the close/reopen was a total button failure on my part! Also apologies for not getting to this yesterday, it's been an absolutely crazy week!

I'm so excited that this is in!

@acburdine acburdine deleted the acburdine:passprotect branch May 2, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.