Skip to content

Implementation of a context-based permissions API#340

Closed
zml2008 wants to merge 1 commit intomasterfrom
feature/permissions
Closed

Implementation of a context-based permissions API#340
zml2008 wants to merge 1 commit intomasterfrom
feature/permissions

Conversation

@zml2008
Copy link
Member

@zml2008 zml2008 commented Dec 29, 2014

This API provides a lot of functionality for permissions providers,
including transient permissions, various contexts, and a basic
inheritance structure. This API is meant to serve as a flexible base to
build on.

There may be some utility classes added as I get to implementing this API in PEX and in Sponge (the default MC permissions system), but this PR has the API as I think is good

@zml2008 zml2008 force-pushed the feature/permissions branch from 9d2cdbb to ece2e54 Compare December 29, 2014 05:26
This was referenced Dec 29, 2014
@zml2008 zml2008 force-pushed the feature/permissions branch from ece2e54 to 60e4ad4 Compare December 29, 2014 06:43
Copy link
Contributor

Choose a reason for hiding this comment

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

missind period

@phroa
Copy link
Contributor

phroa commented Dec 29, 2014

I probably missed some line length comments, you might want to review your javadocs. I think we're doing a max of 80 chars/line for javadocs.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldnt there be a global context too instead of only world context?

Copy link
Member Author

Choose a reason for hiding this comment

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

Global context is empty context set

On Mon, Dec 29, 2014 at 03:28:12AM -0800, ST-DDT wrote:

  • * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
  • * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
  • * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
  • * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
  • * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  • * THE SOFTWARE.
  • /
    +package org.spongepowered.api.service.permission.context;
    +
    +import com.google.common.base.Preconditions;
    +
    +/
    *
  • * The context that a permission check occurs in. Instances of a context are designed
  • * to function as cache keys, meaning they should be fairly lightweight and not hold references to large objects
  • */
    +public final class Context {

Shouldnt there be a global context too instead of only world context?


Reply to this email directly or view it on GitHub:
https://github.com/SpongePowered/SpongeAPI/pull/340/files#r22309117

@ST-DDT
Copy link
Member

ST-DDT commented Dec 29, 2014

I have a few questions regarding this PR.

  • Is there a reason why you don't number or string value support to the Api? (Chat pefix and suffix stuff)
  • Is it possible to register a description for a permission?
  • Using this Api how would i set the default permission levels? Especially Op and NoOp.

Copy link
Member

Choose a reason for hiding this comment

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

If checking if the subject has a parent do i have to request the same combination of contexts or is it enough to request one of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

When setting, you give the contexts that all have to match for the
given permission to be true

When getting from Subject, you give the contexts, any of which have to match a
combination (basically getContexts.contains setContexts)

Remember that SubjectData is raw data tho, so doesn't do these
calculations. so getting with a given combination returns only parents
set for that exact combination

On Mon, Dec 29, 2014 at 04:37:46AM -0800, ST-DDT wrote:

  • /**
  • \* Return all registered parent subjects for a given context. The returned map is immutable and not a live view.
    
  • \* The results of this method do not traverse any sort of inheritance structure a permissions plugin may implement.
    
  • *
    
  • \* @param contexts The context to check
    
  • \* @return names of parents valid in the given context
    
  • */
    
  • List getParents(Set contexts);
  • /**
  • \* Adds a parent in a particular context combination. Passing an empty context combination means the parent is added in the global context
    
  • *
    
  • \* @param contexts The context combination this operation is applicable to
    
  • \* @param parent The name of the parent to add
    
  • */
    
  • void addParent(Set contexts, String parent);

If checking if the subject has a parent do i have to request the same combination of contexts or is it enough to request one of them?


Reply to this email directly or view it on GitHub:
https://github.com/SpongePowered/SpongeAPI/pull/340/files#r22310394

Copy link
Contributor

Choose a reason for hiding this comment

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

Methods in Preconditions should be imported statically

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that really something we do?

On Mon, Dec 29, 2014 at 10:44:24AM -0800, Simon816 wrote:

  • /
    +package org.spongepowered.api.service.permission.context;
    +
    +import com.google.common.base.Preconditions;
    +
    +/
    *
  • * The context that a permission check occurs in. Instances of a context are designed
  • * to function as cache keys, meaning they should be fairly lightweight and not hold references to large objects
  • */
    +public final class Context {
  • public static final String WORLD_KEY = "world";
  • private final String type, name;
  • public Context(String type, String name) {
  •    Preconditions.checkNotNull(type, "type");
    

Methods in Preconditions should be imported statically


Reply to this email directly or view it on GitHub:
https://github.com/SpongePowered/SpongeAPI/pull/340/files#r22322939

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you can see that elsewhere and Guava wiki recommends it

Guava provides a number of precondition checking utilities. We strongly recommend importing these statically.

(https://code.google.com/p/guava-libraries/wiki/PreconditionsExplained)

@zml2008
Copy link
Member Author

zml2008 commented Dec 31, 2014

On Mon, Dec 29, 2014 at 03:57:36AM -0800, ST-DDT wrote:

I have a few questions regarding this PR.

  • Is there a reason why you don't number or string value support to the Api?
    Because that's options, not permissions
  • Is it possible to register a decription for a permission?
    no. the api has no concept of registering a permission
  • Using this Api how would i set the default permission levels? Especially Op and NoOp.
    op/notop are implementation details, will be presented as group subjects
    in the default permissions implementation. they are optional for any
    other permission plugin tho.

If you want to set default data, there is PermissionService.defaultData
(or whatever I called it)


Reply to this email directly or view it on GitHub:
#340 (comment)

@ST-DDT
Copy link
Member

ST-DDT commented Dec 31, 2014

@zml2008 Thank you for your detailed explanations. i now agree with the team's(?) opinion that your suggestions are better than mine. (Although there are some things that i would do differently/"my way", but i can understand why you did it that/your way)

I have a few questions regarding this PR.

  • Is there a reason why you don't number or string value support to the Api? (Chat prefix and suffix stuff)

Because that's options, not permissions

Are there plans to add "options" support to the player/user?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants