Bug #17748: OIDC should read given name / family name fields - Arvados


    } 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.

Actions

Copy link

Updated by Nico César almost 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

Actions

Copy link

Updated by Nico César almost 3 years ago

Related to Feature #16171: Support generic OpenID Connect login provider added

Actions

Copy link

Updated by Tom Clegg almost 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.

Actions

Copy link

Updated by Peter Amstutz almost 3 years ago

Status changed from New to In Progress

Actions

Copy link

Updated by Nico César almost 3 years ago

ae1f6a36ad655640138378657f1bc33c657ad56b 17748-extra-claims,

developer-run-tests: #2513

Actions

Copy link

#10

Updated by Peter Amstutz almost 3 years ago

Target version changed from 2021-06-23 sprint to 2021-07-07 sprint

Actions

Copy link

#11

Updated by Tom Clegg almost 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.

Actions

Copy link

#12