Support error_subcode - Githubissues

sliekens commented 6 years ago

When you do a login() with prompt=select_account then you also get the option to cancel the login.

Clicking Back causes a redirect with the following callback hash:

#error=access_denied&error_subcode=cancel&state=1f4a27f1-0868-437a-bec9-e62ab9b2f854&add_account=

This is not currently supported because it doesn't contain an error_description. Calling handleWindowCallback() does nothing.

nehaagrawal commented 5 years ago

@StevenLiekens can you please confirm if this issue is still reproducible?

sliekens commented 5 years ago

I don't know, it seems that the cancel button was removed? Or I'm doing something different that makes it not show up anymore.

sliekens commented 5 years ago

Yep, still reproducible. I must have been doing something different earlier.

See screenshot. The callback contains error but not error_description. Adal.js requires error_description.

This is the offending code: https://github.com/AzureAD/azure-activedirectory-library-for-js/blob/98d7200bc11ebf50405c2dd1a142d99e324c9804/lib/adal.js#L1001-L1014

nehaagrawal commented 5 years ago

@StevenLiekens Yesterday you reported that the back button was missing. Now you are able to see it again. Ideally you shouldn't see the back button at all. I am curious to know in what condition you are seeing the back button. Can you please tell me how to reproduce the scenario where you can see the back button and one where you don't see it?

sliekens commented 5 years ago

It sounds like you already know more about this than I do. I'm not doing anything unusual other than sticking &prompt=select_account at the end.

Maybe it's something unusual in my client registration?

nehaagrawal commented 5 years ago

@StevenLiekens I am not able to see the back button and the expected behavior is not to see it. There must be an edge case scenario where you can see the back button. I tried sticking prompt=select_account and I can't reproduce this scenario. Can you please send me your adal config? I want to see if there is something there? can you please tell me how you are sticking the &prompt=select_account? Are you passing it as an extraqueryparameter in the adal config?

sliekens commented 5 years ago

Yep it's an extraqueryparameter in my config. I'll show you the rest of my config on Monday. I can't access my code from home (company policy).

sliekens commented 5 years ago

PS I think the back button is nice to have, is there a reason why it's expected to be invisible?

sliekens commented 5 years ago

Here's my config (details obfuscated because ehhhh)

"config": {
  "clientId": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx",
  "tenant": "xxxxxxx.onmicrosoft.com",
  "redirectUri": "http://localhost:8080/callback",
  "postLogoutRedirectUri": "http://localhost:8080/login",
  "navigateToLoginRequestUrl": true,
  "endpoints": {
    "https://xxxxxxxxxxx.xxxx.xxxxx.xx": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
  },
  "popUp": false,
  "extraQueryParameter": "prompt=select_account",
  "loginResource": "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx",
  "anonymousEndpoints": []
}

nehaagrawal commented 5 years ago

@StevenLiekens I can't reproduce the back button. Is it possible for you to send the AAD login url that is generated in the url after obfuscated sensitive information ?

sliekens commented 5 years ago

I will get back to you on that but in the meantime could you tell me why the button should be invisible?

sliekens commented 5 years ago

Full request URL (with extra line breaks for legibility)

https://login.microsoftonline.com/xxxxxxx.onmicrosoft.com/oauth2/authorize
?response_type=id_token
&client_id=xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
&redirect_uri=http%3A%2F%2Flocalhost%3A8080%2Fcallback
&state=7d951f31-7f67-4597-af61-9cb974f8b81b
&prompt=select_account
&client-request-id=cf579406-5031-4c66-bbf0-183d5c3b5331
&x-client-SKU=Js&x-client-Ver=1.0.17
&nonce=f65725f7-e2c5-4b55-9512-c551542218b2
&sso_reload=true

Other info, in case you have access to server logs

Request Id: 103172c8-8474-4787-ba5c-1b60a1d66700 Correlation Id: cf579406-5031-4c66-bbf0-183d5c3b5331 Timestamp: 2018-10-23T07:48:55.123Z

sliekens commented 5 years ago

@nehaagrawal I narrowed it down to the client_id. If I change 1 character of the client_id, the back button disappears.

So this is caused by something funny in my app registration. I don't know what to do at this point?

navyasric commented 5 years ago

@StevenLiekens Are you still seeing this issue or has it been resolved for you? What is the behavior you expect if you still see the issue?

sliekens commented 5 years ago

Yes, still an issue. I don't know what other info I can provide. I'm seeing a back button that's not supposed to be there according to @nehaagrawal. I narrowed the issue down to "something" in my app registration (manifest.json).

Long story short, I expect the Back button to go away if it's not supposed to be there. Or if it turns out this works as intended, make adal.js handle Back button redirects (which use error_subcode instead of error_description).

PS so far nobody has been able to tell me why the back button shouldn't be there... or why it does exist if it's not supposed to...

devconcept commented 5 years ago

If you need special permissions for an app and the admin has not granted you those, then you get an option to "Return to the application without granting consent" and you hit this bug too.

sliekens commented 5 years ago

I propose the following fix:

add a constant for the error_subcode parameter

/**
 * Enum for storage constants
 * @enum {string}
 */
this.CONSTANTS = {
    ERROR: 'error',
+   ERROR_SUBCODE: 'error_subcode',
    STORAGE: {
        ERROR: 'adal.error',
+       ERROR_SUBCODE: 'adal.error.subcode'
    },
    ...
};

Change isCallback to check for errors, not error descriptions

AuthenticationContext.prototype.isCallback = function (hash) {
    hash = this._getHash(hash);
    var parameters = this._deserialize(hash);
    return (
-       parameters.hasOwnProperty(this.CONSTANTS.ERROR_DESCRIPTION) ||
+       parameters.hasOwnProperty(this.CONSTANTS.ERROR) ||
        parameters.hasOwnProperty(this.CONSTANTS.ACCESS_TOKEN) ||
        parameters.hasOwnProperty(this.CONSTANTS.ID_TOKEN)
    );
};

Change getRequestInfo to check for errors, not error descriptions

-if (parameters.hasOwnProperty(this.CONSTANTS.ERROR_DESCRIPTION) ||
+if (parameters.hasOwnProperty(this.CONSTANTS.ERROR) ||
    parameters.hasOwnProperty(this.CONSTANTS.ACCESS_TOKEN) ||
    parameters.hasOwnProperty(this.CONSTANTS.ID_TOKEN)) {

    requestInfo.valid = true;

Change saveTokenFromHash to check for errors, not error descriptions

AuthenticationContext.prototype.saveTokenFromHash = function (requestInfo) {
    this.info('State status:' + requestInfo.stateMatch + '; Request type:' + requestInfo.requestType);
    this._saveItem(this.CONSTANTS.STORAGE.ERROR, '');
+   this._saveItem(this.CONSTANTS.STORAGE.ERROR_SUBCODE, '');
    this._saveItem(this.CONSTANTS.STORAGE.ERROR_DESCRIPTION, '');

    var resource = this._getResourceFromState(requestInfo.stateResponse);

    // Record error
-   if (requestInfo.parameters.hasOwnProperty(this.CONSTANTS.ERROR_DESCRIPTION)) {
+   if (requestInfo.parameters.hasOwnProperty(this.CONSTANTS.ERROR)) {
        this.infoPii('Error :' + requestInfo.parameters.error + '; Error description:' + requestInfo.parameters[this.CONSTANTS.ERROR_DESCRIPTION]);
        this._saveItem(this.CONSTANTS.STORAGE.ERROR, requestInfo.parameters.error);
+       this._saveItem(this.CONSTANTS.STORAGE.ERROR_SUBCODE, requestInfo.parameters[this.CONSTANTS.ERROR_SUBCODE]);
        this._saveItem(this.CONSTANTS.STORAGE.ERROR_DESCRIPTION, requestInfo.parameters[this.CONSTANTS.ERROR_DESCRIPTION]);

        if (requestInfo.requestType === this.REQUEST_TYPE.LOGIN) {
            this._loginInProgress = false;
            this._saveItem(this.CONSTANTS.STORAGE.LOGIN_ERROR, requestInfo.parameters.error_description);
        }
    }

Handle ERROR_SUBCODE in _handlePopupError

- AuthenticationContext.prototype._handlePopupError = function (loginCallback, resource, error, errorDesc, loginError) {
+ AuthenticationContext.prototype._handlePopupError = function (loginCallback, resource, error, errorSubcode, errorDesc, loginError) {
    this.warn(errorDesc);
    this._saveItem(this.CONSTANTS.STORAGE.ERROR, error);
+   this._saveItem(this.CONSTANTS.STORAGE.ERROR_SUBCODE, errorSubcode);
    this._saveItem(this.CONSTANTS.STORAGE.ERROR_DESCRIPTION, errorDesc);

and so on and so on

jmckennon commented 4 years ago

Currently, the sub_error or error_subcode fields are supported by neither adal@1.0.18, nor by msal@1.2.0. sub_error specifically will be supported sometime in Q12020 in msal, after the release of 2.0.0 and the introduction of the auth-code flow. The server team currently doesn't provide any of this information on their implicit flow endpoints, so we are unable to surface this info currently.

Nonetheless, I do believe that this bug should not exist in msal@1.2.0.

All current authentication work from Microsoft is delivered through the msal js library here. adal js is still supported only for security fixes. We recommend moving to msal js for any advanced feature requests and bugfixes.

AzureAD / azure-activedirectory-library-for-js

Support error_subcode #804