Skip to content
This repository has been archived by the owner on Mar 3, 2022. It is now read-only.

'scope' in SigninResponse #140

Closed
henetiriki opened this issue Oct 11, 2016 · 7 comments
Closed

'scope' in SigninResponse #140

henetiriki opened this issue Oct 11, 2016 · 7 comments
Labels
Milestone

Comments

@henetiriki
Copy link

Hi Brock

I've been spending some time trying to understand why user claims were not being processed upon login/silent login.

I found that ResponseValidator#response.isOpenIdConnect is returning false. Tracing it back further, it appears that there is no scope parameter sent back as part of the redirect callback hash from the OP..

If I intercept SigninResponse and add scope=openid to the URL before parsing it (UrlUtility.parseUrlFragment), claims are processed as expected.

Am I missing something or should there be another way of ensuring that claims are being processed. It appears that the spec does not require this param to be sent back.

My workaround is to call the user endpoint myself using the access_token from the User, but would be great to be able to skip this step.

Thanks :)

@brockallen
Copy link
Member

Yes, you're right. Perhaps the logic should key off the presence/absence of the id_token in the response. Mind testing with this change to SigninResponse:

    get isOpenIdConnect(){
        return !!this.id_token;
    }

@brockallen brockallen added the bug label Oct 11, 2016
@brockallen brockallen added this to the 1.2.1 milestone Oct 11, 2016
@henetiriki henetiriki changed the title [Question] 'scope' in SigninResponse 'scope' in SigninResponse Oct 11, 2016
@henetiriki
Copy link
Author

This works well.

Would it hurt to have both?

    get isOpenIdConnect() {
        return this.scopes.indexOf(OidcScope) >= 0 || !!this.id_token;
    }

I'll get a PR ready.

@brockallen
Copy link
Member

hmm, yea, i guess. i'd have to think about it.

@brockallen
Copy link
Member

brockallen commented Oct 11, 2016

merged, thanks.

@brockallen
Copy link
Member

1.2.1-beta.1 pushed to npm. please test and let me know. thanks.

@henetiriki
Copy link
Author

Will do, thanks.

@henetiriki
Copy link
Author

Confirmed that it's working 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

2 participants