hanami / contributors

All hanami contributors in one place
http://contributors.hanamirb.org
MIT License
14 stars 9 forks source link

Add protection for admin page using Rack simple auth #41

Closed Bounga closed 6 years ago

Bounga commented 6 years ago

Issue #39 is requesting to add a basic auth for admin app. This PR adds simple auth for an admin page that is dedicated to become the page that lists all the projects available.

It tries to find env variable provided on app boot or uses hanami / hanami as default.

This PR adds tests to ensure access is avoided without providing credential or even wrong credentials. It also add a test that ensures it works with valid credentials.

Hope it fits the requirements.

Bounga commented 6 years ago

/cc @davydovanton Here is the first PR. Hope it fits yours needs. If it does I'm going to create a new one to handle the projects list.

cllns commented 6 years ago

Thanks! I know you were just following @davydovanton's request but I'd personally rather have no default authentication. We should be secure by default, not insecure by default. If we use ENV.fetch with no second param, there will be a KeyError alerting the user that there's no Username and Password. (I'd also be OK with a default user name but not a default password, that's the important one for me.)

Bounga commented 6 years ago

@cllns I'm 100% ok with this. I was pretty uncomfortable with the default values. It's a big security hole to me but hey @davydovanton is kind of my boss here. I'd be happy to remove the default password to raise an exception if not provided as an env var on boot.

davydovanton commented 6 years ago

agree with @cllns here. Let's put ENV variables to .env.development and .env.test files and raise error if variables doesn't set :)

Bounga commented 6 years ago

Using fetch in admin/application.rb controller.prepare block will raise the error at runtime. I mean when we try to access the admin pages and not when the server is started. Doesn't sound great to me.

I'd like to move the fetch calls at the beginning of the configure block and store it in constants so it will raise as soon as you start the app rather than only when trying to access the admin pages.

What do you think about it?

cllns commented 6 years ago

I like that :+1: Great idea. (Kind of goes against Ruby being dynamic, but for a good reason, I think)

Bounga commented 6 years ago

Here is the modified version.

davydovanton commented 6 years ago

thanks for contribution, it's awesome work! 💯

Bounga commented 6 years ago

@davydovanton Does CI deploys the website automatically when something is merged in master because http://contributors.hanamirb.org returns a 500. Coincidence or the CI just didn't set the required env variables for the admin pages?