Overall review (couldn't test but here read through review)
1) unable to test because of paswword stengthening and ssl cert stuff required (not your issue, so don't worry)
2) I'm assuming there's gonna be more code for the functionality (logging in, etc.)
3) Seems an odd use for an ACL, especially considering the danger of this aco you will create (ie. users may easily place this aco by mistake in other acl's in admin->acl) seems a user flag/database entry may be safer, which of course as acl case can elicit warnings and admin emails when creating.
4) if still decide on acl route, i'm assuming you plan on making a breakglass group, acl, and aco. Here are some acl references: http://www.openmedsoftware.org/wiki/PhpGaclhttp://www.openmedsoftware.org/wiki/Development_Policies#Access_Control_Objects
5) also note users can be placed in multiple ACL groups on the admin pages (shouldn't you search entire array?)
interface/usergroup/user_admin.php
-d/w Thomas regarding optimal email method (note we are likely about to commit his upgrade to phpmailer with email/smtp server settings in globals.php)
-misspelled $mail->FromName = "Adminstrator OpenEMR";
-line 105 in patch leading space in xl()
-Can't you get the chosen acl groups via the ACL selector on that page rather than using acl_get_group_titles? (i may be wrong, since not sure of placement of this command). Also note a user can select more than one acl group.
interface/usergroup/usergroup_admin.php
-Note a user can select more than one acl group.
-brady
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Just also realized another problem if go with an ACL group. Easy to get around the admin email notification if do the following:
-Create normal user with a normal ACL
-In Administration->ACL, then place the user in the Breakglass group
-(Could also do this in the 'Advanced' admin->acl section in several different ways)
-brady
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
2 - The audit patch contains the logging details of breakglass
3 & 4 - A quick question. Can the creation of the acl group "BreakGlass" be handled by the administrator since they would be aware of the rights that need to be given to this emergency user _?
Do share your views here.
Thanks
ViCarePlus team_
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
If go with ACL, I'd suggest creating the ACL, group, and aco's you want this user to have as default (see above links).
Instead of 'BreakGlass', couldn't we use a name like "Emergency Login" for the acl and group name.
Also, realize that catching every time a user is added to this "Emergency Login" group will be very difficult, since can be done in admin->users screen, admin->acl screen, and admin->acl->advanced screens.
-brady
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
hey,
If want to ensure catch everytime a user is added to the "Emergency Login", may be best to go to the source and place within the applicable php-gacl admin API.
-brady
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Few items for creating a new ACL "Emergency Login"……
I think we need to modify the following files to create a new ACL
1. Header notes of the library/acl.inc file
2. acl_setup.php file
3. acl_upgrade.php file
Can we provide the same administraor privilges to this "Emergency Login" since we aren't sure of the emergency needs.
Do share your views.
Thanks
ViCarePlus Team
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
You're correct on the pieces you need to modify. Just take a stab at it; you'll find the learning curve to do this is very minimal. To avoid issues in the upgrade script(note the function that will make a new acl in the upgrade script will also create the new group if it doesn't yet exist), make the Group and the ACL name the same. I'd go with following:
Group and ACL title -> "Emergency Login"
Group and ACL id -> "breakglass"
Give whatever permissions (aco's) you think you need. Note the mechanism of the upgrade script can only add stuff (so for example, if in the next version you wanted to remove aco's from the breakglass, can't do this in the upgrade script. This shouldn't stop you from what you want to do, but just good to know the mechanism)
-brady
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
To clarify above, by taking a stab at it, I mean post a patch (don't commit this stuff to cvs until we see the patch just so we can make sure your doing it correctly)
-brady
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
1) Main issue is hacking the user active flag to get this stuff to work. Just isn't pretty (also potential for lock out issues if for example a sole admin user adds itself to breakglass group in admin->acl, then logs out, now an inactive user and locked out), and can still get around your email mechanism by adding group via admin->acl->advanced. Seems like the add_group_object() function in php-gacl would be a nice central place to catch if a user is added to the "Emergency Login" group. If no admin email is set (see 2 below), then could simply not allow. Check out the php-gacl api's in admin->acl->advanced->api guide->gacl_api. As I recall the actual functions are in gacl/gacl_api.class.php .
2) Regarding the email, is it stored anywhere? I don't see it being stored in the email column of 'admin' user? Another option is to control this via the globals.php (place a switch to allow adding users to the breakglass group (placed into the above add_group_object() function), and could even consider putting the main admin email there, which could also be used for other system messages, such as logging and auditing messages).
openemr/acl_setup.php
-ok
openemr/acl_upgrade.php
---For the updateAcl() statements, ensure using the correct titles of aco's (most of yours seem wrong). For example:
BELOW IS WRONG:
updateAcl($emergency_write, 'Emergency Login', 'acct', 'Accounting', 'eob', 'Explaination of Benefit', 'write');
SHOULD BE:
updateAcl($emergency_write, 'Emergency Login', 'acct', 'Accounting', 'eob', 'EOB Data Entry', 'write');
Get the ACL titles from the acl_setup.php script (for example above is at line 69)
---It is extremely vital to ensure all this stuff is correct (ie. don't type it, use copy/paste,check it, double check it, triple check it), becuase once you add a incorrect setting, it can not be removed with the current upgrading mechanism. Also, double test it(so first time will upgrade it, and ensure looks good in acl->administration->advanced, then run it again to ensure everything shows up as current).
openemr/interface/usergroup/usergroup_admin.php
---Why are you hard-coding these?:
mail->isMail();
mail->Host = "localhost";
mail->Mailer = "mail";
-Aren't the above now set in /classes/postmaster.php via globals.php settings? Thomas added this mechanism, but I do not think it has yet been tested. Hopefully it will work.
-brady
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
1. add_group_object() function in php-gacl is used to track the users for the "Emergency Login"
2. Sending the mail during emergency user activation is made optional
3. The following configurations are added
a. Emergency_Login_email - Mail will be circulated during Emergency user activation if this variable is set to 1
b. Emergency_Login_id - Emergency user activation mail will be circulated to this email id
4. Proper titles for aco's are given in acl_upgrade.php
5. For sending mails, configurations from globals.php is used.
Do let me know for any queries.
Thanks
ViCarePlus team
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Main issue here again is hacking the active flag to get this stuff to work. There is a huge global and systemic risk in your code for locking out users, especially when users are using the admin user in a sole practice situation. For example, the new admin/practiioner user says, "what is that cool Emrgency Login thing", click on it, logs out, and now is locked out. Attaching this ACL to switching off/on the active flag will cause confusion in the entire openemr userbase for a function that only few will use. But, it is a function that is important to offer. So, I'm thinking that creating 'breakglass_account' and 'breakglass_active' flags in the users table is much more simple and less risky; then can ask for this stuff and put all logic in the user admin screen, and deal with login authorization (ie. only login if a breakglass_account has breakglass_active flag on), and can mess around with turning on/off the breakglass_active flag without any issues to normal users. Then it's up to the admin to place the user into the 'Emergency Login' ACL (ie. no need for code to monitor this action, since can just monitor the admin user screen when setting/unsetting the breakglass_account and breakglass_active flags). Doing this should require minimal coding changes.
openemr/interface/globals.php
---If go with the breakglass_account and breakglass_active flags then might as well place a switch that hides this selection in the admin users screen (effectively turning this option off)
---Make more clear that the admin ID is supposed to be an email address. (Emergency_Login_id variable makes it sound like an id and not an email address, this email address will likely be used in other parts of openemr at some point)
thanks,
-brady
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I think the main issue you are foreseeing here is the deactivation of any user when "Emergency User" ACL is selected for this user.
Thanks for suggesting flags. How about the following one?
Let us not deactivate the user when "Emergency user ACL" is created. Instead of that we can issue a warning "Emergency user ACL is chosen. The user is still in active state, please de-activate the user and activate the same when required during emergency situations".
This way we can avoid the mis-handling of Emergency ACL.
Do you see any issues with the above approach? Do share your views here.
Thanks
ViCareplus Team
(www.vicareplus.com)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
That sounds like a great idea, and is why you get paid the big bucks :) The beauty of your solution is that it plays nicely with the general openemr userbase, it requires only little more work, and it allows the option of creating flags if needed in the future (ie. certification requirement changes) In the message probably good to put where to de-activate and re-activate user:
""Emergency user ACL is chosen. The user is still in an active state. To finish configuration of this emergency user, please de-activate the user in the 'Administration->Users' menu. Then can re-activate this user when required during emergency situations."
(above string was written quickly just to illustrate showing where to de-activate the user; feel free to put what you want)
thanks,
brady
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Looks good, and is ready to commit. After you commit, I'll probably make some minor mods:
-put the admin_id email variable in globals.php near the top with an explanation that this should be the admin email. then can use this for other parts of openemr
-make the instructions/warnings strings in the admin->users and the admin->acl more conducive for the translators
I'll post links to changes here to ensure you agree
thanks,
brady
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi Team,
The patch for 'Emergency Access Procedure' is submitted in the sourceforge tracker at https://sourceforge.net/tracker/?func=detail&aid=2939371&group_id=60081&atid=1245239.
Do share your views.
Thanks
ViCarePlus Team
review:
Overall review (couldn't test but here read through review)
1) unable to test because of paswword stengthening and ssl cert stuff required (not your issue, so don't worry)
2) I'm assuming there's gonna be more code for the functionality (logging in, etc.)
3) Seems an odd use for an ACL, especially considering the danger of this aco you will create (ie. users may easily place this aco by mistake in other acl's in admin->acl) seems a user flag/database entry may be safer, which of course as acl case can elicit warnings and admin emails when creating.
4) if still decide on acl route, i'm assuming you plan on making a breakglass group, acl, and aco. Here are some acl references: http://www.openmedsoftware.org/wiki/PhpGacl http://www.openmedsoftware.org/wiki/Development_Policies#Access_Control_Objects
5) also note users can be placed in multiple ACL groups on the admin pages (shouldn't you search entire array?)
interface/usergroup/user_admin.php
-d/w Thomas regarding optimal email method (note we are likely about to commit his upgrade to phpmailer with email/smtp server settings in globals.php)
-misspelled $mail->FromName = "Adminstrator OpenEMR";
-line 105 in patch leading space in xl()
-Can't you get the chosen acl groups via the ACL selector on that page rather than using acl_get_group_titles? (i may be wrong, since not sure of placement of this command). Also note a user can select more than one acl group.
interface/usergroup/usergroup_admin.php
-Note a user can select more than one acl group.
-brady
hey,
Just also realized another problem if go with an ACL group. Easy to get around the admin email notification if do the following:
-Create normal user with a normal ACL
-In Administration->ACL, then place the user in the Breakglass group
-(Could also do this in the 'Advanced' admin->acl section in several different ways)
-brady
hey,
Regarding email method, here's the mechanism we just checked in to the SF cvs (from Thomas's code).
-brady
oops, forgot the link, here it is:
http://www.openmedsoftware.org/wiki/Sending_Email
Hi brady,
2 - The audit patch contains the logging details of breakglass
3 & 4 - A quick question. Can the creation of the acl group "BreakGlass" be handled by the administrator since they would be aware of the rights that need to be given to this emergency user _?
Do share your views here.
Thanks
ViCarePlus team_
If go with ACL, I'd suggest creating the ACL, group, and aco's you want this user to have as default (see above links).
Instead of 'BreakGlass', couldn't we use a name like "Emergency Login" for the acl and group name.
Also, realize that catching every time a user is added to this "Emergency Login" group will be very difficult, since can be done in admin->users screen, admin->acl screen, and admin->acl->advanced screens.
-brady
hey,
If want to ensure catch everytime a user is added to the "Emergency Login", may be best to go to the source and place within the applicable php-gacl admin API.
-brady
Hi Brady,
Few items for creating a new ACL "Emergency Login"……
I think we need to modify the following files to create a new ACL
1. Header notes of the library/acl.inc file
2. acl_setup.php file
3. acl_upgrade.php file
Can we provide the same administraor privilges to this "Emergency Login" since we aren't sure of the emergency needs.
Do share your views.
Thanks
ViCarePlus Team
ViCarePlus Team,
You're correct on the pieces you need to modify. Just take a stab at it; you'll find the learning curve to do this is very minimal. To avoid issues in the upgrade script(note the function that will make a new acl in the upgrade script will also create the new group if it doesn't yet exist), make the Group and the ACL name the same. I'd go with following:
Group and ACL title -> "Emergency Login"
Group and ACL id -> "breakglass"
Give whatever permissions (aco's) you think you need. Note the mechanism of the upgrade script can only add stuff (so for example, if in the next version you wanted to remove aco's from the breakglass, can't do this in the upgrade script. This shouldn't stop you from what you want to do, but just good to know the mechanism)
-brady
To clarify above, by taking a stab at it, I mean post a patch (don't commit this stuff to cvs until we see the patch just so we can make sure your doing it correctly)
-brady
Hi,
The modified patch with the incorporation of community suggestions for "Emergency Login" are placed in the sourceforge tracker..
Thanks
ViCarePlus team
Review of this patch:
http://sourceforge.net/tracker/?func=detail&aid=2939371&group_id=60081&atid=1245239
1) Main issue is hacking the user active flag to get this stuff to work. Just isn't pretty (also potential for lock out issues if for example a sole admin user adds itself to breakglass group in admin->acl, then logs out, now an inactive user and locked out), and can still get around your email mechanism by adding group via admin->acl->advanced. Seems like the add_group_object() function in php-gacl would be a nice central place to catch if a user is added to the "Emergency Login" group. If no admin email is set (see 2 below), then could simply not allow. Check out the php-gacl api's in admin->acl->advanced->api guide->gacl_api. As I recall the actual functions are in gacl/gacl_api.class.php .
2) Regarding the email, is it stored anywhere? I don't see it being stored in the email column of 'admin' user? Another option is to control this via the globals.php (place a switch to allow adding users to the breakglass group (placed into the above add_group_object() function), and could even consider putting the main admin email there, which could also be used for other system messages, such as logging and auditing messages).
openemr/acl_setup.php
-ok
openemr/acl_upgrade.php
---For the updateAcl() statements, ensure using the correct titles of aco's (most of yours seem wrong). For example:
BELOW IS WRONG:
updateAcl($emergency_write, 'Emergency Login', 'acct', 'Accounting', 'eob', 'Explaination of Benefit', 'write');
SHOULD BE:
updateAcl($emergency_write, 'Emergency Login', 'acct', 'Accounting', 'eob', 'EOB Data Entry', 'write');
Get the ACL titles from the acl_setup.php script (for example above is at line 69)
---It is extremely vital to ensure all this stuff is correct (ie. don't type it, use copy/paste,check it, double check it, triple check it), becuase once you add a incorrect setting, it can not be removed with the current upgrading mechanism. Also, double test it(so first time will upgrade it, and ensure looks good in acl->administration->advanced, then run it again to ensure everything shows up as current).
openemr/interface/usergroup/user_admin.php
---See 1) above
openemr/library/ajax/adminacl_ajax.php
---See 1) above
openemr/interface/usergroup/usergroup_admin.php
---Why are you hard-coding these?:
mail->isMail();
mail->Host = "localhost";
mail->Mailer = "mail";
-Aren't the above now set in /classes/postmaster.php via globals.php settings? Thomas added this mechanism, but I do not think it has yet been tested. Hopefully it will work.
-brady
Hi Brady/Rod,
The following changes are performed for Emergency access procedure and the updated patch is placed in the sourceforge tracker (https://sourceforge.net/tracker/?func=detail&aid=2939371&group_id=60081&atid=1245239)
1. add_group_object() function in php-gacl is used to track the users for the "Emergency Login"
2. Sending the mail during emergency user activation is made optional
3. The following configurations are added
a. Emergency_Login_email - Mail will be circulated during Emergency user activation if this variable is set to 1
b. Emergency_Login_id - Emergency user activation mail will be circulated to this email id
4. Proper titles for aco's are given in acl_upgrade.php
5. For sending mails, configurations from globals.php is used.
Do let me know for any queries.
Thanks
ViCarePlus team
hey,
Main issue here again is hacking the active flag to get this stuff to work. There is a huge global and systemic risk in your code for locking out users, especially when users are using the admin user in a sole practice situation. For example, the new admin/practiioner user says, "what is that cool Emrgency Login thing", click on it, logs out, and now is locked out. Attaching this ACL to switching off/on the active flag will cause confusion in the entire openemr userbase for a function that only few will use. But, it is a function that is important to offer. So, I'm thinking that creating 'breakglass_account' and 'breakglass_active' flags in the users table is much more simple and less risky; then can ask for this stuff and put all logic in the user admin screen, and deal with login authorization (ie. only login if a breakglass_account has breakglass_active flag on), and can mess around with turning on/off the breakglass_active flag without any issues to normal users. Then it's up to the admin to place the user into the 'Emergency Login' ACL (ie. no need for code to monitor this action, since can just monitor the admin user screen when setting/unsetting the breakglass_account and breakglass_active flags). Doing this should require minimal coding changes.
openemr/interface/globals.php
---If go with the breakglass_account and breakglass_active flags then might as well place a switch that hides this selection in the admin users screen (effectively turning this option off)
---Make more clear that the admin ID is supposed to be an email address. (Emergency_Login_id variable makes it sound like an id and not an email address, this email address will likely be used in other parts of openemr at some point)
thanks,
-brady
Hi Brady,
I think the main issue you are foreseeing here is the deactivation of any user when "Emergency User" ACL is selected for this user.
Thanks for suggesting flags. How about the following one?
Let us not deactivate the user when "Emergency user ACL" is created. Instead of that we can issue a warning "Emergency user ACL is chosen. The user is still in active state, please de-activate the user and activate the same when required during emergency situations".
This way we can avoid the mis-handling of Emergency ACL.
Do you see any issues with the above approach? Do share your views here.
Thanks
ViCareplus Team
(www.vicareplus.com)
hey,
That sounds like a great idea, and is why you get paid the big bucks :) The beauty of your solution is that it plays nicely with the general openemr userbase, it requires only little more work, and it allows the option of creating flags if needed in the future (ie. certification requirement changes) In the message probably good to put where to de-activate and re-activate user:
""Emergency user ACL is chosen. The user is still in an active state. To finish configuration of this emergency user, please de-activate the user in the 'Administration->Users' menu. Then can re-activate this user when required during emergency situations."
(above string was written quickly just to illustrate showing where to de-activate the user; feel free to put what you want)
thanks,
brady
Hi Brady
We've done the following changes in the code and uploaded the same in the tracker.
a. Not de-activating the user while Emergency ACL is chosen
b. Included a warning to de-activate the Emergency user
Do share your views here.
Thanks
Vicare Team
hey,
Looks good, and is ready to commit. After you commit, I'll probably make some minor mods:
-put the admin_id email variable in globals.php near the top with an explanation that this should be the admin email. then can use this for other parts of openemr
-make the instructions/warnings strings in the admin->users and the admin->acl more conducive for the translators
I'll post links to changes here to ensure you agree
thanks,
brady
hey,
Here are changes i just committed to make the strings easier for the translators (the string from the admin->acl ajax call and the admin->users screen are now identical):
http://openemr.cvs.sourceforge.net/viewvc/openemr/openemr/interface/usergroup/usergroup_admin.php?r1=1.36&r2=1.37
-brady