-
Notifications
You must be signed in to change notification settings - Fork 25.7k
[Entitlements] Instrument nio path #122507
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.
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
Conversation
rjernst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple questions
...src/main/java/org/elasticsearch/entitlement/runtime/api/ElasticsearchEntitlementChecker.java
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/entitlement/runtime/api/ElasticsearchEntitlementChecker.java
Outdated
Show resolved
Hide resolved
rjernst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| class PathActions { | ||
|
|
||
| @EntitlementTest(expectedAccess = PLUGINS) | ||
| static void checkToRealPath() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we haven't been consistent, but in other files we've named this as classMethodName, no "check"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're fairly inconsistent, see NioFileSystemActions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've auto-generated most of the names in my tests, so I don't mess them up, like this. Not pretty, especially when suffixes are needed for overloads. 🤷
This reverts commit 8cc0a71.
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
I've updated to PR to check links using |
...src/main/java/org/elasticsearch/entitlement/runtime/api/ElasticsearchEntitlementChecker.java
Outdated
Show resolved
Hide resolved
💔 Backport failed
You can use sqren/backport to manually backport by running |
(cherry picked from commit 7fd1add) # Conflicts: # libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/api/ElasticsearchEntitlementChecker.java
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
(cherry picked from commit 7fd1add) # Conflicts: # libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/api/ElasticsearchEntitlementChecker.java
Instrument nio
PathtoRealPathandregister.Some other methods may throw
SecurityExceptions, though they are not relevant in our context:Path.of/Paths.get, same asFileSystemProvider#getPath... no need to protect#toAbsolutePath, we grant read access to sys properties (user.dir) anyways, no need to protect in that case#toUri, same as#toAbsolutePathRelates to ES-10794