#913 #1066 ScopesAuthorizer refactoring

[mehyaa]

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

[mehyaa]

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

[PatrickDelancy]

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

[raman-m]

Please do the following:

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

[raman-m]

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]

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]

@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]

@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]

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

[mehyaa]

Interestingly some irrelevant tests fail irregularly.

[raman-m]

@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]

@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]

@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]

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

[raman-m]

Possible dependency

• OpenIddict implementation, role section, override scope and role key #1431
  which will be merged first❕

[raman-m]

The branch has been rebased onto release/24.0 with top commit 59b63ea !

[raman-m]

@mehyaa Mehmet,
If you're no longer interested in your PR, please allow me to take it over. I will then deliver the feature with my own design vision.

Development is required❕

[raman-m]

Failed acceptance test: Ocelot.AcceptanceTests.AuthorizationTests.should_return_response_200_using_identity_server_with_allowed_scope

[alex0360]

Denial is more costly:

To make scope checking more efficient and less costly, a good practice is to invert the condition.

You can also use a method that minimizes the number of iterations required in the collections, making sure that all required scopes are present in the user scopes.

if (routeAllowedScopes.TrueForAll(userScopes.Contains))
{
    return new OkResponse<bool>(true);
}

return new ErrorResponse<bool>(
    new ScopeNotAuthorizedError($"User scopes: '{string.Join(",", userScopes)}' do not have all allowed scopes: '{string.Join(",", routeAllowedScopes)}'"));

[raman-m]

Right! Thanks!
It makes sense to invert the condition.