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

Check if db exists in /db/_ensure_full_commit call #1588

Merged
merged 1 commit into from Aug 30, 2018

Conversation

Projects
None yet
2 participants
@eiri
Member

eiri commented Aug 29, 2018

Overview

We removed a security call in do_db_req in #1246 to avoid duplicate authorization check and as a result there are now no db validation in noop call /db/_ensure_full_commit. This makes it always return a success code, even for the missing databases.

This fix places the security check directly in _ensure_full_commit call and adds eunit tests for a good measure.

Testing recommendations

make eunit apps=chttpd suites=chttpd_db_test for isolated tests

Related Issues or Pull Requests

Closes #1585

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;
chttpd:validate_ctype(Req, "application/json"),
fabric:get_security(DbName, [{user_ctx, Ctx}]),

This comment has been minimized.

@iilyak

iilyak Aug 29, 2018

Contributor

I think there is a need for a comment here to describe why the call is needed and the reason for fabric:get_security/2 to be used for database existence check (get_security uses local shards while fabric:get_db_info for example uses non local shards as well)

This comment has been minimized.

@eiri

eiri Aug 29, 2018

Member

I'm using get_security here because 1) it is what old code was using 2) get security calls fabric_util:get_db that actually does open remote shards, but gives priority to local ones, so it allows to avoid round-trip when possible 3) it presumably also confirms user to be reader, that what old code was saying, but I don't see it in fabric, tbh.

I can drop a comment here or I can switch to fabric:get_db_info if you think that'd be less subtle.

@eiri

This comment has been minimized.

Member

eiri commented Aug 29, 2018

@iilyak ok, updated with the comment. does it look sufficient for you?

@iilyak

iilyak approved these changes Aug 30, 2018

+1

@eiri eiri force-pushed the cloudant:fix/ensure_full_commit-database-check branch from 0db097b to 171d135 Aug 30, 2018

Check if db exists in /db/_ensure_full_commit call
We removed a security call in `do_db_req` to avoid
a duplicate authorization check and as a result
there are now no db validation in noop call
`/db/_ensure_full_commit`. This makes it always
return a success code, even for missing databases.

This fix places the security check directly
in _ensure_full_commit call and adds eunit tests
for a good measure.

@eiri eiri force-pushed the cloudant:fix/ensure_full_commit-database-check branch from 171d135 to 5c0548e Aug 30, 2018

@eiri eiri merged commit b2b6988 into apache:master Aug 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eiri eiri deleted the cloudant:fix/ensure_full_commit-database-check branch Aug 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment