MakeMeAdmin
MakeMeAdmin copied to clipboard
pseymour
Reason Dialog Box
Implement an optional dialog box, where users can enter the reason they need administrator rights.
Features should include a drop-down list of "canned" responses, provided by the administrator. Also, allow a free-form text box, where users can enter their own reason. Administrators should be able to control which of these features is/are available to the user.
+1 would really love this with just a free-form text box (the rest is extra)
You can enable just the free-form part of the dialog, and the drop-down box will be disabled.
Tested this a little bit.
Primary issues I noticed:
-
Selecting "Cancel" from the "Reason Dialog Box" continues on with the elevation process. One would expect that the cancel button would simply close that dialog without submitting to elevate.
-
You have RequireReason functionality commented out. I assume there was some reason/issues to workout? https://github.com/pseymour/MakeMeAdmin/blob/reason-dialog/Settings/Settings.cs#L496
Once some of these issues are addressed, any chance this would be merged into master? Would love to see this along with terminate-elevated-processes merged into master.
I'll answer in reverse order... The reason for the code you mentioned being commented out is that it is replaced by the code below it. PromptForReason is now an enum, and it can be set to None, Optional or Required. The commented-out code is how I originally wrote it, and I didn't like it.
If you have the "Prompt for Reason" policy set to Optional, then you get the behavior you describe. If you set it to Required, then cancelling out of the reason dialog prevents the users from getting admin rights.
I'll answer in reverse order... The reason for the code you mentioned being commented out is that it is replaced by the code below it.
PromptForReasonis now an enum, and it can be set to None, Optional or Required. The commented-out code is how I originally wrote it, and I didn't like it.If you have the "Prompt for Reason" policy set to Optional, then you get the behavior you describe. If you set it to Required, then cancelling out of the reason dialog prevents the users from getting admin rights.
In my opinion, I think clicking the cancel button should simply close the comments dialog regardless of any enum/setting. I think that is just a typical UX expectation for a cancel button. I would never expect a cancel button to continue forward and submit the form. Instead, if PromptForReason is set to Optional the OK button should but enabled whether any text is input or not. That way, if the user optionally decides to not make a comment then they click OK to submit, otherwise, they click cancel to close the comments dialog.
The cancel button isn't submitting the form though. It's being used to "close a dialog box without making pending changes," which is standard for a command labelled Cancel.
@pseymour I see, the current logic is basically that clicking the "Grant Me Administrator Rights" button on the main form submits that the user will be added. The cancel button on the Reason Dialog simply closes the Dialog form. That makes sense in its own right, but I don't think it makes sense as part of the overall flow to obtain the Admin rights.
It seems unintuitive when considering the whole interaction overall. For example, I click the "Grant Me Administrator Rights" button, I now see that I'm prompted to enter a reason. I decide I don't really have a good reason or maybe I just decide I want to cancel the interaction. There is no way for me to stop the process at this point. As a user I would expect the Reason Dialog to be part of the process of me getting Admin rights. If you want to think of it sort of like a Wizard; Step 1, I start the process, Step 2 I provide reason/comments. If I cancel, I don't really expect the entire process to carry out, I would expect to be put back on Step 1.
I do think this piece could be improved, it stuck out to me in particular for a reason. If you do not agree, that's is perfectly fine and I'll let it be.
In any case, I will re-install this and try to more fully test this enhancement and let you know if I notice any issues.
PS: What are you using to create those screen recorded gifs? :)
@pseymour I'm sure you saw this in Issue #19 , but this feedback really should have been put here. Do let me know if there's anything else you'd want tested. The UI is pretty solid. There are bits that could be a bit better, but the only show stopper seems to be the missing length limit for the free-form text box.
The UI is pretty solid for the reason prompting. But the instruction text is as clear as it could be. Please provide a reason for requesting administrator rights. makes it pretty clear what's happening in the window, but not necessarily the specifics of what needs to be done. I think it would be great if that text (as well as most of the rest of the text though out the app ... i.e "Request Administrator Privileges" instead of "Grant Me Administrator Rights") could be replaced by GP/Registry Keys.
For example, if I'm only providing a drop down list the text should read Please select a reason from the dropdown below for requesting administrator rights.
Or if I'm allowing free form and a drop down Please select a reason from the dropdown below for requesting administrator rights. If nothing in the list works, please select Other and enter a reason in the text box. (Maybe too long but you get the gist).
It may be better for some folks to not have the dropdown populated when first presenting it. Or dis-allow the first choice in the list so it can be set to something like Select a reason ...
I was going to mention that the input dialogue should have been grayed out but, I actually prefer that you pop the drop down to Other.
A setting for min/max characters allowed in the free form dialog box would be appreciated. I'm unsure how your syslog is implemented, but I understand it can be possible to have messages that are too large to transfer and that can cause problems. Actually this is a big one. I tried to paste the full text of Catcher in the Rye as a reason and crashed the UI.
I noticed that the event log for the reason doesn't include the username of the requester. I do understand that it's in the next event, but for monitoring the log and tying in an alert it could prove simpler to include the username in the event ...DOMAIN\User provided the following reason for administrator rights: Other: Some reason or another. or Reason for administrator rights: User: DOMAIN\USER Reason: Other: Some reason or another. instead of The user provided the following reason for administrator rights: Other: Some reason or another.
If you have any questions, I'm happy to provide more feedback.
Hey, I have been digging around for how to implement the reason. Please could you give me a quick load down if you don't mind on how to implement reason dialog box and how to compile it back into a package. Thanks for your assistance. I tried to find you on slack but couldn't reach out. Thanks for the product its really good.
It's controlled by a few settings...
- Prompt For Reason - whether to prompt for a reason at all (0 : no prompt, 1 : optional reason, 2 : mandatory reason)
- Allow Free-Form Reason - whether to allow the user to type free-form text as a reason.
- Canned Reasons - a set of strings from which the user can choose (e.g., driver installation, software installation, etc.)
- Maximum Reason Length - the maximum length of the text the user can enter in the free-form area.