#913 #1066 ScopesAuthorizer refactoring #1478

mehyaa · 2021-05-31T13:43:56Z

Fixes #913 #1066

AllowedScopes does not match string array in JWT scope claim #913
Ocelot forwards Route when userScopes contains Any from routeAllowedScopes, but docs says that it should contains all of them #1066

Scopes can be a space separated list in a single claim. Include this possibility on allowed scopes check.

Proposed Changes

Split user scope values with space character if scope claim is an single claim and its value has space character(s)
User scopes must have all allowed scopes

Predecessor

#1739 #2177 OpenIddict authentication: role section, override scope and role key #1431

mehyaa · 2021-06-01T05:59:36Z

The build is broken, it fails on irrelevant part from this PR. #1436 fixes is I think.

PatrickDelancy · 2021-07-14T20:48:37Z

This is still an issue. Adding my 👍 to get this small and valuable PR merged.

raman-m · 2023-06-30T19:12:37Z

Please do the following:

Merge PR 1
Rebase feature branch onto develop
Resolve all merge conflicts

raman-m · 2023-08-18T16:36:01Z

Mehmet,
Thanks for resolving of merge conflicts!

Unfortunately the last build is failed: 5 acceptance tests have failed!

Why is your PR code so unstable?

mehyaa · 2023-08-18T18:30:36Z

I haven't found out the reason why the tests failed with a quick look. I couldn't figure out the tests structure. When I have time I'll look into it.

raman-m · 2023-08-24T14:51:46Z

@mehyaa
The feature branch has been rebased onto ThreeMammals:develop successfully!
The build has failed with 5 tests!
Code review will start after fixing of these failed tests.
Also, some new tests should cover your proposed changes in the ScopesAuthorizer class.

Could you add me as collaborator to your forked repo please? I will fix develop branch because now it has the diff, but both develop branches should be identical.

mehyaa · 2023-09-14T11:38:47Z

@raman-m I've fixed the tests. Failing tests were written for the bug that requires one of allowed scopes. I've changed the claims and allowed scopes on tests so they can test the correct conditions.

For adding new tests to test ScopesAuthorizer, the current tests seem pretty sufficient.

mehyaa · 2023-09-14T11:42:23Z

@raman-m I've added you as collaborator on my fork, you can fix the diff or guide me to how-to.

mehyaa · 2023-09-15T07:03:34Z

Interestingly some irrelevant tests fail irregularly.

raman-m · 2023-09-15T17:15:31Z

@mehyaa commented on Sep 14, 11:38 AM

Thanks for fixing of failed tests!

For adding new tests to test ScopesAuthorizer, the current tests seem pretty sufficient.

No, at least one new test should cover claims logic having them multiple in the related config property.
Simultaneously, we should update current tests to be green. Because each test covers specific atomic feature.

Come on! We've changed the logic from single Scope to multiple ones! And it is definitely right time to cover these changes.

I have idea: let's write tests for each linked issue:

a test for AllowedScopes does not match string array in JWT scope claim #913 where you can ask @fsmullinslc to help you
a test for Ocelot forwards Route when userScopes contains Any from routeAllowedScopes, but docs says that it should contains all of them #1066 where you can ask @papugamichal to help you

Sounds good?

raman-m · 2023-09-15T17:24:27Z

@mehyaa commented on Sep 14, 11:42 AM

Thanks for adding me as collaborator!
Now your develop branch is up to date with ThreeMammals:develop. So, I've performed Sync fork operation.
Done!
You can start rebasing of current branches onto (creation of new ones from) your develop branch.

raman-m · 2023-09-15T17:29:21Z

@mehyaa commented on Sep 15

Don't worry! This is unstable scenario: Ocelot.AcceptanceTests.ConfigurationReloadTests.should_reload_config_on_change
The next run fixes the build usually.
Truly speaking, I am tired of this test too. I will create bug issue soon for this test.

raman-m · 2024-01-21T12:23:11Z

@mehyaa
Why not to continue working? Firstly resolving all merge conflicts and merging from develop...

raman-m · 2024-10-19T13:05:19Z

Predecessor

#1739 #2177 OpenIddict authentication: role section, override scope and role key #1431 will be merged first

Scopes can be a space separated list in a single claim. Include this possibility on allowed scopes check.

…anges

raman-m

Reviewing the currently open PR #1431 would be beneficial, as it involves the development of similar changes, but with a more in-depth analysis of the problem within the context of existing auth-provider functionality.

raman-m · 2024-11-05T08:56:24Z

src/Ocelot/Authorization/ScopesAuthorizer.cs

+                if (scope.Contains(' '))
+                {
+                    userScopes = scope.Split(' ', StringSplitOptions.RemoveEmptyEntries);
+                }


It's impossible to predict the body (serialized data) of the token from an unknown Auth-provider.
May I ask which Auth provider you utilize in your project?

raman-m · 2024-11-05T09:02:44Z

src/Ocelot/Authorization/ScopesAuthorizer.cs


-            if (!matchesScopes.Any())
+            if (routeAllowedScopes.Except(userScopes).Any())


This logic inversion is feasible, yet it appears redundant. It seems to be a minor refactoring aimed at reducing the number of lines in the code.
Finally, it is useless change!

Additionally, the valuable suggestion from the previous code review was overlooked. This recommendation is more logical than the favored Except helper.

raman-m · 2024-11-05T09:05:22Z

src/Ocelot/Authorization/ScopesAuthorizer.cs


-            var matchesScopes = routeAllowedScopes.Intersect(userScopes);
+            if (userScopes.Count == 1)


The if-block ought to be relocated to IClaimsParser to preserve the existing logic intact. Consequently, you must inject your specialized IClaimsParser service to generate the precise list of claims.

raman-m · 2024-11-05T09:32:15Z

@mehyaa commented on Sep 14, 2023

I've added you as collaborator on my fork, you can fix the diff or guide me to how-to.

Okay, I just visited your repository and realized I don't have permission to sync the fork because you haven't added me as a collaborator yet.

Why are you lying to me? This is ridiculous and unacceptable!

mehyaa · 2024-11-05T21:04:42Z

@mehyaa commented on Sep 14, 2023

I've added you as collaborator on my fork, you can fix the diff or guide me to how-to.

Okay, I just visited your repository and realized I don't have permission to sync the fork because you haven't added me as a collaborator yet.
...
Why are you lying to me? This is ridiculous and unacceptable!

Easy pal, I've added you as collaborator on Sep 14, 2023 and you have performed Sync fork operation on Sep 15, 2023. I've not perform any operation on that fork afterwards so I have no idea why you are not a collaborator now. I've added you as collaborator again now but your attitude is not elegant, I have no grudge or what so ever towards you so why should I lie for anything, you can ask again kindly for this or anything within my reach. Later.

raman-m · 2024-11-06T10:16:07Z

Thank you for adding me as a collaborator!
Are you planning to address the code review issues further to resolve them?

raman-m assigned mehyaa Jun 30, 2023

raman-m force-pushed the patch-1 branch from fe25ede to dc6076f Compare August 24, 2023 14:28

raman-m added bug Identified as a potential bug proposal Proposal for a new functionality in Ocelot needs feedback Issue is waiting on feedback before acceptance labels Aug 24, 2023

This was referenced Aug 24, 2023

AllowedScopes does not match string array in JWT scope claim #913

Open

Ocelot forwards Route when userScopes contains Any from routeAllowedScopes, but docs says that it should contains all of them #1066

Open

raman-m linked an issue Aug 24, 2023 that may be closed by this pull request

Ocelot forwards Route when userScopes contains Any from routeAllowedScopes, but docs says that it should contains all of them #1066

Open

raman-m added help wanted Not actively being worked on. If you plan to contribute, please drop a note. medium effort Likely a few days of development effort labels Aug 24, 2023

raman-m changed the title ~~ScopesAuthorizer refactoring~~ #913 #1066 ScopesAuthorizer refactoring Sep 15, 2023

raman-m added this to the Annual 2023 milestone Mar 5, 2024

raman-m modified the milestones: Annual 2023, October'24 Oct 9, 2024

This was referenced Oct 19, 2024

AllowedScopes feature is not functioning as anticipated during authentication with the OpenIddict server #2177

Open

#1739 #2177 OpenIddict authentication: role section, override scope and role key #1431

Open

raman-m force-pushed the patch-1 branch from b13d747 to da7faa9 Compare October 19, 2024 13:01

raman-m added Dec'24 December 2024 release and removed Oct'24 October 2024 release labels Oct 26, 2024

raman-m modified the milestones: October'24, Autumn'24 Oct 26, 2024

mehyaa and others added 11 commits November 5, 2024 11:20

ScopesAuthorizer refactoring

c85b7c1

Scopes can be a space separated list in a single claim. Include this possibility on allowed scopes check.

Useless block removed

5090cbc

Update ScopesAuthorizer.cs

63f8973

Fix ScopesAuthorizer

3c9fc3d

Fix ScopesAuthorizer

a05514c

Refactoring

d8510a5

Fix

659cb1e

Fix errors and messages

708c321

Tests fixed

f1f613f

Test names corrected

ecb22c7

Tiny change

528db09

raman-m force-pushed the patch-1 branch from da7faa9 to 528db09 Compare November 5, 2024 08:28

Revert variable names to their previous identifiers to compare the ch…

d9fd3bd

…anges

raman-m requested changes Nov 5, 2024

View reviewed changes

Merge branch 'develop' into patch-1

d9e8e74

raman-m requested review from RaynaldM and ggnaegi November 6, 2024 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#913 #1066 ScopesAuthorizer refactoring #1478

#913 #1066 ScopesAuthorizer refactoring #1478

mehyaa commented May 31, 2021 •

edited by raman-m

Loading

mehyaa commented Jun 1, 2021

PatrickDelancy commented Jul 14, 2021

raman-m commented Jun 30, 2023

raman-m commented Aug 18, 2023

mehyaa commented Aug 18, 2023

raman-m commented Aug 24, 2023 •

edited

Loading

mehyaa commented Sep 14, 2023 •

edited

Loading

mehyaa commented Sep 14, 2023

mehyaa commented Sep 15, 2023 •

edited

Loading

raman-m commented Sep 15, 2023 •

edited

Loading

raman-m commented Sep 15, 2023

raman-m commented Sep 15, 2023

raman-m commented Jan 21, 2024 •

edited

Loading

raman-m commented Oct 19, 2024

raman-m left a comment

raman-m Nov 5, 2024

raman-m Nov 5, 2024 •

edited

Loading

raman-m Nov 5, 2024

raman-m commented Nov 5, 2024

mehyaa commented Nov 5, 2024

raman-m commented Nov 6, 2024


		if (!matchesScopes.Any())
		if (routeAllowedScopes.Except(userScopes).Any())


		var matchesScopes = routeAllowedScopes.Intersect(userScopes);
		if (userScopes.Count == 1)

#913 #1066 ScopesAuthorizer refactoring #1478

Are you sure you want to change the base?

#913 #1066 ScopesAuthorizer refactoring #1478

Conversation

mehyaa commented May 31, 2021 • edited by raman-m Loading

Fixes #913 #1066

Proposed Changes

Predecessor

mehyaa commented Jun 1, 2021

PatrickDelancy commented Jul 14, 2021

raman-m commented Jun 30, 2023

raman-m commented Aug 18, 2023

mehyaa commented Aug 18, 2023

raman-m commented Aug 24, 2023 • edited Loading

mehyaa commented Sep 14, 2023 • edited Loading

mehyaa commented Sep 14, 2023

mehyaa commented Sep 15, 2023 • edited Loading

raman-m commented Sep 15, 2023 • edited Loading

raman-m commented Sep 15, 2023

raman-m commented Sep 15, 2023

raman-m commented Jan 21, 2024 • edited Loading

raman-m commented Oct 19, 2024

Predecessor

raman-m left a comment

Choose a reason for hiding this comment

raman-m Nov 5, 2024

Choose a reason for hiding this comment

raman-m Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

raman-m Nov 5, 2024

Choose a reason for hiding this comment

raman-m commented Nov 5, 2024

mehyaa commented Nov 5, 2024

raman-m commented Nov 6, 2024

mehyaa commented May 31, 2021 •

edited by raman-m

Loading

raman-m commented Aug 24, 2023 •

edited

Loading

mehyaa commented Sep 14, 2023 •

edited

Loading

mehyaa commented Sep 15, 2023 •

edited

Loading

raman-m commented Sep 15, 2023 •

edited

Loading

raman-m commented Jan 21, 2024 •

edited

Loading

raman-m Nov 5, 2024 •

edited

Loading