Skip to content

Limit scope of the machine token signed requests#11215

Merged
mshaposhnik merged 18 commits intomasterfrom
che-10710
Sep 18, 2018
Merged

Limit scope of the machine token signed requests#11215
mshaposhnik merged 18 commits intomasterfrom
che-10710

Conversation

@mshaposhnik
Copy link
Copy Markdown
Contributor

@mshaposhnik mshaposhnik commented Sep 14, 2018

What does this PR do?

  • Adds permissions limitation to perform operations only with workspace for which the token was issued;
  • Limits list of method tat can be calles using machine token

What issues does this PR fix or reference?

#10710

Release Notes

Docs PR

@mshaposhnik
Copy link
Copy Markdown
Contributor Author

ci-test

@riuvshin
Copy link
Copy Markdown
Contributor

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

@riuvshin
Copy link
Copy Markdown
Contributor

ci-test

@mshaposhnik mshaposhnik changed the title [WIP] Limit scope of the machine token signed requests Limit scope of the machine token signed requests Sep 17, 2018
@riuvshin
Copy link
Copy Markdown
Contributor

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:11215
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

Copy link
Copy Markdown
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.

ok for me. please wait for @sleshchenko feedback.

"/workspace", asList("getByKey", "addProject", "updateProject", "deleteProject"));
allowedMethods.putAll("/ssh", asList("getPair", "generatePair"));
allowedMethods.put("/preferences", "find");
allowedMethods.put("/activity", "active");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There one more service which should be accessible with machine token - it's Factory Service. #10710 (comment)
Are there any plans to make it accessible?

Copy link
Copy Markdown
Contributor Author

@mshaposhnik mshaposhnik Sep 18, 2018

Choose a reason for hiding this comment

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

getFactoryJson is already covered by permissions, so it will be restricted with MachineAuthenticatedSubject, nothing to do there. For the rest of methods, we can add getFactory, getFactoryByAttribute and resolveFactory here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

getFactoryJson is already covered by permissions

It's true. But it won't be allowed since it is not listed in allowed methods here. Am I wrong?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes, you're correct

@Path("/{path:.*}")
public class MachineTokenAccessFilter extends CheMethodInvokerFilter {

private final SetMultimap<String, String> allowedMethods = HashMultimap.create();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It makes sense to change variable name to make clear what is key and value, like parentPathToAllowedMethods. WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

allowedMethodsByPath ?

Copy link
Copy Markdown
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

LGTM

@benoitf benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Sep 18, 2018
@mshaposhnik mshaposhnik merged commit 01d9fc7 into master Sep 18, 2018
@mshaposhnik mshaposhnik deleted the che-10710 branch September 18, 2018 14:24
@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 18, 2018
@benoitf benoitf added this to the 6.12.0 milestone Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/task Internal things, technical debt, and to-do tasks to be performed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants