Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 20 additions & 3 deletions api/parts/mgmt/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,26 @@ var crypto = require('crypto');
* @returns {boolean} true
**/
usersApi.getCurrentUser = function(params) {
delete params.member.password;

common.returnOutput(params, params.member);
//Answer with a copy, so removing fields here cannot affect the member object the rest
//of the request still uses.
var member = Object.assign({}, params.member);

//The api_key is not scoped: it grants everything its owner can do, on every app they
//can reach. getUserById and getAllUsers already project it away, and the member event
//payloads delete it, so this was the one read path that handed it out. It matters here
//because a request can be authorized by a token rather than by the key itself, and a
//token can be limited to a single app, so returning the key would let a token that is
//limited to one app produce a credential that is limited to nothing. Anyone who needs
//their own key can still read it from the dashboard's /api-key route.
delete member.password;
delete member.api_key;
//Same reasoning for the second factor, whose secret lives on the member document: a
//response carrying both the key and the secret behind the factor protecting it protects
//nothing. The whole object goes, since this endpoint has no consumer that needs it and
//the enabled flag is available from the user listing.
delete member.two_factor_auth;

common.returnOutput(params, member);
return true;
};

Expand Down
26 changes: 25 additions & 1 deletion api/utils/requestProcessor.js
Original file line number Diff line number Diff line change
Expand Up @@ -1607,7 +1607,31 @@ const processRequest = (params) => {
validateUserForGlobalAdmin(params, countlyApi.mgmt.users.getAllUsers);
break;
case 'me':
validateUserForMgmtReadAPI(countlyApi.mgmt.users.getCurrentUser, params);
validateUserForMgmtReadAPI(function() {
//This endpoint answers with the caller's own account and belongs to no
//application, so a token that was deliberately limited to some
//applications has no business reading it. Without this an app limited
//token still reached account level data, because the app restriction in
//verify_token is only compared when the request itself names an app.
var authToken = params.qstring.auth_token || params.req.headers["countly-token"] || "";
if (!authToken) {
return countlyApi.mgmt.users.getCurrentUser(params);
}
authorize.read({
db: common.db,
token: authToken,
callback: function(tokenErr, tokenData) {
//save() stores app as "" when unrestricted and as an array
//otherwise, so a non empty length is what marks a restriction
if (tokenData && tokenData.app && tokenData.app.length) {
common.returnMessage(params, 401, 'Token is restricted to specific applications');
return false;
}
return countlyApi.mgmt.users.getCurrentUser(params);
}
});
return true;
}, params);
break;
case 'id':
validateUserForGlobalAdmin(params, countlyApi.mgmt.users.getUserById);
Expand Down
20 changes: 20 additions & 0 deletions test/2.api/02.read.user.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,26 @@ describe('Initial reading', function() {
});
});
});
describe('Reading users /me does not return credentials', function() {
it('should omit api_key and the second factor secret', function(done) {
request
.get('/o/users/me?api_key=' + API_KEY_ADMIN)
.expect(200)
.end(function(err, res) {
if (err) {
return done(err);
}
var ob = JSON.parse(res.text);
// the account's own fields are still there
ob.should.have.property('email', testUtils.email);
// but nothing that authenticates as this account
ob.should.not.have.property('api_key');
ob.should.not.have.property('password');
ob.should.not.have.property('two_factor_auth');
done();
});
});
});
describe('Reading users /all', function() {
it('should return information', function(done) {
request
Expand Down
41 changes: 41 additions & 0 deletions test/2.api/14.authorize.token.js
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,47 @@ describe('Testing global admin user token', function() {
*/
});

describe('Token restricted to an application cannot read account information', function() {
var appScopedToken = "";

it('creating a token restricted to one application', function(done) {
authorize.save({
db: testUtils.db,
multi: true,
owner: testowner,
app: [APP_ID],
callback: function(err, token) {
if (err) {
return done(err);
}
if (!token) {
return done("token not created");
}
appScopedToken = token;
done();
}
});
});

it('should refuse /o/users/me, which belongs to no application', function(done) {
request
.get('/o/users/me?auth_token=' + appScopedToken)
.expect(401)
.end(function(err) {
if (err) {
return done(err);
}
done();
});
});

it('cleaning up the app restricted token', function(done) {
testUtils.db.collection("auth_tokens").remove({_id: appScopedToken}, function() {
done();
});
});
});

describe('Creating token to allow only paths under /o/users/', function() {
it('creating token for user', function(done) {
authorize.save({
Expand Down
Loading