Secure v2 user responses and access control

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.

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,

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,
     );
 

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,
+};

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();
+      });
+  });
+});

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}`);
             }