Ocelot
Ocelot copied to clipboard
ThreeMammals
#913 #1066 ScopesAuthorizer refactoring
Fixes #913 #1066
- #913
- #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
- #1431
The build is broken, it fails on irrelevant part from this PR. #1436 fixes is I think.
This is still an issue. Adding my :+1: to get this small and valuable PR merged.
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?
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.
@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.
@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.
@raman-m I've added you as collaborator on my fork, you can fix the diff or guide me to how-to.
@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 #913 where you can ask @fsmullinslc to help you
- a test for #1066 where you can ask @papugamichal to help you
Sounds good?
@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.
@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.
@mehyaa Why not to continue working? Firstly resolving all merge conflicts and merging from develop...
The branch has been rebased onto release/24.0 with top commit https://github.com/ThreeMammals/Ocelot/commit/59b63eab9c2cd2ab33c5752bbc3bd00a079cd74c !
@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❕
Failed acceptance test: Ocelot.AcceptanceTests.AuthorizationTests.should_return_response_200_using_identity_server_with_allowed_scope