diff --git a/PR_DRAFT.md b/PR_DRAFT.md new file mode 100644 index 0000000000..db1b57d771 --- /dev/null +++ b/PR_DRAFT.md @@ -0,0 +1,23 @@ +## Summary + +- Restrict v2 user list access to admins and enforce self-or-admin checks on per-user reads and the legacy per-user password update route. +- Add centralized user response sanitization for v2 user responses, removing password data and confidential extension fields such as raw bounded-cluster tokens and SSH private keys while preserving safe metadata. +- Add focused regression coverage for access control and response redaction behavior. +- Keep the job-list user filter usable for non-admin portal sessions when the all-user API is unavailable. + +## Validation + +- `node -c src/rest-server/src/utils/userResponse.js` +- `node -c src/rest-server/src/controllers/v2/user.js` +- `node -c src/rest-server/src/routes/v2/user.js` +- `node -c src/rest-server/test/userResponseSecurity.js` + +Focused mocha/lint commands were attempted but local dependencies are not installed in this worktree: + +- `npm run mocha -- --grep "user response security"` -> `sh: 1: mocha: not found` +- `npm run lint` -> `sh: 1: eslint: not found` + +## Notes + +- Existing `/api/v2/user` and `/api/v2/users` aliases are preserved. +- Sanitization is applied at response construction; stored user extension data is not modified. diff --git a/src/rest-server/src/controllers/v2/user.js b/src/rest-server/src/controllers/v2/user.js index 9f87bf72e5..af3f429a18 100644 --- a/src/rest-server/src/controllers/v2/user.js +++ b/src/rest-server/src/controllers/v2/user.js @@ -24,6 +24,20 @@ const logger = require('@pai/config/logger'); const groupModel = require('@pai/models/v2/group'); const vcModel = require('@pai/models/v2/virtual-cluster'); const tokenModel = require('@pai/models/token'); +const { sanitizeUser, sanitizeUserList } = require('@pai/utils/userResponse'); + +const checkSelfOrAdmin = async (req, _, next) => { + if (req.user.admin || req.user.username === req.params.username) { + return next(); + } + return next( + createError( + 'Forbidden', + 'ForbiddenUserError', + `Non-admin is not allow to do this operation.`, + ), + ); +}; const getUserVCs = async (username) => { const userInfo = await userModel.getUser(username); @@ -45,8 +59,7 @@ const getUser = async (req, res, next) => { userInfo.storageConfig = await groupModel.getStorageConfigsWithGroupInfo( groupItems, ); - delete userInfo.password; - return res.status(200).json(userInfo); + return res.status(200).json(sanitizeUser(userInfo)); } catch (error) { if (error.status === 404) { return next( @@ -85,11 +98,10 @@ const getAllUser = async (req, res, next) => { userItem.storageConfig = await groupModel.getStorageConfigsWithGroupInfo( groupItems, ); - delete userItem.password; return userItem; }), ); - return res.status(200).json(retUserList); + return res.status(200).json(sanitizeUserList(retUserList)); } catch (error) { return next(createError.unknown(error)); } @@ -783,6 +795,7 @@ const deleteUser = async (req, res, next) => { // module exports module.exports = { checkSelf, + checkSelfOrAdmin, getUser, getAllUser, createUserIfUserNotExist, diff --git a/src/rest-server/src/routes/v2/user.js b/src/rest-server/src/routes/v2/user.js index 6aa2e298d5..09c50c98f3 100644 --- a/src/rest-server/src/routes/v2/user.js +++ b/src/rest-server/src/routes/v2/user.js @@ -28,12 +28,12 @@ const router = new express.Router(); router .route('/:username/') /** Get /api/v2/users/:username */ - .get(token.check, userController.getUser); + .get(token.check, userController.checkSelfOrAdmin, userController.getUser); router .route('/') /** Get /api/v2/users */ - .get(token.check, userController.getAllUser); + .get(token.check, token.checkAdmin, userController.getAllUser); /** Legacy API and will be deprecated in the future. Please use put /api/v2/users */ router @@ -133,6 +133,7 @@ if (authnConfig.authnMethod === 'basic') { .put( token.checkNotApplication, param.validate(userInputSchema.userPasswordUpdateInputSchema), + userController.checkSelfOrAdmin, userController.updateUserPassword, ); diff --git a/src/rest-server/src/utils/userResponse.js b/src/rest-server/src/utils/userResponse.js new file mode 100644 index 0000000000..0650c295a4 --- /dev/null +++ b/src/rest-server/src/utils/userResponse.js @@ -0,0 +1,64 @@ +// Copyright (c) Microsoft Corporation +// All rights reserved. +// +// MIT License +// +// Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated +// documentation files (the "Software"), to deal in the Software without restriction, including without limitation +// the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and +// to permit persons to whom the Software is furnished to do so, subject to the following conditions: +// The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED *AS IS*, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING +// BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND +// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + +const sensitiveFieldPattern = /(^password$|token|secret|private|credential|connectionstring|sas)$/i; +const privateKeyPattern = /-----BEGIN [A-Z ]*PRIVATE KEY-----/; + +const clone = (value) => JSON.parse(JSON.stringify(value)); + +const isSensitiveField = (fieldName) => { + return sensitiveFieldPattern.test(fieldName) || fieldName === 'key'; +}; + +const sanitizeValue = (value) => { + if (Array.isArray(value)) { + return value.map(sanitizeValue).filter((item) => item !== undefined); + } + if (value && typeof value === 'object') { + const sanitized = {}; + for (const [key, childValue] of Object.entries(value)) { + if (isSensitiveField(key)) { + continue; + } + const sanitizedChild = sanitizeValue(childValue); + if (sanitizedChild !== undefined) { + sanitized[key] = sanitizedChild; + } + } + return sanitized; + } + if (typeof value === 'string' && privateKeyPattern.test(value)) { + return undefined; + } + return value; +}; + +const sanitizeUser = (userInfo) => { + const sanitized = clone(userInfo); + delete sanitized.password; + if (sanitized.extension) { + sanitized.extension = sanitizeValue(sanitized.extension); + } + return sanitized; +}; + +const sanitizeUserList = (userList) => userList.map(sanitizeUser); + +module.exports = { + sanitizeUser, + sanitizeUserList, +}; diff --git a/src/rest-server/test/userResponseSecurity.js b/src/rest-server/test/userResponseSecurity.js new file mode 100644 index 0000000000..3b2925219b --- /dev/null +++ b/src/rest-server/test/userResponseSecurity.js @@ -0,0 +1,165 @@ +// Copyright (c) Microsoft Corporation +// All rights reserved. +// +// MIT License + +const nockUtils = require('./utils/nock'); +const { sanitizeUser } = require('@pai/utils/userResponse'); + +const userSecretPath = (username) => + `/api/v1/namespaces/pai-user-v2/secrets/${Buffer.from(username).toString('hex')}`; + +const userPayloadWithExtension = (username, extension) => { + const payload = nockUtils.getUserPayload({ username, grouplist: [] }); + payload.data.extension = Buffer.from(JSON.stringify(extension)).toString( + 'base64', + ); + return payload; +}; + +describe('user response security', () => { + afterEach(() => { + if (!nock.isDone()) { + nock.cleanAll(); + } + }); + + it('removes confidential fields while preserving safe extension metadata', () => { + const sanitized = sanitizeUser({ + username: 'alice', + password: 'hashed-password', + extension: { + boundedClusters: { + remote: { + uri: 'https://remote.example', + username: 'alice', + token: 'raw-token', + }, + }, + jobSSH: { + key: 'redaction-test-private-key-placeholder', + pubKey: 'ssh-rsa public', + }, + sshKeys: [{ title: 'laptop', value: 'ssh-rsa public-key' }], + nested: { refreshToken: 'raw-refresh-token', label: 'safe' }, + }, + }); + + global.expect(sanitized).to.not.have.property('password'); + global.expect(sanitized.extension.boundedClusters.remote).to.not.have.property( + 'token', + ); + global.expect(sanitized.extension.boundedClusters.remote.uri).to.equal( + 'https://remote.example', + ); + global.expect(sanitized.extension.boundedClusters.remote.username).to.equal( + 'alice', + ); + global.expect(sanitized.extension.jobSSH).to.not.have.property('key'); + global.expect(sanitized.extension.jobSSH.pubKey).to.equal('ssh-rsa public'); + global.expect(sanitized.extension.sshKeys[0].value).to.equal( + 'ssh-rsa public-key', + ); + global.expect(sanitized.extension.nested).to.not.have.property( + 'refreshToken', + ); + global.expect(sanitized.extension.nested.label).to.equal('safe'); + }); + + it('requires admin for the user list endpoint', (done) => { + const nonAdminToken = nockUtils.registerUserTokenCheck('alice'); + global.chai + .request(global.server) + .get('/api/v2/users') + .set('Authorization', 'Bearer ' + nonAdminToken) + .end((err, res) => { + global.expect(res, 'status code').to.have.status(403); + global.expect(res.body.code, 'response code').equal('ForbiddenUserError'); + done(); + }); + }); + + it('allows self user reads and redacts the response', (done) => { + const validToken = nockUtils.registerUserTokenCheck('alice'); + nock(apiServerRootUri) + .get(userSecretPath('alice')) + .reply( + 200, + userPayloadWithExtension('alice', { + boundedClusters: { + remote: { + uri: 'https://remote.example', + username: 'alice', + token: 'raw-token', + }, + }, + jobSSH: { key: 'private-key', pubKey: 'ssh-rsa public' }, + safe: 'metadata', + }), + ); + + global.chai + .request(global.server) + .get('/api/v2/user/alice') + .set('Authorization', 'Bearer ' + validToken) + .end((err, res) => { + global.expect(res, 'status code').to.have.status(200); + global.expect(res.body).to.not.have.property('password'); + global.expect(res.body.extension.boundedClusters.remote).to.not.have.property( + 'token', + ); + global.expect(res.body.extension.boundedClusters.remote.uri).to.equal( + 'https://remote.example', + ); + global.expect(res.body.extension.jobSSH).to.not.have.property('key'); + global.expect(res.body.extension.jobSSH.pubKey).to.equal('ssh-rsa public'); + global.expect(res.body.extension.safe).to.equal('metadata'); + done(); + }); + }); + + it('allows admin reads of another user', (done) => { + const adminToken = nockUtils.registerAdminTokenCheck('adminX'); + nock(apiServerRootUri) + .get(userSecretPath('bob')) + .reply(200, userPayloadWithExtension('bob', { safe: 'metadata' })); + + global.chai + .request(global.server) + .get('/api/v2/users/bob') + .set('Authorization', 'Bearer ' + adminToken) + .end((err, res) => { + global.expect(res, 'status code').to.have.status(200); + global.expect(res.body.username).to.equal('bob'); + global.expect(res.body.extension.safe).to.equal('metadata'); + done(); + }); + }); + + it('blocks non-admin reads of another user', (done) => { + const nonAdminToken = nockUtils.registerUserTokenCheck('alice'); + global.chai + .request(global.server) + .get('/api/v2/users/bob') + .set('Authorization', 'Bearer ' + nonAdminToken) + .end((err, res) => { + global.expect(res, 'status code').to.have.status(403); + global.expect(res.body.code, 'response code').equal('ForbiddenUserError'); + done(); + }); + }); + + it('blocks non-admin password changes for another user', (done) => { + const nonAdminToken = nockUtils.registerUserTokenCheck('alice'); + global.chai + .request(global.server) + .put('/api/v2/user/bob/password') + .set('Authorization', 'Bearer ' + nonAdminToken) + .send({ oldPassword: 'default_password', newPassword: 'new_password' }) + .end((err, res) => { + global.expect(res, 'status code').to.have.status(403); + global.expect(res.body.code, 'response code').equal('ForbiddenUserError'); + done(); + }); + }); +}); diff --git a/src/webportal/src/app/job/job-view/fabric/JobList/TopBar.jsx b/src/webportal/src/app/job/job-view/fabric/JobList/TopBar.jsx index 9d4ffc8038..5061d972c1 100644 --- a/src/webportal/src/app/job/job-view/fabric/JobList/TopBar.jsx +++ b/src/webportal/src/app/job/job-view/fabric/JobList/TopBar.jsx @@ -117,6 +117,8 @@ function TopBar() { if (data.code === 'UnauthorizedUserError') { alert(data.message); clearToken(); + } else if (data.code === 'ForbiddenUserError') { + setUser({ [cookies.get('user')]: true }); } else { throw new Error(`Failed to fetch user info: ${data.message}`); }