search

aerogear / keycloak-connect-graphql

Add Keyloak Authentication and Authorization to your GraphQL server.
Apache License 2.0
157 stars 23 forks source link

Implement PEP feature/ Resource-based access control #119

Closed slavko-vega closed 3 years ago

slavko-vega commented 3 years ago

Implemented are hasPermission function and schema decorator. Tests and examples are added for the new functions. Documentation updated with hasPermission text.

Related to the issue: https://github.com/aerogear/keycloak-connect-graphql/issues/102

wtrocki commented 3 years ago

@slavko-vega This is one of the best community contributions I have seen in my life. Really impressed about how this PR covers everything we do and aligns with the highest standards of contributing to os:

As this is a security change that can affect current projects I would need to have longer than usual time to review your changes and run some internal tests but this looks really interesting and there is not much I would suggest to improve or fix at this point. Just need more time to verify. If I do not came back to this in a week feel free to ping me again.

wtrocki commented 3 years ago

Rebase needed. I will need to create upstream PR to trigger some external builds for security checks we have. https://github.com/aerogear/keycloak-connect-graphql/pull/120

wtrocki commented 3 years ago

I think we should:

1) Rebase this PR and limit number of commits (if not I will rebase it but push your contribution as single commit (I like to have multiple commits but messages are not great in some cases.

2) Resolve conflicts.

wtrocki commented 3 years ago

I can merge when rebased (and some commits squashed). 10 commits are ok. 30 is too much. Thank you so much for your contribution and your time on the project!

slavko-vega commented 3 years ago

I think we should:

  1. Rebase this PR and limit number of commits (if not I will rebase it but push your contribution as single commit (I like to have multiple commits but messages are not great in some cases.
  2. Resolve conflicts.

It's perfectly fine to push all the commits as a single one, that's ok.

wtrocki commented 3 years ago

@slavko-vega I preffer multiple commits (thus if you can simply rebase your branch - I cannot do that on your fork sadly so needed separate PR.

slavko-vega commented 3 years ago

@slavko-vega I preffer multiple commits (thus if you can simply rebase your branch - I cannot do that on your fork sadly so needed separate PR.

Hey @wtrocki I spent some time on the rebase issue and current state of our repo. Unfortunately I wasn't able to find an easy way to rebase our fork. Could we go with squash merge?

wtrocki commented 3 years ago

@slavko-vega anything you like. I'm ok to merge.change once conflicts are resolved

slavko-vega commented 3 years ago

@wtrocki the merge conflict is resolved.

wtrocki commented 3 years ago

Thank you so much for contribution. Can I have your twitter handle? Anounce this change?

slavko-vega commented 3 years ago

Unfortunately I don't have a Twitter account...

wtrocki commented 3 years ago

https://twitter.com/typeapi/status/1351868938041364480

wtrocki commented 3 years ago

https://twitter.com/typeapi/status/1351871739022741505 Changed it little bit