Bug #17748
closedOIDC should read given name / family name fields
Added by Peter Amstutz over 3 years ago. Updated almost 3 years ago.
Description
Arvados should be using the standard given_name
and family_name
claims:
https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims
Related issues
Updated by Nico César over 3 years ago
currently in getAuthInfo() we assume that a verified email can have the Name splitted into 2 with the last space being the divider
} else if verified, _ := claims[ctrl.EmailVerifiedClaim].(bool); verified || ctrl.EmailVerifiedClaim == "" { // Fall back to this info if the People API call // (below) doesn't return a primary && verified email. name, _ := claims["name"].(string) if names := strings.Fields(strings.TrimSpace(name)); len(names) > 1 { ret.FirstName = strings.Join(names[0:len(names)-1], " ") ret.LastName = names[len(names)-1] } else if len(names) > 0 { ret.FirstName = names[0] } ret.Email, _ = claims[ctrl.EmailClaim].(string) }
I think this logic is wrong, or at least there is a lot of double surnames https://en.wikipedia.org/wiki/Spanish_naming_customs
but then on line 220:
// The given/family names returned by the People API and // flagged as "primary" (if any) take precedence over the // split-by-whitespace result from above. for _, name := range person.Names { if name.Metadata != nil && name.Metadata.Primary { ret.FirstName = name.GivenName ret.LastName = name.FamilyName break } }
if given name and family name are part of the claims, they are priotized.
Updated by Nico César over 3 years ago
5c8a7da1f403b44adfdd1f132988bdffc21d9228 17748-extra-claims,
reveals a decision to be made... if PeopleAPI is present, but we have claims that have given_name and first_name .... how should we proceed?
======= test lib/controller/localdb time="2021-06-07T19:51:17Z" level=info msg="ldap lookup returned no results" URL="ldap://127.0.0.1:33343/" error="LDAP Result Code 32 \"No Such Object\": " search="(uid=goodusername)" time="2021-06-07T19:51:17Z" level=info msg="ldap user authentication failed" DN="cn=badusername,dc=example,dc=com" URL="ldap://127.0.0.1:33343/" error="LDAP Result Code 49 \"Invalid Credentials\": " search="(uid=badusername)" time="2021-06-07T19:51:18Z" level=info msg="skipping unverified email address" address=joe.smith@unverified.example.com ---------------------------------------------------------------------- FAIL: login_oidc_test.go:476: OIDCLoginSuite.TestGoogleLogin_OIDCRealName serveOIDC: got req: GET /.well-known/openid-configuration map[] serveOIDC: got req: POST /token map[code:[abcdefgh-1623095479] grant_type:[authorization_code] redirect_uri:[https://127.0.0.1:55689/login]] fakeToken("{\"sub\":\"example\"}") == "eyJhbGciOiJSUzI1NiIsImtpZCI6IiJ9.eyJzdWIiOiJleGFtcGxlIn0.Co-CmjGC6fchwhxeMevkmdeNj0rw1u_siLcv_G9ynmszqgU9vSc8p9Aj2swYYVSPtMlcskQAkTn8FfDYadJ08nzyiCH5vT4DC1sw5-PDdL0Mu899MbU5k6FE0_-cKRBubsBofTr23ulhebpOTY1h035ZgVHMQJ6QQ4soVH2DGmTA7gQlUK-SeylIYREDc8D5CojMyKIFAGH2j1egSAqQgZIYQWEoC0hB78PUROQ_v37p7gF-kUKjp27hfDg6vAnylVWqIIRp1aM8NdUz-eRmTfAekWKOZ65PjFFvTH_YApTXPF692Fu5mhU07sy_E9nZCi_6zwwLjKZEqTF2xhKThg" fakeToken("{\"alt_email\":\"alt_email@example.com\",\"alt_username\":\"desired-username\",\"alt_verified\":true,\"aud\":[\"test%client$id\"],\"email\":\"joe.smith@primary.example.com\",\"email_verified\":true,\"exp\":1623095539,\"family_name\":\"User Name\",\"given_name\":\"Fake\",\"iat\":1623095479,\"iss\":\"http://127.0.0.1:36479\",\"name\":\"Joe\u00a0P.\u00a0Smith\",\"nonce\":\"fake-nonce\",\"sub\":\"fake-user-id\"}") == "eyJhbGciOiJSUzI1NiIsImtpZCI6IiJ9.eyJhbHRfZW1haWwiOiJhbHRfZW1haWxAZXhhbXBsZS5jb20iLCJhbHRfdXNlcm5hbWUiOiJkZXNpcmVkLXVzZXJuYW1lIiwiYWx0X3ZlcmlmaWVkIjp0cnVlLCJhdWQiOlsidGVzdCVjbGllbnQkaWQiXSwiZW1haWwiOiJqb2Uuc21pdGhAcHJpbWFyeS5leGFtcGxlLmNvbSIsImVtYWlsX3ZlcmlmaWVkIjp0cnVlLCJleHAiOjE2MjMwOTU1MzksImZhbWlseV9uYW1lIjoiVXNlciBOYW1lIiwiZ2l2ZW5fbmFtZSI6IkZha2UiLCJpYXQiOjE2MjMwOTU0NzksImlzcyI6Imh0dHA6Ly8xMjcuMC4wLjE6MzY0NzkiLCJuYW1lIjoiSm9lwqBQLsKgU21pdGgiLCJub25jZSI6ImZha2Utbm9uY2UiLCJzdWIiOiJmYWtlLXVzZXItaWQifQ.QjRco1DDffXVNUsyP7F8uO3w6DRERN7_7BfDaIazHqIhsg0tlqHu1r2jJBVz0O_qZifAd03iNmjrlgOr8XTtA1mV6YeZml2vEa7vfUVeWGbRW2JBYhw1KH4vQsKBnhiW6vRFfUScfM4eSBi3JdANYc--WbzdMRpKxNHhR_J-P67GaWAn5psY08nKluL79-oZLXy6MS9DzE69YSGL8TA1dh3o-jUZhbVziQND0DUwVCNcJTgDJBMjAiQBKmLPkY-q-vg60TzINKfnbcNsJZuer3jfhEymj_9Hgvy_4yBedpRExM_artvRj4eMNdPRiCFCq99eKsjYg63YO4ksdOA8yA" serveOIDC: got req: GET /jwks map[] servePeopleAPI: got req: GET /v1/people/me?alt=json&personFields=emailAddresses%2Cnames&prettyPrint=false map[alt:[json] personFields:[emailAddresses,names] prettyPrint:[false]] spied request: "POST /auth/controller/callback HTTP/1.1\r\nHost: 127.0.0.1:33635\r\nAccept-Encoding: gzip\r\nAuthorization: Bearer systemusertesttoken1234567890aoeuidhtnsqjkxbmwvzpy\r\nContent-Length: 301\r\nContent-Type: application/x-www-form-urlencoded\r\nUser-Agent: Go-http-client/1.1\r\nX-Request-Id: req-wbbb800w4on26h33uxvx\r\n\r\nauth_info=%7B%22alternate_emails%22%3Anull%2C%22email%22%3A%22joe.smith%40primary.example.com%22%2C%22expires_at%22%3Anull%2C%22first_name%22%3A%22Fake%22%2C%22last_name%22%3A%22User+Name%22%2C%22user_uuid%22%3A%22%22%2C%22username%22%3A%22%22%7D&return_to=%2Chttps%3A%2F%2Fapp.example.com%2Ffoo%3Fbar" login_oidc_test.go:486: c.Check(authinfo.FirstName, check.Equals, "Joe P.") ... obtained string = "Fake" ... expected string = "Joe P." login_oidc_test.go:487: c.Check(authinfo.LastName, check.Equals, "Smith") ... obtained string = "User Name" ... expected string = "Smith" time="2021-06-07T19:51:20Z" level=error msg="People API is not enabled" email=active-user@arvados.local error="googleapi: got HTTP response code 403 with body: Error 403: accessNotConfigured\n" time="2021-06-07T19:51:21Z" level=info msg="unacceptable access token scope" have="openid profile foobar" need=foo OOPS: 26 passed, 3 skipped, 1 FAILED --- FAIL: Test (8.37s) FAIL coverage: 68.5% of statements FAIL git.arvados.org/arvados.git/lib/controller/localdb 8.378s FAIL ======= lib/controller/localdb tests -- FAILED ======= test lib/controller/localdb -- 11s
Tom, maybe this is not part of the scope. but probably we should make a decision
Updated by Nico César over 3 years ago
- Related to Feature #16171: Support generic OpenID Connect login provider added
Updated by Tom Clegg over 3 years ago
A name received from the People API (which looks like it already preserves given/family names correctly) should continue to take precedence over the OIDC provider response.
Updated by Peter Amstutz over 3 years ago
- Status changed from New to In Progress
Updated by Nico César over 3 years ago
ae1f6a36ad655640138378657f1bc33c657ad56b 17748-extra-claims,
Updated by Peter Amstutz over 3 years ago
- Target version changed from 2021-06-23 sprint to 2021-07-07 sprint
Updated by Tom Clegg over 3 years ago
The "fall back to this info if..." comment seems to be in the wrong place now -- it applies to the whole "if verified" block, but its placement suggests that it only applies to the "if no family_name" block.
The given/family name logic seems a bit complicated -- if we get given_name but no family_name, then we set FirstName to given_name but then change it to the split-by-space version. Taking advantage of the fact that "foo, _ := val.(string)" sets foo to "" if val is not a string, and we handle "" the same way as missing/non-string anyway, would this make it more obvious?
givenName, _ := claims["given_name"].(string)
familyName, _ := claims["family_name"].(string)
if givenName != "" && familyName != "" {
ret.FirstName = givenName
ret.LastName = familyName
} else {
name, _ := claims["name"].(string)
// ...
}
Perhaps renaming the existing TestGoogleLogin_OIDCRealName (to ...OIDCNameWithoutGivenAndFamilyNames?) would make it more obvious how it differs from TestGoogleLogin_OIDCClamisWithGivenNames (which should be Claims, not Clamis).
In the interest of reducing redundant tests: It looks like if we change Given/Family names to {"Fake", "User Name"} in SetUpTest, and update TestGoogleLogin_Success accordingly, then we wouldn't even need a separate TestGoogleLogin_OIDCClamisWithGivenNames test to check the normal case, and the old/renamed OIDCNameWithoutGivenAndFamilyNames test would check the case where we fall back to splitting on whitespace.
Updated by Peter Amstutz over 3 years ago
- Target version changed from 2021-07-07 sprint to 2021-07-21 sprint
Updated by Nico César over 3 years ago
6a62a1628b5775facb14cfcdb0cb8b6d830367c1 17748-extra-claims
Updated by Nico César about 3 years ago
b13e32f7df598fcf027a9f126f180b9d66be8c17 (HEAD -> 17748-extra-claims,
Updated by Nico César about 3 years ago
0f25737426b76ac41c9e69226cf322842b793933 17748-extra-claims is ready to review
Updated by Nico César about 3 years ago
Updated by Lucas Di Pentima about 3 years ago
Just one comment:
- I think the test
TestGoogleLogin_OIDCClaimsWithGivenNames
is already included inTestGoogleLogin_Success
, so it could be removed, right?
Other than that, it LGTM.
Updated by Nico César about 3 years ago
what a mess that branch.
8a0e803df2dc4ce35596faa6c17f6bb22db72668 17748-extra-claims-take2 should have everything and just 1 commit
Updated by Lucas Di Pentima about 3 years ago
Re-ran jenkins: developer-run-tests: #2595
LGTM
Updated by Nico César about 3 years ago
- Status changed from In Progress to Resolved