search

namibmap / IPPR

2 stars 1 forks source link

Issue#69 #83

Closed nsetru closed 8 years ago

nsetru commented 8 years ago

Basic HTTP authentication to block direct public access to dev site - instead requires password authentication Issue#69

missfunmi commented 8 years ago

Hey @nsetru!

Would you like one of us to code review these changes, or are you happy to keep it merged in to the develop branch? In the past, we've assigned the pull request to someone else on the team so that they can do the code review and then merge the changes in if there is no feedback.

cc: @raquel-ucl

nsetru commented 8 years ago

Hiya!

@missfunmi @raquel-ucl

Yes please. I forgot to request for code review before merging. Sorry!

I just checked comment from Lesedi in issue#69 It looks like he has already applied this patch on dev.

But, I don't see why it can't be reviewed now. In case there is an issue we could always revert those changes.

lbewlay commented 8 years ago

@nsetru , @missfunmi , @raquel-ucl . Let me know if there are any additional changes and i will upload the latest branch to the server.

missfunmi commented 8 years ago

Hey @nsetru sorry this is late. I've just got the latest code and tried to run it locally but then I got prompted for the login credentials. So I disabled the password authentication locally in server.js and it worked fine.

Could we maybe pass an environment flag when the server starts up and use that to determine whether to enable or disable auth? So for example, when environment = "dev" (or "local"), don't require auth, but when environment = "live" (or "production"), then require auth.

See the process.env documentation and this example. Or we could also just store the environment flag in the secret.js file like we do with the Google Analytics code since @lbewlay is using a different file on the live server.

Let me know what you think and if this makes sense.

Probably better to implement this on a separate issue/feature branch since this pull request has already been merged, but I'll leave that up to you.

missfunmi commented 8 years ago

Hi @nsetru - just checking if there is any more work to be done here. Since this branch is already merged into the main develop branch, I think we should delete it and submit any additional changes on a new pull request and just reference this one in it. I have created a separate issue #88 to track this feedback.