Added cookie forwarding (T88016) #272

Pchelolo · 2015-07-02T10:12:13Z

Cookies set on original request are forwarded to parsoid and mediawiki endpoints to let user auth and read content.

Two tests added, verifying that cookies are actually forwarded - one for parsoid and one for mediawiki api.

This solves only a part of the problem, after this fix user can read content from private wiki, but we still do not have a mechanism to forbid read of content already stored in restbase.

Added simple authentication module which checks permissions for content stored in restbase. Permissions are specified in the spec, and then collected along the path. On request, permission are checked with mediawiki api and 401 is returned in case user lacks any rights.

Bug: https://phabricator.wikimedia.org/T88016

Pchelolo · 2015-07-02T10:13:59Z

test/features/errors/notfound.js

@@ -168,7 +168,6 @@ describe('404 handling', function() {
        })
        .finally(function() {
            nock.cleanAll();
-            nock.restore();
        });


Apparently this was too much - we don't need to restore nock after test, because it breaks following nock tests. Cleaning all errors is enough to make everything run properly both if test pass and fail.

Permissions are collected along the path and returned. This is needed for PR: wikimedia/restbase#272 Bug: https://phabricator.wikimedia.org/T88016

Pchelolo · 2015-07-02T16:14:31Z

Please note that travis build will fail until swagger-router is updated, so this change depend on PR:
wikimedia/swagger-router#20

d00rman · 2015-07-07T11:48:37Z

lib/restbase.js

@@ -268,7 +279,7 @@ RESTBase.prototype._request = function (req) {
        childReq.params = match.params;

        // Call the handler with P.try to catch synchronous exceptions.
-        return P.try(handler, [childRESTBase, childReq])
+        var request = P.try(handler, [childRESTBase, childReq])


request is confusing as a name since we usually use req / request to denote the actual request object, but here it's a Promise. Maybe reqHandlerPromise or reqHandling would be more appropriate.

d00rman · 2015-07-09T11:42:25Z

mods/action.js

+    } else if (res.body.query.userinfo) {
+        return res.body.query.userinfo;
+    } else {
+        throw apiError({info: 'Unable to parse PHP action API response.'});


Hmmm, not sure about this. Throwing a cannot parse response because we don't know the meta in question seems wrong given that the response might be a legitimate one.

@d00rman This is exactly how this used to work, but previously we "knew" only 'pages' field, and now we know 'pages' and 'userinfo'.

I agree that it's not really a change from the old behavior. We can generalize this further as needed.

gwicke · 2015-07-20T21:26:24Z

lib/router.js

@@ -219,6 +219,13 @@ Router.prototype._handleSwaggerPathSpec = function(node, pathspec,
        });
    }

+    var security = pathspec.security;


I think you can actually handle this synchronously, before the loaderPromise is even created. If your concern is about modules adding security restrictions, then I think we can add logic for that above.

@gwicke but we know the module's security restrictions only after we load it. And may be it loads submodules etc. Also like this we preserve the order, so more top-level restrictions come first and checked first (doesn't make any difference now, but could if we have special restrictions that would need separate requests)

gwicke · 2015-07-20T23:03:06Z

/cc @Stype

gwicke · 2015-08-10T17:17:17Z

@Pchelolo @d00rman: Are there any changes you plan for this since the request templating changes have landed? Once ready, it might be a good idea to set up a meeting with Chris to walk through the logic & check with him on review / things we should look into.

Pchelolo · 2015-08-10T17:22:04Z

@gwicke yes, there's some stuff I wanted to change, will work on this tomorrow most likely

gwicke · 2015-08-10T19:37:42Z

@Pchelolo, okay.

gwicke · 2015-08-20T00:09:09Z

lib/restbase.js

@@ -168,6 +172,10 @@ RESTBase.prototype.defaultWebRequestHandler = function(req) {
    // Enforce the usage of UA
    req.headers = req.headers || {};
    req.headers['User-Agent'] = req.headers['User-Agent'] || this.rb_config.user_agent;
+    if (this._accessRestrictions.length > 0
+        && req.headers && req.headers['x-internal-request']) {


This header can be injected in the request. We'd either have to set it to false unconditionally when receiving a request, or use some other mechanism to distinguish internal from external services.

I'm wondering if a whitelist regexp to identify internal services would be cleaner than the x-internal-request header. For WMF-internal services, this could be something like /^https?:\/\/[^/]+\.wmnet(:[0-9]+)?\//.

gwicke · 2015-08-20T00:22:48Z

lib/rbUtil.js

+ * the forwarded header.
+ */
+rbUtil.copyForwardedCookies = function(restbase, req) {
+    if (restbase._rootReq


Do you think it would be worth extracting the cookie once, when assigning a new rootreq?

Pchelolo · 2015-08-20T14:49:47Z

Updated according to all comments. Also while working on this I found a bug in router https://phabricator.wikimedia.org/T109702 that was related, so fixed it together with this one, as it's dependent.

gwicke · 2015-08-22T02:16:57Z

lib/restbase.js

+        var uri = req.uri.toString();
+        return this.rb_config.internal_request_whitelist.some(function(regex) {
+            if (/^\/.+\/$/.test(regex)) {
+                return new RegExp(regex.substring(1, regex.length - 1)).test(uri);


It would be cheaper to build a single regexp once at startup, and then just call that.

Example:

var regexps = ['/foo/', '/bar/'].map(function(s) { new RegExp(return s.substring(1, regex.length - 1)).toString(); }); var whitelist = new RegExp(regexps.join('|'));

Cookies set on original request are forwarded to parsoid and mediawiki endpoints to let user auth and read content. Two tests added, verifying that cookies are actually forwarded - one for parsoid and one for mediawiki api. This solves only a part of the problem, after this fix user can read content from private wiki, but we still do not have a mechanism to forbid read of content already stored in restbase. Bug: https://phabricator.wikimedia.org/T88016

…nt stored in restbase. Permissions are specified in the spec, and then collected along the path. On request, permission are checked with mediawiki api and 401 is returned in case user lacks any rights. Bug: https://phabricator.wikimedia.org/T88016

gwicke · 2015-08-31T19:36:43Z

config.example.wikimedia.yaml

    x-subspecs:
      - mediawiki/v1/content
      - mediawiki_v1_graphoid
      - mediawiki/v1/mobileapps
    # - mediawiki/v1/revision-scoring
+    securityDefinitions:
+      mediawiki_auth:
+        description: Checks permissions using MW api


Super minor: This description should ideally be useful to third-party developers. Perhaps something like "MediaWiki cookie authentication", so that they get an idea what kind of cookie they are supposed to pass in here?

Added cookie forwarding (T88016)

Pchelolo reviewed Jul 2, 2015
View reviewed changes

Pchelolo added a commit to Pchelolo/swagger-router that referenced this pull request Jul 2, 2015

Permission collection.

8d31ccc

Permissions are collected along the path and returned. This is needed for PR: wikimedia/restbase#272 Bug: https://phabricator.wikimedia.org/T88016

Pchelolo mentioned this pull request Jul 2, 2015

Permission collection. wikimedia/swagger-router#20

Merged

Pchelolo force-pushed the cookie_forward branch from 3544f35 to fcafed8 Compare July 6, 2015 12:22

d00rman reviewed Jul 7, 2015
View reviewed changes

Pchelolo force-pushed the cookie_forward branch from fcafed8 to 8b61c33 Compare July 7, 2015 15:55

d00rman reviewed Jul 9, 2015
View reviewed changes

Pchelolo force-pushed the cookie_forward branch from 8b61c33 to 033f0f0 Compare July 20, 2015 15:52

gwicke reviewed Jul 20, 2015
View reviewed changes

Pchelolo mentioned this pull request Aug 14, 2015

Added support for http HEAD method #304

Merged

gwicke reviewed Aug 20, 2015
View reviewed changes

Pchelolo force-pushed the cookie_forward branch from 178c2bd to 4228260 Compare August 20, 2015 14:25

gwicke reviewed Aug 22, 2015
View reviewed changes

Pchelolo added 16 commits August 26, 2015 11:57

Cookie-forwarding experiment

4ea957b

Fixed a stupid foreach on cookie copy and added a sample test

e410db6

last

2caaf5f

Updated docs to realate to the security impl

9a1fa14

Fixed according to comments

47f1e44

Fixed merge

14e3828

Addressed comments, added tests, refactored

07ec849

Forgot to uncomment code commented for debugging

2fa687e

Rebase on master

cfa4c65

Rebase on master

27ba0f4

Updated according to comments and fixed issue with resources

0ab40f3

Several more little improvements

343f4c7

Prepare internal request regex on startup

215fd1a

Rebased on top of master

e44f050

Pchelolo force-pushed the cookie_forward branch from cb301fc to e44f050 Compare August 26, 2015 09:08

Pchelolo added 2 commits August 31, 2015 21:00

Use securityDefinition objects

1f44fa0

Removed some stale code

464738e

gwicke reviewed Aug 31, 2015
View reviewed changes

gwicke added a commit that referenced this pull request Aug 31, 2015

Merge pull request #272 from Pchelolo/cookie_forward

3ca0b43

Added cookie forwarding (T88016)

gwicke merged commit 3ca0b43 into wikimedia:master Aug 31, 2015

Pchelolo deleted the cookie_forward branch December 21, 2015 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added cookie forwarding (T88016) #272

Added cookie forwarding (T88016) #272

Pchelolo commented Jul 2, 2015

Pchelolo Jul 2, 2015

Pchelolo commented Jul 2, 2015

d00rman Jul 7, 2015

d00rman Jul 9, 2015

Pchelolo Jul 9, 2015

gwicke Jul 21, 2015

gwicke Jul 20, 2015

Pchelolo Aug 11, 2015

gwicke commented Jul 20, 2015

gwicke commented Aug 10, 2015

Pchelolo commented Aug 10, 2015

gwicke commented Aug 10, 2015

gwicke Aug 20, 2015

gwicke Aug 20, 2015

Pchelolo commented Aug 20, 2015

gwicke Aug 22, 2015

gwicke Aug 31, 2015

Added cookie forwarding (T88016) #272

Added cookie forwarding (T88016) #272

Conversation

Pchelolo commented Jul 2, 2015

Choose a reason for hiding this comment

Pchelolo commented Jul 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwicke commented Jul 20, 2015

gwicke commented Aug 10, 2015

Pchelolo commented Aug 10, 2015

gwicke commented Aug 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pchelolo commented Aug 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment