TAN-8614 Omniauth vulnerability - #14728
Conversation
|
jamesspeake
left a comment
jamesspeake
left a comment
There was a problem hiding this comment.
Logic is sound I think, but arranging a quick catchup as I've added some comments to the ticket in terms of the methods this should affect (or not) and I think there is overlap with the ticket I was working on. Important to get this right, not only for security but to not put extra barriers in place for customers when not needed.
| end | ||
|
|
||
| def email_always_present? | ||
| config[:provider] == 'rheinbahn' |
There was a problem hiding this comment.
This logic will need to stay as keycloak covers two providers which work in slightly different ways
| 'name' => 'Theo Musterman', | ||
| 'email' => 'Theo.Musterman@ctie.etat.lu', | ||
| 'email_verified' => nil, | ||
| 'email_verified' => true, |
There was a problem hiding this comment.
Need to check the actual response here (only on staging at the moment)
| expect(response).to redirect_to('/fr-FR/?param=some-param&sso_flow=signup&sso_success=true') | ||
| end | ||
|
|
||
| context 'when the auth response says the email is not verified' do |
There was a problem hiding this comment.
This would contradict confirmed_email_always_present?
| email: nil, | ||
| new_email: 'alexander@citizenlab.co', |
There was a problem hiding this comment.
So, if email is not confirmed it always gets created as new_email?
|
|
||
| def email_always_present? | ||
| true | ||
| def confirmed_email_always_present? |
There was a problem hiding this comment.
Not sure I'm totally getting why confirmed_email_always_present? if we still rely on email_confirmed? as well
3 participants
Please read the "Summary and solution" section on the ticket for more context:
https://app.notion.com/p/govocal/Fix-omniauth-vulnerability-3ce9663b7b2680fc8a2be45b6b5042c6
Let me know if the changes in behavior are fine. If the changes in behavior will cause problems for clients, and we are sure that the affected methods return a confirmed email (even though they don't properly follow the openid spec), we can add more overrides. But in these cases it would be good if we could add a comment justifying this override, e.g. by linking to the documentation of the method itself or an explanation that we checked this with the client or something.
Changelog
Technical
email_verifiedattribute