#842 AuthenticationOptions in GlobalConfiguration #1215

jlukawska · 2020-04-28T19:00:40Z

Closes #842

New AuthenticationOptions property in global configuration #842

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), AllowAnonymousForGlobalAuthenticationOptions property should be set to true in the route AuthenticationOptions.

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

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

rathga · 2020-07-16T14:29:37Z

+1 to please add/merge this feature.

Question: does your implementation allow for a given route to NOT have authentication? Like an 'allow anonymous' setting. In my project this would be needed for my identity service.

jlukawska · 2020-07-17T11:18:25Z

@rathga, it should be quite easy to add this option. I'll try to update the pull request soon.

Thanks for the feedback!

raman-m · 2023-07-27T19:12:11Z

@jlukawska Hi J!
Thanks for the great PR!
Wow! I am impressed. 😍

What I've done:

Feature branch has been rebased onto ThreeMammals:develop to remove all merge commits and introduce top commits from target
Merge conflicts have been resolved during rebasing. That was quite difficult, but done
Fixed errors. There was some ambiguity in resolutions, not an issue now
Fixed some errors, warning and messages
Made green build finally

Now we can start code review...

raman-m · 2023-07-27T19:20:03Z

@rathga commented on Jul 16, 2020

Hey, Richard!
It was a long story, but I hope, we will get this feature delivered soon... 😄

Could you join to code review please?
Your opinion is highly prioritized.

Also, check please,
Is your small feature request implemented?

raman-m

@RaynaldM @wast
Wow! 2 approvals from you, guys, without issues, really? 😉 👇
I am impressed! 😄

I see 2 major issues now:

Unit & acceptance tests, and their methods have a lot of copied & pasted code snippets. The fact-methods are long. But they could be shorter.
Some lines are too long. It is hard to read.

I would appreciate if PR author will fix them.

raman-m · 2023-08-03T16:07:00Z

docs/features/authentication.rst

@@ -42,6 +42,63 @@ When Ocelot runs it will look at this Routes AuthenticationOptions.Authenticatio

 If a Route is authenticated Ocelot will invoke whatever scheme is associated with it while executing the authentication middleware. If the request fails authentication Ocelot returns a http status code 401.

+If you want to configure AuthenticationOptions the same for all Routes, do it in GlobalConfiguration the same way as for Route. If there are AuthenticationOptions configured both for GlobalConfiguration and Route (AuthenticationProviderKey is set), the Route section has priority. If you want to exclude route from global AuthenticationOptions, you can do that by setting AllowAnonymousForGlobalAuthenticationOptions to true in the route AuthenticationOptions - then this route will not be authenticated.


It is better to write a new subsection of the chapter like:

Global Authentication ^^^^^^^^^^^^^^^^^^^^^

before this line.

And this line is too long! Splitting phrases should help.

raman-m · 2023-08-03T16:10:32Z

src/Ocelot/Configuration/Creator/AuthenticationOptionsCreator.cs

+ public AuthenticationOptions Create(FileAuthenticationOptions routeAuthOptions, 
+ FileAuthenticationOptions globalConfAuthOptions)


Suggested change

public AuthenticationOptions Create(FileAuthenticationOptions routeAuthOptions,

FileAuthenticationOptions globalConfAuthOptions)

public AuthenticationOptions Create(

FileAuthenticationOptions routeAuthOptions,

FileAuthenticationOptions globalConfAuthOptions)

Or, Don't make new line at all. Having 2-3 parameters in signature on the same line is totally fine.

raman-m · 2023-08-03T16:13:27Z

src/Ocelot/Configuration/File/FileAuthenticationOptions.cs

+ public List<string> AllowedScopes { get; set; }
+
+ /// <summary>
+ /// Allows anonymous authentication globally.


It allows per route actually.

raman-m · 2023-08-03T16:16:39Z

src/Ocelot/Configuration/File/FileAuthenticationOptions.cs

+ /// <value>
+ /// <see langword="true"/> if it is allowed; otherwise, <see langword="false"/>.
+ /// </value>
+ public bool AllowAnonymousForGlobalAuthenticationOptions { get; set; }


The name could be shorter, like that: AllowAnonymousAuthentication or even AllowAnonymous.

raman-m · 2023-08-03T16:18:19Z

src/Ocelot/Configuration/Validator/FileAuthenticationOptionsValidator.cs

+
+ RuleFor(authOptions => authOptions.AuthenticationProviderKey)
+ .MustAsync(IsSupportedAuthenticationProviders)
+ .WithMessage("{PropertyName}: {PropertyValue} is unsupported authentication provider");


What placeholders are here?
Are they implicit? Where is definition of these variables?

raman-m · 2023-08-03T16:46:12Z

test/Ocelot.UnitTests/Configuration/AuthenticationOptionsCreatorTests.cs

+ var route = new FileRoute
+ {
+ AuthenticationOptions = new FileAuthenticationOptions()
+ {
+ AuthenticationProviderKey = "routeKey",
+ AllowedScopes = new List<string>() { "routeScope1", "routeScope2" },
+ },
+ };
+ var globalConfig = new FileGlobalConfiguration
+ {
+ AuthenticationOptions = new FileAuthenticationOptions()
+ {
+ AuthenticationProviderKey = "globalKey",
+ AllowedScopes = new List<string>() { "globalScope1", "globalScope2" },
+ },
+ };


Could you define a helper to construct FileRoute object plz?
The goal is writing less code.

raman-m · 2023-08-03T16:51:15Z

test/Ocelot.UnitTests/Configuration/RouteOptionsCreatorTests.cs

I believe that all new facts could call new defined helpers.
So, too many copy-pasted code snippets!

raman-m · 2023-08-03T16:54:15Z

test/Ocelot.UnitTests/Configuration/Validation/FileConfigurationFluentValidatorTests.cs

 var provider = new ServiceCollection()
 .BuildServiceProvider();

 // Todo - replace with mocks
- _configurationValidator = new FileConfigurationFluentValidator(provider, new RouteFluentValidator(_authProvider.Object, new HostAndPortValidator(), new FileQoSOptionsFluentValidator(provider)), new FileGlobalConfigurationFluentValidator(new FileQoSOptionsFluentValidator(provider)));
+ _configurationValidator = new FileConfigurationFluentValidator(provider, new RouteFluentValidator(new HostAndPortValidator(), new FileQoSOptionsFluentValidator(provider), _fileAuthenticationOptionsValidator), new FileGlobalConfigurationFluentValidator(new FileQoSOptionsFluentValidator(provider), _fileAuthenticationOptionsValidator));


Too long line!
It is hard to read the code...
Could you apply new line vs indentation approach to make code readability better plz?

raman-m · 2023-08-03T16:58:25Z

test/Ocelot.UnitTests/Configuration/Validation/FileConfigurationFluentValidatorTests.cs

@@ -1554,7 +1598,7 @@ private void GivenAServiceDiscoveryHandler()
 ServiceDiscoveryFinderDelegate del = (a, b, c) => new FakeServiceDiscoveryProvider();
 collection.AddSingleton<ServiceDiscoveryFinderDelegate>(del);
 var provider = collection.BuildServiceProvider();
- _configurationValidator = new FileConfigurationFluentValidator(provider, new RouteFluentValidator(_authProvider.Object, new HostAndPortValidator(), new FileQoSOptionsFluentValidator(provider)), new FileGlobalConfigurationFluentValidator(new FileQoSOptionsFluentValidator(provider)));
+ _configurationValidator = new FileConfigurationFluentValidator(provider, new RouteFluentValidator(new HostAndPortValidator(), new FileQoSOptionsFluentValidator(provider), _fileAuthenticationOptionsValidator), new FileGlobalConfigurationFluentValidator(new FileQoSOptionsFluentValidator(provider), _fileAuthenticationOptionsValidator));


Too long lines 1592 and 1601 !

raman-m · 2023-08-03T17:07:27Z

src/Ocelot/Configuration/Creator/RouteOptionsCreator.cs

+ private static bool IsAuthenticated(FileRoute fileRoute, FileGlobalConfiguration globalConfiguration)
+ {
+ return (!string.IsNullOrEmpty(globalConfiguration?.AuthenticationOptions?.AuthenticationProviderKey) &&
+ !fileRoute.AuthenticationOptions.AllowAnonymousForGlobalAuthenticationOptions) ||
+ !string.IsNullOrEmpty(fileRoute.AuthenticationOptions?.AuthenticationProviderKey);
+ }


Because the body consists of one operator only, why not to define via expression body 👇

Suggested change

private static bool IsAuthenticated(FileRoute fileRoute, FileGlobalConfiguration globalConfiguration)

{

return (!string.IsNullOrEmpty(globalConfiguration?.AuthenticationOptions?.AuthenticationProviderKey) &&

!fileRoute.AuthenticationOptions.AllowAnonymousForGlobalAuthenticationOptions) ||

!string.IsNullOrEmpty(fileRoute.AuthenticationOptions?.AuthenticationProviderKey);

}

private static bool IsAuthenticated(FileRoute fileRoute, FileGlobalConfiguration globalConfiguration)

=> (!string.IsNullOrEmpty(globalConfiguration?.AuthenticationOptions?.AuthenticationProviderKey)

&& !fileRoute.AuthenticationOptions.AllowAnonymousForGlobalAuthenticationOptions)

|| !string.IsNullOrEmpty(fileRoute.AuthenticationOptions?.AuthenticationProviderKey);

… characters: {should_*}

wudiqiang2024 · 2024-04-11T08:58:58Z

I need this change,What time releases this PR?

@raman-m

raman-m · 2024-04-11T11:03:33Z

@wudiqiang2024 commented on Apr 11

Sorry for delaying the delivery! We have a lack of dev time!
This PR has low priority. We have another more important PRs in Annual 2023 milestone.
Having possibility to define in global section is just beneficial option and not required at the moment.
Please try to merge all routes as Catch All ones and define AuthenticationOptions for those Catch All routes.
How many downstream services you have? How many routes are defined in ocelot.json?
Do you consider Consul usage?

jlukawska marked this pull request as draft July 17, 2020 11:19

jlukawska marked this pull request as ready for review July 22, 2020 11:20

raman-m changed the title ~~Feature/842 AuthenticationOptions in GlobalConfiguration~~ #842 AuthenticationOptions in GlobalConfiguration Jul 19, 2023

raman-m assigned jlukawska Jul 19, 2023

raman-m changed the base branch from master to develop July 19, 2023 20:03

raman-m added the conflicts Feature branch has merge conflicts label Jul 19, 2023

raman-m mentioned this pull request Jul 19, 2023

New AuthenticationOptions property in global configuration #842

Open

raman-m force-pushed the feature/842-authenticationoptions-in-globalconfiguration branch from 6e53f98 to c1afca2 Compare July 27, 2023 18:21

raman-m added feature A new feature needs feedback Issue is waiting on feedback before acceptance and removed conflicts Feature branch has merge conflicts labels Jul 27, 2023

raman-m requested review from binarymash, jps, geffzhang, RaynaldM, philproctor, anktsrkr, sharpn, abooraja, wast, raman-m, yangzhongke, margaale and thiagoloureiro July 27, 2023 19:30

RaynaldM approved these changes Jul 28, 2023

View reviewed changes

wast approved these changes Aug 2, 2023

View reviewed changes

raman-m requested changes Aug 3, 2023

View reviewed changes

jlukawska and others added 10 commits October 12, 2023 15:44

add AuthenticationOptions in GlobalConfiguration

928b24a

add AllowAnonymous

8c7866f

little changes

06354dc

validation, unit tests

0bf90a1

docs updated

bc12ee4

Review after rebasing: fix errors, warnings, merge code

a1b4c7a

File-scoped namespace declaration

78a70dc

IDE1006 Naming rule violation: These words must begin with upper case…

eb4e3f8

… characters: {should_*}

IDE0044 Make field readonly

645b827

Add property xml-docs

c99d512

raman-m force-pushed the feature/842-authenticationoptions-in-globalconfiguration branch from 19d5e00 to c99d512 Compare October 12, 2023 12:47

Fix integration test

e8af97f

raman-m added low Low priority and removed needs feedback Issue is waiting on feedback before acceptance labels Apr 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#842 AuthenticationOptions in GlobalConfiguration #1215

#842 AuthenticationOptions in GlobalConfiguration #1215

jlukawska commented Apr 28, 2020 •

edited by raman-m

rathga commented Jul 16, 2020

jlukawska commented Jul 17, 2020

raman-m commented Jul 27, 2023

raman-m commented Jul 27, 2023

raman-m left a comment •

edited

raman-m Aug 3, 2023

raman-m Aug 3, 2023

raman-m Aug 3, 2023

raman-m Aug 3, 2023

raman-m Aug 3, 2023

raman-m Aug 3, 2023

raman-m Aug 3, 2023

raman-m Aug 3, 2023

raman-m Aug 3, 2023

raman-m Aug 3, 2023

wudiqiang2024 commented Apr 11, 2024 •

edited

raman-m commented Apr 11, 2024

		@@ -42,6 +42,63 @@ When Ocelot runs it will look at this Routes AuthenticationOptions.Authenticatio

		If a Route is authenticated Ocelot will invoke whatever scheme is associated with it while executing the authentication middleware. If the request fails authentication Ocelot returns a http status code 401.

		If you want to configure AuthenticationOptions the same for all Routes, do it in GlobalConfiguration the same way as for Route. If there are AuthenticationOptions configured both for GlobalConfiguration and Route (AuthenticationProviderKey is set), the Route section has priority. If you want to exclude route from global AuthenticationOptions, you can do that by setting AllowAnonymousForGlobalAuthenticationOptions to true in the route AuthenticationOptions - then this route will not be authenticated.

		public AuthenticationOptions Create(FileAuthenticationOptions routeAuthOptions,
		FileAuthenticationOptions globalConfAuthOptions)

#842 AuthenticationOptions in GlobalConfiguration #1215

Are you sure you want to change the base?

#842 AuthenticationOptions in GlobalConfiguration #1215

Conversation

jlukawska commented Apr 28, 2020 • edited by raman-m

Closes #842

Proposed Changes

rathga commented Jul 16, 2020

jlukawska commented Jul 17, 2020

raman-m commented Jul 27, 2023

raman-m commented Jul 27, 2023

raman-m left a comment • edited

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

Choose a reason for hiding this comment

wudiqiang2024 commented Apr 11, 2024 • edited

raman-m commented Apr 11, 2024

jlukawska commented Apr 28, 2020 •

edited by raman-m

raman-m left a comment •

edited

wudiqiang2024 commented Apr 11, 2024 •

edited