Pass request to authorizationParams to support additional parameters

[leandrakelly]

Summary:

This pull request introduces an update to the authorizationParams method in the OAuth2Strategy class. The changes allow additional URL search parameters to be included in the authorization request. This update is reflected in both the code and the documentation.

Issue Addressed:

This update addresses a regression issue in the authorizationParams method. Previously, the code for handling request parameters was:

let params = new URLSearchParams(
  this.authorizationParams(new URL(request.url).searchParams),
);

In this implementation, the method effectively integrated both default and request-specific parameters. The regression occurred when the authorizationParams method was updated and no longer received request-specific parameters. This change disrupted the integration of these parameters into the final authorization URL, impacting the OAuth2 authentication flow.

Updated Implementation:

The updated authorizationParams method now accepts an additional requestParams parameter to handle request-specific parameters alongside default parameters:

protected authorizationParams(
  params: URLSearchParams,
  requestParams?: URLSearchParams,
): URLSearchParams {
  if (this.audience) params.set("audience", this.audience);
  if (requestParams?.get('screen_hint')) params.set('screen_hint', 'signup');
  return params;
}

In this example, params represent the default parameters, and requestParams include additional parameters like 'screen_hint' based on specific requirements.

closes #104

[ryanwoodcox]

Nice change! Can we get this published to npm?

@sergiodxa I am implementing a Google oauth strategy based on remix-auth-oauth2 and noticed there have been some breaking changes since v1.11.2. I'd love to see how you handle what I'm running into if/when you upgrade your remix-auth-github package to use the latest version remix-auth-oauth2.

It seems callbackURL was renamed to redirectURI and this path is no longer processed by logic like the former getCallbackURL function that checked if the protocol and the host should be appended to the redirectURI.

I don't have access to the request object when creating my new GoogleStrategy, but with this change, I could leverage the request passed into authorizationParams and do something like:

    params.set(
      'redirect_uri',
      getRedirectURI(this.options.redirectURI.toString(), request!).toString(),
    );

I'm also curious why is the request typed as Request | undefined in the new signature for authorizationParams?

[ryanwoodcox]

@sergiodxa Thanks for all of the fast updates!

I got a bit further then ran into the same issue in the updated async authenticate(request, sessionStorage, options) function. This function was previously leveraging getCallbackURL via let callbackURL = this.getCallbackURL(request); in order to conditionally append the protocol and the host to relative URLs provided by the developer.

I'm not as sure about how to handle this one. Something hacky that may lead to bugs could be to, within the overridden authorizeParams method, not only massage set the redirect_uri param, but also update the redirectURI in the options:

    this.options.redirectURI = getRedirectURI(this.options.redirectURI.toString(), request);
    params.set(
      'redirect_uri',
      this.options.redirectURI.toString(),
    );

I'm still eager to see how you would handle updating code bases such as epic-stack that are providing relative callbackURLs (redirectURIs) to consume the updated version of remix-auth-github and remix-auth-oauth2.

[sergiodxa]

The best approach IMO, is to put the code that creates the authenticator and strategies in a function that receives the current request and use that to create the URL you need.

So instead of a singleton instance if the authenticator you create one per request.

The classes are simple enough for this to not be a problem, but if it's a problem you can also cache it so you only really create one instance the first time.

[ryanwoodcox]

@sergiodxa Sounds reasonable!

It also does not look like google returns a refresh_token after the the first time the user authorizes the app. What do you think about making Result.refreshToken optional so that the toJSON doesn't throw an error on the refreshToken method of TokenRequestResult?