8000 revert HTTP return code change for user API, add tests by jsteemann · Pull Request #11331 · arangodb/arangodb · GitHub
[go: up one dir, main page]
Skip to content

revert HTTP return code change for user API, add tests #11331

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Mar 25, 2020
Merged

revert HTTP return code change for user API, add tests #11331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2b7558c
revert HTTP return code change for user API, add tests
jsteemann Mar 24, 2020
9009fe8
Merge branch 'devel' of github.com:arangodb/arangodb into bug-fix/fix…
jsteemann Mar 25, 2020
4740069
remove commented line
jsteemann Mar 25, 2020
bf4ffef
simplification
jsteemann Mar 25, 2020
c03c95e
Merge branch 'devel' of github.com:arangodb/arangodb into bug-fix/fix…
jsteemann Mar 25, 2020
bd03f0a
honor review comment
jsteemann Mar 25, 2020
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
8 changes: 4 additions & 4 deletions arangod/GeneralServer/CommTask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,8 +606,9 @@ CommTask::Flow CommTask::canAccessPath(auth::TokenCache::Entry const& token,

VocbaseContext* vc = static_cast<VocbaseContext*>(req.requestContext());
TRI_ASSERT(vc != nullptr);
// deny access to database with NONE, (except /_api/user)
if (vc->databaseAuthLevel() == auth::Level::NONE/* && !StringUtils::isPrefix(path, ApiUser)*/) {
// deny access to database with NONE
if (result == Flow::Continue &&
Copy link
Contributor

Choose a reason for hiding this comment

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

bit unnecessary to do this check then is it?

vc->databaseAuthLevel() == auth::Level::NONE) {
result = Flow::Abort;
LOG_TOPIC("0898a", TRACE, Logger::AUTHORIZATION) << "Access forbidden to " << path;
}
Expand All @@ -629,7 +630,6 @@ CommTask::Flow CommTask::canAccessPath(auth::TokenCache::Entry const& token,

if (result == Flow::Abort && _auth->authenticationSystemOnly()) {
// authentication required, but only for /_api, /_admin etc.

if (!path.empty()) {
// check if path starts with /_
// or path begins with /
Expand Down Expand Up @@ -660,7 +660,7 @@ CommTask::Flow CommTask::canAccessPath(auth::TokenCache::Entry const& token,
// `/_api/users/<name>` to check their passwords
result = Flow::Continue;
vc->forceReadOnly();
} else if (StringUtils::isPrefix(path, ApiUser)) {
} else if (userAuthenticated && StringUtils::isPrefix(path, ApiUser)) {
result = Flow::Continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually do not get why this is not equivalent ?

Copy link
Contributor Author
@jsteemann jsteemann Mar 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Probably what is missing there is the extra check for vc->databaseAuthLevel() != auth::Level::NONE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I debugged it and the difference is that the broken code sets the status from Abort to Continue via the now-deleted code:

       } else if (StringUtils::isPrefix(path, ApiUser)) {
         result = Flow::Continue;
       }

The correct behavior (as in 3.6 and the code of this PR) is to not make this change, and leave the status on Abort as decided here:

Flow result = userAuthenticated ? Flow::Continue : Flow::Abort;

// deny access to database with NONE
if (vc->databaseAuthLevel() == auth::Level::NONE && !StringUtils::isPrefix(path, ApiUser)) {
  result = Flow::Abort;
}

For the particular problem, the initial status is already Abort, and the correct code does not change it to anything else from there.
The broken code determined the initial status as Abort, but then overwrote it with Continue in the final else if that this PR deletes.

Copy link
Contributor
@graetzer graetzer Mar 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

oh ok, so the problem is the user is not authenticated at all, why not just check for that later then ?
else if (userAuthenticated && StringUtils::isPrefix(path, ApiUser))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, did.

}
}
Expand Down
118 changes: 115 additions & 3 deletions tests/js/client/authentication/auth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*jshint globalstrict:false, strict:false */
/*global fail, assertTrue */
/*global fail, assertTrue, assertEqual */

////////////////////////////////////////////////////////////////////////////////
/// @brief test the authentication
Expand Down Expand Up @@ -58,7 +58,7 @@ function AuthSuite() {
////////////////////////////////////////////////////////////////////////////////

setUp: function () {
arango.reconnect(arango.getEndpoint(), db._name(), "root", "");
arango.reconnect(arango.getEndpoint(), '_system', "root", "");

try {
users.remove(user);
Expand All @@ -72,13 +72,126 @@ function AuthSuite() {
////////////////////////////////////////////////////////////////////////////////

tearDown: function () {
arango.reconnect(arango.getEndpoint(), '_system', "root", "");
try {
users.remove(user);
}
catch (err) {
}
},

testApiUserRW: function () {
users.save(user, "foobar");
users.grantDatabase(user, '_system');
users.grantCollection(user, '_system', "*");
users.reload();

arango.reconnect(arango.getEndpoint(), '_system', user, "foobar");

let result = arango.GET('/_api/user/' + encodeURIComponent(user));
assertEqual(user, result.user);
},

testApiUserRO: function () {
users.save(user, "foobar");
users.grantDatabase(user, '_system', 'ro');
users.reload();

arango.reconnect(arango.getEndpoint(), '_system', user, "foobar");

let result = arango.GET('/_api/user/' + encodeURIComponent(user));
assertEqual(user, result.user);
},

testApiUserWrongCredentials: function () {
users.save(user, "foobar");
users.grantDatabase(user, '_system', 'ro');
users.reload();

arango.reconnect(arango.getEndpoint(), '_system', user, "foobar");
users.update(user, "foobar!!!!!");

let result = arango.GET('/_api/user/' + encodeURIComponent(user));
assertEqual(401, result.code);
},

testApiUserNone: function () {
users.save(user, "foobar");
users.grantDatabase(user, '_system', 'rw');
users.reload();

arango.reconnect(arango.getEndpoint(), '_system', user, "foobar");
users.revokeDatabase(user, '_system');
try {
users.reload();
} catch (err) {
}

let result = arango.GET('/_api/user/' + encodeURIComponent(user));
assertEqual(user, result.user);
},

testApiUserDeleted: function () {
users.save(user, "foobar");
users.grantDatabase(user, '_system', 'rw');
users.reload();

arango.reconnect(arango.getEndpoint(), '_system', user, "foobar");
users.remove(user);
try {
users.reload();
} catch (err) {
}

let result = arango.GET('/_api/user/' + encodeURIComponent(user));
assertEqual(401, result.code);
},

testApiNonExistingUserRW: function () {
users.save(user, "foobar");
users.grantDatabase(user, '_system');
users.grantCollection(user, '_system', "*");
users.reload();

arango.reconnect(arango.getEndpoint(), '_system', user, "foobar");

let result = arango.GET('/_api/user/noone');
assertEqual(404, result.code);
},

testApiNonExistingUserRO: function () {
users.save(user, "foobar");
users.grantDatabase(user, '_system', 'ro');
users.reload();

arango.reconnect(arango.getEndpoint(), '_system', user, "foobar");

let result = arango.GET('/_api/user/noone');
assertEqual(403, result.code);
},

testApiNonExistingUserNone: function () {
users.save(user, "foobar");
users.grantDatabase(user, '_system', 'rw');
users.reload();

try {
// connection will fail, but it will effectively set the username
// for all follow-up requests (which is what we need)
arango.reconnect(arango.getEndpoint(), '_system', user, "foobar");
} catch (err) {
}

users.revokeDatabase(user, '_system');
try {
users.reload();
} catch (err) {
}

let result = arango.GET('/_api/user/noone');
assertEqual(403, result.code);
},

////////////////////////////////////////////////////////////////////////////////
/// @brief test creating a new user
////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -573,7 +686,6 @@ function AuthSuite() {
db._dropDatabase("other");
}
}

};
}

Expand Down
0