openid / AppAuth-JS

JavaScript client SDK for communicating with OAuth 2.0 and OpenID Connect providers.
Apache License 2.0
986 stars 161 forks source link

Issues with handling the azure B2C user flows #188

Open coicoronado opened 3 years ago

coicoronado commented 3 years ago

Expected Behavior

Let the user handle the query params that are inside the Url for configuration discovery with attached query params

[REQUIRED] Describe expected behavior

Given an URL with a query params (ex. https://login.microsofonline.com/{tenant}/v2.0?p=B2C_1_nfwSignIn) make the URL formulated to construct a valid URL (ex. https://login.microsofonline.com/{tenant}/v2.0/.well-known/openid-configuration?p=B2C_1_nfwSignIn)

Describe the problem

The Issue comes when you pass the URL as stated above the formulated URL is not a valid one given the concatenation that occurs on the file authorization_service_configuration.ts

Also see this reference from Azure documentation

[REQUIRED] Actual Behavior

On function fetchFromIssuer Input https://login.microsofonline.com/{tenant}/v2.0?p=B2C_1_nfwSignIn Returns https://login.microsofonline.com/{tenant}/v2.0?p=B2C_1_nfwSignIn/.well-known/openid-configuration

[REQUIRED] Steps to reproduce the behavior

Just place the URL https://login.microsofonline.com/{tenant}/v2.0?p=B2C_1_nfwSignIn in the fetchFromIssuer function, I'm using a 3rd party plugin that has a dependency on this repo ionic-appauth

[REQUIRED] Environment

This code resolves my issue but I'm not sure how to make a pull request

static fetchFromIssuer(openIdIssuerUrl: string, requestor?: Requestor):
      Promise<AuthorizationServiceConfiguration> {
    const searchForQueryParams = function(url: string) {
      let result;
      let queryOr: any = url.split('/');
      let query = queryOr[queryOr.length - 1].split('?');
      if (query.length > 1) {
        queryOr.splice(queryOr.length - 1, 1);
        queryOr = queryOr.join('/');
        result = [queryOr, `?${query[query.lenght - 1]}`];
      } else {
        result = [url, ''];
      }

      return result;
    };
    const newUrl = searchForQueryParams(openIdIssuerUrl);
    const fullUrl = `${newUrl[0]}/${WELL_KNOWN_PATH}/${OPENID_CONFIGURATION}${newUrl[1]}`;

    const requestorToUse = requestor || new JQueryRequestor();

    return requestorToUse
        .xhr<AuthorizationServiceConfigurationJson>({url: fullUrl, dataType: 'json', method: 'GET'})
        .then(json => new AuthorizationServiceConfiguration(json));
  }
tikurahul commented 3 years ago

That is a bug. Will look into it.

coicoronado commented 3 years ago

The pull request is still pending approval does it need anything else to be worked on? so this can take the next step

vanpyrzericj commented 2 years ago

@tikurahul curious if you will have time soon to review this PR as I also have a project that is dependent on this being merged and resolved. Thanks!

TechnoChimp commented 2 years ago

Since this isn't merged in yet, as a workaround I was able to get past this by creating a custom Requestor which just adds the needed query param:

export default class MyRequestor extends Requestor {
  xhr<T>(settings: JQueryAjaxSettings): Promise<T> {
    return new Promise<T>((resolve, reject) => {
      // implementing a subset that is required.
      const url = Url.parse(settings.url!);
      const data = settings.data;
      console.log(url)
      const options = {
        hostname: url.hostname,
        port: url.port,
        path: url.path,
        query: { client_id: config.auth.clientId },
        method: settings.method,
        headers: settings.headers || {}
      };
...
mvisosky commented 2 years ago

Outside of the underlying issue involving the interpretation of querystring parameters, I wanted to point out that the Azure AD B2C policy name does not have to be included as a querystring parameter for the OpenID Connect discovery document. The B2C policy name can be included as a path segment in the url which negates the need for a querystring parameter.

Example:

https://fabrikamb2c.b2clogin.com/fabrikamb2c.onmicrosoft.com/b2c_1_sign_in/v2.0/.well-known/openid-configuration

coicoronado commented 2 years ago

Sure, but some configurations inside enterprises flow cant be changed for some reason (bureaucracy or legacy) that are available inside azure b2c, therefore the need for this change.

Is not like maintaining an old way of doing this, but unless Azure removes completely this option it should be able to handle them as mentioned in the request and MR