Discover access token audiences #17469

NobodysNightmare · 2024-12-16T09:30:46Z

Ticket

https://community.openproject.org/projects/cross-application-user-integration-stream/work_packages/60162

What are you trying to accomplish?

This PR is an extension of previous work in #16940. We want to be able to use tokens stored in the corresponding database model for access to third party services, such as Nextcloud.

There are different ways that we can use these existing tokens for that. The case handled in this PR is that the token might already be immediately usable for use in certain services, which we can discover from the token's audience.

What approach did you choose and why?

We are expecting the access token to be a JWT that we can parse and verify using the metadata we have configured for the corresponding OIDC provider. While there is no guarantee that access tokens can be parsed as JWTs, it's very common to find when dealing with an OIDC IDP, since those are required to provide their ID tokens as JWTs, so the "infrastructure" for signing JWTs exists anyways.

To perform the parsing I extracted previously existing code from the JwtOidc warden strategy into a new parser service and adapted it to the common needs of the warden strategy and our new code.

Merge checklist

Added/updated tests
- For JWT parsing
- For audience discovery
~~Added/updated documentation in Lookbook (patterns, previews, etc)~~
Tested major browsers (Chrome, Firefox, Edge, ...)

NobodysNightmare · 2024-12-16T13:44:29Z

modules/openid_connect/app/services/openid_connect/provider_token_parser.rb

+# +
+
+module OpenIDConnect
+  class ProviderTokenParser


I am starting to become unhappy about names.

I called the UserToken like that, because it's a token that grants access in the name of the user.

I called the ProviderTokenParser like that, because it only allows proper parsing of JWTs that were issued by a configured provider (they are not parseable without knowing the provider).

But now looking at both names next to each other, it doesn't seem to make sense, because there are no tokens to do stuff in the name of a provider. It seems counter intuitive, that the ProviderTokenParser would parse a UserToken and yet that's what it does...

Names are hard as hell. This could easily become just OIDC::JWTParser and the UserToken could just be OIDC::Token.

I suck at naming. XD

It's definitely not a JWTParser, because the token being a JSON Web Token is optional. Though I'll give TokenParser and Token a second thought.

You could have multiple parsers being chosen at runtime for each type of token, might even clean up the code as each parser gets more and more specialized.

I was even mixing things up. The token parser completely expects a JWT. Just other parts of the code (AssociateUserToken) treat JWT as optional. Thanks for the input :)

JWT parsing is rather involved, because we need to fetch proper certificates first. We will need to parse JWTs in a different context than authorization as well, so it makes sense to have the parsing centralized. This also allowed to add specs for this previously not (unit) tested piece of code.

We want to know for which purposes tokens can be used. Assuming that we receive JWTs as access tokens, it's possible to read their audience and thus check where these tokens are usable. Importantly, it's still possible that an access token is not a JWT, so we have to allow that as well. The code could be extended in the future to send such tokens to the introspection endpoint of the IDP, hoping to receive an audience list as a result of that.

mereghost

Overall seems solid just one point that'd like to check viability.

mereghost · 2025-01-10T15:12:21Z

modules/openid_connect/app/services/openid_connect/provider_token_parser.rb

+# +
+
+module OpenIDConnect
+  class ProviderTokenParser


Names are hard as hell. This could easily become just OIDC::JWTParser and the UserToken could just be OIDC::Token.

I suck at naming. XD

mereghost · 2025-01-10T15:15:18Z

modules/openid_connect/app/services/openid_connect/provider_token_parser.rb

+      raise Error, "Token signature algorithm #{alg} is not supported" if SUPPORTED_JWT_ALGORITHMS.exclude?(alg)
+
+      provider = fetch_provider(issuer)
+      raise Error, "The access token issuer is unknown" if provider.blank?


🟡 My guess here from the code, is that this exception is being used as flow control to represent an invalid but not exceptional state.

That's were we usually would rely on a ServiceResult, Dry::Monad::Result or similar object to represent a failed state without all the costs of raising an exception.

That's a fair comment. My excuse is that all of this started from refactoring previously existing code and extracting it into a common class.

But that excuse is bad. I'll look into getting friends with Dry::Monad::Result once more ;-)

Feel free to enlist me if you get stuck at using it. =)

mereghost · 2025-01-10T15:18:34Z

modules/openid_connect/app/services/openid_connect/provider_token_parser.rb

+    def fetch_provider(issuer)
+      return nil if issuer.blank?
+
+      OpenIDConnect::Provider.where(available: true).find { |p| p.issuer == issuer }


🔴 This will load all the providers in memory then run Array#find, can't the block become part of the where clause?

issuer is part of the options. I'll have to check, but assuming that options is a JSONB this should still be possible.

Now that I see this code, I also want to double check whether the new code circumvents a cache that the previous code may have been using (via OpenProject::OpenIDConnect.providers).

mereghost · 2025-01-10T15:23:56Z

modules/openid_connect/spec/services/openid_connect/associate_user_token_spec.rb

+    subject
+
+    expect(OpenIDConnect::ProviderTokenParser).to have_received(:new)
+      .with(verify_audience: false, required_claims: ["aud"])
+    expect(parser).to have_received(:parse).with(access_token)


🟢 isn't this testing the internals of the service, checking if a forcebly injected spy actually is being called with the correct params?

Can't we use dependency injection here so that we don't need to stub the .new method? Is there a easy way to not have to mock/spy at all? Wouldn't be tested on the other examples below?

One way avoid the mocking is to pass a real JWT that has certain properties. We do this in spec/requests/api/v3/authentication_spec.rb where I think this approach is feasible, because it's an integrative spec.

Here I wanted to stay encapsulated, while still checking whether the tested unit properly behaves at its boundaries (e.g. whether it correctly sets-up token parsing). E.g. the token parser should not raise an error when the received token is for a foreign audience, but still it should ensure that an audience claim is part of the token.

Let me try to follow along with your suggestion: Your idea would be to hand in the token parser into the AssociateUserToken service. This would allow us to replace it with a mock during testing more easily (no need to overwrite new).

However, I'd still ask myself where we'd test that the "production parser" is configured correctly. The correct configuration is very much related to the use case of the service under test.

However, I'd still ask myself where we'd test that the "production parser" is configured correctly. The correct configuration is very much related to the use case of the service under test.

This could be done by the request specs you mentioned. I'm okay with how it is now, but every time I see a stub of .new, I think of it as a code smell.

NobodysNightmare changed the title ~~Discover audiences~~ Discover access token audiences Dec 16, 2024

NobodysNightmare force-pushed the discover-audiences branch 3 times, most recently from d31e204 to efc65c8 Compare December 16, 2024 13:40

NobodysNightmare commented Dec 16, 2024

View reviewed changes

NobodysNightmare force-pushed the discover-audiences branch 2 times, most recently from d4c2af6 to 178b622 Compare December 17, 2024 08:19

NobodysNightmare requested a review from a team December 17, 2024 09:07

NobodysNightmare force-pushed the save-oidc-tokens-to-open-project-database branch from 1ab8a1c to 06f4d7d Compare December 18, 2024 07:56

NobodysNightmare force-pushed the discover-audiences branch from 178b622 to 0411a3d Compare December 18, 2024 08:21

NobodysNightmare force-pushed the discover-audiences branch from 0411a3d to 555e27a Compare December 18, 2024 08:29

mereghost requested changes Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discover access token audiences #17469

Discover access token audiences #17469

NobodysNightmare commented Dec 16, 2024 •

edited

Loading

NobodysNightmare Dec 16, 2024

mereghost Jan 10, 2025

NobodysNightmare Jan 10, 2025

mereghost Jan 10, 2025 •

edited

Loading

NobodysNightmare Jan 10, 2025

mereghost left a comment

mereghost Jan 10, 2025

mereghost Jan 10, 2025

NobodysNightmare Jan 10, 2025

mereghost Jan 10, 2025

mereghost Jan 10, 2025

NobodysNightmare Jan 10, 2025

mereghost Jan 10, 2025

NobodysNightmare Jan 10, 2025

mereghost Jan 10, 2025

Discover access token audiences #17469

Are you sure you want to change the base?

Discover access token audiences #17469

Conversation

NobodysNightmare commented Dec 16, 2024 • edited Loading

Ticket

What are you trying to accomplish?

What approach did you choose and why?

Merge checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mereghost Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mereghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NobodysNightmare commented Dec 16, 2024 •

edited

Loading

mereghost Jan 10, 2025 •

edited

Loading