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

Enable Validation for all Attachments #2114

Closed
kolotu opened this issue Nov 27, 2019 · 4 comments
Closed

Enable Validation for all Attachments #2114

kolotu opened this issue Nov 27, 2019 · 4 comments
Milestone

Comments

@kolotu
Copy link
Member

kolotu commented Nov 27, 2019

When an attachment is created, it will only be validated, when it has the tag IMPORTED. Validation of attachments should always be enabled.

See ModelRepository.java, attachFile()

@kolotu kolotu changed the title Enable Tag Validation for all Attachments Enable Validation for all Attachments Nov 27, 2019
@aedelmann aedelmann added this to the 1.0-M1 milestone Dec 2, 2019
@ghost
Copy link

ghost commented Dec 9, 2019

Having a look.

@ghost
Copy link

ghost commented Dec 9, 2019

Bit of a clarification on this.

  • The validation checks for size and file extension - the latter is: ext, pdf, doc, zip (see repo.attachment.allowed.extension in application.properties)
  • The validation takes place only when the attachment is not tagged as imported

The salient case when the method is invoked with an "IMPORTED" tag seems to be when importing models with dependencies - however in that case the file passed is the original zip file, so the validation would be successful - see AbstractModelImporter#postProcessImportedModel.

There might also be another case when copying models from somewhere to somewhere else, as all tags are passed (i.e. it might include the "IMPORTED" for all I know) - however, this looks like it's dealing with already imported attachments, so they can hardly have an invalid extension (unless we decide one day that we want to remove something from the list - hopefully never).

Other usages seem even safer.

@aedelmann removing the filtering and PRing this shortly, let me know if you're ok with the reasoning above.

@ghost
Copy link

ghost commented Dec 9, 2019

PR here, in the meantime.

@aedelmann
Copy link
Contributor

@mena-bosch
sounds good to me. In the imported use case, the importer plugin takes care of validating the imported file, so it is ok that this use case is not considered in the attachment validation.

Closing issue.

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

No branches or pull requests

2 participants