Added authorization for JsonRpc methods by sleshchenko · Pull Request #10934 · eclipse-che/che

sleshchenko · 2018-08-27T14:27:00Z

What does this PR do?

Introduces new JsonRpcMethodInvokerFilter component which allows to bind filter before JsonRpc method invocation.
Add permissions filter for JsonRpcInstallerService. All json rpc methods are reviewed and filters are added where it is needed.
Add component which allows to bind permissions check by remote subscription method. All json rpc remote subscriptions methods are reviewed and checks are added where it is needed.

Here is an example of an invocating JsonRpc method and remote subscribing without required permissions:

What issues does this PR fix or reference?

#10861

Release Notes

Added authorization checks before calling of JsonRpc methods

Docs PR

N/A

skabashnyuk

direction of this PR looks good to me.

skabashnyuk · 2018-08-28T17:20:10Z

.../multiuser/organization/api/permissions/OrganizationRemoteSubscriptionPermissionsChecks.java

+import org.eclipse.che.multiuser.api.permission.server.jsonrpc.RemoteSubscriptionPermissionManager;
+import org.eclipse.che.multiuser.api.permission.server.model.impl.AbstractPermissions;
+
+/** @author Sergii Leshchenko */


Please describe purpose of this class in javadoc

Do we need a test for this class?

sleshchenko · 2018-08-29T09:36:28Z

@skabashnyuk @garagatyi PR is ready to review, please take a look.

sleshchenko · 2018-08-29T11:43:38Z

ci-test

riuvshin · 2018-08-29T13:10:49Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10934
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

sleshchenko · 2018-08-29T14:09:52Z

ci-test

riuvshin · 2018-08-30T09:34:25Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10934
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

sleshchenko · 2018-08-30T10:56:57Z

ci-test

garagatyi · 2018-08-30T12:48:45Z

As far as I understand there is no default permissions filter in this PR. This leads us to a situation when new JSON_RPC methods are not secure by default, but rather insecure by default. I find it a popular practice in modern software to make stuff secure by default since it leads to less critical vulnerabilities. Please, consider adding such an approach before merging this PR.

sleshchenko · 2018-08-30T13:21:17Z

@garagatyi

As far as I understand there is no default permissions filter in this PR. This leads us to a situation when new JSON_RPC methods are not secure by default, but rather insecure by default. I find it a popular practice in modern software to make stuff secure by default since it leads to less critical vulnerabilities.

I thought about it and decided to implement easier way, where there is no default permissions filter since the same situation as we have with our REST API service/methods (they are public by default).

Please, consider adding such an approach before merging this PR.

I would merge this PR as it is since it introduces the authorization mechanism for Json Rpc methods and it can be improved in a separated issue and PR. BTW It's much secure to have a current approach than in master branch. I'll create a separate issue if you're OK with that.

garagatyi

Looks good, well done!

garagatyi · 2018-08-30T13:30:42Z

.../multiuser/organization/api/permissions/OrganizationRemoteSubscriptionPermissionsChecks.java

+      String currentUserId = EnvironmentContext.getCurrent().getSubject().getUserId();
+
+      try {
+        AbstractPermissions permissions =


Can you add a comment to the code with the explanation of why this part is implemented in this way?

garagatyi · 2018-08-30T13:32:59Z

@sleshchenko I'm OK with implementing the secure-by-default approach in a separate issue. Hope your team would be able to prioritize such a task

riuvshin · 2018-08-30T14:52:53Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10934
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

sleshchenko · 2018-08-31T08:32:26Z

ci-test

sleshchenko · 2018-08-31T09:08:38Z

@garagatyi I've created an issue to make resources exposed by REST and WebSocket private by default #11007. Feel free to comment or edit.

riuvshin · 2018-08-31T12:16:50Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10934
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

sleshchenko · 2018-09-03T08:00:48Z

ci-test

riuvshin · 2018-09-03T14:59:19Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10934
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

It allows to build a project without tests compiling

…scription

…ptions

…riptions

riuvshin · 2018-09-04T10:10:23Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:10934
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

sleshchenko added status/in-progress This issue has been taken by an engineer and is under active development. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Aug 27, 2018

sleshchenko self-assigned this Aug 27, 2018

sleshchenko requested review from garagatyi and skabashnyuk August 27, 2018 14:27

sleshchenko force-pushed the jsonRpcAuthorization branch from 67f018e to e5ee146 Compare August 28, 2018 13:25

sleshchenko requested a review from riuvshin as a code owner August 28, 2018 13:25

sleshchenko force-pushed the jsonRpcAuthorization branch from e5ee146 to 32852e3 Compare August 28, 2018 13:34

skabashnyuk reviewed Aug 28, 2018

View reviewed changes

sleshchenko force-pushed the jsonRpcAuthorization branch 2 times, most recently from 4ff8bdb to 91fc90d Compare August 29, 2018 09:35

sleshchenko changed the title ~~[WIP] Added authorization for JsonRpc methods~~ Added authorization for JsonRpc methods Aug 29, 2018

sleshchenko added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. and removed status/in-progress This issue has been taken by an engineer and is under active development. labels Aug 29, 2018

skabashnyuk approved these changes Aug 29, 2018

View reviewed changes

l0rd mentioned this pull request Aug 29, 2018

EclipseCon Europe Workshop #10644

Closed

57 tasks

sleshchenko mentioned this pull request Aug 29, 2018

Move WS.NEXT flow to k8s infra implementation, rework it to use plugin broker #10740

Merged

sleshchenko force-pushed the jsonRpcAuthorization branch 2 times, most recently from a38532f to 7f3c55b Compare August 30, 2018 08:01

sleshchenko force-pushed the jsonRpcAuthorization branch from 96ff276 to 30d1735 Compare August 30, 2018 10:56

garagatyi approved these changes Aug 30, 2018

View reviewed changes

sleshchenko added 7 commits September 4, 2018 11:07

Fix scope of test dependencies

6a55994

It allows to build a project without tests compiling

CHE-10861 Add concept of JsonRpcMethodInvokerFilter

dc25b5a

CHE-10861 Add permissions filter for installer json rpc methods

6f125e8

CHE-10861 Add an ability to register permissions check for remote sub…

194e2c5

…scription

CHE-10861 Add permissions check for workspaces related remote subscri…

f8e0440

…ptions

CHE-10861 Add permissions check for system related remote subscriptions

dfb5852

CHE-10861 Add permissions check for organization related remote subsc…

e62dbb7

…riptions

sleshchenko force-pushed the jsonRpcAuthorization branch from db03ee2 to e62dbb7 Compare September 4, 2018 08:07

sleshchenko merged commit 5a4fdee into eclipse-che:master Sep 4, 2018

sleshchenko deleted the jsonRpcAuthorization branch September 4, 2018 08:07

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Sep 4, 2018

benoitf added this to the 6.11.0 milestone Sep 4, 2018

Conversation

sleshchenko commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

Uh oh!

skabashnyuk left a comment

Choose a reason for hiding this comment

Uh oh!

skabashnyuk Aug 28, 2018

Choose a reason for hiding this comment

Uh oh!

skabashnyuk Aug 28, 2018

Choose a reason for hiding this comment

Uh oh!

sleshchenko commented Aug 29, 2018

Uh oh!

sleshchenko commented Aug 29, 2018

Uh oh!

riuvshin commented Aug 29, 2018

Uh oh!

sleshchenko commented Aug 29, 2018

Uh oh!

riuvshin commented Aug 30, 2018

Uh oh!

sleshchenko commented Aug 30, 2018

Uh oh!

garagatyi commented Aug 30, 2018

Uh oh!

sleshchenko commented Aug 30, 2018

Uh oh!

garagatyi left a comment

Choose a reason for hiding this comment

Uh oh!

garagatyi Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

garagatyi commented Aug 30, 2018

Uh oh!

riuvshin commented Aug 30, 2018

Uh oh!

sleshchenko commented Aug 31, 2018

Uh oh!

sleshchenko commented Aug 31, 2018

Uh oh!

riuvshin commented Aug 31, 2018

Uh oh!

sleshchenko commented Sep 3, 2018

Uh oh!

riuvshin commented Sep 3, 2018

Uh oh!

riuvshin commented Sep 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sleshchenko commented Aug 27, 2018 •

edited

Loading