New Auth Method Added. #4484

kenglou · 2018-01-05T08:23:07Z

Added the following Authentication Provider:

Game Center.
Google Play Game Services.
Xiao Mi

1. add google Play Games Authentication Method(different from Google due to GPG is using a new ID method). 2. add Game Center Authentication Method. By refering to the URL that provided: https://developer.apple.com/documentation/gamekit/gklocalplayer/1515407-generateidentityverificationsign?language=objc

check using the funciton hasOwnProperty.

fix error for Google Play Games.

Temporary enable this to allow PR Success

1. remove old rest.spec test as it is actually wrong. 2. added the test at AuthenticationAdapters.spec.js. 3. put back the index.js change to search for where to change exactly.

codecov · 2018-01-05T08:36:12Z

Codecov Report

Merging #4484 into master will decrease coverage by 0.82%.
The diff coverage is 40%.

@@            Coverage Diff             @@
##           master    #4484      +/-   ##
==========================================
- Coverage    92.8%   91.97%   -0.83%     
==========================================
  Files         118      121       +3     
  Lines        8394     8515     +121     
==========================================
+ Hits         7790     7832      +42     
- Misses        604      683      +79

Impacted Files	Coverage Δ
src/Adapters/Auth/index.js	`92.85% <100%> (+0.7%)`	⬆️
src/Adapters/Auth/gcenter.js	`20.37% <20.37%> (ø)`
src/Adapters/Auth/gpgames.js	`36.36% <36.36%> (ø)`
src/Adapters/Auth/xiaomi.js	`51.72% <51.72%> (ø)`
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`97.01% <0%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33890bb...2f954c1. Read the comment docs.

dplewis · 2018-01-10T21:33:09Z

src/Adapters/Auth/index.js

+
+var _AdapterLoader2 = _interopRequireDefault(_AdapterLoader);
+
+function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }


Looks like you copied compiled babel. Doesn't look like something humans write.

dplewis · 2018-01-10T21:34:42Z

src/Adapters/Auth/index.js

+  xiaomi
+};
+
+function authDataValidator(adapter, appIds, options)


Sticking to the current coding style / ES6 would be ideal.

dplewis · 2018-01-10T21:35:17Z

src/Adapters/Auth/index.js

 }

 function loadAuthAdapter(provider, authOptions) {
  const defaultAdapter = providers[provider];
  const adapter = Object.assign({}, defaultAdapter);
  const providerOptions = authOptions[provider];

-  if (!defaultAdapter && !providerOptions) {
+  if (!defaultAdapter && !providerOptions)


Its recommended to always use brackets for if / else statements. Without them they can lead to bugs. You might want to go over your code again for consistency

dplewis · 2018-01-10T21:35:57Z

src/Adapters/Auth/index.js


  const appIds = providerOptions ? providerOptions.appIds : undefined;

  // Try the configuration methods
  if (providerOptions) {
-    const optionalAdapter = loadAdapter(providerOptions, undefined, providerOptions);
+    const optionalAdapter = (0, _AdapterLoader2.default)(providerOptions, undefined, providerOptions);


kenglou · 2018-01-15T10:39:42Z

already changed accordingly. Please let me know if there is anything else.

kenglou

changes made.

montymxb

There's a lot of things that need to be changed here. For starters if you can undo those stylistic changes that will make it much easier for us to stay focused on just the functional changes you're introducing.

If you do feel you want to introduce stylistic changes to existing code, and have justification for doing so, feel free to open up a separate PR specifically for doing that.

::edit:: Additionally diff coverage by this PR is negative. You'll need to inspect the coverage results and account for untested code paths, which may involve adjusting your code to make it testable.

montymxb · 2018-01-16T00:16:05Z

src/Adapters/Auth/index.js

        return adapter.validateAppId(appIds, authData, options);
      }
      return Promise.resolve();
    });
-  }
+  };


Can you revert these style changes?

Generally you should avoid unnecessary stylistic changes when adding a feature unless it's part of the change itself (i.e. specifically a stylistic change PR). In this case it's superfluous and adds to what we have to review. It'll make it much easier for us to go through and review your changes if they're only functional changes.

montymxb · 2018-01-16T00:17:08Z

src/Adapters/Auth/index.js

@@ -70,7 +78,7 @@ function loadAuthAdapter(provider, authOptions) {
  if (providerOptions) {
    const optionalAdapter = loadAdapter(providerOptions, undefined, providerOptions);
    if (optionalAdapter) {
-      ['validateAuthData', 'validateAppId'].forEach((key) => {
+      ['validateAuthData', 'validateAppId'].forEach(key => {


Same here. This can and should be suggested separately.

montymxb · 2018-01-16T00:17:17Z

src/Adapters/Auth/index.js

@@ -82,18 +90,26 @@ function loadAuthAdapter(provider, authOptions) {
    return;
  }

-  return {adapter, appIds, providerOptions};
+  return { adapter, appIds, providerOptions };


montymxb · 2018-01-16T00:17:50Z

src/Adapters/Auth/index.js

  // To handle the test cases on configuration
-  const getValidatorForProvider = function(provider) {
+  const getValidatorForProvider = function (provider)
+  {


and above here

montymxb · 2018-01-16T00:18:37Z

src/Adapters/Auth/index.js

-  })
-}
+  });
+};


Not needed for this PR, let's kept it focused on what you're actually introducing.

src/Adapters/Auth/gcenter.js

montymxb · 2018-01-16T00:20:38Z

src/Adapters/Auth/gpgames.js

+
+// Returns a promise that fulfills if this user id is valid.
+function validateAuthData(authData, authOptions)
+{


again, keep the curly braces up on the same line. It's inconsistent throughout this file as well if you can amend that.

montymxb · 2018-01-16T00:23:19Z

src/Adapters/Auth/xiaomi.js

+var crypto = require('crypto');
+
+const PRODUCTION_URL = "https://cn-api.unity.com";
+const DEBUG_URL = " https://cn-api-debug.unity.com";


This url doesn't seem right, xiaomi is unity? Can you clarify this 3rd party provider and the urls?

stale · 2018-09-18T06:17:30Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

kenglou added 29 commits December 12, 2017 18:00

fix underscore module

4fc7e2c

remove the checking for provider

065f2a3

Merge branch 'master' into master

934b7b2

checking to make sure the provider exists!

6173067

Merge branch 'master' of https://github.com/kenglou/parse-server

5feca46

change check provider method

f29099b

check using the funciton hasOwnProperty.

Fix GPGames Error

4f9cf55

fix error for Google Play Games.

Take Away checking for Provider Name

7de2a97

Temporary enable this to allow PR Success

add rest.spec for Google Play Games.

bfd7aec

Fixing the Auth Coverage Test

867dbc5

1. remove old rest.spec test as it is actually wrong. 2. added the test at AuthenticationAdapters.spec.js. 3. put back the index.js change to search for where to change exactly.

trying to fulfil the test case of myoauth

143c94e

commit this for pass the test

fdfeaa9

Added another test case oauth method...

18d6e58

Test Again for request another build pass for CI

10cccc9

Test to Cover Test case of unknown provider

76b2be1

Test to Cover Test case of unknown provider

faa596a

add check test for authentication handler not appear on index.

6026c75

remove the done that doesn't has any function

0fee82b

comply the test function.

cf74430

for the test coverage.

f66c4b6

add test case for game center

9596b8c

forget to pass in the validor as verify is the function of it.

4173c84

again.

9925f3d

test case

0fbeed1

second submission for test

ae1f02c

Second Chance for building test case for this.

4848161

added Xiao Mi Login Authentication Method.

f47e438

edit xiao mi to make it reliable.

0e39a0e

Merge branch 'master' into master

c6974cb

dplewis suggested changes Jan 10, 2018

View reviewed changes

kenglou added 2 commits January 15, 2018 09:12

Make index.js to follow standard

a570d72

Merge branch 'master' into master

2f954c1

kenglou commented Jan 15, 2018

View reviewed changes

montymxb suggested changes Jan 16, 2018

View reviewed changes

flovilmart added the GH Review: needs-revision label Jan 24, 2018

njt1982 mentioned this pull request Aug 10, 2018

Adding GameCenter auth #4946

Closed

stale bot added the wontfix label Sep 18, 2018

stale bot closed this Sep 25, 2018

snyk-bot mentioned this pull request Sep 18, 2020

[Snyk] Upgrade apollo-server-express from 2.16.1 to 2.17.0 #6911

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Auth Method Added. #4484

New Auth Method Added. #4484

kenglou commented Jan 5, 2018

codecov bot commented Jan 5, 2018 •

edited

Loading

dplewis Jan 10, 2018 •

edited

Loading

dplewis Jan 10, 2018

dplewis Jan 10, 2018

dplewis Jan 10, 2018

kenglou commented Jan 15, 2018

kenglou left a comment

montymxb left a comment •

edited

Loading

montymxb Jan 16, 2018

montymxb Jan 16, 2018

montymxb Jan 16, 2018

montymxb Jan 16, 2018

montymxb Jan 16, 2018

montymxb Jan 16, 2018

montymxb Jan 16, 2018

stale bot commented Sep 18, 2018


		var _AdapterLoader2 = _interopRequireDefault(_AdapterLoader);

		function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

New Auth Method Added. #4484

New Auth Method Added. #4484

Conversation

kenglou commented Jan 5, 2018

codecov bot commented Jan 5, 2018 • edited Loading

Codecov Report

dplewis Jan 10, 2018 • edited Loading

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

kenglou commented Jan 15, 2018

kenglou left a comment

Choose a reason for hiding this comment

montymxb left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Sep 18, 2018

codecov bot commented Jan 5, 2018 •

edited

Loading

dplewis Jan 10, 2018 •

edited

Loading

montymxb left a comment •

edited

Loading