Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Add option to unlock user accounts to site admin users menu#45650

Merged
pjlast merged 9 commits into
mainfrom
pjlast/40788-allow-admins-to-unlock-users
Dec 14, 2022
Merged

Add option to unlock user accounts to site admin users menu#45650
pjlast merged 9 commits into
mainfrom
pjlast/40788-allow-admins-to-unlock-users

Conversation

@pjlast

@pjlast pjlast commented Dec 14, 2022

Copy link
Copy Markdown
Contributor

Closes #40788

Adds an extra option to the user options in the Site Admin user management panel, allowing site admins to unlock user accounts after too many sign-in attempts.

I went with expanding the rest endpoints, since user lockouts are backed by a redis store that is created in that handler setup.

image

image

image

Test plan

Unit tests expanded.

@cla-bot cla-bot Bot added the cla-signed label Dec 14, 2022
@pjlast pjlast requested a review from a team December 14, 2022 08:56
@sg-e2e-regression-test-bob

sg-e2e-regression-test-bob commented Dec 14, 2022

Copy link
Copy Markdown

Bundle size report 📦

Initial size Total size Async size Modules
0.01% (+0.25 kb) 0.01% (+1.09 kb) 0.01% (+0.84 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits 644d767 and a641750 or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@mrnugget mrnugget left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very nice.

didn't know we had a REST API for this, TIL. I think keeping things consistent for now is a good idea, so adding a new endpoint here makes sense. But the property "isLocked" should probably go in the GraphQL API (see comment)

Comment thread cmd/frontend/internal/auth/userpasswd/handlers_test.go
"""
eventsCount: Float!
"""
Whether or not the user account is locked.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd add "Only visible to site-admins"

Comment on lines +68 to +69
time.Duration(lockoutOptions.LockoutPeriod)*time.Second,
time.Duration(lockoutOptions.ConsecutivePeriod)*time.Second,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Side-note: I'd expect this stuff to happen in a separate method in userpasswd somewhere, instead of in GraphQL layer.

Example: userpasswd.NewLockoutStoreFromConf(conf.AuthLockout()).

Comment thread cmd/frontend/graphqlbackend/user.go Outdated
Comment on lines +72 to +74
db: db, user: user, logger: log.Scoped("userResolver", "resolves a specific user").With(
log.Object("repo",
log.String("user", user.Username))),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

wow, that's not intendation, that's a massacre

Suggested change
db: db, user: user, logger: log.Scoped("userResolver", "resolves a specific user").With(
log.Object("repo",
log.String("user", user.Username))),
db: db,
user: user,
logger: log.Scoped("userResolver", "resolves a specific user").With(log.Object("repo", log.String("user", user.Username))),

Also: what is that "repo" doing here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did not change anything here, this was go fmt doing its thing 🤷

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, sometimes you need to help it and add linebreaks :P

@pjlast pjlast enabled auto-merge (squash) December 14, 2022 11:07
@pjlast pjlast merged commit c0a9470 into main Dec 14, 2022
@pjlast pjlast deleted the pjlast/40788-allow-admins-to-unlock-users branch December 14, 2022 11:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow admins to unlock users

3 participants