revert HTTP return code change for user API, add tests #11331
Conversation
| @@ -660,8 +660,6 @@ 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)) { | |||
| result = Flow::Continue; | |||
There was a problem hiding this comment.
I actually do not get why this is not equivalent ?
There was a problem hiding this comment.
Probably what is missing there is the extra check for vc->databaseAuthLevel() != auth::Level::NONE
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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))
|
Tests blue |
…-api-user-return-code
| // 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 && |
There was a problem hiding this comment.
bit unnecessary to do this check then is it?
…ql-functions * origin/devel: Bug fix/schema validation return code (#11341) Fix explainer output when restricting collections (#11338) remove tabstops Improve endpoint handling. (#11236) revert HTTP return code change for user API, add tests (#11331) remove unused header files (#11320) Feature/aql subquery execution block impl execute implementation batch sub queries (#11318) Fix isAdminUser. (#11326)
Scope & Purpose
Change HTTP return code back to 401 (from 403), as it is the case in 3.6 and before when calling
/_api/user/<name>with invalid credentials.Testing & Verification
This PR adds tests that were used to verify all changes:
http://172.16.10.101:8080/view/PR/job/arangodb-matrix-pr/9145/