subject:"\[GitHub\] cloudstack pull request\: CLOUDSTACK\-8562\: Dynamic Role\-Based API C..."

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220389940
  
@swill the problem was environment dependent where an unclean environment 
was used to build/run management server, it is recommended to git clean -fdx 
one's local repository (or at least run a mvn clean -P developer,systemvm 
-Dnoredist -Dsimulator) to get rid of old files.

The case has been documented on the FS that for `dynamic roles feature to 
work the setting value should be true and there should be no 
commands.properties file readable on the classpath`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-19 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220385286
  
Thanks guys for getting that sorted out.  Is the problems that @anshul1886 
ran into something that others could potentially run into?  Would it be useful 
to document the problem and the solution so others can benefit from it if they 
run into it?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user anshul1886 commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220243535
  
@rhtyd It looks fine now. It was present in some tomcat folder.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220241843
  
@swill I had a GTM with Anshul to figure out the issue, his environment is 
not clean and I've asked him to setup a new environment (effectively a new dev 
box). I've shared notes with him that if `commands.properties` file is present 
anywhere on the classpath with the dynamic feature enabled would cause issues 
as requirement for dynamic roles feature to work is that the 
commands.properties file does not exist on the classpath.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-19 Thread abhinandanprateek

Github user abhinandanprateek commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220239213
  
@rhtyd @anshul1886 This week I rebased the ospf changes with current master 
and did not have any issues. OSPF adds 2 new APIs and they are using the new 
scheme of things, looked good to Marvin and UI.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220238585
  
@anshul1886 if you need help join this GTM now: 
https://app.gotomeeting.com/?meetingId=981540549 (or use the meeting id for a 
desktop client). I've personally tested master and I think the issues you're 
presenting are your specific env related.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user anshul1886 commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220238146
  
dev machine which I am using for last 2-3 years


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220237753
  
@anshul1886 is this environment installed using rpms/deb or it's a 
developer machine with Ubuntu 12.04? Would you prefer a GTM to get to the 
bottom of the issue, instead of back and forth on PR/ML?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user anshul1886 commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220237502
  
@rhtyd Default value for dynamic.apichecker.enabled is true. role_id for 
default account is 1.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user anshul1886 commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220236230
  
https://cloud.githubusercontent.com/assets/1712815/15384187/2e8d4aa2-1db6-11e6-9358-659ff6f6df3f.png;>
https://cloud.githubusercontent.com/assets/1712815/15384191/2eeeb7ec-1db6-11e6-8bc4-d68a8c10421d.png;>
https://cloud.githubusercontent.com/assets/1712815/15384189/2ee75aec-1db6-11e6-9c6a-b1c91f9bf6e9.png;>
https://cloud.githubusercontent.com/assets/1712815/15384190/2eeb7ae6-1db6-11e6-868d-37c15e214d16.png;>

@rhtyd Same results on master. I have not done anything in setup.

My dev box is ubuntu 12.04


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220230192
  
@anshul1886 I'm yet to see chrome dev-tool screenshots, actual errors 
reported in the UI and see answers for questions I asked; again -- the role_id 
in accounts table, your environment info (win/linux/osx etc).

Now, I cannot guarantee that this will work in your feature branch as I 
don't know what changes your branch has; I've only tested master with a fresh 
environment on a linux machine and it is working for me both using maven and 
using rpms http://packages.shapeblue.com/cloudstack/custom/master-rbac:

![screenshot from 2016-05-19 
10-43-47](https://cloud.githubusercontent.com/assets/95203/15383250/4f5a5d8a-1daf-11e6-8857-762433bb6e45.png)

Created a user with user role:

![screenshot from 2016-05-19 
10-45-19](https://cloud.githubusercontent.com/assets/95203/15383260/5feb11e4-1daf-11e6-91fd-0bccf16fed91.png)

Was able to log in:

![screenshot from 2016-05-19 
10-45-39](https://cloud.githubusercontent.com/assets/95203/15383270/6f99c400-1daf-11e6-8623-6335b01f6cef.png)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user anshul1886 commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220227876
  
@rhtyd Its a clean setup. It is not even complete because of one other bug 
introduced by PR #816.

Summary of the steps:

1. mvn -P systemvm clean install  -DskipTests
2. mvn -P developer -pl developer -Ddeploydb
3. mvn -pl :cloud-client-ui jetty:run
4. Open UI start seeing those errors.

For commit history on my setup see PR #351.

And that commits code is not even coming into picture.

To go ahead I have disabled dynamic.apichecker.enabled and copied the 
commands.properties file. After that I was blocked by the bug introduced by 
#816. So I have not completed my setup. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-18 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220094101
  
@rhtyd thank you sir.  I am trying to get everything ready to freeze.  Damn 
this is a lot of work.  :(


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220093571
  
@swill yes sir. I'll personally make sure to perform another test round 
tomorrow both with UI, marvin and APIs/cloudmonkey.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-18 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220090972
  
I agree.  But I have not tested the UI at all.  Has your team been testing 
through the UI as well as through the API?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220090191
  
@swill Travis and your CI are passing with this feature during which a data 
center is deployed and all sorts of APIs are called so it's likely an env issue 
specific for @anshul1886. We've put a lot of testing effort on this including 
various upgrade scenarios, and have seen huge amount of effort on code review 
so it is not needed to revert the feature; though if there is a valid bug it 
ought to be fixed and that's what freeze is about -- fixing bugs.

Here what I'm speculating where it is failing:
Developer sets up CloudStack with a db schema prior to when RBAC was 
merged. Then, the developer rebuilds CloudStack but does not redeploy a 
fresh/clean db, and commands.properties.in is removed so his static-checker 
won't work. The root admin with role_id=1 is by default allowed all APIs in the 
dynamic checker so the feature ensures that root-admin is never locked out of 
the system. The developers ought to cleanly rebuild CloudStack and re-deploy a 
fresh db after a rebase in their feature branch or on master.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-18 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220037863
  
If we have to revert this, it will be a mess.  Lets try to get to the 
bottom of this ASAP as the freeze is basically here...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-220005496
  
@anshul1886 okay, let's debug;
- After you mvn re-built cleanly, did you run mvn deploydb?
- If yes, what is the output of: select * from configuration where 
name='dynamic.apichecker.enabled'\G;
For a fresh/clean install, the `value` should be 'true'.
- Do you see any rules in cloud.role_permissions table for admin user 
(role_id=1). Default root admins (or account created using the default root 
admin role) are allowed all APIs. Are you using CloudStack as a non-root admin?
- Check that there is no `commands.properties` file in the classpath for 
dynamic-roles to work. None of the apichecker will work if you've a 
commands.properties file in classpath but have dynamic.apichecker.enabled set 
to true. Try git clean -fdx to get rid of non-tracked files, or do a `find` and 
remove commands.properties file.
- Check role_id column in cloud.accounts table, it should be `1` for root 
admin accounts. If it's NULL, something went wrong with the upgrade path.
- Check errors on why the APIs are failing are you seeing, open Chrome 
dev-tools etc and share the results. Also check api/mgmt server logs.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user anshul1886 commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-219990167
  
@rhtyd I have rebuilt my branch which was rebased yesterday. I am creating 
new setup. There is no new API introduced in my branch. I am talking about 
existing APIs which are failing.

After putting commands.properties I am not facing issues. But as you 
pointed that is not what is intended with this PR.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-219988321
  
@anshul1886 with dynamic-roles feature, we no longer want to use or promote 
use of commands.properties file which is why this has been removed in master. 
We no longer want to add changes/new APIs to this file as well though existing 
installations will continue to use the static-checker until they decide to 
upgrade.

You need to use the annotations as described on the wiki (this has been 
shared recently on dev ML as well):

https://cwiki.apache.org/confluence/display/CLOUDSTACK/Annotations+use+in+the+API

https://cwiki.apache.org/confluence/display/CLOUDSTACK/CloudStack+API+Development

I think you've not rebuilt your branch after a rebase, you need to perform 
a clean rebuild.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user anshul1886 commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-219987354
  
@rhtyd Is commands.properties exist in your setup or not? If it exists then 
things are working fine.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-219986204
  
@anshul1886 I'm unable to reproduce the shared issues nor they were caught 
by CI/Jenkins/Travis runs. The role specific rules in role_permissions table 
have been copied from the last commands.properties.in file. If you've added new 
APIs they need to use authorized annotations.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user anshul1886 commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-219980293
  
@rhtyd There are many APIs. I have not prepared any list. But creating 
setup from scratch should reproduce it. Some operations in which it can be 
observed

1. ListAffinity groups
2. Create secondary storage




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-219953009
  
@anshul1886 can you share the details, which APIs. Also, are you facing 
this on master or on your personal/feature branch?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user anshul1886 commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-219944062
  
@swill @rhtyd Some APIs are giving permission exceptions. They can be seen 
by just logging into UI. I have added the commands.properties to overcome these 
exceptions in my setup.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-11 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-218370503
  
Thanks you @swill finally :smile: 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-11 Thread asfgit

Github user asfgit closed the pull request at:

https://github.com/apache/cloudstack/pull/1489


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-11 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-218369488
  
@swill all green now 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-10 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-218359137
  
Thank you.  I am really trying to avoid that, but lets see what happens.  :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-10 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-218358868
  
@swill done, though if it fails again we may ignore that as long as travis 
is green


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-10 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-218358102
  
thank you sir.  can you repush right away again.  jenkins already failed 
and I like to make sure everything is green before i merge stuff.  thx...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-10 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-218357625
  
@swill I've fixed the issues, here are the changes:

[changes.diff.txt](https://github.com/apache/cloudstack/files/258551/changes.diff.txt)

The conflicts were in travis, debian/control and the schema files only. If 
Travis is green, we can merge this without need to re-run the CI.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-10 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-218356331
  
Sorry to do this to you @rhtyd, but I just merged like 10 PRs and we now 
have merge conflicts.  Can you resolve the merge conflicts and re-push.  When 
you resolve the merge conflicts, would you mind not squashing the fixes to the 
merge conflicts so we can more easily review the changes required to resolve 
the merge conflicts.  This will help me know if I need to re-run CI before I 
merge this.  Thanks...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-09 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-217855833
  
We have the required code reviews and CI results.  I will add this to merge 
queue.  Thx...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-08 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-217774436
  
@swill thanks, let's merge this


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-217606327
  


### CI RESULTS

```
Tests Run: 85
  Skipped: 0
   Failed: 2
   Errors: 0
 Duration: 6h 11m 18s
```

**Summary of the problem(s):**
```
FAIL: Test redundant router internals
--
Traceback (most recent call last):
  File 
"/data/git/cs2/cloudstack/test/integration/smoke/test_routers_network_ops.py", 
line 290, in test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true
"Attempt to retrieve google.com index page should be successful!"
AssertionError: Attempt to retrieve google.com index page should be 
successful!
--
Additional details in: /tmp/MarvinLogs/test_network_MN3MB5/results.txt
```

```
FAIL: Test redundant router internals
--
Traceback (most recent call last):
  File 
"/data/git/cs2/cloudstack/test/integration/smoke/test_routers_network_ops.py", 
line 483, in test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false
"Attempt to retrieve google.com index page should be successful once 
rule is added!"
AssertionError: Attempt to retrieve google.com index page should be 
successful once rule is added!
--
Additional details in: /tmp/MarvinLogs/test_network_MN3MB5/results.txt
```



**Associated Uploads**

**`/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_34_18_ZVNY8R:`**
* 
[dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_34_18_ZVNY8R/dc_entries.obj)
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_34_18_ZVNY8R/failed_plus_exceptions.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/DeployDataCenter__May_06_2016_21_34_18_ZVNY8R/runinfo.txt)

**`/tmp/MarvinLogs/test_network_MN3MB5:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_network_MN3MB5/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_network_MN3MB5/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_network_MN3MB5/runinfo.txt)

**`/tmp/MarvinLogs/test_vpc_routers_DZWFXD:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_vpc_routers_DZWFXD/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_vpc_routers_DZWFXD/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_vpc_routers_DZWFXD/runinfo.txt)


Uploads will be available until `2016-07-07 02:00:00 +0200 CEST`

*Comment created by [`upr comment`](https://github.com/cloudops/upr).*



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-06 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-217500553
  
Thanks @swill as soon as we can merge this one, I can rebase and use some 
of the annotations work and ListUtils from this PR into out-of-band management 
PR /cc @jburwell 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-06 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-217456452
  
I will get this CI'ed.  I have 4 new CI boxes in play now, but I am trying 
to figure out how to stabilize master, so I will run this as soon as I get 
master sorted out.  Sorry for the delay...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-06 Thread jburwell

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-217450750
  
@rhtyd @swill LGTM based on code review


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-06 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-217413190
  
@swill this PR is ready for CI test run and merge, thanks


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-05 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-217343619
  
@swill rebased against latest master and pushed
@borisstoyanov can you help test this one last time, I think all major 
issues including ordering related issues have been fixed, thanks


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-05 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-217264750
  
@rhtyd we have merge conflicts.  can you fix?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-04 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-216851115
  
We performed end-to-end testing of this PR by building packages today [1] 
and found an issue with maven (not related to this PR) where users get sent to 
error.jsp due to a missing jstl.jar. The 4th commit on this PR fixed that issue.

[1] https://packages.shapeblue.com/cloudstack/custom/dynamicroles/


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-03 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-216749200
  
@jburwell The API is transactional and you're right if more than one admin 
decide to change the order, client side final order checking will be needed. 
This is also true for VPC ACL rules too, where the same sort of order is used 
(it also supports draggable allow/deny items in the UI).

If I implement the API to apply change the order at once (instead of 
multiple single operations) and there are multiple root admins changing the 
rules, the order saved by the last user will be in effect; and client side 
tracking/verification will still be required.

 Realistically, only root admins will be using this feature and it's 
unlikely to hit such a case often.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-03 Thread jburwell

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-216734107
  
@rhtyd I am trying to understand how reordering should work.   User creates 
a role with rules ordered as follows:

1. Rule A
1. Rule B
1. Rule C
1. Rule D
1. Rule E
1. Rule F

When the user creates this role, the API guarantees that these six rules 
and their order will be persisted atomically.  Later, the user wants to reorder 
the rules as follows:

1. Rule A
1. Rule D
1. Rule B
1. Rule C
1. Rule E
1. Rule F

As I understand the API, the following discrete API calls are required to 
affect this change:

1. ```updateRolePermission(roleId: "Rule D", parentUUID: "Rule A")```
1. ```updateRolePermission(roleId: "Rule B", parentUUID: "Rule D")```
1. ```updateRolePermission(roleId: "Rule C", parentUUID: "Rule B")```

Assuming my understanding regarding the API usage is correct, I have the 
following concerns about this approach:

1. **Operation Interleaving**: Since each API call can only provide a 
guarantee the persistence of a single list mutation, it is possible for two 
users to change the order of rules for permission simultaneously.  The result 
of this operation interleaving would leave the order of the rules in an 
indeterminate order.  
1. **Client Complexity**: It places a burden on clients to track the 
mutations to the list of rules rather than simply tracking the current order of 
the list.  This additional complexity could lead to more error prone client 
code.

As a user my expectation would be to reorder all rules associated with a 
permission in one atomic operation rather than a series of atomic mutation 
operations.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-03 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-216603249
  
@swill I'm fine with all changes, I've ran a final set of tests as well. 
@jburwell please share any outstanding issue that should be fixed, 
@borisstoyanov and I are LGTM on this


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-03 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-216559351
  
Great, thanks for the additional details @borisstoyanov.  ð   Once Rohit 
and John are in agreement on the final details, I think we are ready to merge 
this one.  Thanks everyone...

Sorry for accidentally introducing you into this @borisroman.  :)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-03 Thread borisstoyanov

Github user borisstoyanov commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-216550819
  
Thanks @swill as Rohit mentioned we did migration testing of 4.8.1 to 4.9 
without enabling the feature, to confirm users are able to continue using CS 
without dynamic-checker. Then we executed the migration script and verified the 
commands.properties are no longer available and all the rules are migrated in 
the DB. This was performed with regular users as well as LDAP and SAML users. 
Fresh installation of 4.9 was also tested, by default dynamic-checker is 
enabled. Also regression testing was performed on creating and using custom 
rules. My LGTM is up in the thread. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-03 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-216488843
  
@jburwell I did not get the question, is it for oobm? dynamic-checker has 
no update counter in any of the api or schema. If this was for oobm, the update 
counter is used only for updating the power state and there is no API exposing 
or requiring this field. The power action 'status' causes update of the power 
state if it's different (or if ownership has changed only).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-03 Thread borisroman

Github user borisroman commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-216487485
  
@swill I think you mentioned the wrong Boris :)



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-03 Thread jburwell

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-216486309
  
@rhtyd for optimistic locking, is the update counter returned on the get 
APIs and required on the update APIs?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-02 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-216441837
  
Thanks @rhtyd, this makes me feel a little more comfortable.  @borisroman 
if you can give us a bit of an outline of what you have tested, I think it will 
help everyone in this thread with the confidence in this PR.  I am pretty 
confident, but at the same time, we can't be too careful when introducing 
functionality that can potentially block users.  I really appreciate all the 
work you guys have been doing on both the development front and with testing 
and validation. ð 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-02 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-216435191
  
@swill yes indeed we've performed upgrade tests with 4.8.1 to 
4.9.0-SNAPSHOT (not using the feature by default) and then using 
migrate-dyanamicroles.py script; the other test we performed was fresh 
installation of 4.9.0-SNAPSHOT packages @borisstoyanov can update in more detail


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-02 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-216231216
  
I will make sure this gets into 4.9 because I think it is really needed.  
We do need one more LGTM on the code.  Also, since this PR does have some 
implications regarding access to the system we should be pretty cautious.  I am 
pretty confident with a new install because that is what my CI has tested.  I 
think it would be prudent for us to do more testing of some upgrade scenarios 
to make sure the code behaves as expected.  I don't think this code should come 
into play with upgrades because the old properties files should be ready, but I 
think we need to validate the code behaves as expected.  Has anyone tested this 
yet?

I think we should be testing:
- Upgrading to 4.9 and NOT using this feature.
- Upgrading to 4.9 and running the `migrate-dynamicroles.py` script to 
import the existing roles.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-05-02 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-216228334
  
The PR is ready for merge, any testing/feedback welcome /cc @swill 

tag:mergeready


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-29 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-215889804
  
@swill fixed the issue of orderable permissions, PR is ready for 
testing/merge


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-28 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-215501683
  
@swill existing users won't be automatically migrated to use this feature, 
see the admin-doc PR that documents the upgrade/migrate procedure; though new 
installations will use dynamic-checker by default.

Will, while the PR is ready to be merged -- I think I can improve the UI/UX 
around how rules are added and evaluated (for example, we may drag/drop rules, 
make it more intuitive etc) and I would also like to improve the docs on using 
the dynamic-checker wrt rules (API name and wildcard rules)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-28 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-215479369
  
In reading this, is it the case that if someone upgrades from a previous 
version to 4.9 (with this code included) they will not be able to take 
advantage of this functionality?  Maybe I did not understand those details 
correctly.  This is just a clarification so I know what to expect (I have 
customers who are interested in this functionality).

@koushik-das Regarding: "Regarding writing a plugin, I meant that if 
someone really needed a functionality like this, they could have easily 
developed it outside of Cloudstack as well (like some kind of portal calling 
Cloudstack APIs)."  Given that I work a company who has done exactly that, I 
can assure you it is not "easy" and takes a huge development effort to recreate 
all the functionality provided by the ACS UI.  This is not really a viable 
option for people looking for this feature.

@koushik-das I also don't understand this "Any blocker (including security 
bugs) in this will make the 4.9 release unusable for new installs as this the 
only authorisation mechanism for entering into the system.".  Can you explain 
what you mean?  How does this feature and a 'blocker bug' render 4.9 unusable?  
I am not quite following this.  Maybe someone else can also chime in to help 
explain his concerns.

Thx...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-28 Thread koushik-das

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-215389142
  
+1 on the feature. Based on testing done on the simulator with root admin 
and normal user for a few APIs
+0 on the immediate removal of command.properties/static checker for new 
installs and making dynamic checker as the only option for authorization.
Reason: Any blocker (including security bugs) in this will make the 4.9 
release unusable for new installs as this the only authorisation mechanism for 
entering into the system. It can still be used for pilot/test purposes though.

@rhtyd Who is saying to stop innovating and improving? I am only talking 
about stability and choice.
@jburwell Hopefully the above reason explains how this is different from 
other features (which impacts only a certain part of the product). Regarding 
writing a plugin, I meant that if someone really needed a functionality like 
this, they could have easily developed it outside of Cloudstack as well (like 
some kind of portal calling Cloudstack APIs).



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-28 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-215352448
  
@DaanHoogland @wido @mlsorensen @jburwell @agneya2001 @koushik-das 
@terbolous @resmo @K0zka  review and LGTM please?
 
@swill I think we've resolved all outstanding concerns, and if there are 
still any concern that should be shared, summary;
- Existing users are not migrated automatically, they continue to use 
static-checker, @koushik-das raised concern if dynamic-checker is forced upon 
users -- it is not
- Commands.properties file is only removed from codebase, after an upgrade 
this file won't be removed by the deb/rpm packages, @koushik-das raised concern 
on not removing it -- it is clarified that it's not for users with existing 
deployments
- Valid points raised by @mlsorensen on why dynamic checker is the way 
forward and static-checker has several pain points for real world users
- @koushik-das raised concerns about code quality and testing, my colleague 
@borisstoyanov  and I have spent good time to test the feature against ldap, 
saml and native cloudstack authentication; along with upgrade and regression 
tests around the feature. The integration test runs with Travis as well, 
providing nearly 100% coverage


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-27 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-215291402
  
The CI results show this PR to be in pretty good shape.  We have one LGTM 
and lots of commentary.  Can we get a summary of any concerns and get our code 
reviews in?  Thanks...


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-27 Thread swill

Github user swill commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-215290621
  


### CI RESULTS

```
Tests Run: 100
  Skipped: 1
   Failed: 0
   Errors: 0
```



**Associated Uploads**

**`/tmp/MarvinLogs/DeployDataCenter__Apr_27_2016_22_04_24_8LX15C:`**
* 
[dc_entries.obj](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/DeployDataCenter__Apr_27_2016_22_04_24_8LX15C/dc_entries.obj)
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/DeployDataCenter__Apr_27_2016_22_04_24_8LX15C/failed_plus_exceptions.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/DeployDataCenter__Apr_27_2016_22_04_24_8LX15C/runinfo.txt)

**`/tmp/MarvinLogs/test_network_8F05KM:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_network_8F05KM/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_network_8F05KM/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_network_8F05KM/runinfo.txt)

**`/tmp/MarvinLogs/test_staticroles_E7WUSK:`**
* 
[failed_plus_exceptions.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_staticroles_E7WUSK/failed_plus_exceptions.txt)
* 
[results.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_staticroles_E7WUSK/results.txt)
* 
[runinfo.txt](https://objects-east.cloud.ca/v1/e465abe2f9ae4478b9fff416eab61bd9/PR1489/tmp/MarvinLogs/test_staticroles_E7WUSK/runinfo.txt)


Uploads will be available until `2016-06-28 02:00:00 +0200 CEST`

*Comment created by [`upr comment`](https://github.com/cloudops/upr).*



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-27 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-215197356
  
Rebased against latest master, please consider this for merge

Performed final rounds of testing:
- Build/maven, unit and integration tests
- Upgrade tests from old deployed to new using packages
- Major regression tests against before/after upgrade LDAP, SAML2 and 
native cloudstack accounts, listApis regression testing, along with API/CLI 
testing


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-27 Thread jburwell

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-215171740
  
@koushik-das I don't understand how adoption of this feature differs from 
any other feature.  A user deploys the release into a test/pilot environment 
and determine whether or not it they wish to migrate to it.  To be clear, 
existing users are **not** automatically migrated.

Per the FS, this feature alows the definition of custom roles as you 
describe.  For backwards compatibility, all roles must be mapped to one of the 
four types.  

I don't understand your point about someone writing a plugin to allow 
custom roles.  This PR is exactly the plugin you describe.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-27 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-215165382
  
@mlsorensen 
- presently, setting bitmask to 0 in commands.properties does not disable 
the API but that no roles would have that API; though if an API through its 
`authroized` annotation enables itself for a roletype that will be considered
- this feature implements a DENY rule that is checked before annotations 
are checked to provide a mechanism to disable an API (or group of APIs if a 
wildcard rule is used) that could have been enabled by the annotation (for 
example)

@koushik-das 
- Existing users are not migrated automatically, they continue to use 
static-checker
- Commands.properties file is only removed from codebase, after an upgrade 
this file won't be removed by the deb/rpm packages
- New users and installations are no longer encouraged to use the old 
static-checker, therefore with the aim of deprecating the static-checker over 
time dynamic-checker is enabled by default
- Any change can introduce side-effects and bugs but that does not mean we 
should stop innovating or stop improving cloudstack; such an attitude would be 
harmful for any project
- I really appreciate your code review, improvement suggestions and testing 
feedback instead, on this PR. Thanks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-27 Thread mlsorensen

Github user mlsorensen commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-215147096

@koushik-das -- I'd say yes, that testing is good enough. If a test can
determine that the default roles allow the expected access to the APIs then
we're good.

There was some discussion of commands.properties deprecation in 2013. In
particular, plugin developers had issues with the way it works. People should
really have been using the annotations by now, but I believe it was not well
communicated and the fact that commands.properties is still around has confused
new developers. From the original discussion, the only reason it was left in
place was in case users wanted to disable APIs entirely by setting the bitmask
of an API to 0. Do we have that functionality covered now?

http://mail-archives.apache.org/mod_mbox/cloudstack-dev/201310.mbox/%3c2b439486-9c4b-41b7-a698-b7ade234b...@netapp.com%3E

https://github.com/apache/cloudstack/commit/9f7b4884a7a0bf0b15d94777cff449fde4664af2

Note also that existing installations are not being affected, they have to
choose to use the new mechanism. In theory, I'm fine with some sort of staged
transition where the dynamic checking is opt-in for a release, but I worry that
it will perpetuate the issue we've seen since 2013, and puts a burden on the
community to go back and clean up at some later date. For these reasons, I
think if it's functionally identical on new installs and doesn't affect
existing installs use of commands.properties, we should just cut over.

---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-27 Thread koushik-das

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-215008749
  
>>That is to say that the API permissions on a fresh install before and 
after this is merged should behave the same out of the box. If that's the case 
then I don't think users will feel forced into anything or even notice that 
something was changed, and if someone really does care, it sounds simple enough 
to enable commands.properties.

@mlsorensen @jburwell Unless users try it out how will they verify that the 
behaviour is same out of box before and after. I know testing have been done 
but is it good enough to say that things are consistent before and after. Note 
that there is data migration happening from commands.properties to DB. I know 
that commands.properties has it limitations but that doesn't mean it needs to 
be removed immediately. Let the new feature be there for atleast a couple of 
releases so that it can be tried out and   stabilized before deprecating the 
old one. If the concern is about the file getting changed then that can be 
easily prevented (same is done for the old db schema files as well). All I am 
saying is if the file needs to be removed do it after a few releases by 
following the proper deprecation process.

>>Second, command.properties does not permit the definition of new roles. 
Limiting users to 4 roles in a modern cloud environment is a barrier to 
CloudStack adoption.

@jburwell Correct me if I am wrong but based on what I have seen in the 
code even after this feature there will still be 4 roles. I think what this 
feature allows is creating some grouping of permissions and assigning them a 
name. I can create a role with name "Operator" having say API1 and API2, 
someone else can create a role with same name but with API3 and API4. The same 
can be implemented as an independent plugin outside of cloudstack as well. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-26 Thread jburwell

Github user jburwell commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-214960750
  
@mlsorensen +1 -- well said.  command.properties presents a number of 
significant problems.  First, in a clustered environment, it is possible (and 
quite easy) for there to be inconsistent security policies across the cluster.  
Second, command.properties does not permit the definition of new roles.  
Limiting users to 4 roles in a modern cloud environment is a barrier to 
CloudStack adoption.

As Marcus has stated, we can't move forward if we keep trying to maintain 
outdated mechanisms.  Existing users can continue to use command.properties and 
migrate if/when they desire.  New users gain the benefits of a more reliable 
and flexible authorization implementation.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-26 Thread mlsorensen

Github user mlsorensen commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-214843061
  
It seems complicated to try to actively promote a choice of multiple ways 
to check for API authorization (dynamic vs static). I am for deprecating 
commands.properties, and honestly I wish it would have been removed from the 
source tree altogether long ago instead of informally deprecated, because 
people have been adding to it rather than using annotations. I've spoken to 
committers even recently who didn't realize that you didn't have to use 
commands.properties. The annotation and dymanic methods are so much cleaner, 
particularly for plugin developers as it's often the only piece of the source 
tree that they have to change outside of their own plugin directory.

I don't really think the majority of users will care about the 
implementation, so long as functionally the result is the same. That is to say 
that the API permissions on a fresh install before and after this is merged 
should behave the same out of the box. If that's the case then I don't think 
users will feel forced into anything or even notice that something was changed, 
and if someone really does care, it sounds simple enough to enable 
commands.properties. If someone is used to fiddling with this file, they won't 
have difficulty creating it and toggling a global setting manually. Speaking 
from having the experience of trying to manage a custom commands.properties, 
the work involved here to choose static is minor compared to simply maintaining 
a custom commands.properties, making sure an upgrade doesn't overwrite your 
change, and merging in new changes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-26 Thread koushik-das

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-214728007
  
@rhtyd I think you still didn't get my point. Read the comment again. I 
would like to see the option for using command.properties available for both 
new and old installations until the user explicitly wants to migrate. Don't 
remove the file as you have done in the PR.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-26 Thread borisstoyanov

Github user borisstoyanov commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-214725108
  
LGTM. :+1:


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-26 Thread borisstoyanov

Github user borisstoyanov commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-214724962
  
LGTM. :+1:


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-26 Thread borisstoyanov

Github user borisstoyanov commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-214724784
  
LGTM. :+1:


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-26 Thread rhtyd

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-214723830
  
> @rhtyd The current way you have implemented it using global config is 
confusing. Especially when you say that simply disabling the flag won't fall 
back to the static checker automatically and additional stuff needs to be done 
to make it work.

If you read the code, the global setting is restricted, read-only that 
admin are not allowed to change from the UI or using the API. The intention is 
to not expose the logistics, and let users know that this is a one way process, 
discourage them from moving back to static-checker while hide the details from 
them. The documentation cover migration procedure among other details.

From your comments, I think you are beginning to understand what you would 
like to have already exists. For a better understanding, it would help if you 
would actually read the code in the PR and the detailed FS.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-26 Thread koushik-das

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-214707925
  
>>Consider this an admin creates a read-only admin role and accounts with 
that (such users can only do list* calls), now if they go back to 
static-checker all such users now become default root admin (based on account 
type, translation) and can call all APIs defined in commands.properties -- this 
is a significant security risk. Therefore, I personally don't want to put users 
at risk and just discourage them using the static checker.

@rhtyd This is what I would ideally like to see. If the user doesn't want 
the dynamic roles for whatever reason he shouldn't be forced to use it. There 
should be a one time choice given to users if they want to use dynamic roles. 
If user decides to go ahead with it, do the data migration and there shouldn't 
be an option to come back to static checker (for the reasons you have rightly 
mentioned). But if user wants to continue with static checker, it should 
continue to work on the basis of commands.properties.

The current way you have implemented it using global config is confusing. 
Especially when you say that simply disabling the flag won't fall back to the 
static checker automatically and additional stuff needs to be done to make it 
work.

Let me know what you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1489#discussion_r60944411
  
--- Diff: test/integration/smoke/test_dynamicroles.py ---
@@ -0,0 +1,478 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from marvin.cloudstackAPI import *
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.cloudstackException import CloudstackAPIException
+from marvin.lib.base import Account, Role, RolePermission
+from marvin.lib.utils import cleanup_resources
+from nose.plugins.attrib import attr
+
+import random
+import re
+
+
+class TestData(object):
+"""Test data object that is required to create resources
+"""
+def __init__(self):
+self.testdata = {
+"account": {
+"email": "mtu@test.cloud",
+"firstname": "Marvin",
+"lastname": "TestUser",
+"username": "roletest",
+"password": "password",
+},
+"role": {
+"name": "MarvinFake Role ",
+"type": "User",
+"description": "Fake Role created by Marvin test"
+},
+"roleadmin": {
+"name": "MarvinFake Admin Role ",
+"type": "Admin",
+"description": "Fake Admin Role created by Marvin test"
+},
+"roledomainadmin": {
+"name": "MarvinFake DomainAdmin Role ",
+"type": "DomainAdmin",
+"description": "Fake Domain-Admin Role created by Marvin 
test"
+},
+"rolepermission": {
+"roleid": 1,
+"rule": "listVirtualMachines",
+"permission": "allow",
+"description": "Fake role permission created by Marvin 
test"
+},
+"apiConfig": {
+"listApis": "allow",
+"listAccounts": "allow",
+"listClusters": "deny",
+"*VM*": "allow",
+"*Host*": "deny"
+}
+}
+
+
+class TestDynamicRoles(cloudstackTestCase):
+"""Tests dynamic role and role permission management in CloudStack
+"""
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.dbclient = self.testClient.getDbConnection()
+self.testdata = TestData().testdata
+
+feature_enabled = 
self.apiclient.listCapabilities(listCapabilities.listCapabilitiesCmd()).dynamicrolesenabled
+if not feature_enabled:
+self.skipTest("Dynamic Role-Based API checker not enabled, 
skipping test")
+
+self.testdata["role"]["name"] += self.getRandomString()
+self.role = Role.create(
+self.apiclient,
+self.testdata["role"]
+)
+
+self.testdata["rolepermission"]["roleid"] = self.role.id
+self.rolepermission = RolePermission.create(
+self.apiclient,
+self.testdata["rolepermission"]
+)
+
+self.account = Account.create(
+self.apiclient,
+self.testdata["account"],
+roleid=self.role.id
+)
+self.cleanup = [
+self.account,
+self.rolepermission,
+self.role
+]
+
+
+def tearDown(self):
+try:
+   cleanup_resources(self.apiclient, self.cleanup)
+except Exception as e:
+self.debug("Warning! Exception in tearDown: %s" % e)
+
+
+def translateRoleToAccountType(self, role_type):
+if role_type == "User":
+return 0
+elif role_type == "Admin":
+return 1
+elif role_type == "DomainAdmin":
+return 2

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-214430311
  
> @rhtyd My comment regarding the test was more in the context of perf. 
test. In the DB for regular user I saw ~250 permissions got created. So this 
means iterating over all these entries twice (ALLOW and DENY) to find a match 
and then perform access check.

The two checkPermissions calls would not cause significant overhead as they 
are done in memory and cost a maximum (worst case) of O(n) computation (on same 
machine). While making two DB calls (once to get all ALLOW rules and once to 
get all DENY rules) is more expensive as we do two network calls to get the 
data and still hit worst case O(n) computation.

For a dynamic access checking system, this is a trade off and also a 
feature. In case of static checker we've now, rules are loaded at load-time 
only; any change requires restart and rules can be inconsistent across 
server(s).

> There will be a perf. overhead due to this and user should have an option 
to decide whether to use static or dynamic.

Users do have a choice, they can choose to not migrate to this feature. 
Typically, in production organizations would run multiple management servers; 
mgmt servers can be horizontally scaled to meet demanding usage. For example, 
they can can mgmt server on a separate machine (which they generally do) and 
tune their databases to accept up to 50k requests (or qps), optimize innodb 
settings (buffer pool size to 60% of memory etc.) and in db.properties increase 
num. of db connections from 250 to 1000.

> Also if the user finds some issues/bugs later during their testing there 
should be a fallback option.

There is a way to revert back to using static checker as I explained 
earlier, (1) switch off the global settings and (2) put back a 
commands.properties file in class-path usually at /etc/cloudstack/management; 
it's just that we don't want users to do it easily. Consider this an admin 
creates a read-only admin role and accounts with that (such users can only do 
list* calls), now if they go back to static-checker all such users now become 
default root admin (based on account type, translation) and can call all APIs 
defined in commands.properties -- this is a significant security risk. 
Therefore, I personally don't want to put users at risk and just discourage 
them using the static checker.

> Regarding upgrade implications, I went through the docs/FS but some 
things are still confusing. If existing user can continue using 
commands.properties then what happens to the new APIs that gets added.

In case you're not aware, with the current system each time a user upgrade 
they have to edit/add new rules to commands.properties by hand. Upgrading using 
packages does not update commands.properties file; it would create a 
.rpmnew/rpmsave file for example. In case of multiple mgmt server, you have to 
do this on all server(s) and restart all of them. While in case of dynamic 
roles based checker, you don't need to restart mgmt server at all (even during 
migration). I'm not sure why you say the static checker way is flexible, on the 
contrary I think it is not and a pain point of a lot of people.

> If the argument is that the permission can be put in as an annotation in 
code for new APIs then that removes the flexibility of the earlier mechanism 
(there is no way to modify the default in code).

This has existed for so long, but  not popular among API writers. Even 
before this feature, there have been few APIs using the annotation; the static 
checker also uses annotation as a fallback (i.e. not something I've introduced).

There is a way to modify default behavior by adding authorized field in 
@APIParam. See the new APIs implementation for example. I've added a section in 
the FS on how new APIs should be written if they need to be enabled by default 
for a role type; alternatively the release notes should properly document new 
APIs and leave the choice of allowing/denying those APIs to (custom) roles.

> We don't know how people are customising commands.properties and removing 
the flexibility may not be a good idea.

On the contrary, we've giving flexibility to people. It might make sense to 
enable certain features for all role types or a subset of them. We have deny 
rules (with API and wildcards supported) in case they want to override the 
default. Consider this, you wrote an API and enabled for users; the system 
admin can explicitly add allow rules and add a \* deny rule that is to say deny 
all (if not allowed) and the dynamic roles system would not consider default 
rules in annotations at all.

> The question is not about advantage of static checker, but more about 
choice and stability of the new mechanism.

There is both choice and stability. We've tried our best to make this

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1489#discussion_r60938011
  
--- Diff: 
plugins/acl/dynamic-role-based/src/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java
 ---
@@ -0,0 +1,167 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.acl;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.user.Account;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.component.PluggableService;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.log4j.Logger;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+@Local(value = APIChecker.class)
+public class DynamicRoleBasedAPIAccessChecker extends AdapterBase 
implements APIChecker {
+
+protected static final Logger LOGGER = 
Logger.getLogger(DynamicRoleBasedAPIAccessChecker.class);
+
+@Inject
+private AccountService accountService;
+@Inject
+private RoleService roleService;
+
+private List services;
+private Map annotationRoleBasedApisMap = new 
HashMap<>();
+
+protected DynamicRoleBasedAPIAccessChecker() {
+super();
+for (RoleType roleType : RoleType.values()) {
+annotationRoleBasedApisMap.put(roleType, new 
HashSet());
+}
+}
+
+private void denyApiAccess(final String commandName) throws 
PermissionDeniedException {
+throw new PermissionDeniedException("The API does not exist or is 
blacklisted for the account's role. " +
+"The account with is not allowed to request the api: " + 
commandName);
+}
+
+private boolean checkPermission(final List  
permissions, final RolePermission.Permission permissionToCheck, final String 
commandName) {
+if (permissions == null || permissions.isEmpty() || 
Strings.isNullOrEmpty(commandName)) {
+return false;
+}
+for (final RolePermission permission : permissions) {
+if (permission.getPermission() != permissionToCheck) {
+continue;
+}
+try {
+if (permission.getRule().matches(commandName)) {
+return true;
+}
+} catch (InvalidParameterValueException e) {
+LOGGER.warn("Invalid rule permission, please fix id=" + 
permission.getId() + " rule=" + permission.getRule());
+}
+}
+return false;
+}
+
+public boolean isDisabled() {
+return !roleService.isEnabled();
+}
+
+@Override
+public boolean checkAccess(User user, String commandName) throws 
PermissionDeniedException {
+if (isDisabled()) {
+return true;
+}
+Account account = accountService.getAccount(user.getAccountId());
+if (account == null) {
+throw new PermissionDeniedException("The account id=" + 
user.getAccountId() + "for user id=" + user.getId() + "is null");
+}
+
+final Role accountRole = roleService.findRole(account.getRoleId());
+if (accountRole == null || accountRole.getId() < 1L) {
+denyApiAccess(commandName);
+}
+
+// Allow all APIs for root admins
+if (accountRole.getRoleType() == RoleType.Admin && 
accountRole.getId() == RoleType.Admin.getId()) {
+return true;
+}
+
+final List rolePermissions =

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1489#discussion_r60937993
  
--- Diff: 
plugins/acl/dynamic-role-based/src/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java
 ---
@@ -0,0 +1,167 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.acl;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.user.Account;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.component.PluggableService;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.log4j.Logger;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+@Local(value = APIChecker.class)
+public class DynamicRoleBasedAPIAccessChecker extends AdapterBase 
implements APIChecker {
+
+protected static final Logger LOGGER = 
Logger.getLogger(DynamicRoleBasedAPIAccessChecker.class);
+
+@Inject
+private AccountService accountService;
+@Inject
+private RoleService roleService;
+
+private List services;
+private Map annotationRoleBasedApisMap = new 
HashMap<>();
+
+protected DynamicRoleBasedAPIAccessChecker() {
+super();
+for (RoleType roleType : RoleType.values()) {
+annotationRoleBasedApisMap.put(roleType, new 
HashSet());
+}
+}
+
+private void denyApiAccess(final String commandName) throws 
PermissionDeniedException {
+throw new PermissionDeniedException("The API does not exist or is 
blacklisted for the account's role. " +
+"The account with is not allowed to request the api: " + 
commandName);
+}
+
+private boolean checkPermission(final List  
permissions, final RolePermission.Permission permissionToCheck, final String 
commandName) {
+if (permissions == null || permissions.isEmpty() || 
Strings.isNullOrEmpty(commandName)) {
+return false;
+}
+for (final RolePermission permission : permissions) {
+if (permission.getPermission() != permissionToCheck) {
+continue;
+}
+try {
+if (permission.getRule().matches(commandName)) {
+return true;
+}
+} catch (InvalidParameterValueException e) {
+LOGGER.warn("Invalid rule permission, please fix id=" + 
permission.getId() + " rule=" + permission.getRule());
+}
+}
+return false;
+}
+
+public boolean isDisabled() {
+return !roleService.isEnabled();
+}
+
+@Override
+public boolean checkAccess(User user, String commandName) throws 
PermissionDeniedException {
+if (isDisabled()) {
+return true;
+}
+Account account = accountService.getAccount(user.getAccountId());
+if (account == null) {
+throw new PermissionDeniedException("The account id=" + 
user.getAccountId() + "for user id=" + user.getId() + "is null");
+}
+
+final Role accountRole = roleService.findRole(account.getRoleId());
+if (accountRole == null || accountRole.getId() < 1L) {
+denyApiAccess(commandName);
+}
+
+// Allow all APIs for root admins
+if (accountRole.getRoleType() == RoleType.Admin && 
accountRole.getId() == RoleType.Admin.getId()) {
+return true;
+}
+
+final List rolePermissions =

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-25 Thread koushik-das

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-214296336
  
@rhtyd My comment regarding the test was more in the context of perf. test. 
In the DB for regular user I saw ~250 permissions got created. So this means 
iterating over all these entries twice (ALLOW and DENY) to find a match and 
then perform access check. There will be a perf. overhead due to this and user 
should have an option to decide whether to use static or dynamic. Also if the 
user finds some issues/bugs later during their testing there should be a 
fallback option.

Regarding upgrade implications, I went through the docs/FS but some things 
are still confusing. If existing user can continue using commands.properties 
then what happens to the new APIs that gets added. If the argument is that the 
permission can be put in as an annotation in code for new APIs then that 
removes the flexibility of the earlier mechanism (there is no way to modify the 
default in code). We don't know how people are customising commands.properties 
and removing the flexibility may not be a good idea.

The question is not about advantage of static checker, but more about 
choice and stability of the new mechanism.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-214281121
  
@koushik-das can you share what you think are the advantages of static 
role-base api checker?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-214276787
  
@koushik-das Yes, all tests run as a regular user too. See the integration 
test, we're using user api clients (search self.getUserApiClient) to perform 
tests -- i.e. tests are not run all as root admin only. What you're asking is 
already covered, they are also run by Travis.

I'm sorry if the discussion confused you, please re-read FS again but let 
me try to explain below as well;

By default, we don't want to encourage new users to use static checker 
which is why dynamic-checker is enabled for developers/new-users. For this 
reason the commands.properties.in file in codebase has been deprecated. In 
packaging too, we're not including commands.properties file.

For existing deployments, we are *NOT* forcing users to migrate to the 
dynamic roles feature and their existing commands.properties file won't be 
renamed or removed during upgrade. Though, the upgrade path will add 
dynamic-role specific tables/schema and default roles. There is an 
upgrade/migrate script for such users who can migrate in future at their wish, 
the script will read rules from commands.properties file and put them in DB.

Please read the admin docs too if they help you understand the process:
https://github.com/apache/cloudstack-docs-admin/pull/37


Now, once a users is already using dynamic checker (fresh or migrated at a 
later stage), we don't want them to be easily able to migrate back to static 
checker as allowing admin to do that with a global setting switch is a security 
issue (sorry being pessimistic here). Therefore, we do two checks to evaluate 
if dynamic roles is allowed:
- check if the global setting says that dynamic roles is enabled
- check that commands.properties does not exist
The reverse is true for static checker, see the isEnabled()/isDisabled 
method in the checker implementation.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

2016-04-25 Thread koushik-das

Github user koushik-das commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-214269005
  
@rhtyd Have you ran the tests using a regular user? As per dynamic checker 
code, for root admin all checks are bypassed. I think a good comparison will be 
to run the tests on master with and without dynamic role checker using a 
regular user so that the DB code path gets exercised.

So are you saying static role checker will no longer be used and the 
migration of data from commands.properties to DB is forced. Rather than 
completely removing commands.properties, it will be good to keep both - static 
checker will only use commands.properties and dynamic checker only goes to DB. 
The choice should be left with users when they want to migrate.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on the pull request:

https://github.com/apache/cloudstack/pull/1489#issuecomment-214240215
  
I've fixed all outstanding issues, please comment if you see any 
outstanding issue.
LGTMs welcome, thanks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1489#discussion_r60882026
  
--- Diff: 
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java
 ---
@@ -119,6 +131,9 @@ private Long getDomainId() {
 
 @Override
 public void execute() throws ServerApiException {
+if (getAccountType() == null && getRoleId() == null) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Both 
account type and role ID are not provided");
--- End diff --

@jburwell before this changes `accountType` was required, now it's made 
optional with introduction of roleId, this check is to ensure at least one of 
the two are provided.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1489#discussion_r60881906
  
--- Diff: 
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java
 ---
@@ -70,10 +72,12 @@
 
 @Parameter(name = ApiConstants.ACCOUNT_TYPE,
type = CommandType.SHORT,
-   required = true,
--- End diff --

@jburwell see here ^^, accountype was a required arg but with our changes 
we've made it non-required also added the roleId arg. Therefore, at least one 
of the two must be provided, if both are provided we consider roleId (and 
account type of the role).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1489#discussion_r60881499
  
--- Diff: engine/schema/src/com/cloud/upgrade/dao/Upgrade481to490.java ---
@@ -53,6 +62,139 @@ public boolean supportsRollingUpgrade() {
 
 @Override
 public void performDataMigration(Connection conn) {
+setupRolesAndPermissionsForDynamicRBAC(conn);
+}
+
+private void createDefaultRole(final Connection conn, final Long id, 
final String name, final RoleType roleType) {
+final String insertSql = String.format("INSERT INTO 
`cloud`.`roles` (`id`, `uuid`, `name`, `role_type`, `description`) values (%d, 
UUID(), '%s', '%s', 'Default %s role');",
+id, name, roleType.name(), roleType.name().toLowerCase());
+try ( PreparedStatement updatePstmt = 
conn.prepareStatement(insertSql) ) {
+updatePstmt.executeUpdate();
+} catch (SQLException e) {
+throw new CloudRuntimeException("Unable to create default role 
with id: " + id + " name: " + name, e);
+}
+}
+
+private void createRoleMapping(final Connection conn, final Long 
roleId, final String apiName) {
+final String insertSql = String.format("INSERT INTO 
`cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`) values 
(UUID(), %d, '%s', 'ALLOW') ON DUPLICATE KEY UPDATE rule=rule;",
+roleId, apiName);
+try ( PreparedStatement updatePstmt = 
conn.prepareStatement(insertSql)) {
+updatePstmt.executeUpdate();
+} catch (SQLException ignored) {
+s_logger.debug("Unable to insert mapping for role id:" + 
roleId + " apiName: " + apiName);
+}
+}
+
+private void addRoleColumnAndMigrateAccountTable(final Connection 
conn, final RoleType[] roleTypes) {
+final String alterTableSql = "ALTER TABLE `cloud`.`account` ADD 
COLUMN `role_id` bigint(20) unsigned COMMENT 'role id for this account' AFTER 
`type`, " +
+"ADD KEY `fk_account__role_id` (`role_id`), " +
+"ADD CONSTRAINT `fk_account__role_id` FOREIGN KEY 
(`role_id`) REFERENCES `roles` (`id`);";
+try (PreparedStatement pstmt = 
conn.prepareStatement(alterTableSql)) {
+pstmt.executeUpdate();
+s_logger.info("Altered cloud.account table and added column 
role_id");
+} catch (SQLException e) {
+if (e.getMessage().contains("role_id")) {
+s_logger.warn("cloud.account table already has the role_id 
column, skipping altering table and migration of accounts");
+return;
+} else {
+throw new CloudRuntimeException("Unable to create column 
quota_calculated in table cloud_usage.cloud_usage", e);
+}
+}
+migrateAccountsToDefaultRoles(conn, roleTypes);
+}
+
+private void migrateAccountsToDefaultRoles(final Connection conn, 
final RoleType[] roleTypes) {
+try (PreparedStatement selectStatement = 
conn.prepareStatement("SELECT `id`, `type` FROM `cloud`.`account`;");
+ ResultSet selectResultSet = selectStatement.executeQuery()) {
+while (selectResultSet.next()) {
+Long accountId = selectResultSet.getLong(1);
+Short accountType = selectResultSet.getShort(2);
+Long roleId = null;
+for (RoleType roleType : roleTypes) {
+if (roleType.getAccountType() == accountType) {
+roleId = roleType.getId();
+break;
+}
+}
+if (roleId == null) {
+continue;
+}
+try (PreparedStatement updateStatement = 
conn.prepareStatement("UPDATE `cloud`.`account` SET role_id = ? WHERE id = 
?;")) {
+updateStatement.setLong(1, roleId);
+updateStatement.setLong(2, accountId);
+updateStatement.executeUpdate();
+updateStatement.close();
+
+} catch (SQLException e) {
+s_logger.error("Failed to update cloud.account role_id 
for account id:" + accountId + " with exception: " + e.getMessage());
+throw new CloudRuntimeException("Exception while 
updating cloud.account role_id", e);
+}
+}
+} catch (SQLException e) {
+throw new CloudRuntimeException("Exception while migrating 
existing account table's role_id column to a role based on account type", e);
+}
+s_logger.debug("Done migrating existing accounts to use one of

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1489#discussion_r60881018
  
--- Diff: server/src/org/apache/cloudstack/acl/RoleManagerImpl.java ---
@@ -0,0 +1,273 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.acl;
+
+import com.cloud.event.ActionEvent;
+import com.cloud.event.EventTypes;
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.user.Account;
+import com.cloud.user.dao.AccountDao;
+import com.cloud.utils.PropertiesUtil;
+import com.cloud.utils.component.ManagerBase;
+import com.cloud.utils.component.PluggableService;
+import com.cloud.utils.db.Transaction;
+import com.cloud.utils.db.TransactionCallback;
+import com.cloud.utils.db.TransactionStatus;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.acl.dao.RoleDao;
+import org.apache.cloudstack.acl.dao.RolePermissionsDao;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.command.admin.acl.CreateRoleCmd;
+import org.apache.cloudstack.api.command.admin.acl.CreateRolePermissionCmd;
+import org.apache.cloudstack.api.command.admin.acl.DeleteRoleCmd;
+import org.apache.cloudstack.api.command.admin.acl.DeleteRolePermissionCmd;
+import org.apache.cloudstack.api.command.admin.acl.ListRolePermissionsCmd;
+import org.apache.cloudstack.api.command.admin.acl.ListRolesCmd;
+import org.apache.cloudstack.api.command.admin.acl.UpdateRoleCmd;
+import org.apache.cloudstack.api.command.admin.acl.UpdateRolePermissionCmd;
+import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.Configurable;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import java.io.File;
+import java.util.ArrayList;
+import java.util.List;
+
+@Local(value = {RoleService.class})
+public class RoleManagerImpl extends ManagerBase implements RoleService, 
Configurable, PluggableService {
+@Inject
+private AccountDao accountDao;
+@Inject
+private RoleDao roleDao;
+@Inject
+private RolePermissionsDao rolePermissionsDao;
+
+private void checkCallerAccess() {
+if (!isEnabled()) {
+throw new PermissionDeniedException("Dynamic api checker is 
not enabled, aborting role operation");
+}
+Account caller = CallContext.current().getCallingAccount();
+if (caller == null || caller.getRoleId() == null) {
+throw new PermissionDeniedException("Restricted API called by 
an invalid user account");
+}
+Role callerRole = findRole(caller.getRoleId());
+if (callerRole == null || callerRole.getRoleType() != 
RoleType.Admin) {
+throw new PermissionDeniedException("Restricted API called by 
an user account of non-Admin role type");
+}
+}
+
+@Override
+public boolean isEnabled() {
+File apiCmdFile = 
PropertiesUtil.findConfigFile(PropertiesUtil.getDefaultApiCommandsFileName());
+return RoleService.EnableDynamicApiChecker.value() && (apiCmdFile 
== null || !apiCmdFile.exists());
+}
+
+@Override
+public Role findRole(final Long id) {
+if (id == null || id < 1L) {
+return null;
+}
+return roleDao.findById(id);
+}
+
+@Override
+public RolePermission findRolePermission(final Long id) {
+if (id == null) {
+return null;
+}
+return rolePermissionsDao.findById(id);
+}
+
+@Override
+@ActionEvent(eventType = EventTypes.EVENT_ROLE_CREATE, 
eventDescription = "creating Role")
+public Role createRole(final String name, final RoleType roleType, 
final String description) {
+

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1489#discussion_r60880846
  
--- Diff: test/integration/smoke/test_dynamicroles.py ---
@@ -0,0 +1,474 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from marvin.cloudstackAPI import *
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.cloudstackException import CloudstackAPIException
+from marvin.lib.base import Account, Role, RolePermission
+from marvin.lib.utils import cleanup_resources
+from nose.plugins.attrib import attr
+
+import random
+import re
+
+
+class TestData(object):
+"""Test data object that is required to create resources
+"""
+def __init__(self):
+self.testdata = {
+"account": {
+"email": "mtu@test.cloud",
+"firstname": "Marvin",
+"lastname": "TestUser",
+"username": "roletest",
+"password": "password",
+},
+"role": {
+"name": "MarvinFake Role ",
+"type": "User",
+"description": "Fake Role created by Marvin test"
+},
+"roleadmin": {
+"name": "MarvinFake Admin Role ",
+"type": "Admin",
+"description": "Fake Admin Role created by Marvin test"
+},
+"roledomainadmin": {
+"name": "MarvinFake DomainAdmin Role ",
+"type": "DomainAdmin",
+"description": "Fake Domain-Admin Role created by Marvin 
test"
+},
+"rolepermission": {
+"roleid": 1,
+"rule": "listVirtualMachines",
+"permission": "allow",
+"description": "Fake role permission created by Marvin 
test"
+},
+"apiConfig": {
+"listApis": "allow",
+"listAccounts": "allow",
+"listClusters": "deny",
+"*VM*": "allow",
+"*Host*": "deny"
+}
+}
+
+
+class TestDynamicRoles(cloudstackTestCase):
+"""Tests dynamic role and role permission management in CloudStack
+"""
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.dbclient = self.testClient.getDbConnection()
+self.testdata = TestData().testdata
+
+feature_enabled = 
self.apiclient.listCapabilities(listCapabilities.listCapabilitiesCmd()).dynamicrolesenabled
+if not feature_enabled:
+self.skipTest("Dynamic Role-Based API checker not enabled, 
skipping test")
+
+self.testdata["role"]["name"] += self.getRandomString()
+self.role = Role.create(
+self.apiclient,
+self.testdata["role"]
+)
+
+self.testdata["rolepermission"]["roleid"] = self.role.id
+self.rolepermission = RolePermission.create(
+self.apiclient,
+self.testdata["rolepermission"]
+)
+
+self.account = Account.create(
+self.apiclient,
+self.testdata["account"],
+roleid=self.role.id
+)
+self.cleanup = [
+self.account,
+self.rolepermission,
+self.role
+]
+
+
+def tearDown(self):
+try:
+   cleanup_resources(self.apiclient, self.cleanup)
+except Exception as e:
+self.debug("Warning! Exception in tearDown: %s" % e)
+
+
+def translateRoleToAccountType(self, role_type):
+if role_type == "User":
+return 0
+elif role_type == "Admin":
+return 1
+elif role_type == "DomainAdmin":
+return 2

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1489#discussion_r60880204
  
--- Diff: test/integration/smoke/test_dynamicroles.py ---
@@ -0,0 +1,474 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from marvin.cloudstackAPI import *
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.cloudstackException import CloudstackAPIException
+from marvin.lib.base import Account, Role, RolePermission
+from marvin.lib.utils import cleanup_resources
+from nose.plugins.attrib import attr
+
+import random
+import re
+
+
+class TestData(object):
+"""Test data object that is required to create resources
+"""
+def __init__(self):
+self.testdata = {
+"account": {
+"email": "mtu@test.cloud",
+"firstname": "Marvin",
+"lastname": "TestUser",
+"username": "roletest",
+"password": "password",
+},
+"role": {
+"name": "MarvinFake Role ",
+"type": "User",
+"description": "Fake Role created by Marvin test"
+},
+"roleadmin": {
+"name": "MarvinFake Admin Role ",
+"type": "Admin",
+"description": "Fake Admin Role created by Marvin test"
+},
+"roledomainadmin": {
+"name": "MarvinFake DomainAdmin Role ",
+"type": "DomainAdmin",
+"description": "Fake Domain-Admin Role created by Marvin 
test"
+},
+"rolepermission": {
+"roleid": 1,
+"rule": "listVirtualMachines",
+"permission": "allow",
+"description": "Fake role permission created by Marvin 
test"
+},
+"apiConfig": {
+"listApis": "allow",
+"listAccounts": "allow",
+"listClusters": "deny",
+"*VM*": "allow",
+"*Host*": "deny"
+}
+}
+
+
+class TestDynamicRoles(cloudstackTestCase):
+"""Tests dynamic role and role permission management in CloudStack
+"""
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.dbclient = self.testClient.getDbConnection()
+self.testdata = TestData().testdata
+
+feature_enabled = 
self.apiclient.listCapabilities(listCapabilities.listCapabilitiesCmd()).dynamicrolesenabled
+if not feature_enabled:
+self.skipTest("Dynamic Role-Based API checker not enabled, 
skipping test")
+
+self.testdata["role"]["name"] += self.getRandomString()
+self.role = Role.create(
+self.apiclient,
+self.testdata["role"]
+)
+
+self.testdata["rolepermission"]["roleid"] = self.role.id
+self.rolepermission = RolePermission.create(
+self.apiclient,
+self.testdata["rolepermission"]
+)
+
+self.account = Account.create(
+self.apiclient,
+self.testdata["account"],
+roleid=self.role.id
+)
+self.cleanup = [
+self.account,
+self.rolepermission,
+self.role
+]
+
+
+def tearDown(self):
+try:
+   cleanup_resources(self.apiclient, self.cleanup)
+except Exception as e:
+self.debug("Warning! Exception in tearDown: %s" % e)
+
+
+def translateRoleToAccountType(self, role_type):
+if role_type == "User":
+return 0
+elif role_type == "Admin":
+return 1
+elif role_type == "DomainAdmin":
+return 2

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1489#discussion_r60880231
  
--- Diff: test/integration/smoke/test_dynamicroles.py ---
@@ -0,0 +1,474 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from marvin.cloudstackAPI import *
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.cloudstackException import CloudstackAPIException
+from marvin.lib.base import Account, Role, RolePermission
+from marvin.lib.utils import cleanup_resources
+from nose.plugins.attrib import attr
+
+import random
+import re
+
+
+class TestData(object):
+"""Test data object that is required to create resources
+"""
+def __init__(self):
+self.testdata = {
+"account": {
+"email": "mtu@test.cloud",
+"firstname": "Marvin",
+"lastname": "TestUser",
+"username": "roletest",
+"password": "password",
+},
+"role": {
+"name": "MarvinFake Role ",
+"type": "User",
+"description": "Fake Role created by Marvin test"
+},
+"roleadmin": {
+"name": "MarvinFake Admin Role ",
+"type": "Admin",
+"description": "Fake Admin Role created by Marvin test"
+},
+"roledomainadmin": {
+"name": "MarvinFake DomainAdmin Role ",
+"type": "DomainAdmin",
+"description": "Fake Domain-Admin Role created by Marvin 
test"
+},
+"rolepermission": {
+"roleid": 1,
+"rule": "listVirtualMachines",
+"permission": "allow",
+"description": "Fake role permission created by Marvin 
test"
+},
+"apiConfig": {
+"listApis": "allow",
+"listAccounts": "allow",
+"listClusters": "deny",
+"*VM*": "allow",
+"*Host*": "deny"
+}
+}
+
+
+class TestDynamicRoles(cloudstackTestCase):
+"""Tests dynamic role and role permission management in CloudStack
+"""
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.dbclient = self.testClient.getDbConnection()
+self.testdata = TestData().testdata
+
+feature_enabled = 
self.apiclient.listCapabilities(listCapabilities.listCapabilitiesCmd()).dynamicrolesenabled
+if not feature_enabled:
+self.skipTest("Dynamic Role-Based API checker not enabled, 
skipping test")
+
+self.testdata["role"]["name"] += self.getRandomString()
+self.role = Role.create(
+self.apiclient,
+self.testdata["role"]
+)
+
+self.testdata["rolepermission"]["roleid"] = self.role.id
+self.rolepermission = RolePermission.create(
+self.apiclient,
+self.testdata["rolepermission"]
+)
+
+self.account = Account.create(
+self.apiclient,
+self.testdata["account"],
+roleid=self.role.id
+)
+self.cleanup = [
+self.account,
+self.rolepermission,
+self.role
+]
+
+
+def tearDown(self):
+try:
+   cleanup_resources(self.apiclient, self.cleanup)
+except Exception as e:
+self.debug("Warning! Exception in tearDown: %s" % e)
+
+
+def translateRoleToAccountType(self, role_type):
+if role_type == "User":
+return 0
+elif role_type == "Admin":
+return 1
+elif role_type == "DomainAdmin":
+return 2

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1489#discussion_r60880213
  
--- Diff: test/integration/smoke/test_dynamicroles.py ---
@@ -0,0 +1,474 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from marvin.cloudstackAPI import *
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.cloudstackException import CloudstackAPIException
+from marvin.lib.base import Account, Role, RolePermission
+from marvin.lib.utils import cleanup_resources
+from nose.plugins.attrib import attr
+
+import random
+import re
+
+
+class TestData(object):
+"""Test data object that is required to create resources
+"""
+def __init__(self):
+self.testdata = {
+"account": {
+"email": "mtu@test.cloud",
+"firstname": "Marvin",
+"lastname": "TestUser",
+"username": "roletest",
+"password": "password",
+},
+"role": {
+"name": "MarvinFake Role ",
+"type": "User",
+"description": "Fake Role created by Marvin test"
+},
+"roleadmin": {
+"name": "MarvinFake Admin Role ",
+"type": "Admin",
+"description": "Fake Admin Role created by Marvin test"
+},
+"roledomainadmin": {
+"name": "MarvinFake DomainAdmin Role ",
+"type": "DomainAdmin",
+"description": "Fake Domain-Admin Role created by Marvin 
test"
+},
+"rolepermission": {
+"roleid": 1,
+"rule": "listVirtualMachines",
+"permission": "allow",
+"description": "Fake role permission created by Marvin 
test"
+},
+"apiConfig": {
+"listApis": "allow",
+"listAccounts": "allow",
+"listClusters": "deny",
+"*VM*": "allow",
+"*Host*": "deny"
+}
+}
+
+
+class TestDynamicRoles(cloudstackTestCase):
+"""Tests dynamic role and role permission management in CloudStack
+"""
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.dbclient = self.testClient.getDbConnection()
+self.testdata = TestData().testdata
+
+feature_enabled = 
self.apiclient.listCapabilities(listCapabilities.listCapabilitiesCmd()).dynamicrolesenabled
+if not feature_enabled:
+self.skipTest("Dynamic Role-Based API checker not enabled, 
skipping test")
+
+self.testdata["role"]["name"] += self.getRandomString()
+self.role = Role.create(
+self.apiclient,
+self.testdata["role"]
+)
+
+self.testdata["rolepermission"]["roleid"] = self.role.id
+self.rolepermission = RolePermission.create(
+self.apiclient,
+self.testdata["rolepermission"]
+)
+
+self.account = Account.create(
+self.apiclient,
+self.testdata["account"],
+roleid=self.role.id
+)
+self.cleanup = [
+self.account,
+self.rolepermission,
+self.role
+]
+
+
+def tearDown(self):
+try:
+   cleanup_resources(self.apiclient, self.cleanup)
+except Exception as e:
+self.debug("Warning! Exception in tearDown: %s" % e)
+
+
+def translateRoleToAccountType(self, role_type):
+if role_type == "User":
+return 0
+elif role_type == "Admin":
+return 1
+elif role_type == "DomainAdmin":
+return 2

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1489#discussion_r60880195
  
--- Diff: test/integration/smoke/test_dynamicroles.py ---
@@ -0,0 +1,474 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from marvin.cloudstackAPI import *
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.cloudstackException import CloudstackAPIException
+from marvin.lib.base import Account, Role, RolePermission
+from marvin.lib.utils import cleanup_resources
+from nose.plugins.attrib import attr
+
+import random
+import re
+
+
+class TestData(object):
+"""Test data object that is required to create resources
+"""
+def __init__(self):
+self.testdata = {
+"account": {
+"email": "mtu@test.cloud",
+"firstname": "Marvin",
+"lastname": "TestUser",
+"username": "roletest",
+"password": "password",
+},
+"role": {
+"name": "MarvinFake Role ",
+"type": "User",
+"description": "Fake Role created by Marvin test"
+},
+"roleadmin": {
+"name": "MarvinFake Admin Role ",
+"type": "Admin",
+"description": "Fake Admin Role created by Marvin test"
+},
+"roledomainadmin": {
+"name": "MarvinFake DomainAdmin Role ",
+"type": "DomainAdmin",
+"description": "Fake Domain-Admin Role created by Marvin 
test"
+},
+"rolepermission": {
+"roleid": 1,
+"rule": "listVirtualMachines",
+"permission": "allow",
+"description": "Fake role permission created by Marvin 
test"
+},
+"apiConfig": {
+"listApis": "allow",
+"listAccounts": "allow",
+"listClusters": "deny",
+"*VM*": "allow",
+"*Host*": "deny"
+}
+}
+
+
+class TestDynamicRoles(cloudstackTestCase):
+"""Tests dynamic role and role permission management in CloudStack
+"""
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.dbclient = self.testClient.getDbConnection()
+self.testdata = TestData().testdata
+
+feature_enabled = 
self.apiclient.listCapabilities(listCapabilities.listCapabilitiesCmd()).dynamicrolesenabled
+if not feature_enabled:
+self.skipTest("Dynamic Role-Based API checker not enabled, 
skipping test")
+
+self.testdata["role"]["name"] += self.getRandomString()
+self.role = Role.create(
+self.apiclient,
+self.testdata["role"]
+)
+
+self.testdata["rolepermission"]["roleid"] = self.role.id
+self.rolepermission = RolePermission.create(
+self.apiclient,
+self.testdata["rolepermission"]
+)
+
+self.account = Account.create(
+self.apiclient,
+self.testdata["account"],
+roleid=self.role.id
+)
+self.cleanup = [
+self.account,
+self.rolepermission,
+self.role
+]
+
+
+def tearDown(self):
+try:
+   cleanup_resources(self.apiclient, self.cleanup)
+except Exception as e:
+self.debug("Warning! Exception in tearDown: %s" % e)
+
+
+def translateRoleToAccountType(self, role_type):
+if role_type == "User":
+return 0
+elif role_type == "Admin":
+return 1
+elif role_type == "DomainAdmin":
+return 2

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1489#discussion_r60880160
  
--- Diff: test/integration/smoke/test_dynamicroles.py ---
@@ -0,0 +1,474 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from marvin.cloudstackAPI import *
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.cloudstackException import CloudstackAPIException
+from marvin.lib.base import Account, Role, RolePermission
+from marvin.lib.utils import cleanup_resources
+from nose.plugins.attrib import attr
+
+import random
+import re
+
+
+class TestData(object):
+"""Test data object that is required to create resources
+"""
+def __init__(self):
+self.testdata = {
+"account": {
+"email": "mtu@test.cloud",
+"firstname": "Marvin",
+"lastname": "TestUser",
+"username": "roletest",
+"password": "password",
+},
+"role": {
+"name": "MarvinFake Role ",
+"type": "User",
+"description": "Fake Role created by Marvin test"
+},
+"roleadmin": {
+"name": "MarvinFake Admin Role ",
+"type": "Admin",
+"description": "Fake Admin Role created by Marvin test"
+},
+"roledomainadmin": {
+"name": "MarvinFake DomainAdmin Role ",
+"type": "DomainAdmin",
+"description": "Fake Domain-Admin Role created by Marvin 
test"
+},
+"rolepermission": {
+"roleid": 1,
+"rule": "listVirtualMachines",
+"permission": "allow",
+"description": "Fake role permission created by Marvin 
test"
+},
+"apiConfig": {
+"listApis": "allow",
+"listAccounts": "allow",
+"listClusters": "deny",
+"*VM*": "allow",
+"*Host*": "deny"
+}
+}
+
+
+class TestDynamicRoles(cloudstackTestCase):
+"""Tests dynamic role and role permission management in CloudStack
+"""
+
+def setUp(self):
+self.apiclient = self.testClient.getApiClient()
+self.dbclient = self.testClient.getDbConnection()
+self.testdata = TestData().testdata
+
+feature_enabled = 
self.apiclient.listCapabilities(listCapabilities.listCapabilitiesCmd()).dynamicrolesenabled
+if not feature_enabled:
+self.skipTest("Dynamic Role-Based API checker not enabled, 
skipping test")
+
+self.testdata["role"]["name"] += self.getRandomString()
+self.role = Role.create(
+self.apiclient,
+self.testdata["role"]
+)
+
+self.testdata["rolepermission"]["roleid"] = self.role.id
+self.rolepermission = RolePermission.create(
+self.apiclient,
+self.testdata["rolepermission"]
+)
+
+self.account = Account.create(
+self.apiclient,
+self.testdata["account"],
+roleid=self.role.id
+)
+self.cleanup = [
+self.account,
+self.rolepermission,
+self.role
+]
+
+
+def tearDown(self):
+try:
+   cleanup_resources(self.apiclient, self.cleanup)
+except Exception as e:
+self.debug("Warning! Exception in tearDown: %s" % e)
+
+
+def translateRoleToAccountType(self, role_type):
+if role_type == "User":
+return 0
+elif role_type == "Admin":
+return 1
+elif role_type == "DomainAdmin":
+return 2

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...

Github user rhtyd commented on a diff in the pull request:

https://github.com/apache/cloudstack/pull/1489#discussion_r60879938
  
--- Diff: utils/src/main/java/com/cloud/utils/PropertiesUtil.java ---
@@ -34,6 +34,10 @@
 public class PropertiesUtil {
 private static final Logger s_logger = 
Logger.getLogger(PropertiesUtil.class);
 
+public static String getDefaultApiCommandsFileName() {
+return "commands.properties";
+}
--- End diff --

commands.properties is consumed by server, plugins/acl/static-role-based 
and engine/schema packages. I'm keeping it this way so it can be consumed by 
all these packages, as utils is used by all and PropertiesUtils is a utility 
class to manage .properties files


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...