Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#842 #1414 AuthenticationOptions in global configuration #2114

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

jlukawska
Copy link
Contributor

@jlukawska jlukawska commented Jun 28, 2024

Closes #842 #1414

Proposed Changes

It is possible to configure AuthenticationOptions in GlobalConfiguration. Then all routes use these settings. To configure an exception to this rule (e.g. for identity service), AllowAnonymous property should be set to true in the route AuthenticationOptions.

If a route has its own AuthenticationOptions with AuthenticationProviderKey/AuthenticationProviderKeys configured additionally, it has priority over the global one.

At the moment if a route uses a global AuthenticationProviderKey/AuthenticationProviderKeys (when AuthenticationProviderKey/AuthenticationProviderKeys is not configured for route explicitly), it uses also global AllowedScopes, even if AllowedScopes is configured for the route additionally.

Note

This PR was prepared 4 years ago →

The revival of the Ocelot brought many changes, also tightly connected with the PR code. Now there is additional property: AuthenticationProviderKeys. The merge was so hard that I created the PR from scratch and tried to apply all the code review comments.

@jlukawska jlukawska marked this pull request as ready for review June 29, 2024 19:25
@ggnaegi
Copy link
Member

ggnaegi commented Jul 2, 2024

@jlukawska, very interesting PR, I would love to review it, ok @raman-m?

@ggnaegi ggnaegi requested review from ggnaegi and raman-m July 2, 2024 14:46
@raman-m
Copy link
Member

raman-m commented Jul 2, 2024

@jlukawska, very interesting PR, I would love to review it, ok @raman-m?

No objections! But I'll review later this/next week...

@raman-m raman-m changed the title #842 AuthenticationOptions in GlobalConfiguration #842 AuthenticationOptions in global configuration Jul 9, 2024
@raman-m raman-m requested a review from RaynaldM July 9, 2024 15:38
@raman-m raman-m changed the title #842 AuthenticationOptions in global configuration #842 #1414 AuthenticationOptions in global configuration Jul 9, 2024
@raman-m raman-m linked an issue Jul 9, 2024 that may be closed by this pull request
@raman-m raman-m added feature A new feature Authentication Ocelot feature: Authentication Configuration Ocelot feature: Configuration labels Jul 9, 2024
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

Excellent work, Jolanta!
Everything looks good; I've only added a few minor suggestions.
The main issue is the lack of documentation. However, you can reintroduce the draft docs from the previous PR, and then I will go over the docs and suggest possible revisions.

src/Ocelot/Configuration/Creator/RouteOptionsCreator.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/File/FileAuthenticationOptions.cs Outdated Show resolved Hide resolved
.Then(x => x.ThenTheResultIsNotValid())
.And(x => ThenTheErrorMessageAtPositionIs(0, $"AuthenticationOptions: AuthenticationProviderKey:'JwtLads',AuthenticationProviderKeys:[]," +
$"AllowedScopes:[] is unsupported authentication provider"))
.BDDfy();
Copy link
Member

Choose a reason for hiding this comment

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

Minor issue: BDDfy in unit tests

@raman-m
Copy link
Member

raman-m commented Jul 9, 2024

@ggnaegi commented on Jul 2, 2024

I'm all for a review! This PR seems solid as a rock. 🤩 But, oops, where did the docs for the new feature scamper off to?

And about that AuthenticationProviderKey property from the days of yore, are we going to keep it around like an old family heirloom? 😄

@jlukawska
Copy link
Contributor Author

I've fixed the code according to the code review suggestions. I've also added the docs based on the previous PR.

@raman-m
Copy link
Member

raman-m commented Jul 25, 2024

TODO

  • Final code review
  • Docs review and build

@raman-m raman-m added the Oct'24 October 2024 release label Aug 30, 2024
@raman-m raman-m added this to the September'24 milestone Aug 30, 2024
@raman-m raman-m added Dec'24 December 2024 release and removed Oct'24 October 2024 release labels Oct 26, 2024
@raman-m raman-m modified the milestones: October'24, Autumn'24 Oct 26, 2024
@raman-m raman-m force-pushed the feature/842-authenticationoptions-in-globalconfig branch from 919abc8 to 3688cc3 Compare November 2, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Authentication Ocelot feature: Authentication Configuration Ocelot feature: Configuration Dec'24 December 2024 release feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AuthenticationOptions under GlobalConfiguration? New AuthenticationOptions property in global configuration
4 participants