Skip to content
This repository was archived by the owner on May 25, 2021. It is now read-only.

add configurable blacklist for all URL endpoints #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add configurable blacklist for all URL endpoints #47

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/chttpd_handlers.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,21 @@ provider(App, Module) ->
App, chttpd_handlers, Module).

url_handler(HandlerKey, DefaultFun) ->
ok = ensure_enabled(HandlerKey),
case collect(url_handler, [HandlerKey]) of
[HandlerFun] -> HandlerFun;
[] -> DefaultFun
end.

db_handler(HandlerKey, DefaultFun) ->
ok = ensure_enabled(HandlerKey),
case collect(db_handler, [HandlerKey]) of
[HandlerFun] -> HandlerFun;
[] -> DefaultFun
end.

design_handler(HandlerKey, DefaultFun) ->
ok = ensure_enabled(HandlerKey),
case collect(design_handler, [HandlerKey]) of
[HandlerFun] -> HandlerFun;
[] -> DefaultFun
Expand All @@ -51,6 +54,20 @@ design_handler(HandlerKey, DefaultFun) ->
%% Internal Function Definitions
%% ------------------------------------------------------------------

is_disabled(KeyBin) ->
BlacklistConfig = config:get("chttpd", "url_handler_blacklist", []),
Copy link
Contributor

@iilyak iilyak Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about dedicating whole section in config for this. Then you would be able to choose error message
Something like:

[chttpd_blacklist]
_users = {not_found, "This endpoint is disabled"}
_global_changes = not_implemented

just an idea. Fill free to ignore.

Copy link
Member

@kxepal kxepal Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. But how to avoid collisions when same-named resource is defined on different levels like /_foo and /db/_foo? We don't have such cases, but theoretically this is a limitation.

Copy link
Member

@kxepal kxepal Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If involve your idea, then we'll end with chttpd_blacklist_global_handlers, chttpd_blacklist_database_handlers, chttpd_blacklist_design_handlers ala httpd defines them now. Not bad decision after all.

Copy link
Member

@rnewson rnewson Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think that's getting complicated. I think we just send 404 for all these things.

Copy link
Member

@kxepal kxepal Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good side of @iilyak idea as it also allows to make previously existed resources gone gracefully. It has a difference from regular 404. Not implemented also may have a point for basement of missed features. But that's all about details.

Copy link
Member Author

@janl janl Jul 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kxepal I get the benefits there, but I think giving end-users control over what to return per resource is taking it too far. We can do this smarter :)

Copy link
Member

@kxepal kxepal Jul 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janl If end user decided to black list something, they may (or need in this proposal) define a reason why, right?

We can do this smarter, but smarter solutions takes more time/resources while that's stupid one just works and quite trivial in implementation. We can always improve it later. Especially, after collecting user experience for this blacklist management - may be they wouldn't touch it a lot (actually, they indeed will not) and that decision is good enough.

Copy link
Member Author

@janl janl Jul 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kxepal Well, configuration is part of what the end-user sees, so we can’t just fix it later. We need to consider an upgrade plan and everything. I’d like to avoid doing that and I don’t mind spending another day of nailing this one shut so we can forget about it.

But you make a good point, I’ll keep thinking about this.

Copy link
Member

@kxepal kxepal Jul 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played a bit with your proposal. One more point against lists: it's hard to operate with them via API.
You cannot simple remove one endpoint from blacklist: you need to GET /_config/chttpd/httpd_global_handler_blacklist, decode, pop element, encode, PUT it back. Same story if you need to add something to blacklist. Compare it with a single PUT/DELETE /_config/chttpd_global_handlers_blacklist/_uuids request, which both are atomic.

Simplicity of using that blacklist via API also matters, imho.

Copy link
Member Author

@janl janl Jul 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this. Thanks, I’ll wrap things around.

{ok, Blacklist} = couch_util:parse_term(BlacklistConfig),
Copy link
Member

@kxepal kxepal Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll get badmatch with the default value here:

([email protected])2> BlacklistConfig = config:get("chttpd", "url_handler_blacklist", []).
[]
([email protected])3> {ok, Blacklist} = couch_util:parse_term(BlacklistConfig).
** exception error: no match of right hand side value {error,{1,erl_parse,["syntax error before: ","'.'"]}}

Key = binary_to_list(KeyBin),
lists:member(Key, Blacklist).
Copy link
Member

@rnewson rnewson Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all a bit expensive per request, how about case config:get_boolean("chttpd", ?b2l(<<"blacklisted.",KeyBin/binary>>))?

Copy link
Member

@kxepal kxepal Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like it. Thought, may be "disable.foo = true" or something else in imperative mood.

Copy link
Member Author

@janl janl Jul 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we could cache the parsed list representation somewhere any only access it here. Or is the lists:member what you are concerned about?

Copy link
Member Author

@janl janl Jul 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A (very) dirty version of this, just to illustrate the idea:

get_blacklist() ->
    case config:get("chttpd", "_url_handler_blacklist", []) of
        [] -> % no cached version
            case config:get("chttpd", "url_handler_blacklist", []) of
                [] -> []; % no config value
                BlacklistConfig -> % we have a config value, let’s parse it once
                    {ok, Blacklist} = couch_util:parse_term(BlacklistConfig),
                    config:set("chttpd", "_url_handler_blacklist", Blacklist),
                    Blacklist
            end;
        Blacklist1 -> Blacklist1 % we already have the parsed list cached
    end.

is_disabled(KeyBin) ->
    Blacklist = get_blacklist(),
    Key = ?b2l(KeyBin),
    lists:member(Key, Blacklist).

Of course, we’d need a thing that invalidates the “cache”.

I’m abusing the config system here as a cache, we obviously don’t want to do that for real. I’m not too familiar with chttpd yet, would there be an obvious location for this?

Copy link
Member

@rnewson rnewson Jul 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mochiglobal would be a better spot for a cache, but I think we should avoid additional caching.

Copy link
Member

@kxepal kxepal Jul 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with avoid. Cache invalidation is one of the hardest CS problems (:
Also, you probably mean config:set("chttpd", "_url_handler_blacklist", Blacklist, false) - there is no need to have that cache to be persistent.


ensure_enabled(Key) ->
case is_disabled(Key) of
true ->
throw("The URL endpoint " ++ Key ++ "has been disabled.");
Copy link
Member

@rnewson rnewson Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw({not_found, "The URL to send a 404?

Copy link
Member

@kxepal kxepal Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor

@iilyak iilyak Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please select different value for error so it matches one of the clauses here:
https://github.com/apache/couchdb-chttpd/blob/master/src/chttpd.erl#L715
Possible options are:

Copy link
Member

@rnewson rnewson Jul 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was aiming for;

error_info({not_found, Reason}) ->
{404, <<"not_found">>, Reason};

false ->
ok
end.

collect(Func, Args) ->
Results = do_apply(Func, Args, [ignore_providers]),
[HandlerFun || HandlerFun <- Results, HandlerFun /= no_match].
Expand Down