added an RFC 7662 compliant OAuth2 auth adapter #4910

zsmuller · 2018-07-24T19:23:31Z

Shortly after the OAuth2 specification (meaning the two core specs: RFC 6749 and 6750) were finalized and approved in October 2012, people realized that one important aspect was completely overlooked: access token validation by resource servers.

In 2012 November a new RFC draft was started to cover this area: RFC 7662 with the title "OAuth 2.0 Token Introspection". It was finalized and published in October 2015, three full years later!
At this point most companies in the OAuth2 business already implemented their own custom endpoints for token validation and Parse Server's authentication adapters are based on these custom solutions.

However there are many OAuth2 providers and products that already implement the token introspection endpoint.
Eg.

Connect2id server: https://connect2id.com/products/server/docs/api/token-introspection
PingFederate Server: https://docs.pingidentity.com/bundle/pf_sm_pingfederateOauth20Endpoints_pf82/page/pf_c_introspectionEndpoint.html
Commercetools: https://docs.commercetools.com/http-api-authorization
IBM Security Access Manager: https://www.ibm.com/support/knowledgecenter/SSPREK_9.0.5/com.ibm.isam.doc/config/concept/con_oauth_introspection.html
WSO2 Identity Server: https://docs.wso2.com/display/IS530/Invoke+the+OAuth+Introspection+Endpoint
Keycloak: https://www.keycloak.org/docs/2.5/authorization_services/topics/service/protection/token-introspection.html
WP (WordPress) OAuth Server: https://wp-oauth.com/documentation/how-tos/token-introspection/
Liferay Portal: https://dev.liferay.com/en/discover/deployment/-/knowledge_base/7-1/oauth-2-0
Onegini Token Server: https://docs.onegini.com/msp/token-server/6.0.0/reference/api-reference/token-introspection.html
Okta: https://developer.okta.com/docs/api/resources/oidc#introspect
(etc.)

This PR contains an implementation of a Parse Server authentication adapter based on RFC 7662 and it's flexible enough (with various configuration options) to be used with any RFC-compatible token introspection implementation.

It was written for a project at NNG Llc (www.nng.com).

codecov · 2018-07-24T19:35:50Z

Codecov Report

Merging #4910 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #4910      +/-   ##
=========================================
+ Coverage   93.93%     94%   +0.06%     
=========================================
  Files         123     125       +2     
  Lines        8975    9091     +116     
=========================================
+ Hits         8431    8546     +115     
- Misses        544     545       +1

Impacted Files	Coverage Δ
src/Adapters/Auth/index.js	`92.72% <100%> (+0.41%)`	⬆️
src/Adapters/Auth/oauth2.js	`100% <100%> (ø)`
src/Adapters/Auth/facebook.js	`76.47% <0%> (-3.53%)`	⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`91.48% <0%> (-0.73%)`	⬇️
src/ParseServer.js	`96.29% <0%> (-0.46%)`	⬇️
src/RestQuery.js	`96% <0%> (-0.11%)`	⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`97.08% <0%> (-0.08%)`	⬇️
src/Options/Definitions.js	`100% <0%> (ø)`	⬆️
src/rest.js	`98.85% <0%> (ø)`	⬆️
...rc/Adapters/Storage/Mongo/MongoSchemaCollection.js	`95.29% <0%> (ø)`	⬆️
... and 10 more

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 2d7b992...81cac2c. Read the comment docs.

* changed option names in auth adapter from snake case to camel case * added underscore prefix to helper function names * merged consecutive logger calls into one call and use JSON.stringify() to convert JSON objects to strings * changed error handling (ParseErrors are no longer thrown, but returned)

zsmuller · 2018-07-25T12:21:39Z

I've added some test coverage, but I'll need some help here.

First of all: how can my PR affect test coverage of code in files like MongoStorageAdapter.js or RestWrite.js? :o I'm new to nodejs unit testing, have no idea how codecov.io works, etc.

Second: how can I mock an OAuth2 provider? It seems to me that the largest part of my code missing test coverage is the one that does the HTTP request to the OAuth2 server.

…tests too

flovilmart · 2018-07-25T12:27:12Z

First of all, thanks for the PR.

how can my PR affect test coverage of code in files like MongoStorageAdapter.js or RestWrite.js

Don’t worry about thiose, those are false positives that we’ll need to address at one point.

how can I mock...

Did you try jasmine spies that can help you with that?

zsmuller · 2018-07-25T12:30:26Z

@flovilmart Thanks for the tip. I'm not really familiar with jasmine spies, but I'll look into it.

zsmuller · 2018-07-25T15:50:50Z

@flovilmart

I've noticed that all authentication adapters (that do any logging) use a require() to get what's needed for logging. All the other adapters use import to do the same.
The former looks like this (eg. in src/Adapters/Auth/twitter.js):

var logger = require('../../logger').default;

The latter looks like this (eg. in src/Adapters/Storage/Mongo/MongoStorageAdapter.js):

import logger                from '../../../logger';

It seems to me that the two behave quite differently.
The require() in the authentication adapters seems to create a new logger (?) with default logging options (verbose: false, logLevel: info), disregarding whatever these options were set in the parse server config.
The import fetches a logger that is already initialized with the real parse server config options.

I tested this by passing the path to a config.json to the parse server launching commandline and set the "logLevel" in this config.json to "silly". Then I added logging calls to my auth adapter (for each loglevel that exists in LoggerController.js) and tested both the require() method and the import method for defining a logger variable. In the require case only logger.error(), logger.warn() and logger.info() produced any log records. In the import case all log records appeared as expected.

I've no idea why do these two approaches work differently, but they do. So I opted to use the import method in this PR's auth adapter, despite the fact that all other auth adapters use the require method. They don't experience any issues, because they do not use any loglevels below info, thus the default info level works for them perfectly.

…uire() of the logger with an import (the former does not work correctly)

flovilmart · 2018-07-25T16:02:31Z

@zsmuller this is likely that when using require, it loads the default logger from before it’s configured, and the other method gets the logger after it’s configured. I need to have a look at it at one point or refactor so the logger is passed as a contextual parameter or accessed differently. For now, don’t worry much about it :)

flovilmart · 2018-07-26T11:42:31Z

@zsmuller I have a question for you, How do we use multiple auth providers with this? The client always referes to the oauth by it's name, in this case this would be oauth2, which is problematic.

Perhaps the options for oauth2 should be an object with the key being the auth service name:

{
applicationId: ....,
masterKey: ....
auth: {
    oauth2: {
        connect2id: { /* connectID2 options */ },
        wordpress: { /* wordpress options */ }
    }
}
}

What do you think?

zsmuller · 2018-07-26T12:44:33Z

How do we use multiple auth providers with this?
(...)
What do you think?

Currently you don't. Since all present authentication adapters support accesstoken validation only with a single source, I didn't anticipate the use-case of having multiple OAuth2 providers behind a single Parse instance. But it's a good idea and easy to implement. I'll look into it.

flovilmart · 2018-07-26T12:45:54Z

I believe it’s critical to implement (as everyone will expect it to work this way).

Also, do you think we can refactor all oauth 2 providers to leverage your module?

zsmuller · 2018-07-26T13:00:19Z

Also, do you think we can refactor all oauth 2 providers to leverage your module?

Unfortunately no. I added already in the description of this PR that many social network providers implemented their own solutions for access token validation way before RFC 7662 was finalized and these implementations are not compliant (with the RFC). So you cannot use this module with Facebook, Google, GitHub, Instagram, etc.

I'd have to check each auth adapter whether the respective providers have any token introspection endpoint, but I seem to remember that eg. Google and Facebook do not.

flovilmart · 2018-07-26T13:01:26Z

That's too bad :/ but I guess it's ok.

zsmuller · 2018-07-26T13:02:53Z

Yes. It would have been nice if two auth adapters could handle all providers (one for OAuth1 and one for OAuth2).

zsmuller · 2018-07-26T13:11:01Z

Supporting multiple OAuth2 providers with a single oauth2 adapter would also mean that the client should send a provider name (as specified in the Parse Server auth config for the adapter) in the authData request.
Eg.

{
  "oauth2": {
    "provider": "connect2id",
    "access_token": "c631b8b549b4601e34e032bcd83332de1532594940252",
    "id": "346757347467"
  }
}

Otherwise the adapter would have to try accesstoken authentication with each configured provider. This would be very ineffective from a performance PoV and also a security issue since the accesstoken from the client would end up at all the configured OAuth2 providers and only one of them should have it (the others could "misuse" the received accesstoken ... even if that's quite unlikely, they shouldn't ever have the chance to do so).

flovilmart · 2018-07-26T13:16:04Z

would also mean that the client should send a provider name

Yeah it has to send the provider name, but not this way (otherwise we'll need a lot more refactoring).

The client should send it this way:

{
  "connect2id": {
    "provider": "oauth2", // optional actually
    "access_token": "c631b8b549b4601e34e032bcd83332de1532594940252",
    "id": "346757347467"
  }
}

Then on the server we could have this logic:

const provider = Object.keys(authData)[0] // "connect2id"
if (isBaseProvider(provider)) { // like facebook, twitter et. al.

} else if (isOAuth2ProviderConfigured(provider)) { // use the oauth2 with specified configuration

} else {
   // FATAL ERROR: Not supported
}

zsmuller · 2018-07-26T13:46:39Z

Yeah it has to send the provider name, but not this way (otherwise we'll need a lot more refactoring).

I don't see why would we need more refactoring. I thought that the client can send in the authData almost whatever it wants, only the "id" field is mandatory, but other fields are allowed as well. Not even the "access_token" field is mandatory ... "twitter" doesn't seem to use it.

Eg. in the Parse Server Guide there're many adapter-specific fields that only occur with a few or even a single adapter.

https://docs.parseplatform.org/parse-server/guide/#oauth-and-3rd-party-authentication

The "facebook" adapter accepts and uses a "expiration_date" field, the "facebookaccountkit" adapter accepts a "last_refresh" field, the "twitter" adapter has lots of custom fields (and no "access_token" field! since it's based on OAuth 1.0), "google" accepts an "id_token" as well, "linkedin" accepts an "is_mobile_sdk", etc.

Why couldn't we put in the oauth2 authData a "provider" field (or an "oauth2_provider" if that's less likely to conflict with possible future changes of the Parse authentiction adapter API)? :o

Moreover my suggestion would require changes only in the oauth2 adapter. Your approach requires modification of the auth adapter handling code.

flovilmart · 2018-07-26T13:50:43Z

I don't see why would we need more refactoring. I thought that the client can send in the authData almost whatever it wants, only the "id" field is mandatory, but other fields are allowed as well. Not even the "access_token" field is mandatory ... "twitter" doesn't seem to use it.

The problem is not the payload but the key. Auth data is stored as _auth_data_[provider_name] from the provided payload:

{
   provider_name: {
      id: ...
      token: ...
   }
}

In the oauth2 case we're designing now, provider_name is oauth2 which doesn't make sense at all.
IF you want to pass provider part of the payload, then a lot of changes will be required in RestWrite.js to support it.

This is why, I suggest we keep the same strategy, and oauth2 not being a top level auth provider, but a 'generator' for each supported provider.

The oauth2.js file should not expose directly the validation methods, but instead generate validators based on the configuration.

zsmuller · 2018-07-26T14:15:38Z

I see what you mean. But if that's the case, your example for configuration is not aligned with this concept.

You wrote this:

{
  applicationId: ....,
  masterKey: ....
  auth: {
    oauth2: {
        connect2id: { /* connectID2 options */ },
        wordpress: { /* wordpress options */ }
    }
  }
}

The following seems to be a better match conceptwise:

{
  applicationId: ....,
  masterKey: ....
  auth: {
    connect2id: {
      provider: "oauth2",
      /* connect2id oauth2 options */
    },
    wordpress: {
      provider: "oauth2",
       /* wordpress options */
    }
  }
}

I mean if oauth2.js is not a "provider" itself and it generates the validators for the actual providers, then shouldn't the configuration reflect this structure as well?

flovilmart · 2018-07-26T14:16:50Z

That works this way too :) I like it!

zsmuller · 2018-07-26T14:19:01Z

src/Adapters/Auth/index.js would require some refactoring.
Currently the provider "list" is in a const object, but following your idea the list of providers should be generated based on the actual Parse config.

zsmuller · 2018-07-26T14:22:24Z

I'm not convinced that the provider/adapter object generation should be in oauth2.js.
Currently all adapter loading (etc.) logic is in index.js.
I think it'd be better to keep oauth2.js as a sort of template/prototype for oauth2 provider objects and have the loading/generator logic in index.js.

If this seems OK to you, I'll go in this direction with the next commit.

flovilmart · 2018-07-26T14:56:52Z

That makes sense as it’s index.js that provides all the logic :)

zsmuller · 2018-07-26T17:12:34Z

Is the AuthAdapter class (in src/Adapters/Auth/AuthAdapter.js) used for anything? I don't see any other code referencing it. :o

flovilmart · 2018-07-26T17:32:19Z

It’s used as an example for valid auth adapters, but you’re right it’s not in used now. The intent was to use it as a base to validate the passed adapters, like the other adapters (email, files etc...) in the project.

zsmuller · 2018-07-26T18:38:47Z

Currently most/many authentication adapters can be used ootb, without any configuration.

This is by design, as described in the documentation:
https://docs.parseplatform.org/parse-server/guide/#supported-3rd-party-authentications

Note, most of them don’t require a server configuration so you can use them directly, without particular server configuration.

Is this not a (security) problem?
A client application can use most authentication methods/adapters on a Parse server that it knows the appid for, regardless of the intention of the server operator. And there doesn't seem to be a way to disable an authentication adapter.

Shouldn't authentication adapters be disabled by default and only enabled if the config (server admin) says so? Eg. having an "enabled" property set to "true" in the config for the given adapter?

I know that this would be a backwards incompatible change and obviously something like this would at least mandate it's own pull request or even maybe a major Parse server release (i.e. a step in the major version). So my question is theoretical at this point.

zsmuller · 2018-10-06T03:05:09Z

I'm not quite sure what's happening.
Github wrote at the end of this PR that my branch (?) was outdated from the original master and should be updated. I found it strange since I already synced it with the upstream master and commited and pushed the result. Never mind. I pushed the "Update branch" button, which resulted in some merge conflict. I resolved it, pushed again.
Now Github still says that:

Merging is blocked
Merging can be performed automatically once the requested changes are addressed.

What am I supposed to do? :o

stale · 2018-11-22T18:21:40Z

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.

stale · 2019-02-13T15:16:42Z

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.

flovilmart · 2019-02-20T15:30:24Z

@dplewis @acinader I believe this is still a good feature, could you pick it up what it left?

zsmuller · 2019-02-20T15:48:42Z

Hi @flovilmart! I can continue as well, I was waiting for some help (see my last comments) and you never replied.

dplewis

@zsmuller Thanks for getting this to this point!

I've read over the conversation and a quick review from me.

Left some comments on how more tests are needed.

spec/AuthenticationAdapters.spec.js

src/Adapters/Auth/oauth2.js

acinader

this looks good to me.

the adapter validation would be a nice addition so we can detect mis config at config time instead of when trying to use, if i'm groking it right, but this seems plenty useful as is.

I have a question about the handling of promises and throwing that isn't specific to this functionality, but I'd like to resolve.

I can try and test my concern later this afternoon or tonight if no one beats me to it....

acinader · 2019-04-10T17:24:52Z

src/Adapters/Auth/oauth2.js

+ * }
+ */
+
+const Parse = require('parse/node').Parse;


prefer destructuring.

acinader · 2019-04-10T17:27:27Z

spec/AuthenticationAdapters.spec.js

+      });
+    });
+    try {
+      await adapter.validateAuthData(authData, providerOptions);


if not done async, not sure how the validate auth data will behave? worth trying....

acinader · 2019-04-10T17:27:43Z

src/Adapters/Auth/oauth2.js

+      !response.active ||
+      (options.useridField && authData.id !== response[options.useridField])
+    ) {
+      throw new Parse.Error(Parse.Error.OBJECT_NOT_FOUND, INVALID_ACCESS);


shouldn't this be return Promise.reject(new Parse.Error....);

acinader · 2019-04-10T17:28:42Z

src/Adapters/Auth/oauth2.js

+  }
+  return requestTokenInfo(options, authData.access_token).then(response => {
+    if (!response || !response.active) {
+      throw new Parse.Error(Parse.Error.OBJECT_NOT_FOUND, INVALID_ACCESS);


again, mixing of promise and throw concerns me. maybe not issue, just haven't seen.

used to async / throw, non-async / Promise.reject.

If you look at the other authAdapters this is how it's handled. I'll let you fiddle around with it. I probably missed something somewhere

here's a decent writeup that shows a subtle difference, but in this case, i think we're all good. https://stackoverflow.com/questions/33445415/javascript-promises-reject-vs-throw

...

dplewis · 2019-04-11T15:44:33Z

@zsmuller Thanks for the PR! Sorry it took so long.

zsmuller · 2019-04-12T18:38:03Z

@dplewis, @acinader
Thank you both for the help in taking this PR through it's last steps towards merge into master.

* added an RFC 7662 compliant OAuth2 auth adapter * forgot to add the actual auth adapter to the previous commit * fixed lint errors * * added test coverage * changed option names in auth adapter from snake case to camel case * added underscore prefix to helper function names * merged consecutive logger calls into one call and use JSON.stringify() to convert JSON objects to strings * changed error handling (ParseErrors are no longer thrown, but returned) * added description of the "debug" option and added this option to the tests too * added a check of the "debug" option to the unittests and replaced require() of the logger with an import (the former does not work correctly) * added AuthAdapter based auth adapter runtime validation to src/Adapters/Auth/index.js, added capability to define arbitrary providernames with an "adapter" property in auth config, replaced various "var" keywords with "const" in oauth2.js * incorporated changes requested by flovilmart (mainly that oauth2 is now not a standalone adapter, but can be selected by setting the "oauth2" property to true in auth config * modified oauth2 adapter as requested by flovilmart * bugfix: defaultAdapter can be null in loadAuthAdapter() of index.js (my change broke the tests) * added TODO on need for a validateAdapter() to validate auth adapters * test cases and cleanup

Zsolt MÜLLER added 2 commits July 24, 2018 20:57

added an RFC 7662 compliant OAuth2 auth adapter

01c46e4

forgot to add the actual auth adapter to the previous commit

b1db2d2

Zsolt MÜLLER added 2 commits July 25, 2018 02:06

fixed lint errors

eca9e9e

added description of the "debug" option and added this option to the …

672f06f

…tests too

added a check of the "debug" option to the unittests and replaced req…

fe15745

…uire() of the logger with an import (the former does not work correctly)

merging upstream changes

654c427

Merge branch 'master' into oauth2_auth_adapter

e804995

stale bot added the wontfix label Nov 22, 2018

Merge branch 'master' into oauth2_auth_adapter

f791aee

stale bot removed the wontfix label Nov 22, 2018

stale bot added the wontfix label Feb 13, 2019

stale bot closed this Feb 20, 2019

dplewis added enhancement and removed wontfix labels Feb 21, 2019

dplewis reopened this Feb 21, 2019

dplewis suggested changes Feb 21, 2019

View reviewed changes

spec/AuthenticationAdapters.spec.js Outdated Show resolved Hide resolved

src/Adapters/Auth/oauth2.js Outdated Show resolved Hide resolved

src/Adapters/Auth/oauth2.js Outdated Show resolved Hide resolved

test cases and cleanup

81cac2c

dplewis requested a review from acinader April 3, 2019 20:41

dplewis assigned acinader Apr 3, 2019

acinader reviewed Apr 10, 2019

View reviewed changes

acinader approved these changes Apr 10, 2019

View reviewed changes

flovilmart requested review from flovilmart and removed request for flovilmart April 10, 2019 20:56

dplewis merged commit 019cf0a into parse-community:master Apr 11, 2019

dplewis mentioned this pull request Mar 15, 2021

Implement OAuth method #7257

Closed

7 tasks

mtrezza removed the 🧬 enhancement label Jul 11, 2021

added an RFC 7662 compliant OAuth2 auth adapter #4910

added an RFC 7662 compliant OAuth2 auth adapter #4910

Conversation

zsmuller commented Jul 24, 2018

codecov bot commented Jul 24, 2018 • edited Loading

Codecov Report

zsmuller commented Jul 25, 2018

flovilmart commented Jul 25, 2018

zsmuller commented Jul 25, 2018

zsmuller commented Jul 25, 2018

flovilmart commented Jul 25, 2018

flovilmart commented Jul 26, 2018

zsmuller commented Jul 26, 2018

flovilmart commented Jul 26, 2018

zsmuller commented Jul 26, 2018 • edited Loading

flovilmart commented Jul 26, 2018

zsmuller commented Jul 26, 2018

zsmuller commented Jul 26, 2018

flovilmart commented Jul 26, 2018

zsmuller commented Jul 26, 2018 • edited Loading

flovilmart commented Jul 26, 2018

zsmuller commented Jul 26, 2018

flovilmart commented Jul 26, 2018

zsmuller commented Jul 26, 2018

zsmuller commented Jul 26, 2018 • edited Loading

flovilmart commented Jul 26, 2018

zsmuller commented Jul 26, 2018

flovilmart commented Jul 26, 2018

zsmuller commented Jul 26, 2018

zsmuller commented Oct 6, 2018

stale bot commented Nov 22, 2018

stale bot commented Feb 13, 2019

flovilmart commented Feb 20, 2019

zsmuller commented Feb 20, 2019

dplewis left a comment • edited Loading

Choose a reason for hiding this comment

acinader left a comment

Choose a reason for hiding this comment

acinader Apr 10, 2019

Choose a reason for hiding this comment

acinader Apr 10, 2019

Choose a reason for hiding this comment

acinader Apr 10, 2019

Choose a reason for hiding this comment

acinader Apr 10, 2019

Choose a reason for hiding this comment

dplewis Apr 10, 2019

Choose a reason for hiding this comment

acinader Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

dplewis Apr 10, 2019

Choose a reason for hiding this comment

dplewis commented Apr 11, 2019

zsmuller commented Apr 12, 2019

codecov bot commented Jul 24, 2018 •

edited

Loading

zsmuller commented Jul 26, 2018 •

edited

Loading

zsmuller commented Jul 26, 2018 •

edited

Loading

zsmuller commented Jul 26, 2018 •

edited

Loading

dplewis left a comment •

edited

Loading

acinader Apr 10, 2019 •

edited

Loading