Skip to content
This repository was archived by the owner on Mar 24, 2026. It is now read-only.

Postgres/MySQL: Restrict database permissions to just what is required#2237

Merged
knewmanTE merged 2 commits intoTechEmpower:masterfrom
k-r-g:database-permissions-attempt2
Aug 26, 2016
Merged

Postgres/MySQL: Restrict database permissions to just what is required#2237
knewmanTE merged 2 commits intoTechEmpower:masterfrom
k-r-g:database-permissions-attempt2

Conversation

@k-r-g
Copy link
Copy Markdown
Contributor

@k-r-g k-r-g commented Aug 24, 2016

Some frameworks were misbehaving and changing the schema.

Also remove upper-case verions of MySQL tables as those are not needed.

Some frameworks were misbehaving and changing the schema.

Also remove upper-case verions of MySQL tables as those are not needed.
@LadyMozzarella LadyMozzarella added this to the Round 13 milestone Aug 25, 2016
@knewmanTE knewmanTE merged commit 1745760 into TechEmpower:master Aug 26, 2016
@LadyMozzarella
Copy link
Copy Markdown
Contributor

@nkasvosve (BeyondJ), @jberger (Mojolicious), @zapov (RevenJ) - Heads up! This pull request changes the permissions for the database and, based on Travis-CI results, it appears that frameworks you have worked on are impacted by it. Could you look over your framework implementations and see what is happening? Thanks.

@zapov
Copy link
Copy Markdown
Contributor

zapov commented Aug 27, 2016

Revenj will read database metadata at startup (pg_class, pg_namespace, pg_attribute, pg_type and pg_description). Without seeing the actual error I would assume you disallowed reading from those system tables. So grant select onto those tables and it should be back to running again.

@zapov
Copy link
Copy Markdown
Contributor

zapov commented Aug 27, 2016

Ah.. I see. It fails to do an update due to missing insert/delete permissions.
Revenj calls generic save method which creates SQL for doing insert/update/delete in a single roundtrip, which will fail even if there is no insert/delete data since you have removed permission for doing those stuff.

It will keep failing until you give back those permissions (or run it through different user)

@jberger
Copy link
Copy Markdown
Contributor

jberger commented Aug 27, 2016

I'll take a look, should be today sometime. I needed to do a dependency-pin bump anyway so this is a good motivator.

@jberger
Copy link
Copy Markdown
Contributor

jberger commented Aug 27, 2016

Interestingly mojolicious doesn't seem to care about this change. What IS breaking things is that the server deployment script cannot create files or folders in the mojolicious/ project directory. Since we use carton to deploy dependencies this is breaking dependency installation.

@LadyMozzarella do you have any suggestions about why this is happening? Clearly it didn't used to happen. It seems that the default directory permissions are 0775. Is the user that runs the server setup not the 'vagrant' user (and also not root)?

@jberger
Copy link
Copy Markdown
Contributor

jberger commented Aug 27, 2016

So I'm guessing that the user testrunner is the one that sets up and starts the servers though the directory is owned by vagrant and the perms are 0775. What can be done to address this? I'm also on IRC if this should be discussed a little more synchronously.

@LadyMozzarella
Copy link
Copy Markdown
Contributor

@jberger I'm not sure what the issue was, but I think this Travis run may have been a fluke. Merging this PR into master kicked off a new Travis run based off of master. Mojolicious passed there. I set up a new Vagrant environment based on current master and ran Mojolicious and it also passed there. I can't seem to replicate the issue locally.

@zapov Oh I see, we will have to look into/consider that particular situation specifically.

@jberger
Copy link
Copy Markdown
Contributor

jberger commented Aug 28, 2016

That is interesting too, but if I can't run it in vagrant then something is still not kosher.

@jberger
Copy link
Copy Markdown
Contributor

jberger commented Aug 28, 2016

BTW I think I have a patch that I think we be acceptable. Working now on seeing if I can make Perl versions be specified by frameworks. I'd like to get a more modern perl than the one we configured back several years ago.

@zapov
Copy link
Copy Markdown
Contributor

zapov commented Aug 28, 2016

@LadyMozzarella dont try to make special rules for Revenj. I have to change the submission anyway since you've changed the rules to disallow current Revenj implementation. Hopefully before the next deadline

@jberger
Copy link
Copy Markdown
Contributor

jberger commented Aug 28, 2016

@zapov I read your email comments and I have to say, the persecution complex is unbecoming. My Mojolicious app could open 100 connections and parallel query it and yet I understood from the rules that that was against the spirit; so I make individual queries in a for loop. If parallel (or bundled) queries were allowed mine would be faster too. If it means this much to you, propose a new test; I'll second it.

@zapov
Copy link
Copy Markdown
Contributor

zapov commented Aug 28, 2016

@jberger ;( There was no need to hijack this thread for that discussion.
First of all, calling people names is immature and will lead nowhere.
Second of all, there are multiple implementations which do a parallel query inside the benchmark.
Third, I did complain about this more than a year ago (suggested to change the name of test/rules of the test): #1622
And lastly, this benchmarks facilitates a myth that if you want to squeeze some more performance out your DB interaction you should start several parallel DB queries. By doing a Revenj submission I showed just how wrong that myth is. But there is no much point about discussing that. Especially not in this thread.

@NateBrady23
Copy link
Copy Markdown
Member

@zapov Are you submitting a PR for a batched query test?

@zapov
Copy link
Copy Markdown
Contributor

zapov commented Aug 28, 2016

yeah, I will try to find some time and add a version which does a loop (and fix this permission issue in the process)

@NateBrady23
Copy link
Copy Markdown
Member

@zapov thanks :)

ashawnbandy-te-tfb pushed a commit to ashawnbandy-te-tfb/FrameworkBenchmarks that referenced this pull request Sep 9, 2016
TechEmpower#2237)

* Postgres/MySQL: Restrict database permissions to just what is required

Some frameworks were misbehaving and changing the schema.

Also remove upper-case verions of MySQL tables as those are not needed.

* Fix GRANT syntax error
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants