New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Firebase authentication provider is implemented #3174

Closed

mockitor wants to merge 4 commits into parse-community:master from mockitor:master

mockitor commented Dec 4, 2016

No description provided.

mockitor added 2 commits

December 4, 2016 16:29


          Firebase authentication provider is added.

9c23b5e


          fixed NPE

e6f0cc2

facebook-github-bot added the GH Review: review-needed label


          fixed lint errors

4c95f55

facebook-github-bot commented Dec 4, 2016

@mockitor updated the pull request - view changes

Contributor

flovilmart commented Dec 4, 2016

In general, while this is very interesting, this has a strong dependency on firebase-admin. Which is not required for all users.

We try to minimize the amount of direct dependencies on that repository.

So, probably you would want to make that dependency an optional one or even better, provide this authentication through a 3rd party package that a user can configure:

new ParseServer({
  oauth: {
    firebase: {
       module: "parse-firebase-auth"
    }
  }
});

See:
https://github.com/ParsePlatform/parse-server/blob/master/src/authDataManager/index.js#L79

I'm planning to refactor the auth layer through proper adapters to match the overall architecture.

flovilmart suggested changes

View reviewed changes

src/authDataManager/firebase.js

		@@ -0,0 +1,41 @@
		// Helper functions for accessing the qq Graph API.

Contributor

flovilmart Dec 4, 2016

bad comment.

src/authDataManager/firebase.js

		var Parse = require('parse/node').Parse;
		var admin = require("firebase-admin");

Contributor

flovilmart Dec 4, 2016

remove empty line

src/authDataManager/firebase.js

		});
		}

Contributor

flovilmart Dec 4, 2016

remove empty line

src/authDataManager/firebase.js

+                    }).catch(function (error) {
+                      throw new Parse.Error(Parse.Error.OBJECT_NOT_FOUND, error.message);
+                    });

Contributor

flovilmart Dec 4, 2016

remove empty line

src/authDataManager/firebase.js

		return Promise.resolve();
		}

Contributor

flovilmart Dec 4, 2016

remove empty line

src/authDataManager/index.js

@@ @@ -13,6 +13,8 @@ let vkontakte = require("./vkontakte"); @@
               let qq = require("./qq");
               let wechat = require("./wechat");
               let weibo = require("./weibo");
+              let firebase = require("./firebase");

Contributor

flovilmart Dec 4, 2016

remove empty line

package.json

@@ @@ -23,6 +23,7 @@ @@
                   "commander": "2.9.0",
                   "deepcopy": "0.6.3",
                   "express": "4.14.0",
+                  "firebase-admin": "^4.0.3",

Contributor

flovilmart Dec 4, 2016

not sure we want to depend on that module for every single user.

You can probably provide that module externally.

Author

mockitor Dec 4, 2016 •

edited

Loading

ok, i'll try to provide externally. thanks for your feedback.

Contributor

acinader commented Dec 4, 2016

would be good if i could find an eslint rule for the blank line formatting stuff....

http://eslint.org/docs/rules/no-multiple-empty-lines

that seems too lienent, but having max at 1 would likely be too strict.

acinader mentioned this pull request

[lint] max empty line rule. #3176

Merged

Contributor

flovilmart commented Dec 6, 2016

@mockitor We've merged the refactoring for the adapter loading, providing your adapter externally should be as easy as

npm install --save firebase-auth-adapter
new ParseServer({
  auth: {
    firebase: "firebase-auth-adapter"
  }
})

That's easy enough to implement, what do you think?


          Removed firebase-admin-sdk dependency.

314d5e8

Firebase id token verification is implemented with third part jsonwebtoken library.

facebook-github-bot commented Dec 7, 2016

@mockitor updated the pull request - view changes

Author

mockitor commented Dec 7, 2016

i could not find any simple example to implement auth adapter. instead of i've removed firebase-admin-sdk. Firebase Id token is verified against Google Public key using jsonwebtoken jwt library. only jsonwebtoken library is extra dependency now.

Contributor

flovilmart commented Dec 7, 2016

The point was that this auth adapter can and should be distributed as it's own npm package (as we do for the push adapters for example)

flovilmart closed this

Contributor

flovilmart commented Dec 8, 2016

Closing as we're prefer to see that distributed as it's own node module, if you need help on that ping me :)

Contributor

ranhsd commented Jan 25, 2017

Hi @flovilmart- I can create it as external module. Can you please create a repository under parse-server-modules? I will find some time to do it later this week or next week.

@mockitor - I will try to use your code first and will let you know if there are issues.

Thanks.

parseplatformorg mentioned this pull request

refactor: Upgrade mongodb from 4.10.0 to 4.11.0 #8309

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet