Bug #17748


OIDC should read given name / family name fields

Added by Peter Amstutz over 1 year ago. Updated about 1 year ago.

Assigned To:
Target version:
Start date:
Due date:
% Done:


Estimated time:
(Total: 0.00 h)
Story points:
Release relationship:


Arvados should be using the standard given_name and family_name claims:

Subtasks 1 (0 open1 closed)

Task #17781: Review 17748-extra-claimsResolvedNico César06/08/2021


Related issues

Related to Arvados - Feature #16171: Support generic OpenID Connect login providerResolvedTom Clegg06/01/2020

Actions #1

Updated by Peter Amstutz over 1 year ago

  • Description updated (diff)
Actions #2

Updated by Peter Amstutz over 1 year ago

  • Category set to Login
Actions #3

Updated by Nico César over 1 year ago

  • Assigned To set to Nico César
Actions #4

Updated by Nico César over 1 year 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

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

if given name and family name are part of the claims, they are priotized.
Actions #5

Updated by Nico César over 1 year 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://" 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://" error="LDAP Result Code 49 \"Invalid Credentials\": " search="(uid=badusername)" 
time="2021-06-07T19:51:18Z" level=info msg="skipping unverified email address"

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:[]]
fakeToken("{\"sub\":\"example\"}") == "eyJhbGciOiJSUzI1NiIsImtpZCI6IiJ9.eyJzdWIiOiJleGFtcGxlIn0.Co-CmjGC6fchwhxeMevkmdeNj0rw1u_siLcv_G9ynmszqgU9vSc8p9Aj2swYYVSPtMlcskQAkTn8FfDYadJ08nzyiCH5vT4DC1sw5-PDdL0Mu899MbU5k6FE0_-cKRBubsBofTr23ulhebpOTY1h035ZgVHMQJ6QQ4soVH2DGmTA7gQlUK-SeylIYREDc8D5CojMyKIFAGH2j1egSAqQgZIYQWEoC0hB78PUROQ_v37p7gF-kUKjp27hfDg6vAnylVWqIIRp1aM8NdUz-eRmTfAekWKOZ65PjFFvTH_YApTXPF692Fu5mhU07sy_E9nZCi_6zwwLjKZEqTF2xhKThg" 
fakeToken("{\"alt_email\":\"\",\"alt_username\":\"desired-username\",\"alt_verified\":true,\"aud\":[\"test%client$id\"],\"email\":\"\",\"email_verified\":true,\"exp\":1623095539,\"family_name\":\"User Name\",\"given_name\":\"Fake\",\"iat\":1623095479,\"iss\":\"\",\"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:\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\" 
    c.Check(authinfo.FirstName, check.Equals, "Joe P.")
... obtained string = "Fake" 
... expected string = "Joe P." 

    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)
coverage: 68.5% of statements
FAIL    8.378s
======= 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 #6

Updated by Nico César over 1 year ago

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

Updated by Tom Clegg over 1 year 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 #8

Updated by Peter Amstutz over 1 year ago

  • Status changed from New to In Progress
Actions #10

Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2021-06-23 sprint to 2021-07-07 sprint
Actions #11

Updated by Tom Clegg over 1 year 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 #12

Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2021-07-07 sprint to 2021-07-21 sprint
Actions #14

Updated by Nico César over 1 year ago

b13e32f7df598fcf027a9f126f180b9d66be8c17 (HEAD -> 17748-extra-claims,

developer-run-tests: #2589

Actions #17

Updated by Lucas Di Pentima over 1 year ago

Just one comment:

  • I think the test TestGoogleLogin_OIDCClaimsWithGivenNames is already included in TestGoogleLogin_Success, so it could be removed, right?

Other than that, it LGTM.

Actions #18

Updated by Nico César over 1 year ago

what a mess that branch.

8a0e803df2dc4ce35596faa6c17f6bb22db72668 17748-extra-claims-take2 should have everything and just 1 commit

developer-run-tests: #2593

Actions #19

Updated by Lucas Di Pentima over 1 year ago

Re-ran jenkins: developer-run-tests: #2595


Actions #20

Updated by Nico César over 1 year ago

  • Status changed from In Progress to Resolved
Actions #21

Updated by Peter Amstutz about 1 year ago

  • Release set to 42

Also available in: Atom PDF