Fatal error if `data['email']` is missing

[kevinrenskers]

https://github.com/hwjeremy/siwa-python/blob/main/siwa/library/token/payload.py#L62

The email is not always there, and when it's not, you get a fatal KeyError. In fact, there are more properties that are optional, for example this is the JWT I get for one of my users:

{
  "iss": "https://appleid.apple.com",
  "aud": "[value]",
  "exp": 1634418396,
  "iat": 1634331996,
  "sub": "[value]",
  "c_hash": "[value]",
  "auth_time": 1634331996,
  "nonce_supported": true
}

So both email and email_verified are missing there and will cause KeyError. Probably a lot safer to user .get(key, default_value) for all these data properties.

[hwjeremy]

@kevinrenskers Hmm - Thinking about this. If I remember correctly (and it has been awhile since I was looking at this in detail), Apple only provides user data on the first request, not subsequent ones. When I wrote siwa-python I was of the mindset that I wanted to roll with Apple's idea of how SIWA flow should work, and not bend it to how I wanted it to work.

If the email is missing, that to me indicates that I've made a subsequent call where I shouldn't have. The user has already gone through the SIWA flow, and I was already given the data I need. I remember this being pretty mind bending when I initially developed SIWA sign ins. Given Apple's behaviour, what would probably be appropriate here is an Exception, making the failure explicit and understandable.

I absolutely despise making properties optional unless there is an extremely good reason to do so. Every optional multiplies potential states.

It's of course possible that I'm wrong here, not remembering this possibly, but I would like to think on it for a bit rather than adding state.

[kevinrenskers]

You can simply not request the email address from the user at all, the user can deny sharing their email address. So that is already a huge reason for making email optional!

Also, if the user is signed in via Apple, then logs out and they want to sign in again, how are we then supposed to verify the ID token we get that second time? We are making a subsequent call, and we're not going to receive the user details, but this is definitely a call we should be making.

Email can't be required if we sometimes get it from Apple and sometimes not. Your library breaks perfectly valid usage scenarios: not requesting the user email at all, and logging in a second time.

[hwjeremy]

@kevinrenskers I get the "not requesting email" argument... that's a good point. The subsequent call argument, I can't describe why right now, but I have strong memories of some painful realisation about subsequent calls when I originally built SIWA sign ins. I can't remember what the realisation was, all I remember was that I realised I was attacking the problem wrong when trying to get data out of Apple more than once.

Let me think on how to handle this... I would rather handle it with strong types rather than optionals. Consider that if I make emails optional, all my systems that use this library will no longer be type safe. They will all have to be modified to handle the multiplied states created by the optionals.

I'll come back with some thoughts soon!

[kevinrenskers]

The way it's meant to work:

1. User logs in for the very first time, you get an id token that contains user details. You store these details in your own database.
2. User logs in a subsequent time, you now get an id token that doesn't contain user details. But since you've (presumably) already stored it that's not really a problem. But this second token should still be verifiable.

There is just no way that making email address required makes sense, as far as I can see.

Consider that if I make emails optional, all my systems that use this library will no longer be type safe.

If you handle the optionals, they're still "type safe" though, right?

But yeah looking forward to hear your thoughts, I am using my fork in the meantime.

[kevinrenskers]

Can we revisit this? I'd like to not rely on my own fork anymore for my project, as that is a bit of a maintenance liability.