Skip to content

Fix gitlab_username_claim support for OmniAuth providers

Drew Blessing requested to merge dblessing_fix_gitlab_username_claim into master

What does this MR do and why?

Fix gitlab_username_claim support for OmniAuth providers

Prior to this fix gitlab_username_claim was not always working correctly. When loading the provider config, the auth hash class used an ivar which may not yet be initialized and would cause false negative config lookup. This fix replaces ivar usage with the method that will correctly get the provider name and allow config to be found.

Tests didn't catch this problem because we were stubbing the provider config loading mechanism. This change also updates tests to instead stub config at a lower level, ensuring config loading works as expected.

This bug was discovered when troubleshooting a customer issue with gitlab_username_claim configuration for an OmniAuth identity provider.

Given an IdP configuration such as the following:

    - {
      name: "saml",
      args: {
        assertion_consumer_service_url: "https://gdk.test:3443/users/auth/saml/callback",
        idp_cert_fingerprint: "BB:9B:AA:66:50:91:AA:FC:AC:BE:D8:2F:BB:D6:F9:E5:84:77:0D:B7",
        idp_sso_target_url: "https://foo.example.com",
        issuer: "https://gdk.test:3443",
        name_identifier_format: "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent",
        attribute_statements: { nickname: ['firstname'] },
        gitlab_username_claim: 'uid'
      }

And generating an OmniAuth Auth Hash as follows:

omni_authhash = OmniAuth::AuthHash.new(credentials: OmniAuth::AuthHash.new, extra: OmniAuth::AuthHash.new(raw_info: OmniAuth::AuthHash.new), info: OmniAuth::AuthHash::InfoHash.new(email: 'DREW@EXAMPLE.COM', name: 'Drew Blessing'), provider: 'saml', uid: 'dblessing')

Current Behavior

pry(main)> auth_hash = Gitlab::Auth::OAuth::AuthHash.new(omni_authhash)
=> #<Gitlab::Auth::OAuth::AuthHash:0x000000014010e938 @auth_hash={"credentials"=>{}, "extra"=>{"raw_info"=>{}}, "info"=>{"email"=>"DREW@EXAMPLE.COM", "name"=>"Drew Blessing"}, "provider"=>"saml", "uid"=>"dblessing"}>
pry(main)> auth_hash.username
=> "DREW"

Expected Behavior

pry(main)> auth_hash = Gitlab::Auth::OAuth::AuthHash.new(omni_authhash)
=> #<Gitlab::Auth::OAuth::AuthHash:0x000000014010e938 @auth_hash={"credentials"=>{}, "extra"=>{"raw_info"=>{}}, "info"=>{"email"=>"DREW@EXAMPLE.COM", "name"=>"Drew Blessing"}, "provider"=>"saml", "uid"=>"dblessing"}>
pry(main)> auth_hash.username
=> "dblessing"

Cause

Username claim relies on looking up the provider config, which is done by calling AuthHash#provider_config which has the code Gitlab::Auth::OAuth::Provider.config_for(@provider) || {} . Unless something previously called the AuthHash#provider method, @provider ivar is not initialized.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #428058 (closed)

Edited by Aboobacker MK

Merge request reports