rappasoft / laravel-boilerplate

The Laravel Boilerplate Project - https://laravel-boilerplate.com
https://rappasoft.com
5.59k stars 1.58k forks source link

Security issue - Seeder for admin account #301

Closed vpratfr closed 8 years ago

vpratfr commented 8 years ago

Hi,

It is quite a bad idea to have the administrator account set in the seeder as that file will be committed to source control.

Even if the user is resetting the password on first login, that still gives out the email addresses, etc.

Something I usually provide is an install route, which:

That allows easy setup when deploying the app to a production server and avoids storing any sensitive info in source control.

If you are interrested, I can submit a pull request for that.

blomdahldaniel commented 8 years ago

This is a pretty good idea in some ways. Personally I do not think there is big enough to call a security issue though. But I see your point. This and #306 would be quite an undertaking. I, personally dont have the time to write something like this right now. What do you think @rappasoft Is this something you would want to do or could you just give pointers to @vpratfr and then he can do a PR?

vpratfr commented 8 years ago

I am currently having a look at it for a current project. I will try to package something generic as a 3rd party dependency and make some custom views compatible with the boilerplate project.

blomdahldaniel commented 8 years ago

cool!

vpratfr commented 8 years ago

Hi @blomdahldaniel & @rappasoft,

I am back with some good news: I have put together a web installer which provides some basic steps for generic applications. Good news is that it should be very easy to extend with our custom wizard steps.

Before I start making some custom steps for the boilerplate, I'd love to get some feedback about that package. Do you feel it would fit nicely into the boilerplate?

It is available there: https://github.com/marvinlabs/laravel-setup-wizard

Regards,

blomdahldaniel commented 8 years ago

Holy moly! Dude this looks awesome. I am totally swamped by work right now so I do not have the time to dig deep into it quite yet but I look forward to taking a look at it! :)

vpratfr commented 8 years ago

Got something pretty nice. Goes with PR #329.

Added a simple form with the basic info required to create a user.

image

Quite happy with the way the package was developed. I had nothing much to do to create the two boilerplate specific wizard steps (access & admin).

I hope you can merge that into the project. I would be using that on almost any project. And if not required, that can be plugged off pretty easily.

rappasoft commented 8 years ago

I'm hesitant, because I did something similar to this a while back, and people didn't like the fact of being tied to installer and forced me to remove it.

So I don't know what the best course of action is.

vpratfr commented 8 years ago

That installer is in no way mandatory and could be left optional.

We could factorize the logic used in the Installer steps to create the data and use that both in the installer and in the basic seeder.

That way, you could unplug the installer if you wanted to and use the seeders with all benefits from refactored code (specifying roles, groups and permissions in a configuration file).

vpratfr commented 8 years ago

However, my initial feeling is really a security issue (having the admin name/password in clear committed to source control. And that one should certainly be addressed somehow (which the installer solves nicely for those who want an installer).

blomdahldaniel commented 8 years ago

Okey so now I have also gotten some time to look at it. And I must say that you have done a great job extracting it and making it easy to use as a package. :+1:

What @rappasoft refers to is the conversation we had in issue #104 (Read that conversation to understand what was said then.)

Rappasoft created a similar functionality. It was very good looking and mainly it checked for php-packages and directory permissions and then everything got migrated and seeded, no questions asked. Do I remember it correctly @rappasoft ?

That functionality and the way it was done made things a bit more confusing. And like I stated in #104 I do not think that it was a bad idea. But in the format that it was then, it was not big enough nor flexible enough. And it also removed/took away the ability to configure or do it more custom. And the terminal instructions was removed from the readme.

Two ways to install..

  1. Through the terminal just like now. We should never force users to go through a GUI in my opinion.(But then we need to inform about the security issue(s).)
  2. If through an installer GUI: it has to bee a full scale installer. It has to be noob proof and cover setup in many aspects.

Demands for a installation GUI

The koncept of a GUI installer in this case solves the possible security issue with the hardcoded version controlled admin email and password. This is a nice way to solve it. But in this case, just like in #104 a installer cannot just be a screen where you click next. The GUI must be a full blown setup wizzard to make sense. I understand that your package is in an early stage. But the installer has major flaws in user experience as of now.

What I think a GUI installer needs to be/have

Sorry

I actually don't have time to explain or declare more about the issues with the current state of this PR. But as of now, the user experience is not good enough. (instructions, info, guides) etc..

And to be clear, I dont think this is a bad idéa. But if we will add an installer to this boilerplate, it has to be a full scale instaler and all of what that means.

vpratfr commented 8 years ago

This web GUI is actually not supposed to be an installer for end users but rather a way for developers to deploy their app easily to servers where they may not have ssh access for instance.

Hence it is kept very simple but also meant to be very easily extensible if you ever need to turn it into a more end-user friendly installer.

How do you create your admin user on your dev/integration/prod servers?

That installer solves problems for developers, not so much for end users.

vpratfr commented 8 years ago

I feel that installer is something needed, I agree it may not be documented enough, or could be improved in some ways. However, this is a starting point and I think we should work towards getting it ready for a stable release.

Maybe you could open a few issues regarding that installer and we could start by fixing those.

First decision would be to agree on the point made by that GUI installer. According to me that installer should allow :

Again, according to me, that installer should not be :

Potential points where I think we could improve :

Some future feature that could be a killer feature :

blomdahldaniel commented 8 years ago

Hi!

Okey then I understand more what you where aiming for. And I agree somewhat but where I really could jump onboard is if we, just like you said, can make this optional and also available from a artisan command.

Things get a bit complex when it comes to config vs. seeder though. But if documented well enogugh, sure!

Still a bit more GUI features should be implemented such as small instructions/descriptions and also error-messages with discriptions. Sure, we dont have to do it for end-users but still provide as much required info as possible. So features like:

This is very exciting and I think that this could lead to some nice stuff.

The sad part about it is that I have no time right now and must focus on other projects and aspects of life so I will not be able to contribute to this in the next couple of weeks.

Anyhow, * I don't think this is something we should rush into!* It is nice and it helps with a possible security risk but this is a big addition and therefore we should not be afraid to let it sink in and develop a bit more over time before we go ahead and merge this in. Thats just my opinion.

vpratfr commented 8 years ago

Ok. I'll start working on some improvements which go that way. I need that for a project of mine so no problem.

I need your help just to give feedback and open issues. I will handle development.

blomdahldaniel commented 8 years ago

Sure I can take a look at it every now and then. I now follow your repo so that I can get notified when stuff happens.

blomdahldaniel commented 8 years ago

This issue has been very helpful and have driven ideas in the right way for the repo. An installer will be added to version 3.0, at least that is the plan. The installer will work similar to the installer of Spark, which would also allow fetch updates easier. Therefore I will close this issue as of now.