Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 350, 352 #353

Merged
merged 10 commits into from
May 7, 2020
Merged

Issue 350, 352 #353

merged 10 commits into from
May 7, 2020

Conversation

naphelps
Copy link
Member

@naphelps naphelps commented May 6, 2020

References #350, #352

Changes:

  • Issue 350: Removed read_all_nodes from default permissions for user role, and removed read my nodes from default permissions for node role.
  • Issue 352: Removed test container. Testing is now done locally to the source code.

naphelps and others added 8 commits April 30, 2020 14:13
Removed READ ALL NODES from default permissions for the USER role.
Altered the business policy search api to limit which nodes are visible
to the user role.
Added v2.24.0 changes to readme.
Removed testing container.
@naphelps
Copy link
Member Author

naphelps commented May 6, 2020

All local and Anax tests have passed.

@naphelps naphelps requested review from sf2ne and bmpotter May 6, 2020 21:26
-e "EXCHANGE_IAM_EMAIL=$$EXCHANGE_IAM_EMAIL" \
-e "EXCHANGE_IAM_ACCOUNT_ID=$$EXCHANGE_IAM_ACCOUNT_ID" \
$(DOCKER_NAME)_test /bin/bash -c 'cd $(EXCHANGE_API_DIR) && sbt test'
- sbt test
Copy link
Member

Choose a reason for hiding this comment

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

How does the travis process have sbt, scala, and java installed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Java and Scala are defined at lines 2 - 4 of the Travis yaml. When defining Scala as your language Travis will automatically search for a build.sbt and pull-down sbt just before the script phase in the yaml.

https://docs.travis-ci.com/user/languages/scala/

logger.debug("ident.role.equals(AuthRoles.Agbot): " + ident.role.equals(AuthRoles.Agbot))
logger.debug("ident.role: " + ident.role)
logger.debug("(ident.isAdmin || ident.role.equals(AuthRoles.Agbot)): " + (ident.isAdmin || ident.role.equals(AuthRoles.Agbot)))
logger.debug("ident.getIdentity: " + ident.getIdentity)
Copy link
Member

Choose a reason for hiding this comment

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

Do you really want all of these debug statements long term? Or were they just to verify this code change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant to clean all of the up before pushing.

logger.debug("ident.role: " + ident.role)
logger.debug("(ident.isAdmin || ident.role.equals(AuthRoles.Agbot)): " + (ident.isAdmin || ident.role.equals(AuthRoles.Agbot)))
logger.debug("ident.getIdentity: " + ident.getIdentity)
if (ident.isAdmin || ident.role.equals(AuthRoles.Agbot)) {
Copy link
Member

Choose a reason for hiding this comment

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

I think i agree with sadiyah that it will be much less confusing to the non-admin user if we prevent him from running this route vs. let him run it but return less nodes.

So i think it best to remove your changes in this route, because that way exchAuth(TNode(OrgAndId(orgid,"#").toString), Access.READ) will reject a non-admin user.

Copy link
Member Author

Choose a reason for hiding this comment

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

Business policy search changes have been reverted.

Copy link
Collaborator

@sf2ne sf2ne left a comment

Choose a reason for hiding this comment

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

Lgtm!

@sf2ne
Copy link
Collaborator

sf2ne commented May 7, 2020

This PR also addresses: #352

@sf2ne sf2ne merged commit 620b4ce into open-horizon:master May 7, 2020
@naphelps naphelps deleted the issue-350 branch May 7, 2020 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants