Skip to content

Add single static instance of SpecialPermission#22726

Merged
Tim-Brooks merged 4 commits intoelastic:masterfrom
Tim-Brooks:sp_single_instance
Jan 21, 2017
Merged

Add single static instance of SpecialPermission#22726
Tim-Brooks merged 4 commits intoelastic:masterfrom
Tim-Brooks:sp_single_instance

Conversation

@Tim-Brooks
Copy link
Copy Markdown
Contributor

@Tim-Brooks Tim-Brooks commented Jan 20, 2017

This commit adds a SpecialPermission constant and uses that constant
opposed to introducing new instances everywhere.

Additionally, this commit introduces a single static method to check that
the current code has permission. This avoids all the duplicated access
blocks that exist currently.

Copy link
Copy Markdown
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM. I'm good with it, let's make sure @s1monw is before this is merged in.

Copy link
Copy Markdown
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM too

final SecurityManager sm = System.getSecurityManager();
if (sm != null) {
sm.checkPermission(new SpecialPermission());
sm.checkPermission(SpecialPermission.INSTANCE);
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.

Can this be replaced with SpecialPermission.checkSpecialPermission()?

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.

Yes. It is slightly different in form which is why autorefactoring probably did not catch it.

/**
* Check that the current stack has {@link SpecialPermission} access according to the {@link SecurityManager}.
*/
public static void checkSpecialPermission() {
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.

Maybe just call this method check() since it is used statically?

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.

Yeah. I added a committing changing that.

Copy link
Copy Markdown
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

LGTM 2

@Tim-Brooks Tim-Brooks merged commit a4ac29c into elastic:master Jan 21, 2017
@Tim-Brooks Tim-Brooks deleted the sp_single_instance branch January 21, 2017 18:03
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 23, 2017
* master: (33 commits)
  Docs fix - Added missing link to new Adjacency-matrix agg
  Pass `forceExecution` flag to transport interceptor (elastic#22739)
  Version: Add missing releases from 2.x in Version.java (elastic#22594)
  CONSOLE-ify filter aggregation docs
  CONSOLE-ify date_range aggregation docs
  Add single static instance of SpecialPermission (elastic#22726)
  Simplify InternalEngine#innerIndex (elastic#22721)
  Upgrade to Lucene 6.4.0 (elastic#22724)
  Fix broken TaskInfo.toString()
  Add CheckedSupplier and CheckedRunnable to core (elastic#22725)
  Revert "Make build Gradle 2.14 / 3.x compatible (elastic#22669)"
  Fixes retrieval of the latest snapshot index blob (elastic#22700)
  CONSOLE-ify date histogram docs
  CONSOLE-ify min and max aggregation docs
  CONSOLE-ify global-aggregation.asciidoc
  Fix script score function that combines _score and weight (elastic#22713)
  Corrected a plural verb to a singular one. (elastic#22681)
  Fix duplicates from search.query (elastic#22701)
  Readd unconverted snippets mark for doc
  Deguice rest handlers (elastic#22575)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >non-issue v6.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants