Bug #17748

OIDC should read given name / family name fields

Added by Peter Amstutz 5 months ago. Updated 3 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Login
Target version:
Start date:
06/08/2021
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-

Description

Arvados should be using the standard given_name and family_name claims:

https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims


Subtasks

Task #17781: Review 17748-extra-claimsResolvedNico César


Related issues

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

Associated revisions

Revision 79e7d4e7
Added by Nico Cesar 3 months ago

Merge branch '17748-extra-claims-take2'

closes #17748

Arvados-DCO-1.1-Signed-off-by: Nico Cesar <>

History

#1 Updated by Peter Amstutz 5 months ago

  • Description updated (diff)

#2 Updated by Peter Amstutz 5 months ago

  • Category set to Login

#3 Updated by Nico César 4 months ago

  • Assigned To set to Nico César

#4 Updated by Nico César 4 months 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.

#5 Updated by Nico César 4 months 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

#6 Updated by Nico César 4 months ago

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

#7 Updated by Tom Clegg 4 months 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.

#8 Updated by Peter Amstutz 4 months ago

  • Status changed from New to In Progress

#10 Updated by Peter Amstutz 4 months ago

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

#11 Updated by Tom Clegg 4 months 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.

#12 Updated by Peter Amstutz 3 months ago

  • Target version changed from 2021-07-07 sprint to 2021-07-21 sprint

#14 Updated by Nico César 3 months ago

b13e32f7df598fcf027a9f126f180b9d66be8c17 (HEAD -> 17748-extra-claims,

https://ci.arvados.org/view/Developer/job/developer-run-tests/2589/

#17 Updated by Lucas Di Pentima 3 months 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.

#18 Updated by Nico César 3 months ago

what a mess that branch.

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

https://ci.arvados.org/view/Developer/job/developer-run-tests/2593/

#20 Updated by Nico César 3 months ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF