Skip to content

MSAL writing null family_id value in JSON string on iOS #1176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jennyf19 opened this issue May 29, 2019 · 7 comments
Closed

MSAL writing null family_id value in JSON string on iOS #1176

jennyf19 opened this issue May 29, 2019 · 7 comments

Comments

@jennyf19
Copy link
Collaborator

Which Version of MSAL are you using ?
i think from 3.0.1-preview

Platform
xamarin iOS

Actual behavior
In the JSON string, we are writing null for the family_id value, it should be string.empty or ""

  "home_account_id": "9f44...a8ca",

  "environment": "login.windows.net",

  "client_info": "eyJ1aWQiOiI5Z...RhOGNhIn0",

  "client_id": "4b0db...6f8e0",

  "secret": "OAQ...A9K1dIAA",

  "credential_type": "RefreshToken",

  "family_id": null

Am working on a repro of this to provide more context. will update tomorrow.

@bgavrilMS
Copy link
Member

No need to spend time reproducing this, we probably have unit tests around it.

According to the JSON spec, null is a valid value and is equivalent to empty string: http://www.json.org/
See also: https://stackoverflow.com/questions/21120999/representing-null-in-json

I do not understand customer impact here.

@jennyf19
Copy link
Collaborator Author

@bgavrilMS Here's the issue:

  1. We are omitting all the other missing fields, and only writing "null" for family_id
  2. Null is valid, but in Obj-C it can cause issues as it's translated to NSNull, which can result in issues for apps that don't except NSNulls.
  3. When we did cache compat testing w/native iOS, we were behaving like the other SDKs and not writing null fields to JSON. This change might cause production apps to crash.

@jmprieur
Copy link
Contributor

@bgavrilMS : is it the issue you fixed?

@bgavrilMS
Copy link
Member

Waiting for @henrik-me with instructions on this. PR is out, but not merged.

@MarkZuber
Copy link
Contributor

effectively a dup of #1189 which is fixed.

@MarkZuber MarkZuber added this to the 4.1 milestone Jun 18, 2019
@henrik-me
Copy link
Contributor

Yup, it's a dupe as 1189 is more generic. Should validate this scenario though with the fix for #1189

@jmprieur
Copy link
Contributor

jmprieur commented Jul 2, 2019

this is now fixed and available in MSAL.NET 4.1

@jmprieur jmprieur closed this as completed Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants