Conversation
|
Pinging @elastic/code (Team:Code) |
💔 Build Failed
|
fantapsody
force-pushed
the
space_aware_1_on_1
branch
from
8f3d90d to
3141160
Compare
💚 Build Succeeded
|
💚 Build Succeeded
|
kobelb
left a comment
kobelb
left a comment
There was a problem hiding this comment.
The parts of the code which are using the RepositoryReferenceHelper to enforce space-awareness look good! How confident are you that all data-access is consuming the RepositoryReferenceHelper appropriately?
There was a problem hiding this comment.
This isn't a "security issue", but other errors can be thrown in this situation which will be ignored.
There was a problem hiding this comment.
Thanks, the error handling is refined in new commits.
💔 Build Failed
|
💚 Build Succeeded
|
💔 Build Failed
|
💚 Build Succeeded
|
I have added checks on all code REST APIs having accesses to repositories, files, status, and LSPs to make sure only requests from the space with a reference to the repository can have privileges to access. It seems to be proper and complete to me, and I have requested reviews from other backend developers to confirm this. |
There was a problem hiding this comment.
nit: can we wrap this repeated code lines in to a function?
There was a problem hiding this comment.
I have refined the related code, please check if it is better.
There was a problem hiding this comment.
Thank you. It looks great!
For APIs related to repository, file, and search, user should only see contents from repositories belongs to his/her space. When the same repository is added in different spaces, they should point to the same underlying repository. We use references from spaces to repositories to represent this relationship. - after importing a repo R in a space S, a reference is added between R and S. - after deleting a repo R in a space S, the reference between R and S is removed. - a repo is imported and cloned after the first reference to it is added. - a repo is deleted after the last reference to it is removed.
already imported in another space
fantapsody
force-pushed
the
space_aware_1_on_1
branch
from
75b1c27 to
b66c164
Compare
💚 Build Succeeded |
💚 Build Succeeded |
kobelb
left a comment
kobelb
left a comment
There was a problem hiding this comment.
LGTM on the condition that the Code team is sure that the authorization checks are implemented everywhere they should be. It'd make me feel much more comfortable if we implemented some type of abstraction which would perform the appropriate authorization directly prior to the data-access. This is what we do for Saved Objects in general, so it's easier to be confident that authorization is being done in all of the proper places.
💔 Build Failed |
|
retest |
💚 Build Succeeded |
* [Code] makes Code space aware. For APIs related to repository, file, and search, user should only see contents from repositories belongs to his/her space. When the same repository is added in different spaces, they should point to the same underlying repository. We use references from spaces to repositories to represent this relationship. This patch implements the 1:1 model, which means a repository can only be imported in on space.
* [Code] makes Code space aware. For APIs related to repository, file, and search, user should only see contents from repositories belongs to his/her space. When the same repository is added in different spaces, they should point to the same underlying repository. We use references from spaces to repositories to represent this relationship. This patch implements the 1:1 model, which means a repository can only be imported in on space.
5 participants
Related issue https://github.com/elastic/code/issues/838
For APIs related to repository, file, and search, user should only see
contents from repositories belongs to his/her space.
One repository can only exists in the space where it is imported.
Summary
Summarize your PR. If it involves visual changes include a screenshot or gif.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] Unit or functional tests were updated or added to match the most common scenarios[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
[ ] This was checked for breaking API changes and was labeled appropriately[ ] This includes a feature addition or change that requires a release note and was labeled appropriately