|
| 1 | +# Facebook OAuth tests change |
| 2 | + |
| 3 | +## Overview and Motivation |
| 4 | +After some updates Facebook Graph API throttling mechanisms, OAuth tests started failing randomly with 403 code or |
| 5 | +`invalid credentials` ms-users error. |
| 6 | +Graph API limits requests by a sliding window algorithm, which allows some(not all) requests even after block. |
| 7 | +The changes listed here will provide better stability in OAuth error handling and overall test suites stability. |
| 8 | + |
| 9 | +## Facebook Throttling |
| 10 | +After some changes in Facebook GraphAPI `throttling` behavior became much stronger. |
| 11 | +On Every GraphAPI request, facebook response contains special header `X-App-Usage`: |
| 12 | +```javascript |
| 13 | +const XAppUsage = { |
| 14 | + 'call_count': 28, //Percentage of calls made |
| 15 | + 'total_time': 15, //Percentage of total time |
| 16 | + 'total_cputime' : 24 //Percentage of total CPU time |
| 17 | +} |
| 18 | +``` |
| 19 | +If the value of any field becomes greater than 100 causes `throttling`. |
| 20 | +This header helps to determine how soon `throttling` will happen. |
| 21 | + |
| 22 | +### OAuth API v3.3 v4.0 Update |
| 23 | +Updated OAuth API. There is no braking changes and there's no additional work to upgrade version. |
| 24 | +API 3.3 is not available for new Applications and all requests made to this version are redirected to v4.0. |
| 25 | + |
| 26 | +### Tests `Missing Credentials` Error |
| 27 | +Mishandling `@hapi/boom` error causes the OAuth strategy to continue its execution but without mandatory `credentials` and this caused the random tests to fail. |
| 28 | + |
| 29 | +##### Solution |
| 30 | +Implement additional error conversion from `@boom/error` to general `HTTPError`. |
| 31 | +This return OAuth's error processing to normal behavior. |
| 32 | + |
| 33 | +### Tests `403 - Application throttled` Error |
| 34 | +Before every test, some direct GraphApi calls made. In some `throttling` conditions requests rejected with `403` Error, |
| 35 | +this happens because of Facebook call Throttling. |
| 36 | + |
| 37 | +### @hapi/bell error handling |
| 38 | +When the user declines an application to access the 500 Error returned from `@hapi/bell`. |
| 39 | +When application throttled or request `200 < statusCode > 299` the same 500 Error returned with Response as `data` field. |
| 40 | + |
| 41 | +On catch block after `http.auth.test` call added additional error processing on @hapi/boom error: |
| 42 | +* Cleans references to `http.IncomingMessage` from error, otherwise further error serialization shows all contents of `http.IncommingMessage` class. |
| 43 | +IMHO: That's weird to show Buffers and other internal stuff. |
| 44 | +* If error message contains information that user Rejected App access, returns Error.AuthenticationRequiredError` with `OAuth App rejected: Permissions error` . to save current behavior and return 401 StatusCode. |
| 45 | +* Other errors are thrown as @hapi/Bomm error. |
| 46 | + |
| 47 | +Removed `isError` check and function from `auth/oauth/index.js` because it's impossible to receive a response with statusCode other than 200 from `@hapi/bell`. |
| 48 | +According to current source code, only `redirects` and `h.unauthenticated(internal_errors)`(used to report OAuth request errors) thrown from `hapi.auth.test`. |
| 49 | + |
| 50 | +#### Overall Graph API over-usage |
| 51 | +Over-usage of Graph API request time produced by `createTestUser` API calls, this API call uses a lot of API's execution time and increments Facebook's counters fast. |
| 52 | +Before every test, we execute `createTestUser` 3*testCount times, so each suite execution performs about 39 long-running calls. |
| 53 | +Facebook Graph API starts throttling requests after 4-6 suite runs. |
| 54 | + |
| 55 | +##### Solution |
| 56 | +Exclude long-running `createTestUser` operation from tests and reuse test users. |
| 57 | +Attempt to reduce API call count. |
| 58 | + |
| 59 | +## Test logic changes |
| 60 | + |
| 61 | +### Test Users Create |
| 62 | +Test users will be created once before all tests and reused. |
| 63 | +After each test run, users Application permissions revoked depending on user context: |
| 64 | +* For `testUserInstalledPartial` test revokes only `email` permission. |
| 65 | +* For `testUser` test revokes all permissions. |
| 66 | + |
| 67 | +When the test suite finishes users deleted, this saves us from hitting the Facebook TestUser count limit. |
| 68 | + |
| 69 | +### Test Users |
| 70 | +There are 3 types of Facebook users used In the current test suites: |
| 71 | +```javascript |
| 72 | +const createTestUser = (localCache = cache) => Promise.props({ |
| 73 | + testUser: createTestUserAPI(), |
| 74 | + testUserInstalled: createTestUserAPI({ installed: true }), |
| 75 | + testUserInstalledPartial: createTestUserAPI({ permissions: 'public_profile' }), |
| 76 | +}) |
| 77 | +``` |
| 78 | + |
| 79 | +#### testUserInstalled |
| 80 | +**`testUserInstalled`** not used in tests. After review of all commit history for `suites/oauth/facebook.js`, found that |
| 81 | +this user was defined in [this commit](https://github.com/makeomatic/ms-users/blob/733aba371b62d90935c42087ca6d3912152cb63b/test/suites/oauth/facebook.js) |
| 82 | +and never used. |
| 83 | + |
| 84 | +According to the users' props, it defined for `sign-in` tests without creating the new user, |
| 85 | +but these tests use `testUser` in all suite and behave like other sign-in/sign-up tests. |
| 86 | + |
| 87 | +Startup logic and checks for these tests is almost the same and works like: |
| 88 | +1. Call `https://mservice/users/oauth/facebook` |
| 89 | +2. The Service redirects to the Facebook Login form. |
| 90 | +3. Puppeter fills in Login and password. |
| 91 | +4. Puppeter Confirms/Rejects Application request. |
| 92 | + |
| 93 | +`testUser` totally responds to our needs and `testUserInstalled` looks unused. |
| 94 | + |
| 95 | +**!!!** _Assuming that we can remove the `testUserInstalled` user._ |
| 96 | + |
| 97 | +#### testUserInstalledPartial |
| 98 | +**`testUserPartial`** used in tests that checking partial permissions in registration/login and used as the previous scope, |
| 99 | +but users prop `installed` false. |
| 100 | + |
| 101 | +In current partial permission tests, Facebook asks 2 permissions(public_profile, email), this indicates that |
| 102 | +partial `permissions` ignored on the user creation process. So we can safely deauthorize application using 'DELETE /uid/permissions' request. |
| 103 | + |
| 104 | +#### Changes: |
| 105 | +To make tests more readable and a bit easier to read some methods moved to separate files: |
| 106 | +* WebExecuter class - contains all actions performed with the Facebook page using `Puppeter`. |
| 107 | +* GraphApi Class - contains all calls to the Facebook Graph API. |
| 108 | + |
| 109 | +Repeating `asserts` moved outside of tests into functions. |
| 110 | +Tests regrouped by User type. Some duplicate actions moved into `hooks`. |
| 111 | + |
| 112 | +### Additional tests/functionality? |
| 113 | +Current tests cover all possible situations, even for the user with Application `installed === true` property. |
| 114 | +All test cases for this type of user look the same as previous tests and code duplicates because the same API used. |
| 115 | +One difference in them, when the user already has some permissions provided to the application, |
| 116 | +'Facebook Permission Access' screen shows a smaller permission list |
| 117 | +and only `signInAndNavigate` method changed in 1 row(index of the checkbox to click). |
| 118 | + |
| 119 | + |
| 120 | +### GDPR NOTE |
| 121 | +In our case, GDPR not applied inside the scope of the 'Facebook Login' feature. |
| 122 | +Facebook is a Data Provider for us and using own privacy policy that allows us to use |
| 123 | +data provided from Facebook. |
| 124 | +From our side, we must add `Cookie and Data collection notification` - already exists. |
| 125 | + |
| 126 | +Some changes should be made if We use Android or IOS Facebook SDK in event tracking functions. |
| 127 | +For Detailed description visit [this page](https://www.facebook.com/business/m/one-sheeters/gdpr-developer-faqs) |
| 128 | + |
| 129 | +Additional info can be found [here](https://developers.facebook.com/docs/app-events/best-practices/gdpr-compliance) |
0 commit comments