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

Make HttpRequest available to RequestTransformer via the CasbinAuthorizationContext or CasbinAuthorizationData #18

Closed
thoraj opened this issue Mar 7, 2021 · 10 comments · Fixed by #19
Assignees
Labels
enhancement New feature or request

Comments

@thoraj
Copy link
Contributor

thoraj commented Mar 7, 2021

We wish to provide the HttpRequest object to the RequestTransformer. This will allow the transformer to work out the obj (=request path), and the act method (e.g. GET|POST), without having to explicitly provide those in the CasbinAuthorize attribute. It will also enable scenarios where e.g. the domain/tenant is to be found either in the request headers or in the request content.

I have created a proof of concept which seem to work. So I'm wondering if you are accepting PRs? Or if there are any suitable extension points for this, or if we will be required to fork the project?

@sagilio
Copy link
Member

sagilio commented Mar 7, 2021

Welcome to open PRs!
I think it is a good idea and the HttpRequest object can be added to the ICasbinAuthorizationContext without a lot of changes
It may a better way of replacing the User property with HttpConext in ICasbinAuthorizationContext.

@hsluoyz hsluoyz self-assigned this Mar 7, 2021
@hsluoyz hsluoyz added the enhancement New feature or request label Mar 7, 2021
@thoraj
Copy link
Contributor Author

thoraj commented Mar 9, 2021

Ok.

In my proof of concept I added HttpRequest to the ICasbinAuthorizationData, but perhaps a better place is to add it to ICasbinAuthorizationContext?

I will look into it

@hsluoyz
Copy link
Member

hsluoyz commented Mar 10, 2021

@sagilio plz make a new release.

@thoraj
Copy link
Contributor Author

thoraj commented Mar 11, 2021

Is there anything you need from me which is blocking a new release?

Is there CI/CD (bleeding edge) pipeline where I can get the latest merged changes?

The reason for asking is to avoid having to set up my own nuget package pipeline to get the latest changes into my project.

@sagilio
Copy link
Member

sagilio commented Mar 11, 2021

@thoraj You can refer this:https://github.com/casbin-net/casbin-aspnetcore#installation,
and now the latest build version will be auto push to here : https://www.myget.org/feed/casbin-net/package/nuget/Casbin.AspNetCore.
Recently we will release alpha versions on nuget.org, you can get the new version directly at that time.

@hsluoyz
Copy link
Member

hsluoyz commented Mar 11, 2021

@sagilio we should setup semantic-release to push every merged PR into nuget directly. That's fine because we use semantic-versioning to make sure the users can always know what they are doing, does this make sense?

@thoraj
Copy link
Contributor Author

thoraj commented Mar 11, 2021

Ok.

I am consuming the Casbin packages from the myget feed. The latest versions there are:

  • Casbin.AspNetCore: 0.1.0-build.16.master.64cc1dc (from March 10, 2021)
  • Casbin.AspNetCore.Core: 0.1.0-build.71.master.51c2fd0 (from October 2, 2020)
  • Casbin.AspNetCore.Abstractions: 0.1.0-build.71.master.51c2fd0 (from October 2, 2020)

So until a new release of Casbin.AspNetCore.Abstractions is available the HttpRequest will be missing on the ICasbinAuthorizationContext interface

@sagilio
Copy link
Member

sagilio commented Mar 11, 2021

@hsluoyz Yes, I am doing it.

@sagilio
Copy link
Member

sagilio commented Mar 11, 2021

@thoraj
All package last version should be 0.1.0-build.16.master.64cc1dc, I have unlisted the old version, you can check it now.

And now the HttpConext is available to the CasbinAuthorizationContext, You can get the request in it. Because I think user may need the Connection and more properties not only User and HttpReuqest. The changes in this commit : 2fe5e4f

@thoraj
Copy link
Contributor Author

thoraj commented Mar 11, 2021

Yes fine. Looks good. My reason for not adding the HttpContext was that it is available on the HttpRequest object. But it is probably cleaner to replace both User and HttpRequest with HttpContext.

I just updated the packages from the myget feed, so all is good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants