Skip to content

Implements basic claims request parameter (Closes #11)#13

Open
zedtux wants to merge 2 commits into
brandnewbox:masterfrom
zedtux:patch-4
Open

Implements basic claims request parameter (Closes #11)#13
zedtux wants to merge 2 commits into
brandnewbox:masterfrom
zedtux:patch-4

Conversation

@zedtux

@zedtux zedtux commented Jul 26, 2023

Copy link
Copy Markdown
Contributor

This PR closes #11 by implementing the basics of the OpenID claims request parameter allowing a consumer to request more claims in the id_token JWT by passing a JSON in the claims parameter in the Authorization creation request.

The requested claim(s) must be part of the requested scopes so that requested claims belonging to non-requested scopes are logged as a warning and ignored.

This PR implements this parameter for both id_token and userinfo but only the id_token has been tested.

This PR doesn't support the { essential: true } but supports hard coded value or nil.

Here is one of my Rspec test showcasing what this PR does:

  context 'when requesting the "email" claim in the id_token and requesting the "email" scope' do
    let(:claims) { build_claims_from(id_token: { email: nil }) }
    let(:scopes) { ERB::Util.url_encode(%w[openid profile email].join(' ')) }

    before do
      login_and_logout_with_warden do
        get "#{host}/accounts/oauth/authorizations?client_id=#{client.identifier}&response_type=code&redirect_uri=#{client.redirect_uri}&scope=#{scopes}&claims=#{claims}&nonce=#{nonce}"

        authorization_code = response.body.scan(
          %r{<a href="#{client.redirect_uri}\?code=([a-z0-9]+)">redirected</a>}
        ).flatten.first

        post create_token_url(authorization_code)
      end
    end

    it 'returns a 200' do
      expect(response).to have_http_status(:ok)
    end

    it 'responds with an id_token' do
      expect(response.body).to match(/"id_token":"[A-z0-9\-_.]+"/)
    end

    it 'responds with an id_token containing the client_it as aud' do
      expect(decoded_id_token).to include('aud' => client.identifier)
    end

    it 'responds with an id_token containing the issuer URL as iss' do
      expect(decoded_id_token).to include('iss' => host)
    end

    it 'responds with an id_token containing the nonce as nonce' do
      expect(decoded_id_token).to include('nonce' => nonce)
    end

    it 'responds with an id_token containing the "email" claim' do
      expect(decoded_id_token.keys).to include('email')
    end
  end

@zedtux zedtux force-pushed the patch-4 branch 2 times, most recently from 8627007 to 814b2f6 Compare July 29, 2023 07:36
@zedtux

zedtux commented Sep 4, 2023

Copy link
Copy Markdown
Contributor Author

@willtcarey I have solved the conflict in this PR, sorry about that I missed it, could you please have a look?

@zedtux

zedtux commented Oct 10, 2023

Copy link
Copy Markdown
Contributor Author

@willtcarey sorry for pinging you again, but this code runs on our production since 2 months without any issues, I really would love to see this PR merged 😊.

Could you please have a look?

@zedtux

zedtux commented Dec 4, 2023

Copy link
Copy Markdown
Contributor Author

Please @willtcarey could you have a look at my PR?

@nathancolgate

Copy link
Copy Markdown
Member

@zedtux Not sure how this slipped under our radar for so long. We'll take a look ASAP.

@zedtux

zedtux commented Sep 5, 2024

Copy link
Copy Markdown
Contributor Author

@nathancolgate or @willtcarey will you have a look soon please? 😇

@willtcarey willtcarey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh hey, I had a pending review on this.

A few code cleanups for things we aren't using.

Comment thread .rubocop.yml Outdated
Comment thread app/controllers/oidc_provider/authorizations_controller.rb Outdated
Comment thread app/controllers/oidc_provider/authorizations_controller.rb Outdated
t.string :nonce
t.string :code, null: false
t.text :scopes, null: false
t.text :claims

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to happen in a new migration, which it does. So we should remove it here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely agreed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, checking my code again and I see that here it will apply for new projects, while the new migration script db/migrate/20230721112432_add_claims_to_oidc_provider_authorization_if_missing.rb will add that column for existing projects when the column doesn't exist already, so that it works in both cases.

We could keep it like this, but if you really want it I can remove it.

Comment thread lib/oidc_provider.rb Outdated
@zedtux

zedtux commented Sep 11, 2024

Copy link
Copy Markdown
Contributor Author

What about Rspec tests? Would you like them? In a separated PR maybe?

Comment on lines +9 to 17
def call(account)
OpenIDConnect::ResponseObject::UserInfo.new(
sub: account.send(OIDCProvider.account_identifier)
).tap do |user_info|
scopes.each do |scope|
UserInfoBuilder.new(user_info, account).run(&scope.work)
ResponseObjectBuilder.new(user_info, account).run(&scope.work)
end
end
end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make it so that we can add scopes for arbitrary data that doesn't already exist on the UserInfo object?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a weakness we've identified where we sometimes need to allow for scopes that have data that exists outsides of what's available on the UserInfo object

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's the goal of this ResponseObjectBuilder thanks to its method_missing method usage.

@zedtux zedtux requested a review from willtcarey September 19, 2024 13:31
@zedtux

zedtux commented Dec 13, 2024

Copy link
Copy Markdown
Contributor Author

@willtcarey would like to have a look at my code please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for the authorization "claims" parameter

3 participants