search

nhost / hasura-auth

Authentication for Hasura.
https://nhost.io
MIT License
372 stars 110 forks source link

app crashing when no email returned in accesstoken by IDP (AzureAD) #507

Open xmlking opened 2 months ago

xmlking commented 2 months ago

I am trying hasura-auth with AzureAD, building webapp with hasura-auth-js SDK. my app crashing after trying to login with AzureAD SSO. here is the logs

error in hasura-auth logs

auth          | {"time":"2024-04-24T02:53:43.499422588Z","level":"WARN","msg":"email didn't pass access control checks","trace":{"trace_id":"01d71ae9-7306-4a72-884e-78b4f6fbafde","span_id":"","parent_span_id":""},"request":{"client_ip":"192.168.65.1","method":"POST","url":"/v1/token"}}
auth          | {"time":"2024-04-24T02:53:43.501355046Z","level":"ERROR","msg":"call completed with errors","trace":{"trace_id":"01d71ae9-7306-4a72-884e-78b4f6fbafde","span_id":"","parent_span_id":""},"request":{"client_ip":"192.168.65.1","method":"POST","url":"/v1/token"},"response":{"status_code":200,"latency_time":2491666,"errors":["json: error calling MarshalJSON for type types.Email: email: failed to pass regex validation"]}}

webapp crash logs

image image

accesstoken return for AzureAD don't have email at right place image

BUT this should not cause server crash... 

### Tasks
xmlking commented 1 month ago

Just a note, apparently Microsoft have different AD subscriptions, based on plan, expensive corporate plan adds email claim correctly but for low cost plan, email claim is added at upn and unique_name fields. (we have to upgrade to expensive plan to customize claims)

Since getting email claim consistently is crucial for hasura-auth/hasura-auth-js to work, we need custom configuration option for AzureAD provider to map email field OR automatically fallback to upn or unique_name if email field is null or undefined. Please let me know if I can contribute backward compatible fix via PR

dbarrosop commented 1 month ago

Please let me know if I can contribute backward compatible fix via PR

Of course, it will be more than appreciated/welcomed :)

dbarrosop commented 5 days ago

@xmlking would you mind checking if the issue still persists in 0.32.0? While AzuredAD's email is probably not being populated yet we remove the requirement of having an email so the crash should be gone.

xmlking commented 3 days ago

@xmlking would you mind checking if the issue still persists in 0.32.0? While AzuredAD's email is probably not being populated yet we remove the requirement of having an email so the crash should be gone.

Yes crash issue is fixed with recent changes But I will be contributing fallback email mapping for azure AD when I find little time Thanks