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

feat: Add the ability to change user password in the UI #5444

Merged
merged 2 commits into from
Jun 2, 2021

Conversation

jaredclarke
Copy link
Contributor

@jaredclarke jaredclarke commented Feb 7, 2021

Signed-off-by: Jared Clarke jared.clarke@eckoh.com

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • My build is green (troubleshooting builds).

@codecov-io
Copy link

codecov-io commented Feb 7, 2021

Codecov Report

Merging #5444 (55f9ccb) into master (e22da4a) will increase coverage by 0.75%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5444      +/-   ##
==========================================
+ Coverage   40.44%   41.19%   +0.75%     
==========================================
  Files         142      148       +6     
  Lines       18936    19874     +938     
==========================================
+ Hits         7658     8188     +530     
- Misses      10187    10567     +380     
- Partials     1091     1119      +28     
Impacted Files Coverage Δ
util/env/env.go 78.78% <0.00%> (-21.22%) ⬇️
server/logout/logout.go 73.07% <0.00%> (-9.43%) ⬇️
util/helm/helm.go 60.27% <0.00%> (-3.37%) ⬇️
util/git/creds.go 9.62% <0.00%> (-2.68%) ⬇️
controller/sync.go 64.88% <0.00%> (-2.60%) ⬇️
util/helm/cmd.go 28.57% <0.00%> (-2.30%) ⬇️
reposerver/repository/repository.go 60.60% <0.00%> (-1.81%) ⬇️
util/git/client.go 47.67% <0.00%> (-1.53%) ⬇️
controller/state.go 68.42% <0.00%> (-1.23%) ⬇️
util/db/repository.go 62.73% <0.00%> (-1.01%) ⬇️
... and 62 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e22da4a...55f9ccb. Read the comment docs.

@rbreeze rbreeze self-assigned this Feb 8, 2021
<div className='user-info'>
<div className='argo-container'>
<div className='user-info-overview__panel white-box'>
<DataLoader key='userInfo' load={() => services.users.get()}>
Copy link
Member

Choose a reason for hiding this comment

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

Since this is loading the same data as the DataLoader on line 72, can you combine them please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I've done this in a reasonable way

@jaredclarke jaredclarke force-pushed the add-change-password-ui branch 2 times, most recently from 5330c49 to cff8f81 Compare February 9, 2021 09:33
<div className='user-info'>
<div className='argo-container'>
<div className='user-info-overview__panel white-box'>
userInfo.loggedIn ? (
Copy link
Member

Choose a reason for hiding this comment

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

Screen Shot 2021-02-11 at 11 48 22 AM

Could you fix this? Looks like you just need a { on this line and a matching } at the end of line 64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed - also fixed line 70 & 128 which were missing the same braces

Copy link
Member

@rbreeze rbreeze left a comment

Choose a reason for hiding this comment

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

Can you fix this error that occurs when a user clicks the Update Password button but isn't logged in? I think it would be better to hide this button altogether if the user isn't logged in

Screen Shot 2021-02-16 at 5 13 12 PM

jannfis
jannfis previously requested changes Mar 1, 2021
Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

One thing I noted is, that the "Update password" button is also displayed when logged in via SSO.

I think we should make sure to display this button only when the user currently logged in is a local one. This can be done by checking the issuer in the JWT, which has to be argocd in case of local users.

@alexmt
Copy link
Collaborator

alexmt commented Mar 1, 2021

Here is example of isSSO check:

if (loggedIn && iss !== 'argocd') {

Signed-off-by: Jared Clarke <jared.clarke@eckoh.com>
Copy link
Member

@rbreeze rbreeze left a comment

Choose a reason for hiding this comment

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

I still don't see a check for whether the user is logged in with SSO. Could you add this please?

Signed-off-by: Jared Clarke <jared.clarke@eckoh.com>
@jaredclarke
Copy link
Contributor Author

I still don't see a check for whether the user is logged in with SSO. Could you add this please?

https://github.com/argoproj/argo-cd/pull/5444/files#diff-0e6721e087b5175a38e352b36e08d9ef0a6b18a5d0a30769babf799768b83c11R36

I originally added the iss check against the button being displayed but have now added this to the main sliding panel as well

@alexmt alexmt dismissed jannfis’s stale review June 2, 2021 22:25

reviewer notes had been addressed

@alexmt alexmt merged commit d6f579b into argoproj:master Jun 2, 2021
@jannfis jannfis added the needs-verification PR requires pre-release verification label Jun 25, 2021
@jaredclarke jaredclarke deleted the add-change-password-ui branch July 7, 2021 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-verification PR requires pre-release verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants