From: "Vlastimil Elias" <velias(a)redhat.com&gt; To: keycloak-dev(a)lists.jboss.org Sent: Tuesday, 7 April, 2015 10:57:16 AM Subject: [keycloak-dev] How to handle empty strings returned by Social login providers in user info - KEYCLOAK-1182 Hi, during latest testing I find problem with empty string returned in email field from GitHub social provider, which causes http 500 error in later processing (but seems under some other circumstances only, not for all cases), see https://issues.jboss.org/browse/KEYCLOAK-1182 When I look into the code used to take used profile informations (email, name, id) from Social provider REST responses, it simply takes what is returned and do not care too much what is here. But other Keycloak code (eg search user by email etc) typically only check for null values when testing "existence" of information. If value is not null then it takes it as existing one, so empty strings may bring problems here as it is used as valid email later. I believe KC should look at what is returned from Social providers and convert empty strings to null values. It is only small change at one place - AbstractOAuth2IdentityProvider.getJsonProperty() which resolves this problem. What do you think about this solution? I have patch prepared and it works, I can post it as pull request after some additional testing. Vl. -- Vlastimil Elias Principal Software Engineer jboss.org Development Team _______________________________________________ keycloak-dev mailing list keycloak-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/keycloak-dev

From: "Vlastimil Elias" <velias(a)redhat.com&gt; To: "Stian Thorgersen" <stian(a)redhat.com&gt; Cc: keycloak-dev(a)lists.jboss.org Sent: Tuesday, 7 April, 2015 12:49:08 PM Subject: Re: [keycloak-dev] How to handle empty strings returned by Social login providers in user info - KEYCLOAK-1182 Hi, sure, I'll patch the second problem also. I'd also like to add unit test (based on junit) directly into keycloak-broker-oidc project to cover behaviour of getJsonProperty() method, as I think that is is better place than keycloak-testsuite-integration project used for integration tests.

Vl. On 7.4.2015 12:22, Stian Thorgersen wrote: > Sounds good. I guess you'll fix the null -> "null" issue at the same time? > > ----- Original Message ----- >> From: "Vlastimil Elias" <velias(a)redhat.com&gt; >> To: keycloak-dev(a)lists.jboss.org >> Sent: Tuesday, 7 April, 2015 10:57:16 AM >> Subject: [keycloak-dev] How to handle empty strings returned by Social >> login providers in user info - KEYCLOAK-1182 >> >> Hi, >> >> during latest testing I find problem with empty string returned in email >> field from GitHub social provider, which causes http 500 error in later >> processing (but seems under some other circumstances only, not for all >> cases), see https://issues.jboss.org/browse/KEYCLOAK-1182 >> >> When I look into the code used to take used profile informations (email, >> name, id) from Social provider REST responses, it simply takes what is >> returned and do not care too much what is here. >> >> But other Keycloak code (eg search user by email etc) typically only >> check for null values when testing "existence" of information. If value >> is not null then it takes it as existing one, so empty strings may bring >> problems here as it is used as valid email later. >> >> I believe KC should look at what is returned from Social providers and >> convert empty strings to null values. >> It is only small change at one place - >> AbstractOAuth2IdentityProvider.getJsonProperty() which resolves this >> problem. >> >> What do you think about this solution? >> >> I have patch prepared and it works, I can post it as pull request after >> some additional testing. >> >> Vl. >> >> -- >> Vlastimil Elias >> Principal Software Engineer >> jboss.org Development Team >> >> _______________________________________________ >> keycloak-dev mailing list >> keycloak-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/keycloak-dev >> -- Vlastimil Elias Principal Software Engineer jboss.org Development Team