You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@geode.apache.org by "jujoramos (GitHub)" <gi...@apache.org> on 2019/11/14 15:09:12 UTC

[GitHub] [geode] jujoramos opened pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

- Added 'build-docs-output.txt' to list of git ignored files.
- Fixed the 'Authorization Overview' page to reflect new directory
layout and removed deprecated examples.
- Added example to illustrrate how the 'MethodInvocationAuthorizer'
can be used to implement a custom authorization mechanism.
- Added comprehensive documentation about the OQL Method Invocation
Security feature, pros and cons, how it can it customized and
configured, etcetera.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

- [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?

- [X] Is your initial contribution a single, squashed commit?

- [X] Does `gradlew build` run cleanly?

- [X] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on issue #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "jujoramos (GitHub)" <gi...@apache.org>.

Thanks @davebarnes97!.
Please don't merge this yet, though, I'll do it in due time.

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.

I think it would be best to remove the comma after "to note that"

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "jujoramos (GitHub)" <gi...@apache.org>.

👍 

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "jujoramos (GitHub)" <gi...@apache.org>.

Typical `copy-paste` error, thanks for catching this!.

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.

Maybe this should be "How to Implement a Method Authorizer" or "Implementing a Method Authorizer"

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "jujoramos (GitHub)" <gi...@apache.org>.

The fact that one column has text marked as code - "`" character - is preventing markdown from splitting it into different lines... I'll remove the table and use another approach.

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "jujoramos (GitHub)" <gi...@apache.org>.

I've used "permanently" to keep some kind of consistency with the the actual method name in the `RestrictedMethodAuthorizer`: `isPermanentlyForbiddenMethod()`.

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.

Maybe make it clear that **all** the following conditions have to be met for the method to be authorized.

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "jujoramos (GitHub)" <gi...@apache.org>.

Wow, totally missed this one, nice catch!.

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.

The "Description" column should probably be a little wider. The text is quite squashed as it currently is.

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "jujoramos (GitHub)" <gi...@apache.org>.

👍 

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.

Maybe change to "forbids the invocation of all methods during a query..."

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "jujoramos (GitHub)" <gi...@apache.org>.

👍 

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on issue #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "jujoramos (GitHub)" <gi...@apache.org>.

@BenjaminPerryRoss @DonalEvans 

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos closed pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "jujoramos (GitHub)" <gi...@apache.org>.

[ pull request closed by jujoramos ]

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.

I don't think it's necessary to include the "exhaustively studied" part here. Just saying that they have to be configured correctly is enough.

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "jujoramos (GitHub)" <gi...@apache.org>.

👍 

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "jujoramos (GitHub)" <gi...@apache.org>.

Typical copy-paste error, thanks for catching this!.



[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.

Maybe change this to "some of the authorizers as an incorrect configuration might..."

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] jujoramos commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "jujoramos (GitHub)" <gi...@apache.org>.

👍 

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.

Some text missing here, possibly accidentally removed.

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.

I think the ID of this section should be unique. Currently all three sections on this page use "overview" as their ID, which makes it impossible to link to the second two. Maybe change it to "user_authorization_example".

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.

Should be "does belong but is considered safe"

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.

Should be "always use one of **these** authorizers".

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org

[GitHub] [geode] DonalEvans commented on pull request #4325: GEODE-6994: OQL Add Method Invocation Security Docs

Posted by "DonalEvans (GitHub)" <gi...@apache.org>.

I think it's possibly not correct to refer to this as flexible, since its behaviour is fixed and unchangeable. It might be better to refer to it as "less restrictive" or something similar.

[ Full content available at: https://github.com/apache/geode/pull/4325 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org