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

Feature/centralize varnish trigger #241

Merged
merged 1 commit into from
Nov 27, 2013

Conversation

strk
Copy link

@strk strk commented Sep 30, 2013

Closes #197

@luisbosque
Copy link
Contributor

Why are you using LISTEN/NOTIFY? We cannot use LISTEN/NOTIFY since it's not secure

@strk
Copy link
Author

strk commented Nov 27, 2013

I'm only using NOTIFY. Do you see security problems with it ?

Note we're already using it, all the commit does is adding more
comments about what was the intended use for those notifications,
but the invalidation is being performed "manually" right now.

@luisbosque
Copy link
Contributor

Yes yes, I can see that the real work is being done manually. I'm just
warning that we cannot use the notifier because it's not secure. Anyone can
listen or notify in a channel. We can comment that NOTIFY in your commit
since we are not going to use it. Would that be ok? I don't see any place
where it's being used

On Wed, Nov 27, 2013 at 12:21 PM, strk notifications@github.com wrote:

I'm only using NOTIFY. Do you see security problems with it ?

Note we're already using it, all the commit does is adding more
comments about what was the intended use for those notifications,
but the invalidation is being performed "manually" right now.


Reply to this email directly or view it on GitHubhttps://github.com//pull/241#issuecomment-29377151
.

Luis Bosque

@strk
Copy link
Author

strk commented Nov 27, 2013

It would be ok to comment that out, and add a comment
about the reason why it was done.

I don't think anyone can listen trough the SQL api though,
did you try ? It may make sense to write an automated test
for the security case. As we're already broadcasting those
events [1], removing the notification would be a noticeable
change, to accompaign with a testcase and a NEWS item.

1

I'd also note that all you could gather by listening to that
notification is the name of a table which you could currently
easily read from CDB_TableMetadata

--strk;

@luisbosque
Copy link
Contributor

I wasn't able to listen from the sql api, but the reason I think it's
because the answer of a liste is not parseable in json, not because
postgres didnt allow to get that info. I was able to notify from the sql
api to any user without authentication.

On Wed, Nov 27, 2013 at 12:47 PM, strk notifications@github.com wrote:

It would be ok to comment that out, and add a comment
about the reason why it was done.

I don't think anyone can listen trough the SQL api though,
did you try ? It may make sense to write an automated test
for the security case. As we're already broadcasting those
events [1], removing the notification would be a noticeable
change, to accompaign with a testcase and a NEWS item.

1

I'd also note that all you could gather by listening to that
notification is the name of a table which you could currently
easily read from CDB_TableMetadata

--strk;


Reply to this email directly or view it on GitHubhttps://github.com//pull/241#issuecomment-29378502
.

Luis Bosque

@strk
Copy link
Author

strk commented Nov 27, 2013

The answer to LISTEN is asyncrhonous. It takes a client to actively "pull" the notifications to get aware of them.
Unless this was accidentally done by implementation of the WARNING/NOTICE channels in 1.7.0 it will be impossible. You could try harder with a test in SQL-API itself, where you can ensure things happen within the same session (harder on cartodb.com, were you might be using different sessions, thus loosing notifications which would be send to someone else...)

@strk
Copy link
Author

strk commented Nov 27, 2013

Agreed about using a notification as a trigger for invalidating caches might be more prone to users faking changes than a security definer trigger invoking a function for which unauthenticated users have no rights to execute.

A lot of material for testcases, it looks like :)

demimismo added a commit that referenced this pull request Nov 27, 2013
@demimismo demimismo merged commit 0ce0602 into CartoDB:master Nov 27, 2013
@strk strk deleted the feature/centralize-varnish-trigger branch December 10, 2013 16:29
tylerparsons referenced this pull request in bloomberg/cartodb Jan 2, 2018
…geojson-regression-tests

Changes geojson_spec to use the configured ogr2ogr binary
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move varnish management trigger from user tables to CDB_TableMetadata
3 participants