Add reset password button to ops project list

[dgannon991]

What this PR does / why we need it: Adds a reset password button to the project list in the ops view

Which issue(s) this PR fixes:

Fixes #4844

Does this PR introduce a user-facing change?: Yes, a new button and column in the ops list

• How are users affected by this change: the OPs UI is updated
• Is this breaking change: No
• How to migrate (if breaking change): N/A

Just pushing up a draft for this at the moment to get some early feedback. I'll have a look through to see how/if we're testing the ops side of things later, but wanted to provide an early view of the UI.

[dgannon991]

@khanhtc1202 & @ffjlabo - forgot that it wouldn't notify you in draft mode :D Nothing urgent, just hoped to get some feedback on the UI before writing up the tests. Cheers!

[ffjlabo]

@dgannon991 Thank you so much for the quick response!! The button UI is LGTM for me :)

Thanks to your PR, I noticed this action is extremely dangerous because the control plane operator might reset for other projects accidentally. 👀 It would be nice to prevent users from accidentally initializing static admins for other projects.

So, how about adding a confirmation screen? e.g. the project list page -> reset password confirmation page -> success page

Reset password confirmation page like this↓ (the sentences are the draft, and I want more correct them 🙏 )

Reset Static Admin for the project XXX.
You must check the project name before you click the button.

"Reset button"

[khanhtc1202]

@dgannon991 @ffjlabo How about asking the user to reinstate the project name as part of the reset password confirmation page? (Cloud service like AWS use that method as re-check step if I remember correctly)

[dgannon991]

I like the sound of that. I've got some down time over the next couple of days, so I'll give it a go and let you know :)

[ffjlabo]

@dgannon991 @khanhtc1202

How about asking the user to reinstate the project name as part of the reset password confirmation page? (Cloud service like AWS use that method as re-check step if I remember correctly)

Thank you both 🙏 I agree with it!

[dgannon991]

Hi Guys, I've added a confirmation page, and you have to retype the project ID. One note though, the user experience is not great when you get things wrong (due to the server just returning error responses instead of markup.) Are we happy with this, as the ops pages are for technical users? Cheers!

[ffjlabo]

@dgannon991 Thank you for the nice commitment :) I also agree the ops pages are plain because these are for technical users.

[IMO] If possible, it would be nice to show the Reset page with the failed message, or the link to the Reset page.

Project Page

Fail

Succsess

[dgannon991]

Absolutely. I'll give that a whirl over the weekend. Cheers for the continued feedback on this one guys :)

[dgannon991]

Morning! I've updated this to show a nice error message on the confirmation page. Do let me know if it's too big and red! I looked at the material UI library we use in the normal frontend for inspiration, but it is the first think like it on the ops pages!

[ffjlabo]

Hi @dgannon991 Sorry for the late reply. Currently doing the other task. So plz wait a moment 🙏

[dgannon991]

Hi @ffjlabo,

Not a problem at all, the other task looks mega! No rush from my side at all :)

D

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 76.98413% with 29 lines in your changes missing coverage. Please review.

Project coverage is 22.24%. Comparing base (643b96b) to head (c52f1eb). Report is 6 commits behind head on master.

Files	Patch %	Lines
pkg/app/ops/handler/handler_mock.go	57.50%	17 Missing :warning:
pkg/app/ops/handler/handler.go	86.04%	8 Missing and 4 partials :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4910      +/-   ##
==========================================
+ Coverage   22.05%   22.24%   +0.19%     
==========================================
  Files         519      520       +1     
  Lines       57247    57365     +118     
==========================================
+ Hits        12623    12763     +140     
+ Misses      43601    43575      -26     
- Partials     1023     1027       +4     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[dgannon991]

Hi Guys, Just working on the coverage now. I've brought it up, but I think it will now be easy to really get it much higher, so I'll have another go tomorrow night at getting it there :) Cheers! David

[dgannon991]

Hi Guys, Managed to get a few more tests written, and I've got pretty much everything I added covered. It does use gomock (which is a shame) but I couldn't think of a better way to test without actually creating a project store! Let me know what you think! D

[khanhtc1202]

/review

[github-actions[bot]]

PR Analysis

Main theme

"Enhancement"

PR summary

"The PR introduces enhancements to the password reset mechanism within a Go web application, including updated request handlers, a new interface for project updates, and two new HTML templates for the password reset process."

Type of PR

"Enhancement"

PR Feedback:

General suggestions

The PR introduces a new feature for resetting static admin passwords for projects. This includes backend handler implementation, logic to fetch and update project information, as well as frontend templates for the reset password confirmation and result pages. Overall, the code changes appear to correctly implement the new feature, but with some points that need addressing for error handling and server response consistency.

Code feedback

- relevant file: "pkg/app/ops/handler/handler.go"
  suggestion: |-
    To avoid executing unnecessary code when an error is detected, you should return early after writing an error response to the client. This is particularly applicable in the 'handleResetPassword' function where several paths write an error response but do not immediately return, potentially leading to further (and unnecessary) code execution.
    `important`
  relevant line: "+		http.Error(w, "not found", http.StatusNotFound)"

- relevant file: "pkg/app/ops/handler/handler.go"
  suggestion: |-
    Consider implementing rate limiting or CAPTCHA validation to protect the password reset endpoint from being abused by automated scripts or brute force attacks.
    `important`
  relevant line: "+func (h *Handler) handleResetPassword(w http.ResponseWriter, r *http.Request) {"

- relevant file: "pkg/app/ops/handler/handler.go"
  suggestion: |-
    For improved security practices, avoid using predictable patterns for randomly generated passwords and consider integrating a cryptographically secure password generator.
    `medium`
  relevant line: "+	password := model.GenerateRandomString(30)"

Security concerns:

"yes"
"While no direct security vulnerabilities such as SQL injection or XSS are introduced in this PR, it does add new endpoints that could be susceptible to abuse, such as brute force attacks on the password reset function. Recommendations have been provided to mitigate these risks."

[dgannon991]

Hi Guys, more than happy to address these here if you'd like? LMK.

[khanhtc1202]

Hi Guys, more than happy to address these here if you'd like? LMK.

@dgannon991 For sure, let's do as you want 👍

[Warashi]

Thank you for saying that. I dismissed my approval for this PR not to make this PR merge by mistake, but it does not mean the code is not good.

[dgannon991]

Nice one! I’m AFK until Thursday night, but I’ll get it done Friday morning! Cheers all.

[dgannon991]

Hi Guys, The git history got a bit squirrely there, apologies. I rebased on my local master, but didn't realise it was stale! I'd definitely suggest a squash commit on this one! I've made the changes, and run the tests, and things still look good. I'll stand everything up when I get a second tomorrow AM and give a final update :) Cheers all.

[dgannon991]

Hi Guys, This looks better now, removed all the silly extra commits! I've given it a run through and it all works as expected :) Cheers! David

[Warashi]

I checked the behavior, and it worked fine. I commented nits. Please re-request a review or mention us in the comments when you resolve that.