Skip to content
This repository has been archived by the owner on Jul 25, 2018. It is now read-only.

Permission Checks In License Service #345

Merged
merged 1 commit into from Mar 15, 2017

Conversation

alexbrdn
Copy link
Contributor

@alexbrdn alexbrdn commented Feb 21, 2017

Added permission checks in license service for creating licenses and related objects where missing.
Deleted executable class LicenseImporter.java, which was designed to import license data from command line and has since been replaced by the functionality of ComponentUploadPortlet

closes #106

This PR introduces incompatible changes in Thrift API.
Increase the minor version after merging

@maxhbr
Copy link
Member

maxhbr commented Feb 21, 2017

Increase the minor version after merging

this should be formulated as tag

Copy link
Member

@maxhbr maxhbr left a comment

Choose a reason for hiding this comment

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

You remove the file LicenseImporter.java without a replacement and without mentioning it in the commit message or PR description.

Is it just unused?

Other than that the code looks good.

@alexbrdn
Copy link
Contributor Author

  1. Agreed
  2. I asked about LicenseImporter.java in the issue, got no reply, and made a decision on my own. I probably should've mentioned it here, too.

@alexbrdn
Copy link
Contributor Author

@MaximilianHuber, would you give an "Approved" review, please?

@mcjaeger
Copy link
Contributor

ehem, suggestions how to test this are welcome - like I shall write a thrift client? I am testing for "everything behaves normal" then ... / looking at the integration test suite.

@alexbrdn
Copy link
Contributor Author

An important thing to test is that importing license data through ComponentUploadPortlet still works. Given that the portlet is only available to admins, it's hard to test the permission checks without writing a thrift client or temporarily changing page permissions to try license import with a normal user.

@mcjaeger
Copy link
Contributor

mcjaeger commented Mar 9, 2017

is it that assigned white list selection for a todo is not imported? (which could be fine, just mentioning it if this was intended)

Import from export of another branch (Export Spreadsheet at Clearing Status) seems to work fine. The user was with role ADMIN but was imported (not the setup admin).

Please do not merge as I would like to have #352 before version jump.

Copy link
Contributor

@mcjaeger mcjaeger left a comment

Choose a reason for hiding this comment

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

tested with license import export in various combinations

@mcjaeger mcjaeger merged commit 4bc4313 into master Mar 15, 2017
@alexbrdn alexbrdn deleted the fix/checksForLicensePermissions#106 branch May 11, 2017 11:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check create operations in lib-datahandler
3 participants