fastify-oauth2 icon indicating copy to clipboard operation
fastify-oauth2 copied to clipboard

Apple OAuth2 flow bugged

Open playleaks opened this issue 3 years ago • 1 comments
trafficstars

Prerequisites

  • [X] I have written a descriptive issue title
  • [X] I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.8.1

Plugin version

6.1.0

Node.js version

16.14

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

12.3.1

Description

Hello,

I am facing this issue since previous version of @fastify/oauth2: the authorizationMethod for Apple OAuth2 login is wrongly set to header, leading any login attempt to fail, instead the good format is classic form-body.

What I could do until the previous versions of the plugin was the following workaround code, that I was executing right after the plugin registration:

type OAuth2ReservedNamespace = {
      oauth2: {
         authorizationCode: {
            config: {
               options: {
                  authorizationMethod?: string;
               };
            };
         };
      };
   };

   // We need to delete Auth method, cause it has been wrongly set to Header in the library, but apple uses form-body
   delete (fastify.apple as unknown as OAuth2ReservedNamespace).oauth2
      .authorizationCode.config.options.authorizationMethod;

With this piece of code I was force removing the header auth method, allowing the auth flow to work.

But in the last release you have changed the config field to class private field (#config) and now I can't modify it even with the workaround.

Since this problem is becoming quite annoying, can you please change the #config.options.authorizationMethod value for Apple from your repository so that it's set to form-body, or is unset since it should be the default behavior.

Thanks in advance

Steps to Reproduce

Just put in place an Apple OAuth2 flow with the plugin and try to login with Apple ID

Expected Behavior

Usage of form body for authorization

UPDATE: Ok I've finally found the solution:

  credentials: {
     client: {
        id: ...,
        secret: ...,
     },
     **options: {
        authorizationMethod: 'body',
     },**
     auth: oauthPlugin.APPLE_CONFIGURATION,
  },

The options section could be added in th credentials section of the plugin registration option.

A bit tricky to understand. It would be nice a small notion about this at least in the github page, to avoid devs getting mad.

Especially valid for Apple ID that has this special configuration required.

UPDATE 2: Some interfaces still need to be adjusted ad hoc for Apple login. For example the OAuth2Token is missing some extra field outputted by the Apple login process. In particular OAuth2Token.token.id_token is a missing field, needed to extract user email. I do realize that is a special behavior only belonging to Apple, but a specialized version of the token may be good for users to be aware again.

Thanks again

playleaks avatar Oct 11 '22 23:10 playleaks

It is not something we set the authorizationMethod to header, but in the latest version of simple-oauth. The logic inside simple-oauth is completely changed and it required to pass the option in another place.

You can pass a proper authorizationMethod inside the credentials

fastify.register(oauthPlugin, {
  name: 'appleOAuth2',
  credentials: {
    client: {
      id: '<CLIENT_ID>',
      secret: '<CLIENT_SECRET>'
    },
    auth: oauthPlugin.APPLE_CONFIGURATION,
    options: {
      bodyFormat: 'form',
      authorizationMethod: 'body'
    }
  },
  // register a fastify url to start the redirect flow
  startRedirectPath: '/login/apple',
  // appleredirect here after the user login
  callbackUri: 'http://localhost:3000/login/apple/callback'
})

climba03003 avatar Oct 12 '22 03:10 climba03003

@playleaks I suspect it's not bugged, this is the specifics of the Apple OAuth2 flow. A co-related issue emerged for me around a year ago - https://github.com/fastify/fastify-oauth2/issues/116 and PR https://github.com/fastify/fastify-oauth2/pull/117. There has been another implementation. Also for reference - https://github.com/fastify/fastify-oauth2/pull/118.

As for your UPD2 outlined in the issue:

It's a bit tricky, but when authorizing a user, Apple does a POST request (other social providers do GET) to your apple/callback endpoint. Then you use the @Req() req in that endpoint to get Access Token from Authorization Code Flow via OAuth2Namespace.getAccessTokenFromAuthorizationCodeFlow. This will respond with a new tokenObj that will already contain the id_token you need. You can then use this id_token to verify your user and receive the whole user info. See a diagram below and Apple Docs 1 and 2 for ref:

@playleaks let me know if it's still relevant and if you need any further help.

CC @climba03003 given that can we close this issue? It turns out that your solution above works just fine. For other cases, it's worth creating separate bugs and continuing the discussion there.

avmaxim avatar Feb 18 '23 23:02 avmaxim

@climba03003 @avmaxim first of all thank you for the answers and explanations.

Actually I already read more or less the same Apple Developer pages you sent me, and the diagrams as well, that was exactly the way I managed to solve the issue by myself understanding the specific Apple flow, and using the body authorizationMethod.

As mentioned in UPDATE1 I managed to fix the issue, finding out that I could use the options field in the credentials object at plugin registration.

Coming to your comments:

@playleaks I suspect it's not bugged, this is the specifics of the Apple OAuth2 flow. A co-related issue emerged for me

around a year ago - https://github.com/fastify/fastify-oauth2/issues/116 and PR https://github.com/fastify/fastify-oauth2/pull/117. There has been another implementation. Also for reference - https://github.com/fastify/fastify-oauth2/pull/118.

As I said, I already read all of these pages and documents before. But even if you don't want to call this a "bug" at least we should call it unexpected behavior from a user of your library/developer point of view. Because when I select a specific flow (e.g. auth: oauthPlugin.APPLE_CONFIGURATION) I expect that the library by default setups every other configuration (credentials, authorization, urls, and everything else) to those specifically needed, in fact, by the specified provider, in this case Apple. It makes no sense to use a fast usable reliable library if then I have to debug errors and go and study the Apple Developers guidelines to find out the flow specifications and adjust the library config accordingly (that should already be done). I think it's a loss of time. As I mentioned, at the very least it would be good to specify that in the README.MD of the library, mentioning that Apple is a special case and it can be indicated the need of adding the options field filled as you suggested me above. Of course this is just a suggestion to improve things, considering my experience, but in the end I solved. For the rest you did an awesome job with the library.

As for your UPD2 outlined in the issue:

It's a bit tricky, but when authorizing a user, Apple does a POST request (other social providers do GET) to your apple/callback endpoint. Then you use the @Req() req in that endpoint to get Access Token from Authorization Code Flow via OAuth2Namespace.getAccessTokenFromAuthorizationCodeFlow. This will respond with a new tokenObj that will already contain the id_token you need. You can then use this id_token to verify your user and receive the whole user info. See a diagram below and Apple Docs 1 and 2 for ref:

Yeah I know that it's quite tricky, well here too I read and understood Apple Docs, and I have been able to extract from the id_token the user email address, but what I wanted to say here is that it wasn't created a specific interface, an extension of the generic OAuth2Token.token for Apple flow, that included this extra id_token field. (small ot: Apple normally provides user details and email as a parameter of the callback post body, but they provide this data only the first time the user logs in your app, and never again in the next login attempts, leading to problems in case you have a failure during the first user sign in attempt at saving his data. So from each subsequent login the id_token field is the only way to identify the user by email).

I can of course understand you wanted to keep genericity. But at this point I think even more that some guidance in the readme it's needed for Apple oauth2 flow setup.

Thanks again, I hope I have been clear enough, since couple of months are passed by and I may have been not so precise. On my side the issue is fixed anyways.

Have a good day. Marco

playleaks avatar Feb 19 '23 10:02 playleaks

A PR to improve things is always welcomed!

mcollina avatar Feb 19 '23 15:02 mcollina

@mcollina Sure normally I do PR proposals, but this time I didn't have time to. Anyway I well elaborated the problem so that anyone with time and will could work on it!

playleaks avatar Feb 19 '23 16:02 playleaks

@avmaxim @climba03003

I just reopened the code after some time, and double checked my working solution:

  1. I discovered that for me it's not needed adding in the options both authorizationMethod: 'body' and bodyFormat: 'form'. What I need to have is only authorizationMethod, while bodyFormat has no effect on the library behavior.
image
  1. In the callback code I had to extract the state and code from the body and to fake put them among the request query params, so that the fastify.apple.getAccessTokenFromAuthorizationCodeFlow works, otherwise it throws exception, cause it's looking for state and code in the query field that instead are null otherwise.
image

I think that at least point 2) can be considered a bug. Let me know if I did something wrong otherwise. But for me it's been the only way to get it working so far!

playleaks avatar Feb 19 '23 19:02 playleaks

@playleaks @mcollina @climba03003 I've documented and implemented the specifics of the Apple OAuth2 flow in this PR so that other developers don't get confused with it and easily replicate the example here - https://github.com/fastify/fastify-oauth2/pull/189

Now I believe we can close this one when the PR is merged to master

avmaxim avatar Feb 19 '23 23:02 avmaxim

@avmaxim Well done. It seems good to me. I would just point this example out too in the RADME.MD at the repository root. Thanks for taking time to make this PR.

playleaks avatar Feb 20 '23 00:02 playleaks

@playleaks No problem! Btw in README.MD we already have a reference to the Examples folder so that all developers could jump in and learn deeper the implementation specifics of each social provider. So I suggest to leave it as is for now.

avmaxim avatar Feb 20 '23 09:02 avmaxim