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: https://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 b8bca06..82e2b48 100644 (file)
@@ -1,5 +1,42 @@
 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.
         https://bugs.webkit.org/show_bug.cgi?id=184902
 
index 7c1f34d..e1463ca 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 a76d194..a8ec0ed 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 355ee3c..9c3cab4 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 efb11c0..d65c6ce 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 6be2c80..e612582 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 01d23e2..6cfd83b 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 5095e59..9da4b89 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 e7b055c..dc12782 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;