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.

Sign up for GitHub

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

Jump to bottom

Added authorization for JsonRpc methods #10934

Merged
merged 7 commits into from
Sep 4, 2018
Merged

Added authorization for JsonRpc methods #10934

merged 7 commits into from
Sep 4, 2018

Conversation

sleshchenko
Copy link
Member

@sleshchenko sleshchenko commented Aug 27, 2018 •
edited
Loading

What does this PR do?

  1. Introduces new JsonRpcMethodInvokerFilter component which allows to bind filter before JsonRpc method invocation.
  2. Add permissions filter for JsonRpcInstallerService. All json rpc methods are reviewed and filters are added where it is needed.
  3. 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:
screenshot_20180829_142910

What issues does this PR fix or reference?

#10861

Release Notes

Added authorization checks before calling of JsonRpc methods

Docs PR

N/A

@sleshchenko 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 sleshchenko self-assigned this Aug 27, 2018
@sleshchenko sleshchenko force-pushed the jsonRpcAuthorization branch from 67f018e to e5ee146 Compare August 28, 2018 13:25
@sleshchenko sleshchenko requested a review from riuvshin as a code owner August 28, 2018 13:25
@sleshchenko sleshchenko force-pushed the jsonRpcAuthorization branch from e5ee146 to 32852e3 Compare August 28, 2018 13:34
skabashnyuk
skabashnyuk reviewed Aug 28, 2018
View reviewed changes
Copy link
Contributor

@skabashnyuk skabashnyuk left a comment

Choose a reason for hiding this comment

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

direction of this PR looks good to me.

.../multiuser/organization/api/permissions/OrganizationRemoteSubscriptionPermissionsChecks.java Outdated
import org.eclipse.che.multiuser.api.permission.server.jsonrpc.RemoteSubscriptionPermissionManager;
import org.eclipse.che.multiuser.api.permission.server.model.impl.AbstractPermissions;

/** @author Sergii Leshchenko */
Copy link
Contributor

Choose a reason for hiding this comment

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

Please describe purpose of this class in javadoc

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a test for this class?

@sleshchenko sleshchenko force-pushed the jsonRpcAuthorization branch 2 times, most recently from 4ff8bdb to 91fc90d Compare August 29, 2018 09:35
@sleshchenko sleshchenko changed the title [WIP] Added authorization for JsonRpc methods Added authorization for JsonRpc methods Aug 29, 2018
@sleshchenko 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
@sleshchenko
Copy link
Member Author

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

@sleshchenko
Copy link
Member Author

ci-test

@riuvshin
Copy link
Contributor

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
Copy link
Member Author

ci-test

@l0rd l0rd mentioned this pull request Aug 29, 2018
Closed
57 tasks
@sleshchenko sleshchenko mentioned this pull request Aug 29, 2018
Merged
@sleshchenko sleshchenko force-pushed the jsonRpcAuthorization branch 2 times, most recently from a38532f to 7f3c55b Compare August 30, 2018 08:01
@riuvshin
Copy link
Contributor

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 sleshchenko force-pushed the jsonRpcAuthorization branch from 96ff276 to 30d1735 Compare August 30, 2018 10:56
@sleshchenko
Copy link
Member Author

ci-test

@garagatyi
Copy link

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
Copy link
Member Author

@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
garagatyi approved these changes Aug 30, 2018
View reviewed changes
Copy link

@garagatyi garagatyi left a comment

Choose a reason for hiding this comment

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

Looks good, well done!

.../multiuser/organization/api/permissions/OrganizationRemoteSubscriptionPermissionsChecks.java Outdated
String currentUserId = EnvironmentContext.getCurrent().getSubject().getUserId();

try {
AbstractPermissions permissions =
Copy link

Choose a reason for hiding this comment

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

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

@garagatyi
Copy link

@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
Copy link
Contributor

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
Copy link
Member Author

ci-test

@sleshchenko
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Member Author

ci-test

@riuvshin
Copy link
Contributor

riuvshin commented Sep 3, 2018

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 sleshchenko merged commit 5a4fdee into eclipse-che:master Sep 4, 2018
@sleshchenko sleshchenko deleted the jsonRpcAuthorization branch September 4, 2018 08:07
@benoitf 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 benoitf added this to the 6.11.0 milestone Sep 4, 2018
@riuvshin
Copy link
Contributor

riuvshin commented Sep 4, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skabashnyuk skabashnyuk skabashnyuk approved these changes

@garagatyi garagatyi garagatyi approved these changes

@riuvshin riuvshin Awaiting requested review from riuvshin

Assignees

@sleshchenko sleshchenko

Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Milestone
6.11.0
Development

Successfully merging this pull request may close these issues.
5 participants
@sleshchenko @riuvshin @garagatyi @skabashnyuk @benoitf