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

Add disable forced reset credential flag on portal login reset/create #6817

Merged
merged 3 commits into from
Sep 2, 2023

Conversation

sjpadgett
Copy link
Member

Add user setting to persist option for the user setting it in MRD
add checkbox and persist js function to template.

Fixes #6813

Add user setting to persist option for the user setting it in MRD
add checkbox and persist js function to template.
@sjpadgett
Copy link
Member Author

Decided to put up separate PR to ease backport if wanted @adunsulag
I also decided to make the flag to persist by user. I didn't want a global which is logical but would make it harder to backport.

@adunsulag
Copy link
Member

@sjpadgett By putting it on the patient dashboard won't users think the setting will only apply to that one patient instead of it applying globally to all patients? If someone checks/unchecks the box for a single patient I'm not sure they'll realize they ended up changing the setting for all patients. If I'm following the code correctly, I believe that's how things will work with this commit right?

@bradymiller
Copy link
Member

Would make this something that the admin gets to set (ie. global).

Csrf\CsrfUtils,
Utils\RandomGenUtils,
};
use OpenEMR\Common\{Csrf\CsrfUtils,};
Copy link
Member

Choose a reason for hiding this comment

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

was this meant to remove Utils\RandomGenUtils ?

Copy link
Member Author

Choose a reason for hiding this comment

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

not used so not needed

Copy link
Member

Choose a reason for hiding this comment

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

sounds good, then would change to since no longer compounding:

use OpenEMR\Common\Csrf\CsrfUtils;

Copy link
Member Author

Choose a reason for hiding this comment

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

btw already is

Copy link
Member

Choose a reason for hiding this comment

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

sounds like maybe issue with PR since I see use OpenEMR\Common\{Csrf\CsrfUtils,}; instead of use OpenEMR\Common\Csrf\CsrfUtils; in line above

@sjpadgett
Copy link
Member Author

I don't want a global. I want to leave it up to the user and the ability to decide patient by patient if needed. It will persist for each user therefore once a user sets it, it will remain set and up to user to decide to unset.

Because one admin decides they don't like the forced reset that doesn't mean all users agree especially in practice groups.

It can be seen by many as a security hole.

@bradymiller
Copy link
Member

To clarify, trying to ensure no security issues (let me know if i am missing something).

My concern regarding security here is that if a practice wishes to enforce high security (make the user change the password), which has been the status quo, this will essentially remove that protection (since now any user that creates an account gets to decide this; ie. might as well not even use it then in that case). Makes more sense from a practice perspective for an admin/practice to decide what to do and then make it uniform. Having lay users decide likely mean they will just use whatever is set on toggle (and they will likely click is either way by mistake every once in awhile until the next mistaken click :) ). Also recommend keeping status quo on the global (ie. enforce the password change).

@sjpadgett
Copy link
Member Author

I disagree but don't care enough to argue the point. Closing for Stephen to pick up.

@sjpadgett sjpadgett closed this Aug 31, 2023
@sjpadgett sjpadgett reopened this Sep 1, 2023
@sjpadgett
Copy link
Member Author

I hope this compromise satisfies everyone's concerns or wishes.

@bradymiller
Copy link
Member

Looks great! (noted you have the global in another PR)

@bradymiller
Copy link
Member

tenor99

@sjpadgett
Copy link
Member Author

One complaint I often get is from practices that have mental health and/or seniors or otherwise patients that are not computer savvy and want the sign in, register and general portal use easier. This is the main reason to allow force reset to be optional per patient on their credential create/reset.
Secondary we have to get away from using globals for every little option and should refrain when possible. In this case by checking the option in dialog it would be the same as setting in globals but provide the option to change as needed. This is UX 101 stuff.
My old man mental capacity may be affecting my coding, my creativity and ability to think features through is still as sharp as ever. My communication skills on the other hand still needs improvement!:)

@sjpadgett sjpadgett merged commit 585970f into openemr:master Sep 2, 2023
23 checks passed
@sjpadgett sjpadgett deleted the portal_login branch September 2, 2023 13:24
sjpadgett added a commit to sjpadgett/openemr that referenced this pull request Sep 2, 2023
sjpadgett added a commit that referenced this pull request Sep 2, 2023
* Backport of force rest credentials option from PR #6817

* missed class
@bradymiller
Copy link
Member

Two opposing thoughts here. With a provider hat on, agree with being a patient advocate. With a security hat on, sadly it is those patients (no computer savviness) that benefit most from the additional security (ie. they are most likely to not control access to their email etc. ) :)

@sjpadgett
Copy link
Member Author

I almost feel like I need to develop a single sign on option for the portal.
I don't think the one time token would be available option for extended login

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.

feat: Add ability to prevent forced portal credential change
3 participants