Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: Tenant 1.0.0
    • Component/s: Extensions
    • Labels:
      None

      Description

      Tenants currently can only be administered (create, update, remove) through the Web Console. In addition the TenantProvider service interface allows for looking tenants up (read).

      For administrative purposes it would be good to have a TenantManager service interface which allows for these administrative tasks. Something like:

      public interface TenantManager extends TenantProvider

      { Tenant create(String tenantId, Map<String, Object> properties); void setProperty(Tenant tenant, String name, Object value); void remove(Tenant tenant); }
      1. SLING-2710.patch
        5 kB
        Felix Meschberger
      2. SLING-2710-2.patch
        6 kB
        Felix Meschberger

        Activity

        Felix Meschberger made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Felix Meschberger added a comment -

        Tenant 1.0 has been released, hence closing issues now.

        Show
        Felix Meschberger added a comment - Tenant 1.0 has been released, hence closing issues now.
        Gavin made changes -
        Workflow re-open possible,doc-test-required [ 12790189 ] no-reopen-closed,doc-test-required [ 12792551 ]
        Gavin made changes -
        Workflow no-reopen-closed,doc-test-required [ 12767959 ] re-open possible,doc-test-required [ 12790189 ]
        Carsten Ziegeler made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Carsten Ziegeler added a comment -

        As discussed on the mailing list, we can consider this done

        Show
        Carsten Ziegeler added a comment - As discussed on the mailing list, we can consider this done
        Hide
        Carsten Ziegeler added a comment -

        So we expect only a single provider to be available, right? I think the javadocs need some clarifications in this case. And maybe a different name than TenantProvider - I might be biased but it sounds similar to ResourceProvider where we have a potential set of providers and not just a single one.

        Show
        Carsten Ziegeler added a comment - So we expect only a single provider to be available, right? I think the javadocs need some clarifications in this case. And maybe a different name than TenantProvider - I might be biased but it sounds similar to ResourceProvider where we have a potential set of providers and not just a single one.
        Hide
        Felix Meschberger added a comment -

        I think the confusing stems from the names ...

        TenantProvider is used by client applications to read tenants and tenant information.

        TenantManager is used by management agents to actually manage tenants such as creating, removing, or updating them.

        There is only a single TenantProvider and only a single TenantManager in the framework.

        The idea is properly separate read-only access and management access into separate APIs since the read-only case will be the major use case while the update use case should be reserved to management applications, such as for example some Tenant management UI.

        Show
        Felix Meschberger added a comment - I think the confusing stems from the names ... TenantProvider is used by client applications to read tenants and tenant information. TenantManager is used by management agents to actually manage tenants such as creating, removing, or updating them. There is only a single TenantProvider and only a single TenantManager in the framework. The idea is properly separate read-only access and management access into separate APIs since the read-only case will be the major use case while the update use case should be reserved to management applications, such as for example some Tenant management UI.
        Hide
        Carsten Ziegeler added a comment -

        I had a brief look at the current state and I think there is something wrong wrt TenantProvider/TenantManager. While the API of TenantProvider suggests that there is more than a single provider, the implementation uses exactly one, which is also the manager implementation. So either we separate this, or remove the tenant provider interface.
        Right now, I would opt for removing TenantProvider, especially as this opens some questions when creating a new tenant through the manager. Which provider is used and why and wouldn't we need a way to create a tenant based on a provider?

        Show
        Carsten Ziegeler added a comment - I had a brief look at the current state and I think there is something wrong wrt TenantProvider/TenantManager. While the API of TenantProvider suggests that there is more than a single provider, the implementation uses exactly one, which is also the manager implementation. So either we separate this, or remove the tenant provider interface. Right now, I would opt for removing TenantProvider, especially as this opens some questions when creating a new tenant through the manager. Which provider is used and why and wouldn't we need a way to create a tenant based on a provider?
        Hide
        Felix Meschberger added a comment -

        Committed a first implementation in Rev. 1451514.

        Unit tests and potential fixes following.

        Show
        Felix Meschberger added a comment - Committed a first implementation in Rev. 1451514. Unit tests and potential fixes following.
        Felix Meschberger made changes -
        Assignee Felix Meschberger [ fmeschbe ]
        Gavin made changes -
        Workflow Copy of no-reopen-closed,doc-test-required [ 12765216 ] no-reopen-closed,doc-test-required [ 12767959 ]
        Gavin made changes -
        Workflow no-reopen-closed,doc-test-required [ 12746244 ] Copy of no-reopen-closed,doc-test-required [ 12765216 ]
        Felix Meschberger made changes -
        Attachment SLING-2710-2.patch [ 12570288 ]
        Hide
        Felix Meschberger added a comment -

        Updated patch with removeProperties method.

        Show
        Felix Meschberger added a comment - Updated patch with removeProperties method.
        Hide
        Felix Meschberger added a comment -

        Thanks for the feedback. Very appreciated !

        > addPermissions

        I understand that there would be a need to manage permissions. I also agree with Timothee that this is probably a specific use case. Finally I think this can be solved by a TenantCustomizer.

        > removeProperty

        While a property can be removed as a side effect with setProperty I agree that from a orthogonality perspective this might make sense. How about a

        removeProperties(Tentant, String...)

        method ?

        Show
        Felix Meschberger added a comment - Thanks for the feedback. Very appreciated ! > addPermissions I understand that there would be a need to manage permissions. I also agree with Timothee that this is probably a specific use case. Finally I think this can be solved by a TenantCustomizer. > removeProperty While a property can be removed as a side effect with setProperty I agree that from a orthogonality perspective this might make sense. How about a removeProperties(Tentant, String...) method ?
        Hide
        Timothee Maret added a comment -

        Hi Shashank, I missed this, autant pour moi.
        Thus all good

        Show
        Timothee Maret added a comment - Hi Shashank, I missed this, autant pour moi. Thus all good
        Hide
        Shashank Gupta added a comment -

        >- removeProperty(Tenant tenant, String propName)
        setProperty can remove property also. see its api doc.

        • @param name The name of the property to set or remove.
        • @param value The new value of the property. If this value is {@code null}
        • the property is actually removed.
          void setProperty(Tenant tenant, String name, Object value);
        Show
        Shashank Gupta added a comment - >- removeProperty(Tenant tenant, String propName) setProperty can remove property also. see its api doc. @param name The name of the property to set or remove. @param value The new value of the property. If this value is {@code null} the property is actually removed. void setProperty(Tenant tenant, String name, Object value);
        Hide
        Timothee Maret added a comment -

        "Regarding addPermissions, it is restrict role based access at tenant level (like having write access for admins/moderators and read access for everybody else). "
        I think this is a rather specific use case, and it would not work since the TenantProvider#getTenant[s] is using an admin session for retriving tenants (means the TenantProvider API should change as well).
        Maybe you could achieve this by setting the ACL in the content referenced by the tenant, wdyt ?

        Show
        Timothee Maret added a comment - "Regarding addPermissions, it is restrict role based access at tenant level (like having write access for admins/moderators and read access for everybody else). " I think this is a rather specific use case, and it would not work since the TenantProvider#getTenant [s] is using an admin session for retriving tenants (means the TenantProvider API should change as well). Maybe you could achieve this by setting the ACL in the content referenced by the tenant, wdyt ?
        Hide
        kannan iyer added a comment -

        Hi Timothee,
        Thanks for the update on getTenants/getTenant. Regarding addPermissions, it is restrict role based access at tenant level (like having write access for admins/moderators and read access for everybody else).

        Show
        kannan iyer added a comment - Hi Timothee, Thanks for the update on getTenants/getTenant. Regarding addPermissions, it is restrict role based access at tenant level (like having write access for admins/moderators and read access for everybody else).
        Hide
        Timothee Maret added a comment -

        Hi kannan,

        I think getTenants/getTenant don't need to be added as it is already available in the TenantProvider. Beside, what would be the use of addPermissions(ACLEntry[]) ?

        +1 for the proposed API and for adding removeProperty method

        Show
        Timothee Maret added a comment - Hi kannan, I think getTenants/getTenant don't need to be added as it is already available in the TenantProvider. Beside, what would be the use of addPermissions(ACLEntry[]) ? +1 for the proposed API and for adding removeProperty method
        Hide
        kannan iyer added a comment -

        Following API's could be useful:

        • ArrayList<Tenant> getTenants
        • Tenant getTenant(String)
        • removeProperty(Tenant tenant, String propName)
        • addPermissions (ACLEntry[]) //similarly update, delete, get Permissions.
        • assuming tenant can be adapted to JCR node.
        Show
        kannan iyer added a comment - Following API's could be useful: ArrayList<Tenant> getTenants Tenant getTenant(String) removeProperty(Tenant tenant, String propName) addPermissions (ACLEntry[]) //similarly update, delete, get Permissions. assuming tenant can be adapted to JCR node.
        Hide
        Felix Meschberger added a comment -

        Hmm, good point. I am not sure, whether TenantManager should be a TenantProvider or not. So at the time of creating the patch I decided it would not be a good idea: By having two distinct interfaces we do not limit implementations (they could be done with a single or two independent classes) and we also clearly separate read access from create, update, delete access.

        Show
        Felix Meschberger added a comment - Hmm, good point. I am not sure, whether TenantManager should be a TenantProvider or not. So at the time of creating the patch I decided it would not be a good idea: By having two distinct interfaces we do not limit implementations (they could be done with a single or two independent classes) and we also clearly separate read access from create, update, delete access.
        Hide
        Shashank Gupta added a comment -

        two observations:
        1. as per jira's description TenantManager extends TenantProvider interface but in patch it doesn't.
        2. No api to retrieve tenant from TenantManager. In fact TenantProvider has api to retrieve tenant from tenantId.

        Show
        Shashank Gupta added a comment - two observations: 1. as per jira's description TenantManager extends TenantProvider interface but in patch it doesn't. 2. No api to retrieve tenant from TenantManager. In fact TenantProvider has api to retrieve tenant from tenantId.
        Hide
        Amit Gupta added a comment -

        looks good..

        Show
        Amit Gupta added a comment - looks good..
        Felix Meschberger made changes -
        Field Original Value New Value
        Attachment SLING-2710.patch [ 12569922 ]
        Hide
        Felix Meschberger added a comment -

        Proposed patch.

        Show
        Felix Meschberger added a comment - Proposed patch.
        Felix Meschberger created issue -

          People

          • Assignee:
            Felix Meschberger
            Reporter:
            Felix Meschberger
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development