Skip to content
This repository has been archived by the owner on Jun 30, 2023. It is now read-only.

Add openId scope to all requests via ATHandlerBase [do not merge before discussing] #1605

Closed
wants to merge 1 commit into from

Conversation

jennyf19
Copy link
Contributor

@jennyf19 jennyf19 commented May 22, 2019

Check for null values on UserInfo
Add a utest

null ref issue

Also did some manual testing w/all the flows on the desktop app.
If we don't want to take the change of adding openid scope to all requests, we can just fix the null ref instead.

Check for null values on UserInfo
Add a utest
Copy link
Contributor

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jennyf19 . Which problem is it solving?
Does it make sense for client credentials?

@jmprieur
Copy link
Contributor

Assuming here this is attempting to fix #1604
See my comment in the issue. I think that the problem is with the Web app, not with ADAL.

@jmprieur jmprieur changed the title Add openId scope to all requests via ATHandlerBase Add openId scope to all requests via ATHandlerBase [do not merge before discussing] May 23, 2019
@henrik-me
Copy link
Contributor

@jmprieur @jennyf19 : if I undertstand @jennyf19 correctly, we do this for MSAL already. Seems like something we can also for for ADAL? (even though I believe the right/better approach for web apps is what JM as suggested in the issue). Anyway we should definitely improve the error message and add information to the wiki/ref-doc on this.

@@ -326,7 +326,8 @@ protected virtual async Task PostTokenRequestAsync(AdalResultWrapper resultEx)
{
var requestParameters = new DictionaryRequestParameters(Resource, ClientKey)
{
{ OAuthParameter.ClientInfo, "1" }
{ OAuthParameter.ClientInfo, "1" },
{ OAuthParameter.Scope, OAuthValue.ScopeOpenId }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may want to add a comment on why this is added (e.g. needed due to cache requirements on the idt).

Copy link
Contributor

@jmprieur jmprieur May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we add openid for client creds? that would be odd ?

@jennyf19 jennyf19 closed this May 30, 2019
@jennyf19 jennyf19 deleted the jennyf/idTokenNull branch June 14, 2019 19:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants