Tool scripts should not use PrivilegedAPI from 'public/v3/privileged-api.js'.
authordewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 24 Apr 2018 17:17:42 +0000 (17:17 +0000)
committerdewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 24 Apr 2018 17:17:42 +0000 (17:17 +0000)
https://bugs.webkit.org/show_bug.cgi?id=184766

Reviewed by Ryosuke Niwa.

For tools, we should not use PrivilegedAPI for tools as current PrivilegedAPI
is used by UI and it is unnecessary to generate CSRF token for tools.
Will post a followup patch that creates a PrivilegedAPI used by tools.
Make a change on TestServer.inject and MockRemoteAPI.inject to allow specifying
BrowserPrivilegedAPI or NodePrivilegedAPI in the test. Currently defaults to
BrowserPrivilegedAPI as this is the test behavior before this change.

* server-tests/resources/common-operations.js: Allow passing type of privileged api
information to TestServer.inject.
* server-tests/resources/test-server.js: Conditionally inject PrivilegedAPI based on
type of privileged api.
(TestServer.prototype.inject):
(TestServer):
* server-tests/tools-buildbot-triggerable-tests.js: Updated 'prepareServerTest' invocation.
* server-tests/tools-os-build-fetcher-tests.js: Updated 'prepareServerTest' invocation.
* server-tests/tools-sync-buildbot-integration-tests.js: Temporarily injecting
BrowserPrivilegedAPI for mocking UploadedFile. The actual script does not rely on
BrowserPrivilegedAPI at all.
(async.createTestGroupWihPatch):
(beforeEach):
* tools/js/privileged-api.js: Added NodePrivilegedAPI
(NodePrivilegedAPI.prototype.sendRequest):
(NodePrivilegedAPI.configure): Configure the slave name and password.
(NodePrivilegedAPI):
* tools/js/v3-models.js: Removed the import of PrivilegedAPI.
* unit-tests/privileged-api-tests.js:. Added unit tests for NodePrivilegedAPI.
* unit-tests/resources/mock-remote-api.js: Conditionally inject PrivilegedAPI based on
the type of privileged api.
(MockRemoteAPI.inject):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@230960 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/server-tests/resources/common-operations.js
Websites/perf.webkit.org/server-tests/resources/test-server.js
Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js
Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js
Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js
Websites/perf.webkit.org/tools/js/privileged-api.js [new file with mode: 0644]
Websites/perf.webkit.org/tools/js/v3-models.js
Websites/perf.webkit.org/unit-tests/privileged-api-tests.js
Websites/perf.webkit.org/unit-tests/resources/mock-remote-api.js

index b8bca0665de0aba71811046b50319cf77c5d25fd..82e2b48140fa35074e826c0706cf056c55b9a5de 100644 (file)
@@ -1,3 +1,40 @@
+2018-04-23  Dewei Zhu  <dewei_zhu@apple.com>
+
+        Tool scripts should not use PrivilegedAPI from 'public/v3/privileged-api.js'.
+        https://bugs.webkit.org/show_bug.cgi?id=184766
+
+        Reviewed by Ryosuke Niwa.
+
+        For tools, we should not use PrivilegedAPI for tools as current PrivilegedAPI
+        is used by UI and it is unnecessary to generate CSRF token for tools.
+        Will post a followup patch that creates a PrivilegedAPI used by tools.
+        Make a change on TestServer.inject and MockRemoteAPI.inject to allow specifying
+        BrowserPrivilegedAPI or NodePrivilegedAPI in the test. Currently defaults to
+        BrowserPrivilegedAPI as this is the test behavior before this change.
+
+        * server-tests/resources/common-operations.js: Allow passing type of privileged api
+        information to TestServer.inject.
+        * server-tests/resources/test-server.js: Conditionally inject PrivilegedAPI based on
+        type of privileged api.
+        (TestServer.prototype.inject):
+        (TestServer):
+        * server-tests/tools-buildbot-triggerable-tests.js: Updated 'prepareServerTest' invocation.
+        * server-tests/tools-os-build-fetcher-tests.js: Updated 'prepareServerTest' invocation.
+        * server-tests/tools-sync-buildbot-integration-tests.js: Temporarily injecting
+        BrowserPrivilegedAPI for mocking UploadedFile. The actual script does not rely on
+        BrowserPrivilegedAPI at all.
+        (async.createTestGroupWihPatch):
+        (beforeEach):
+        * tools/js/privileged-api.js: Added NodePrivilegedAPI
+        (NodePrivilegedAPI.prototype.sendRequest):
+        (NodePrivilegedAPI.configure): Configure the slave name and password.
+        (NodePrivilegedAPI):
+        * tools/js/v3-models.js: Removed the import of PrivilegedAPI.
+        * unit-tests/privileged-api-tests.js:. Added unit tests for NodePrivilegedAPI.
+        * unit-tests/resources/mock-remote-api.js: Conditionally inject PrivilegedAPI based on
+        the type of privileged api.
+        (MockRemoteAPI.inject):
+
 2018-04-23  Dewei Zhu  <dewei_zhu@apple.com>
 
         Revision information returned by querying measurement set api with analysis task id should contain commit order.
index 7c1f34d6b0e764ca8a8dd908b44f832c047a08bd..e1463cafbd18f94e866d459ca05b5d6e3cd985a8 100644 (file)
@@ -17,15 +17,21 @@ function addSlaveForReport(report)
     });
 }
 
-function prepareServerTest(test)
+function prepareServerTest(test, privilegedAPIType='browser')
 {
     test.timeout(5000);
-    TestServer.inject();
+    TestServer.inject(privilegedAPIType);
 
-    beforeEach(function () {
+    beforeEach(async function () {
+        const database = TestServer.database();
         if (typeof(MockData) != 'undefined')
             MockData.resetV3Models();
-        TestServer.database().connect({keepAlive: true});
+        await database.connect({keepAlive: true});
+        if (privilegedAPIType === 'browser')
+            return;
+        const entry = await TestServer.database().selectFirstRow('build_slaves', {name: 'test'});
+        if (!entry)
+            await addSlaveForReport({slaveName: 'test', slavePassword: 'password'});
     });
 
     afterEach(function () {
index a76d194b549059daad3eff086b8025a8ebb43aa4..a8ec0edf1ea5e5ed609d9e20310433b1e61e361b 100644 (file)
@@ -1,13 +1,15 @@
 'use strict';
 
-let assert = require('assert');
-let childProcess = require('child_process');
-let fs = require('fs');
-let path = require('path');
+const assert = require('assert');
+const childProcess = require('child_process');
+const fs = require('fs');
+const path = require('path');
 
-let Config = require('../../tools/js/config.js');
-let Database = require('../../tools/js/database.js');
-let RemoteAPI = require('../../tools/js/remote.js').RemoteAPI;
+const Config = require('../../tools/js/config.js');
+const Database = require('../../tools/js/database.js');
+const RemoteAPI = require('../../tools/js/remote.js').RemoteAPI;
+const BrowserPrivilegedAPI = require('../../public/v3/privileged-api.js').PrivilegedAPI;
+const NodePrivilegedAPI = require('../../tools/js/privileged-api').PrivilegedAPI;
 
 class TestServer {
     constructor()
@@ -231,15 +233,18 @@ class TestServer {
         resolve();
     }
 
-    inject()
+    inject(privilegedAPIType='browser')
     {
-        let self = this;
+        console.assert(privilegedAPIType === 'browser' || privilegedAPIType === 'node');
+        const useNodePrivilegedAPI = privilegedAPIType === 'node';
+        const self = this;
         before(function () {
             this.timeout(10000);
             return self.start();
         });
 
         let originalRemote;
+        let originalPrivilegedAPI;
 
         beforeEach(function () {
             this.timeout(10000);
@@ -249,7 +254,10 @@ class TestServer {
             global.RemoteAPI = self._remote;
             self._remote.clearCookies();
 
-            if (global.PrivilegedAPI) {
+            originalPrivilegedAPI = global.PrivilegedAPI;
+            global.PrivilegedAPI = useNodePrivilegedAPI ? NodePrivilegedAPI : BrowserPrivilegedAPI;
+
+            if (!useNodePrivilegedAPI) {
                 global.PrivilegedAPI._token = null;
                 global.PrivilegedAPI._expiration = null;
             }
@@ -258,6 +266,7 @@ class TestServer {
         after(function () {
             this.timeout(10000);
             global.RemoteAPI = originalRemote;
+            global.PrivilegedAPI = originalPrivilegedAPI;
             return self.stop();
         });
     }
index 355ee3c001823193f646c468eacbab04b90ffe43..9c3cab444b903977fbf9f148b9481b3c5f5474c2 100644 (file)
@@ -17,7 +17,7 @@ function assertRequestAndResolve(request, method, url, content)
 }
 
 describe('BuildbotTriggerable', function () {
-    prepareServerTest(this);
+    prepareServerTest(this, 'node');
 
     beforeEach(function () {
         MockData.resetV3Models();
index efb11c0dc3b598119cfbf6f760e3083be81920df..d65c6ce1d8c2c0ecbd925fa20b4f03d6813d7f91 100644 (file)
@@ -13,7 +13,7 @@ const MockLogger = require('./resources/mock-logger.js').MockLogger;
 
 describe('OSBuildFetcher', function() {
     this.timeout(5000);
-    TestServer.inject();
+    TestServer.inject('node');
 
     beforeEach(function () {
         MockRemoteAPI.reset('http://build.webkit.org');
index 6be2c80bed1bcdd22579063f269ec6936cc6e2ad..e61258266cd8e0c54d4c036e54d9b7d10f86e15e 100644 (file)
@@ -12,6 +12,7 @@ const TestServer = require('./resources/test-server.js');
 const TemporaryFile = require('./resources/temporary-file.js').TemporaryFile;
 const addSlaveForReport = require('./resources/common-operations.js').addSlaveForReport;
 const prepareServerTest = require('./resources/common-operations.js').prepareServerTest;
+const BrowserPrivilegedAPI = require('../public/v3/privileged-api.js').PrivilegedAPI;
 
 const configWithOneTesterTwoBuilders = {
     triggerableName: 'build-webkit',
@@ -177,21 +178,24 @@ function createTestGroup(task_name='custom task') {
     });
 }
 
-function createTestGroupWihPatch()
+async function createTestGroupWihPatch()
 {
-    return TemporaryFile.makeTemporaryFile('patch.dat', 'patch file').then((patchFile) => {
-        return UploadedFile.uploadFile(patchFile);
-    }).then((patchFile) => {
-        const someTest = Test.findById(MockData.someTestId());
-        const webkit = Repository.findById(MockData.webkitRepositoryId());
-        const set1 = new CustomCommitSet;
-        set1.setRevisionForRepository(webkit, '191622', patchFile);
-        const set2 = new CustomCommitSet;
-        set2.setRevisionForRepository(webkit, '191622');
-        return TestGroup.createWithTask('custom task', Platform.findById(MockData.somePlatformId()), someTest, 'some group', 2, [set1, set2]);
-    }).then((task) => {
-        return TestGroup.findAllByTask(task.id())[0];
-    })
+    const patchFile = await TemporaryFile.makeTemporaryFile('patch.dat', 'patch file');
+    const originalPrivilegedAPI = global.PrivilegedAPI;
+    BrowserPrivilegedAPI._token = null;
+    global.PrivilegedAPI = BrowserPrivilegedAPI;
+    const uploadedPatchFile = await UploadedFile.uploadFile(patchFile);
+    global.PrivilegedAPI = originalPrivilegedAPI;
+
+    const someTest = Test.findById(MockData.someTestId());
+    const webkit = Repository.findById(MockData.webkitRepositoryId());
+    const set1 = new CustomCommitSet;
+    set1.setRevisionForRepository(webkit, '191622', uploadedPatchFile);
+    const set2 = new CustomCommitSet;
+    set2.setRevisionForRepository(webkit, '191622');
+    const task = await TestGroup.createWithTask('custom task', Platform.findById(MockData.somePlatformId()), someTest, 'some group', 2, [set1, set2]);
+
+    return TestGroup.findAllByTask(task.id())[0];
 }
 
 function createTestGroupWihOwnedCommit()
@@ -227,11 +231,12 @@ function uploadRoot(buildRequestId, buildNumber, repositoryList = ["WebKit"], bu
 }
 
 describe('sync-buildbot', function () {
-    prepareServerTest(this);
+    prepareServerTest(this, 'node');
     TemporaryFile.inject();
 
     beforeEach(() => {
         MockRemoteAPI.reset('http://build.webkit.org');
+        PrivilegedAPI.configure('test', 'password');
     });
 
 
diff --git a/Websites/perf.webkit.org/tools/js/privileged-api.js b/Websites/perf.webkit.org/tools/js/privileged-api.js
new file mode 100644 (file)
index 0000000..4d31f2d
--- /dev/null
@@ -0,0 +1,26 @@
+"use strict";
+
+class NodePrivilegedAPI {
+
+    static sendRequest(path, data, options = {useFormData: false})
+    {
+        const clonedData = {slaveName: this._slaveName, slavePassword: this._slavePassword};
+        for (const key in data)
+            clonedData[key] = data[key];
+
+        const fullPath = '/privileged-api/' + path;
+
+        return options.useFormData
+            ? RemoteAPI.postFormDataWithStatus(fullPath, clonedData, options)
+            : RemoteAPI.postJSONWithStatus(fullPath, clonedData, options);
+    }
+
+    static configure(slaveName, slavePassword)
+    {
+        this._slaveName = slaveName;
+        this._slavePassword = slavePassword;
+    }
+}
+
+if (typeof module != 'undefined')
+    module.exports.PrivilegedAPI = NodePrivilegedAPI;
\ No newline at end of file
index 01d23e2da8329aa86ecb43d717918013c158315d..6cfd83b66dc71482c398f1be737cf6097c4a970a 100644 (file)
@@ -34,7 +34,6 @@ importFromV3('models/triggerable.js', 'Triggerable');
 importFromV3('models/triggerable.js', 'TriggerableRepositoryGroup');
 importFromV3('models/uploaded-file.js', 'UploadedFile');
 
-importFromV3('privileged-api.js', 'PrivilegedAPI');
 importFromV3('instrumentation.js', 'Instrumentation');
 importFromV3('lazily-evaluated-function.js', 'LazilyEvaluatedFunction');
 importFromV3('commit-set-range-bisector.js', 'CommitSetRangeBisector');
index 5095e595941dc657eab59121934b01b120a5b314..9da4b89827e707499f0e098586227bd6ee03b3a4 100644 (file)
@@ -5,7 +5,7 @@ const assert = require('assert');
 const MockRemoteAPI = require('./resources/mock-remote-api.js').MockRemoteAPI;
 require('../tools/js/v3-models.js');
 
-describe('PrivilegedAPI', () => {
+describe('BrowserPrivilegedAPI', () => {
     let requests = MockRemoteAPI.inject();
 
     beforeEach(() => {
@@ -50,7 +50,7 @@ describe('PrivilegedAPI', () => {
             });
         });
     });
-    
+
     describe('sendRequest', () => {
 
         it('should generate a new token if no token had been fetched', () => {
@@ -167,3 +167,26 @@ describe('PrivilegedAPI', () => {
     });
 
 });
+
+describe('NodePrivilegedAPI', () => {
+    let requests = MockRemoteAPI.inject(null, 'node');
+    beforeEach(() => {
+        PrivilegedAPI.configure('slave_name', 'password');
+    });
+
+    describe('sendRequest', () => {
+        it('should post slave name and password in data', async () => {
+            const request = PrivilegedAPI.sendRequest('test', {foo: 'bar'});
+
+            assert.equal(requests.length, 1);
+            assert.equal(requests[0].url, '/privileged-api/test');
+            assert.equal(requests[0].method, 'POST');
+            assert.deepEqual(requests[0].data,  {foo: 'bar', slaveName: 'slave_name', slavePassword: 'password'});
+
+            requests[0].resolve({test: 'success'});
+            const result = await request;
+            assert.deepEqual(result, {test: 'success'});
+        });
+    });
+
+});
index e7b055c84a4d0fdc7310ef3676e88fa16a019e70..dc12782f939597b1d41e594c706e083ab6a94515 100644 (file)
@@ -1,3 +1,6 @@
+const BrowserPrivilegedAPI = require('../../public/v3/privileged-api.js').PrivilegedAPI;
+const NodePrivilegedAPI = require('../../tools/js/privileged-api').PrivilegedAPI;
+
 var MockRemoteAPI = {
     url: function (path)
     {
@@ -57,18 +60,24 @@ var MockRemoteAPI = {
         }
         return this._waitingPromise;
     },
-    inject: function (urlPrefix)
+    inject: function (urlPrefix, privilegedAPIType='browser')
     {
-        var originalRemoteAPI = global.RemoteAPI;
+        console.assert(privilegedAPIType === 'browser' || privilegedAPIType === 'node');
+        let originalRemoteAPI = global.RemoteAPI;
+        let originalPrivilegedAPI = global.PrivilegedAPI;
+        const PrivilegedAPI = privilegedAPIType === 'node' ? NodePrivilegedAPI: BrowserPrivilegedAPI;
 
-        beforeEach(function () {
+        beforeEach(() => {
             MockRemoteAPI.reset(urlPrefix);
             originalRemoteAPI = global.RemoteAPI;
             global.RemoteAPI = MockRemoteAPI;
+            originalPrivilegedAPI = global.PrivilegedAPI;
+            global.PrivilegedAPI = PrivilegedAPI;
         });
 
-        afterEach(function () {        
+        afterEach(() => {
             global.RemoteAPI = originalRemoteAPI;
+            global.PrivilegedAPI = originalPrivilegedAPI;
         });
 
         return MockRemoteAPI.requests;