Skip to content
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

feature/VONK-7767: Add information about changes to multi-tenancy #605

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

andrzejskowronski
Copy link
Contributor

No description provided.

features_and_tools/multi-tenancy.rst Outdated Show resolved Hide resolved
features_and_tools/multi-tenancy.rst Outdated Show resolved Hide resolved
Comment on lines 78 to 89
:TenantHeader: The name of the HTTP header to read the tenant from. This is only evaluated if ``AllowTenantFromHeader`` is set to ``true``.
:TenantClaim: The name of the claim in the authorization token to read the tenant from. This is only evaluated if ``AllowTenantFromClaim`` is set to ``true``.
:TenantLabelSystem: The value of the ``meta.security.system`` element to use for the tenant security label. You can only choose this once.
:AllowTenantFromHeader: The tenant may be specified with an HTTP header, with the name specified in ``TenantHeader``.
:AllowTenantFromClaim: The tenant may be specified with a claim in the authorization token, with the name specified in ``TenantClaim``.
:RemoveTenantLabelFromResponse: The label will be removed from the resource by default, but this behaviour can be disabled if this option is set to ``false``.
:ResourceAccess: This structure allows to define set of conditional logic that will be applied to each request.
:ResourceAccess:Priority: This value will be used to choose most important logic entry if multiple entries would match the request.
:ResourceAccess:Tenant: This value specifies whether the tenant is required to be specified when accessing the resource. It has to be either ``Required`` or ``Shared``.
:ResourceAccess:Profiles: This value specifies a list of ``meta.profile`` values to look for in the resource. The resource should have at least one of those values.
:ResourceAccess:ResourceTypes: This value specifies a list of resources this configuration will be applicable to. When left empty, it is assumed to be not constrained.
:ResourceAccess:Filter: This value specifies a filter that resources must fulfill.
Copy link
Member

Choose a reason for hiding this comment

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

This gets ugly to read. I suggest a nested list:

Suggested change
:TenantHeader: The name of the HTTP header to read the tenant from. This is only evaluated if ``AllowTenantFromHeader`` is set to ``true``.
:TenantClaim: The name of the claim in the authorization token to read the tenant from. This is only evaluated if ``AllowTenantFromClaim`` is set to ``true``.
:TenantLabelSystem: The value of the ``meta.security.system`` element to use for the tenant security label. You can only choose this once.
:AllowTenantFromHeader: The tenant may be specified with an HTTP header, with the name specified in ``TenantHeader``.
:AllowTenantFromClaim: The tenant may be specified with a claim in the authorization token, with the name specified in ``TenantClaim``.
:RemoveTenantLabelFromResponse: The label will be removed from the resource by default, but this behaviour can be disabled if this option is set to ``false``.
:ResourceAccess: This structure allows to define set of conditional logic that will be applied to each request.
:ResourceAccess:Priority: This value will be used to choose most important logic entry if multiple entries would match the request.
:ResourceAccess:Tenant: This value specifies whether the tenant is required to be specified when accessing the resource. It has to be either ``Required`` or ``Shared``.
:ResourceAccess:Profiles: This value specifies a list of ``meta.profile`` values to look for in the resource. The resource should have at least one of those values.
:ResourceAccess:ResourceTypes: This value specifies a list of resources this configuration will be applicable to. When left empty, it is assumed to be not constrained.
:ResourceAccess:Filter: This value specifies a filter that resources must fulfill.
- TenantHeader: The name of the HTTP header to read the tenant from. This is only evaluated if ``AllowTenantFromHeader`` is set to ``true``.
- TenantClaim: The name of the claim in the authorization token to read the tenant from. This is only evaluated if ``AllowTenantFromClaim`` is set to ``true``.
- TenantLabelSystem: The value of the ``meta.security.system`` element to use for the tenant security label. You can only choose this once.
- AllowTenantFromHeader: The tenant may be specified with an HTTP header, with the name specified in ``TenantHeader``.
- AllowTenantFromClaim: The tenant may be specified with a claim in the authorization token, with the name specified in ``TenantClaim``.
- RemoveTenantLabelFromResponse: The label will be removed from the resource by default, but this behaviour can be disabled if this option is set to ``false``.
- ResourceAccess: This structure allows to define set of conditional logic that will be applied to each request.
- Priority: This value will be used to choose most important logic entry if multiple entries would match the request.
- Tenant: This value specifies whether the tenant is required to be specified when accessing the resource. It has to be either ``Required`` or ``Shared``.
- Profiles: This value specifies a list of ``meta.profile`` values to look for in the resource. The resource should have at least one of those values.
- ResourceTypes: This value specifies a list of resources this configuration will be applicable to. When left empty, it is assumed to be not constrained.
- Filter: This value specifies a filter that resources must fulfill.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we discussed, since it's no longer strictly under tenancy, I made it into its own page and kept this one as is :)

:ResourceAccess:Priority: This value will be used to choose most important logic entry if multiple entries would match the request.
:ResourceAccess:Tenant: This value specifies whether the tenant is required to be specified when accessing the resource. It has to be either ``Required`` or ``Shared``.
:ResourceAccess:Profiles: This value specifies a list of ``meta.profile`` values to look for in the resource. The resource should have at least one of those values.
:ResourceAccess:ResourceTypes: This value specifies a list of resources this configuration will be applicable to. When left empty, it is assumed to be not constrained.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:ResourceAccess:ResourceTypes: This value specifies a list of resources this configuration will be applicable to. When left empty, it is assumed to be not constrained.
:ResourceAccess:ResourceTypes: This value specifies a list of resource types this configuration will be applicable to. When left empty, it is assumed to be not constrained.
  • 'resources' and 'resource types' are casually confused, but as FHIR pro's we should not do that. A 'resource' is an instance.
  • I assume you update this based on our discussed meaning of an empty array (i.e.: nothing matches).

Copy link
Member

@cknaap cknaap left a comment

Choose a reason for hiding this comment

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

It looks like this comment was not processed yet.
Some of this PR can only be finished after further refinement of VONK-6902

},

:TenantHeader: The name of the HTTP header to read the tenant from. This is only evaluated if ``AllowTenantFromHeader`` is set to ``true``.
:TenantClaim: The name of the claim in the authorization token to read the tenant from. This is only evaluated if ``AllowTenantFromClaim`` is set to ``true``.
:TenantLabelSystem: The value of the ``meta.security.system`` element to use for the tenant security label. You can only choose this once.
:AllowTenantFromHeader: The tenant may be specified with an HTTP header, with the name specified in ``TenantHeader``.
:AllowTenantFromClaim: The tenant may be specified with a claim in the authorization token, with the name specified in ``TenantClaim``.
:RemoveTenantLabelFromResponse: The label will be removed from the resource by default, but this behaviour can be disabled if this option is set to ``false``.
:ResourceAccess:Filter: This value specifies a filter that resources must fulfill.
Copy link
Member

Choose a reason for hiding this comment

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

ResourceAccess is now a new top-level configuration entry, right. So I think it should be removed here altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, missed a line.

:Priority: Will be used to choose most important logic entry if multiple entries would match the request.
:Tenant: Specifies if the tenant is required to be specified when accessing the resource, or should we use internally a ``shared`` tenant. It has to be either ``Required`` or ``Shared``.
:Authorization: Specifies if the authorization is required when accessing the resource. It has to be either ``Required`` or ``None``.
:Filter: Specifies a filter that resources must fulfill.
Copy link
Member

Choose a reason for hiding this comment

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

Reading this I certainly think that we need some additional refinement.

@andrzejskowronski
Copy link
Contributor Author

It looks like this comment was not processed yet.

The first part is handled with a warning above tenancy implications, not sure how well the second part fits being described in multi-tenancy?
Intuitivelly, I'd rather mention it in the doc page for the Operations config, perhaps MT linking to it, and let appsettings.default.json be a guideline for those

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants