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

DisallowUserInstantiation property on Application to prevent manual instantiation of instances #426

Merged
merged 5 commits into from
Jul 4, 2024

Conversation

martinothamar
Copy link
Contributor

@martinothamar martinothamar commented Jun 3, 2024

Description

We need configuration to block "manual instantiation" by users, for applications where service owner
create and prefill instances and then let users fill in the rest.

Changes

  • InstantiationConfig.ManualInstantiationDisabled to disable manual instantiation (not by service owner)
  • Docs improvements on PartyTypesAllowed
  • EditorConfig fixes as it seems all files have LF line endings and not CRLF (my editor was trying to correct line endings all the time)

Lib PR: Altinn/app-lib-dotnet#671

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)
  • All tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

@martinothamar martinothamar force-pushed the feature/prevent-manual-instantiation branch from 70563a1 to 1179a30 Compare June 3, 2024 06:38
@martinothamar
Copy link
Contributor Author

This PR (currently) is just for exploring options, so no need to review specifics of the code just yet. Location, semantics, overlap with existing config etc is the stuff where I don't have enough context to know what's a good idea or not, updating the description

@HauklandJ
Copy link
Contributor

This PR (currently) is just for exploring options, so no need to review specifics of the code just yet. Location, semantics, overlap with existing config etc is the stuff where I don't have enough context to know what's a good idea or not, updating the description

As instantiation is not a party type, but rather the context in which a party type exists, it may be confusing to expand PartyTypesAllowed. Maybe it could be renamed to something that makes sense, but I fail to find an umbrella term thats easy to interpret as encompassing parties, instantiation and copy instantiation (the latter of which I do not fully understand what is represented with).

With this in mind, I vote to keep it as seperate config.

@martinothamar martinothamar force-pushed the feature/prevent-manual-instantiation branch from 1179a30 to 79a731e Compare June 5, 2024 07:20
@martinothamar martinothamar changed the title Configuration in applicationmetadata for disallowing manual instantiation InstantiationConfig property on Application to prevent manual instantiation of instances Jun 5, 2024
@martinothamar
Copy link
Contributor Author

Updated the PR commit and description with what we agreed yesterday

@martinothamar martinothamar requested a review from altinnadmin June 5, 2024 07:53
@martinothamar
Copy link
Contributor Author

@altinnadmin it would be nice if you could review this as well from a domain/naming POV 😄

@martinothamar martinothamar requested review from HauklandJ and ivarne June 5, 2024 07:54
Copy link
Contributor

@HauklandJ HauklandJ left a comment

Choose a reason for hiding this comment

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

lgtm

@altinnadmin
Copy link
Member

  • "Manual instantiation" is not a concept, and I don´t think its wise to introduce it
  • Instantiation is an action in XACML and API in the app, and the app owner currently configures who is allowed to trigger an instantiation. If only the app owner should be allowed to instantiate, it should be configured as an authorization rule.
  • If a user should be allowed to instantiate only when creating a copy, I guess the "clean" way to do that is to introduce a new copy-action to be used in XACML, since these are different concepts that needs different authorization rules?

@martinothamar
Copy link
Contributor Author

This was our reasoning as well initally - we discussed this with @TheTechArch and ended up deciding against extending the XACML setup in this case. Should I setup a meeting to revisit? That seems like a good idea since others have raised the same question as well

@martinothamar
Copy link
Contributor Author

We discussesd this again - XACML based config for this is decided against for practical reasons - changes needed to accomodate this would affect different services such as - authorization, storage, backend/lib. There may also be more dimensions/config to this in future versions

Updated proposal for applicationmetadata:

        /// <summary>
        /// Gets or sets the list of allowed instantiators for this app, and limits it to orgs/service owners.
        /// The "urn:altinn:org" claim of the current identity must match one of the values in this list.
        /// If the list is null, any org/service owner or user can instantiate.
        /// </summary>
        [JsonProperty(PropertyName = "instantiationAllowedBy")]
        public List<string> InstantiationAllowedBy { get; set; }

@martinothamar martinothamar marked this pull request as ready for review July 3, 2024 06:05
@martinothamar
Copy link
Contributor Author

Since XACML already does auth on which orgs can instantiate, I've made the configuration parameter a simple bool again

Copy link

sonarcloud bot commented Jul 4, 2024

@martinothamar martinothamar changed the title InstantiationConfig property on Application to prevent manual instantiation of instances DisallowUserInstantiation property on Application to prevent manual instantiation of instances Jul 4, 2024
@martinothamar martinothamar merged commit a329c97 into main Jul 4, 2024
8 checks passed
@martinothamar martinothamar deleted the feature/prevent-manual-instantiation branch July 4, 2024 08:43
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.

4 participants