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

[battery_manager] Resolved Permission Issues #6858

Conversation

HenriRabalais
Copy link
Collaborator

@HenriRabalais HenriRabalais commented Jul 22, 2020

Brief summary of changes

This PR fixes the getCenterId function in test.class.inc so that it always passes a value to the UserSiteMatch filter by converting a centerId value of null from the database to an array of all sites.

Testing instructions (if applicable)

  1. Create a user and do not assign it battery_manager view or edit permission.
  2. Ensure that battery_manager cannot be accessed.
  3. Assign user battery_manager view permission.
  4. Ensure that battery_manager entries at user's site can be view, but there is no ability to add, activate, deactivate or edit the entries.
  5. Give user battery_manager edit permission.
  6. Ensure that entries can be added, activated, deactivated and edited.

Link(s) to related issue(s)

@HenriRabalais HenriRabalais added Category: Bug PR or issue that aims to report or fix a bug Category: Documentation PR or issue that aims to improve the documentation (test plans, wiki, comments...) labels Jul 22, 2020
@driusan
Copy link
Collaborator

driusan commented Jul 22, 2020

The two issues this PR says it resolved are tagged for the 23.0.x, this should go to the 23.0 branch, not main.

@driusan
Copy link
Collaborator

driusan commented Jul 22, 2020

Actually hold off on that, it involves SQL, so might need some discussion..

@HenriRabalais
Copy link
Collaborator Author

@driusan The SQL is just permission label changes to clear up the confusing that was mentioned in #6683

@HenriRabalais HenriRabalais changed the base branch from main to 23.0-release July 22, 2020 18:48
@ridz1208 ridz1208 changed the base branch from 23.0-release to main July 22, 2020 18:58
@ridz1208
Copy link
Collaborator

@HenriRabalais you will need to separate the pure bugfix from the "clarification" we dont do patches on bugfix releases, specially non-urgent ones like this one

@HenriRabalais HenriRabalais force-pushed the 2020-07-22_batterymanager_fixpermissionissues branch from 6844aa5 to 540c958 Compare July 22, 2020 19:01
@ridz1208 ridz1208 changed the base branch from main to 23.0-release July 22, 2020 19:02
@HenriRabalais HenriRabalais removed the Category: Documentation PR or issue that aims to improve the documentation (test plans, wiki, comments...) label Jul 22, 2020
@HenriRabalais
Copy link
Collaborator Author

@ridz1208 @driusan fixed.

@pierre-p-s
Copy link
Contributor

I am getting this error when testing:

Screen Shot 2020-08-04 at 10 39 45 AM

@ridz1208
Copy link
Collaborator

ridz1208 commented Aug 5, 2020

@HenriRabalais the loadResources() function was added in #6640 in order to be able to check for access before loading the target entity.

because it is being called on the testEndpoint class from the php/libraries/module.class.inc on line 350 the testendpoint class needs to either define the loadResources() function (see code changes in pr #6640) or needs to extend NDB_Page which has an empty implementation of loadResources() (see same PR)

@HenriRabalais
Copy link
Collaborator Author

HenriRabalais commented Aug 5, 2020

@ridz1208 but what does that have to do with this PR? Isn't that an issue that should be solved separately and exists separately to the introduction of this PR?

@ridz1208
Copy link
Collaborator

ridz1208 commented Aug 6, 2020

@HenriRabalais not if u want your pr to pass testing. The base on which your pr is based has these changes which means your changes need to adapt as well. Call it a double bugfix if you want, this bugfix would have been necessary whether you had issued this PR or not (given that the problematic code has not been introduced in this PR directly)

@ridz1208
Copy link
Collaborator

ridz1208 commented Aug 6, 2020

@pierre-p-s can you give it another go

@pierre-p-s
Copy link
Contributor

@ridz1208 @HenriRabalais
Testing worked

@driusan
Copy link
Collaborator

driusan commented Aug 6, 2020

Travis disagrees.

@ridz1208
Copy link
Collaborator

ridz1208 commented Aug 6, 2020

@driusan I disagree with TRAVIS' disagreement

@ridz1208
Copy link
Collaborator

ridz1208 commented Aug 6, 2020

@driusan finally urs

@driusan driusan merged commit 4b8f00a into aces:23.0-release Aug 6, 2020
@ridz1208 ridz1208 added this to the 23.0.2 milestone Aug 6, 2020
spell00 pushed a commit to spell00/Loris that referenced this pull request Aug 13, 2020
This PR fixes the getCenterId function in test.class.inc so that it always passes a value to the UserSiteMatch filter by converting a centerId value of null from the database to an array of all sites.

    Resolves aces#6682
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 15, 2021
This PR fixes the getCenterId function in test.class.inc so that it always passes a value to the UserSiteMatch filter by converting a centerId value of null from the database to an array of all sites.

    Resolves aces#6682
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Bug PR or issue that aims to report or fix a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[battery_manager] Edit permission [battery_manager] View permission
4 participants