Acl xduser #146
Merged
Acl xduser #146
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tyearke
added
the
enhancement
ryanrath
force-pushed
the
acl_xduser
branch
2 times, most recently
from
a8e94da
to
1215cd4
Compare
jpwhite4
reviewed
Jun 24, 2017
classes/XDUser.php
Outdated
| if (null !== self::$_publicUser) { | ||
| return self::$_publicUser; | ||
| } else { | ||
| self::$_publicUser = self::getUserByUserName('Public User'); |
There was a problem hiding this comment.
Where is the code that automatically creates the 'Public User' row in the database? I can't find it anywhere.
There was a problem hiding this comment.
Also as a coding standard comment:
I prefer to not repeat identical lines in the code unless it is unavoidable. I suggest the following, which I think is easier to see what is happening:
if (null === self::$_publicUser) {
self::$_publicUser = self::getUserByUserName('Public User');
}
return self::$_publicUser;
8f97326
to
3c9a255
Compare
jpwhite4
requested changes
Aug 3, 2017
shippable.yml
Outdated
| - cp ./configuration/portal_settings.ini ./configuration/portal_settings.ini.old | ||
| - cp -f /etc/xdmod/portal_settings.ini ./configuration/portal_settings.ini | ||
| - ./open_xdmod/modules/xdmod/component_tests/runtests.sh --log-junit `pwd`/shippable/testresults/componentresults.xml | ||
|
|
There was a problem hiding this comment.
You should restore the portal_settings.ini.old to its original location after running the tests (as we discussed before).
There was a problem hiding this comment.
Thanks! I did totally miss that. Latest commit mv's the portal_settings.ini.old back to portal_settings.ini.
jpwhite4
approved these changes
Aug 3, 2017
- Bringing the format of XDUser up to speed before continuing with ACL
modifications.
Adding models that support the Acl transition
- Adding a number of new models that act as:
- a way to document and pin our code to a particular set of table columns
for a particular code version
- a shim between PDO result sets and the rest of our code as they have been
built with the following use case in mind:
$results = array();
$rows = $db->query("SELECT * FROM some_table;");
foreach($rows as $row) {
$results []= new Model($row);
}
the model code takes care of mapping all of the columns to the correct
properties. It works on a whitelist concept so any extraneous columns are
ignored for the purposes property setting.
- I added the @method annotations purely to make users lives a little easier
as it explicitly lays out which functions are supported by each class.
Meaning if they're new they don't need to dig into DBObject to find out
what the $PROP_MAP does ( although they can obviously ). They can just
rely on the class providing those methods.
- Added a 'service' class Acls ( service naming just being the plural of the
model they support ) that provides a number of helper functions / encapsulates
a great deal about the underlying table structure allowing the user to provide
a few bits of information and let the service take care of the rest.
Porting XDUser changes from acls branch to this one
- These changes update the portions of the XDUser api to use the new acl tables
/ relations where possible. They make use of the new Model / Service classes
that were added in previous commits.
Migrating isDeveloper to be based off of acls
- isDeveloper was previously based off of the roles array, now it's based off of
acls.
Initializing _acls in the constructor of XDUser
- the _acls property needs to be initialized when XDUser is.
Updating the removeUser to use isPublicUser instead of active_role
- Just migrating this like the other instances of comparison to ROLE_ID_PUBLIC
While the return value cannot be false it could be empty
- to prevent accessing an array entry that doesn't exist added a conditional
that only allow the access to occur if there is 1 or more records returned.
XDUserTest: tests the modified and newly added acl functions
- Huge caveat: to run these tests you need a fully functional xdmod install.
Both database and generated classes.
- I have not included the generated coverage html but the % lines covered for
the functions that were modified are as follows:
- isDeveloper: 100%
- saveUser: 95%
- getToken: 100%
- getTokenExpiration: 100%
- removeUser: 96%
- setInstitution: 90%
- disassociateWithInstitution: 90%
- setOrganizations: 90%
- getRoles: 100%
- setRoles: 100%
- getPrimaryRole: 100%
- getActiveRole: 100%
- getAcls: 100%
- setAcls: 100%
- addAcl: 100%
- hasAcl: 100%
- hasAcls: 100%
- getUserByUserName: 100%
Namespace formatting fix
- Removed the extra line after the namespace.
Fixed invalid sql syntax
- The SQL syntax for 'deleteAcl' was invalid. You are not allowed to alias
tables in a DELETE statement.
Simplifying the query / logic of addUserAcl
- Instead of issuing two database queries, one to find and another one to insert.
We instead INSERT the results of a SELECT that will only return results if the
record that would be inserted does not exist ( WHERE cur.user_acl_id IS NULL
).
Updating Acls function documentation
- Updated / Added documentation where it was missing / incorrect.
Updating XDUserTest to utilize pi
- Open XDMoD does not have the program officer user so use pi instead.
- Also update the default institution value to 0 as opposed to -1.
Clarifying getPublicUser
- minor code refactor to increase legibility per code review comment by
@jpwhite4
Adding Component Test folder
- Adding a place for Component Tests to live. i.e. tests that require access to
the database but are not doing so via the REST interface.
Adding sql script to create the Public User
- We now have the code in place to deal with an actual Public User, so we'll be
adding it during the execution of the acl-import pipeline.
Adding component tests to shippable
Updated the XDUserTest namespace
- simple c/p mistake Updated so it reflects the current namespace.
Testing shippable changes to accommodate component tests
Add acl-import to be run during a fresh_install
Resolve Initial Acls not being set
- instead of directly assigning the incoming $role_set to _roles we instead
utilize the function setRoles which will take care of keeping the acls in
sync.
- remove the initialization of _acls to an empty array() as this would override
the work done by setRoles.
- Also needed to remove the additional acl-import in
integration_tests/scripts/bootstrap.sh as we need to see if the changes made
to XDUser work as expected.
Adding some jobviewer tests to exercise the Acl code
- Added some integration tests to exercise the acl code via the single job
viewer - job search end point.
Updating Usr Tests
- Missed the c/p from pi -> usr.
Removing JobViewerTests, these belong in the supremm module
Resolving spacing syntax issue
restoring portal_settings.ini from ini.old
- Adding a line per discussion with @jpwhite4 to restore the original
portal_settings.ini ( from portal_settings.ini.old ).
ryanrath
added a commit
to ryanrath/xdmod
that referenced
this pull request
Sep 18, 2017
- Bringing the format of XDUser up to speed before continuing with ACL
modifications.
Adding models that support the Acl transition
- Adding a number of new models that act as:
- a way to document and pin our code to a particular set of table columns
for a particular code version
- a shim between PDO result sets and the rest of our code as they have been
built with the following use case in mind:
$results = array();
$rows = $db->query("SELECT * FROM some_table;");
foreach($rows as $row) {
$results []= new Model($row);
}
the model code takes care of mapping all of the columns to the correct
properties. It works on a whitelist concept so any extraneous columns are
ignored for the purposes property setting.
- I added the @method annotations purely to make users lives a little easier
as it explicitly lays out which functions are supported by each class.
Meaning if they're new they don't need to dig into DBObject to find out
what the $PROP_MAP does ( although they can obviously ). They can just
rely on the class providing those methods.
- Added a 'service' class Acls ( service naming just being the plural of the
model they support ) that provides a number of helper functions / encapsulates
a great deal about the underlying table structure allowing the user to provide
a few bits of information and let the service take care of the rest.
Porting XDUser changes from acls branch to this one
- These changes update the portions of the XDUser api to use the new acl tables
/ relations where possible. They make use of the new Model / Service classes
that were added in previous commits.
Migrating isDeveloper to be based off of acls
- isDeveloper was previously based off of the roles array, now it's based off of
acls.
Initializing _acls in the constructor of XDUser
- the _acls property needs to be initialized when XDUser is.
Updating the removeUser to use isPublicUser instead of active_role
- Just migrating this like the other instances of comparison to ROLE_ID_PUBLIC
While the return value cannot be false it could be empty
- to prevent accessing an array entry that doesn't exist added a conditional
that only allow the access to occur if there is 1 or more records returned.
XDUserTest: tests the modified and newly added acl functions
- Huge caveat: to run these tests you need a fully functional xdmod install.
Both database and generated classes.
- I have not included the generated coverage html but the % lines covered for
the functions that were modified are as follows:
- isDeveloper: 100%
- saveUser: 95%
- getToken: 100%
- getTokenExpiration: 100%
- removeUser: 96%
- setInstitution: 90%
- disassociateWithInstitution: 90%
- setOrganizations: 90%
- getRoles: 100%
- setRoles: 100%
- getPrimaryRole: 100%
- getActiveRole: 100%
- getAcls: 100%
- setAcls: 100%
- addAcl: 100%
- hasAcl: 100%
- hasAcls: 100%
- getUserByUserName: 100%
Namespace formatting fix
- Removed the extra line after the namespace.
Fixed invalid sql syntax
- The SQL syntax for 'deleteAcl' was invalid. You are not allowed to alias
tables in a DELETE statement.
Simplifying the query / logic of addUserAcl
- Instead of issuing two database queries, one to find and another one to insert.
We instead INSERT the results of a SELECT that will only return results if the
record that would be inserted does not exist ( WHERE cur.user_acl_id IS NULL
).
Updating Acls function documentation
- Updated / Added documentation where it was missing / incorrect.
Updating XDUserTest to utilize pi
- Open XDMoD does not have the program officer user so use pi instead.
- Also update the default institution value to 0 as opposed to -1.
Clarifying getPublicUser
- minor code refactor to increase legibility per code review comment by
@jpwhite4
Adding Component Test folder
- Adding a place for Component Tests to live. i.e. tests that require access to
the database but are not doing so via the REST interface.
Adding sql script to create the Public User
- We now have the code in place to deal with an actual Public User, so we'll be
adding it during the execution of the acl-import pipeline.
Adding component tests to shippable
Updated the XDUserTest namespace
- simple c/p mistake Updated so it reflects the current namespace.
Testing shippable changes to accommodate component tests
Add acl-import to be run during a fresh_install
Resolve Initial Acls not being set
- instead of directly assigning the incoming $role_set to _roles we instead
utilize the function setRoles which will take care of keeping the acls in
sync.
- remove the initialization of _acls to an empty array() as this would override
the work done by setRoles.
- Also needed to remove the additional acl-import in
integration_tests/scripts/bootstrap.sh as we need to see if the changes made
to XDUser work as expected.
Adding some jobviewer tests to exercise the Acl code
- Added some integration tests to exercise the acl code via the single job
viewer - job search end point.
Updating Usr Tests
- Missed the c/p from pi -> usr.
Removing JobViewerTests, these belong in the supremm module
Resolving spacing syntax issue
restoring portal_settings.ini from ini.old
- Adding a line per discussion with @jpwhite4 to restore the original
portal_settings.ini ( from portal_settings.ini.old ).
chakrabortyr
pushed a commit
to chakrabortyr/xdmod
that referenced
this pull request
Oct 17, 2017
- Bringing the format of XDUser up to speed before continuing with ACL
modifications.
Adding models that support the Acl transition
- Adding a number of new models that act as:
- a way to document and pin our code to a particular set of table columns
for a particular code version
- a shim between PDO result sets and the rest of our code as they have been
built with the following use case in mind:
$results = array();
$rows = $db->query("SELECT * FROM some_table;");
foreach($rows as $row) {
$results []= new Model($row);
}
the model code takes care of mapping all of the columns to the correct
properties. It works on a whitelist concept so any extraneous columns are
ignored for the purposes property setting.
- I added the @method annotations purely to make users lives a little easier
as it explicitly lays out which functions are supported by each class.
Meaning if they're new they don't need to dig into DBObject to find out
what the $PROP_MAP does ( although they can obviously ). They can just
rely on the class providing those methods.
- Added a 'service' class Acls ( service naming just being the plural of the
model they support ) that provides a number of helper functions / encapsulates
a great deal about the underlying table structure allowing the user to provide
a few bits of information and let the service take care of the rest.
Porting XDUser changes from acls branch to this one
- These changes update the portions of the XDUser api to use the new acl tables
/ relations where possible. They make use of the new Model / Service classes
that were added in previous commits.
Migrating isDeveloper to be based off of acls
- isDeveloper was previously based off of the roles array, now it's based off of
acls.
Initializing _acls in the constructor of XDUser
- the _acls property needs to be initialized when XDUser is.
Updating the removeUser to use isPublicUser instead of active_role
- Just migrating this like the other instances of comparison to ROLE_ID_PUBLIC
While the return value cannot be false it could be empty
- to prevent accessing an array entry that doesn't exist added a conditional
that only allow the access to occur if there is 1 or more records returned.
XDUserTest: tests the modified and newly added acl functions
- Huge caveat: to run these tests you need a fully functional xdmod install.
Both database and generated classes.
- I have not included the generated coverage html but the % lines covered for
the functions that were modified are as follows:
- isDeveloper: 100%
- saveUser: 95%
- getToken: 100%
- getTokenExpiration: 100%
- removeUser: 96%
- setInstitution: 90%
- disassociateWithInstitution: 90%
- setOrganizations: 90%
- getRoles: 100%
- setRoles: 100%
- getPrimaryRole: 100%
- getActiveRole: 100%
- getAcls: 100%
- setAcls: 100%
- addAcl: 100%
- hasAcl: 100%
- hasAcls: 100%
- getUserByUserName: 100%
Namespace formatting fix
- Removed the extra line after the namespace.
Fixed invalid sql syntax
- The SQL syntax for 'deleteAcl' was invalid. You are not allowed to alias
tables in a DELETE statement.
Simplifying the query / logic of addUserAcl
- Instead of issuing two database queries, one to find and another one to insert.
We instead INSERT the results of a SELECT that will only return results if the
record that would be inserted does not exist ( WHERE cur.user_acl_id IS NULL
).
Updating Acls function documentation
- Updated / Added documentation where it was missing / incorrect.
Updating XDUserTest to utilize pi
- Open XDMoD does not have the program officer user so use pi instead.
- Also update the default institution value to 0 as opposed to -1.
Clarifying getPublicUser
- minor code refactor to increase legibility per code review comment by
@jpwhite4
Adding Component Test folder
- Adding a place for Component Tests to live. i.e. tests that require access to
the database but are not doing so via the REST interface.
Adding sql script to create the Public User
- We now have the code in place to deal with an actual Public User, so we'll be
adding it during the execution of the acl-import pipeline.
Adding component tests to shippable
Updated the XDUserTest namespace
- simple c/p mistake Updated so it reflects the current namespace.
Testing shippable changes to accommodate component tests
Add acl-import to be run during a fresh_install
Resolve Initial Acls not being set
- instead of directly assigning the incoming $role_set to _roles we instead
utilize the function setRoles which will take care of keeping the acls in
sync.
- remove the initialization of _acls to an empty array() as this would override
the work done by setRoles.
- Also needed to remove the additional acl-import in
integration_tests/scripts/bootstrap.sh as we need to see if the changes made
to XDUser work as expected.
Adding some jobviewer tests to exercise the Acl code
- Added some integration tests to exercise the acl code via the single job
viewer - job search end point.
Updating Usr Tests
- Missed the c/p from pi -> usr.
Removing JobViewerTests, these belong in the supremm module
Resolving spacing syntax issue
restoring portal_settings.ini from ini.old
- Adding a line per discussion with @jpwhite4 to restore the original
portal_settings.ini ( from portal_settings.ini.old ).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Motivation and Context
Tests performed
Types of changes
Checklist: