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

OpenIddict implementation, role section, override scope and role key #1431

Open
wants to merge 10 commits into
base: release/24.0
Choose a base branch
from

Conversation

Eits-Ian
Copy link

@Eits-Ian Eits-Ian commented Feb 7, 2021

"ScopeKey": "oi_scp" to enable OpenIddict

"RequiredRole": [ "User" ]

Fixes / New Feature #
???

Proposed Changes

  • Added role section
  • Variable to define an override for the scope or role descriptor

@raman-m
Copy link
Member

raman-m commented Jul 11, 2023

Hi @Eits-Ian !
Thanks for your interest in Ocelot gateway!

Please do the following:

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

@raman-m raman-m added the conflicts Feature branch has merge conflicts label Jul 11, 2023
@raman-m raman-m added feature A new feature proposal Proposal for a new functionality in Ocelot needs feedback Issue is waiting on feedback before acceptance and removed conflicts Feature branch has merge conflicts labels Aug 23, 2023
@raman-m
Copy link
Member

raman-m commented Aug 23, 2023

@Eits-Ian
I thought you needed a help to resolve the conflicts.
The feature branch has been rebased onto ThreeMammals:develop successfully!
Now we have 4 failed tests!
We cannot start code review till all tests have passed...


I see that develop branch in your fork is old.
Could you Sync fork please? So, the develop branch will be updated with all top commits!
Could you add me as collaborator to your forked repo please? I will update develop branch.

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.

4 failed tests!
Could you review & fix them please?

@Eits-Ian
Copy link
Author

Sorry I did not get a chance to look at this till today, should be fixed, have added you as a collaborator, I hadn't looked at this for a couple of years. Need to add process policies, I am really busy at the moment so will not be able to get to this for a while.

@Eits-Ian
Copy link
Author

The error is at ClaimsToDownstreamPathTests it is not authenticating, I need to take a look at what is happening but it will need to be later

@raman-m
Copy link
Member

raman-m commented Aug 28, 2023

@Eits-Ian commented on Aug 24

Thanks for caring!
I am planning to help you in September by having of some pair programming...
Sorry, now I have a lot of things to do with this project...

@raman-m raman-m added the 2023 Annual 2023 release label Mar 5, 2024
@raman-m raman-m added this to the Annual 2023 milestone Mar 5, 2024
@raman-m raman-m changed the base branch from develop to release/24.0 March 5, 2024 10:04
@raman-m
Copy link
Member

raman-m commented Apr 16, 2024

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

  • Further development is required❕
  • Fixing failed tests is required❗

@raman-m
Copy link
Member

raman-m commented Apr 16, 2024

@Eits-Ian Dear author,
Are you still with Ocelot?
And, what's your name? 😉

Will this PR close, is PR related to the following issues?

Please answer ASAP❗

P.S.

OpenIddict implementation, role section, override scope and role key

What is OpenIddict? It should likely not be part of the PR title; instead, the title should include Ocelot's artifact name or, if necessary, the ASP.NET one.

@raman-m raman-m added waiting Waiting for answer to question or feedback from issue raiser and removed needs feedback Issue is waiting on feedback before acceptance labels Apr 16, 2024
@Eits-Ian
Copy link
Author

Eits-Ian commented Apr 18, 2024 via email

@raman-m
Copy link
Member

raman-m commented Apr 19, 2024

I haven't got much time to do much with this at the moment, I will need to get back into it as I must update the api, that use this, but that will not be for a few months

It should be a matter of weeks, not months!

@Eits-Ian
Copy link
Author

Eits-Ian commented Apr 19, 2024 via email

@raman-m
Copy link
Member

raman-m commented Apr 19, 2024

@Eits-Ian
I don't understand you!
Should we close the PR or will you develop it in coming future?

Eits-Ian and others added 3 commits April 19, 2024 16:27
raman-m and others added 5 commits April 19, 2024 16:27
Registered IRolesAuthorizer
Fixed Test should_not_be_valid_if_specified_authentication_provider_isnt_registered()
Fixed test configuration_is_invalid_with_invalid_authentication_provider()
@Eits-Ian
Copy link
Author

Eits-Ian commented Apr 19, 2024 via email

@raman-m
Copy link
Member

raman-m commented Apr 19, 2024

🆗 Got it! You are super busy… In this case, let me decide what to do with this PR…

Comment on lines +63 to +64
if (!IsOptionsHttpMethod(httpContext) && IsAuthenticatedRoute(downstreamRoute))
{
Copy link
Member

@raman-m raman-m Apr 19, 2024

Choose a reason for hiding this comment

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

Why did you copy whole if-block above?

I strongly disapprove of any form of copying of blocks❗

@raman-m
Copy link
Member

raman-m commented Apr 19, 2024

So, @Eits-Ian … The current implication is failing of 5 acceptance tests❗ See the build results!

@raman-m raman-m added the low Low priority label Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2023 Annual 2023 release feature A new feature low Low priority proposal Proposal for a new functionality in Ocelot waiting Waiting for answer to question or feedback from issue raiser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants