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

[UX] Create database at installation time. #496

Closed
klonos opened this issue Dec 22, 2014 · 15 comments
Closed

[UX] Create database at installation time. #496

klonos opened this issue Dec 22, 2014 · 15 comments

Comments

@klonos
Copy link
Member

klonos commented Dec 22, 2014

This is the same as https://www.drupal.org/node/203955 created after request by @jenlampton over at #467

It is convinient for developers, who deploy sites on local computers. Usually they open phpMyAdmin to create a database and then they return to the web installer to go on. We can skip this step and provide a way for them to create a db during installation (provided that they have the right privilege to create databases). This way the installation UX is improved:

  • shorter installation times
  • installation not interrupted by trivial tasks that can be automated or done through the installer UI

PR by @quicksketch: backdrop/backdrop#2126

@serundeputy
Copy link
Member

hrmm ... i'm just spitballing here, but would that be a security risk as you are creating a DB before there is a user 1; and also user 1 can input DBs through like backup_migrate, but not actually create DBs.

Not sure.

@quicksketch
Copy link
Member

Yeah, I have my concerns here as well. You shouldn't be setting up a site using an account that can create (and probably drop) databases in the first place. I had considered that maybe this would be okay if we did some basic domain checking. e.g. the domain contained:

  • .local
  • local.
  • localhost
  • dev.
  • .dev
  • 127.0.0.1

But this wouldn't be fool-proof. Additionally, it would introduce inconsistent behavior between setting up Backdrop on a localhost and setting up Backdrop on a production host. That sort of "sometimes magic" can cause a lot of confusion.

So while I like the idea and I'd certainly appreciate it in my localhost installations, I'm not sure this an idea we should implement.

@Dinornis
Copy link

Dinornis commented May 2, 2017

I also vote against this for the security reasons mentioned above.

@quicksketch
Copy link
Member

On further thought here... I'm not sure my concerns are totally valid. Backdrop already requires all permissions on a database, including the ability to drop tables. If you can drop tables, you can drop the entire database. Similarly, the CREATE permission in MySQL is the same whether you're creating a single table or the entire database. So the permission level needed to create a DB is the same that Backdrop requires anyway.

Many servers or shared hosting accounts only run a single site, or even if they have multiple sites, many users have a single database username for all of them. Automatically creating the database just saves a step. If they are going to use the same username and password, making them do it manually doesn't make things any more secure.

I also thought that eventually https://www.drupal.org/node/203955 would be merged into D7, as it made it into D8 quite a while ago. But seems development there has slowed. I've taken the D7 patch (and the follow-ups noted in the issue) and made a Backdrop port.

@quicksketch
Copy link
Member

quicksketch commented Apr 13, 2018

Here are screenshots of the behaviors in action:

  • User does not have permission to create a database (same as the current error):
    image
  • User exists but does not have permission to create the database (or any tables), new error for edge-case handling:
    image

And in the case the database can be created, no error or message is shown at all. The database is immediately created and then used.

@serundeputy
Copy link
Member

@quicksketch I put an open question/review on the PR: https://github.com/backdrop/backdrop/pull/2126/files#r182904879

what to do when DB specified in the installer is the same as an existing DB (by accident perhaps).

@jenlampton
Copy link
Member

jenlampton commented Apr 19, 2018

what to do when DB specified in the installer is the same as an existing DB (by accident perhaps).

This is how it works now. If you put in a database that already exists, Backdrop will attempt to connect to it, just like you told it to. :)

@serundeputy
Copy link
Member

serundeputy commented Apr 19, 2018

Steps using a database that exists that has nothing to do with backdrop:

  • create database geoffisthebest;
  • add a table to geoffisthebest
  • add some data to the table
  • visit backdrop installer
    • when prompted for the Datbase name use geoffisthebest
  • Backdrop installs to geoffisthebest leaves your table there with its data
    • not sure if this is good or bad, but could definitely lead to confusion and cruft

If you specify a DB that exists that already has Backdrop tables you get:
screen shot 2018-04-19 at 7 34 32 pm

but, then if you visit the site there is a config hash mismatch:

screen shot 2018-04-19 at 7 35 31 pm

maybe we can add a check in the installer for

create database USERSPECIFIEDVIAINSTALLER  IF NOT EXISTS

@serundeputy
Copy link
Member

I guess this is a problem we already have though; nothing to do with this PR.

This PR is checking if it exists, if so go on as usual, which will write to whatever database you told it to that already exists, and if it doesn't exist than it creates it.

So, all the stuff I said above happens with or without this PR ... never mind me ... carry on .... nothing to see here: ⛵️

@quicksketch
Copy link
Member

Yeah, all of that is an existing issue. I agree though, the config mismatch problem can be annoying. Perhaps we should handle that somehow (automatically make a new config directory if the DB is empty?), but it is a separate issue than this one.

@quicksketch
Copy link
Member

FYI here's a screenshot of the installer now if the username/password/host is incorrect. We can remove all the messaging about "Does the database exist" because if it doesn't exist we would have created it. Plus the previous error was overwhelming with its suggestions, this pairs it down a bit:

image

@jenlampton
Copy link
Member

Is it a problem that the user can't see the password here?

@serundeputy
Copy link
Member

Is it a problem that the user can't see the password here?

I don't think it is a problem, but a toggle similar to the user::password field could be added, but I wouldn't hold this up on account of it

@serundeputy
Copy link
Member

Thanks everyone for testing, commenting and coding!
tested w/ mariadb and mysql; looks good works good.
Merged into 1.x will ship with 1.10.0

@jenlampton I'll open a follow up issue for the database password field.

@serundeputy
Copy link
Member

fyi: here is the follow up: #3048

@jenlampton jenlampton mentioned this issue May 15, 2018
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants