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 sudo mode for admins #8210

Merged
merged 12 commits into from
Mar 4, 2024
Merged

Add sudo mode for admins #8210

merged 12 commits into from
Mar 4, 2024

Conversation

LawnGnome
Copy link
Contributor

@LawnGnome LawnGnome commented Feb 29, 2024

This PR adds a concept of "sudo mode" for admins logged into crates.io. Actions that require admin privileges will be disabled by default unless the admin explicitly turns on admin actions from the user menu, at which point they will be given privileges for six hours or until they disable admin actions again from the user menu. Sudo mode is per-user, per-browser; it is not synced across multiple browser sessions, even for the same user.

These cases look like this:

image

image

In addition to this, a new PrivilegedAction component has been added that can gracefully handle rendering controls that can be used either by a regular user or an admin in sudo mode, including showing the control in a disabled mode when an admin is logged in but they have not enabled sudo mode. The disabled mode looks like this:

image

(I don't know why the tooltip wraps, but I don't really care much, honestly.)

Finally, the existing yank/unyank functionality has been wrapped in PrivilegedAction, both because it needs to be anyway, and because it demonstrates how this is used in practice.

This is likely best reviewed commit by commit. (In theory, each of these could be a PR unto themselves, but I don't see much point splitting this out without the motivating example of how it interacts with the yank button.)

Open questions:

  • Is six hours the appropriate amount of time for sudo mode to be enabled by default? (In theory, we could actually allow the admin to specify how long they want, but that seems unnecessarily complex.)
  • Is it OK that sudo mode is per-browser?
  • Do we want a visible indicator next to the user avatar that sudo mode is enabled? I've used a 🧙 emoji as a placeholder, but if we want this, we probably want an actual icon of some kind.
  • Should this just be called "sudo mode" in the UI as well?
  • How many lints have I accidentally broken because I can't figure out how to configure my local environment in the same way as CI expects? 😬

This implements step 1 of the plan laid out in #8049.

This introduces "sudo mode", whereby an admin can enter a privileged
mode for a certain period of time. The session tracks this in the
browser's local storage.
If the user is in sudo mode, the expiry time is also displayed in the
menu.
We probably actually want something different to be displayed, but it
does successfully indicate that something is different about the current
user session.
This component wraps controls that can be used by either an owning user
or by an admin. The `@userAuthorised` argument is used to indicate that
the current user is allowed to perform the action wrapped by the
component, while the admin status is extracted from the current session.

If an admin is logged in, but sudo mode is disabled, then a disabled
version of the control(s) will be shown by default. This can be
customised by specifying the `:placeholder` block.
@LawnGnome LawnGnome requested a review from a team February 29, 2024 02:25
@Turbo87 Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-frontend 🐹 labels Feb 29, 2024
app/components/header.js Outdated Show resolved Hide resolved
app/components/header.module.css Outdated Show resolved Hide resolved
app/components/header.module.css Outdated Show resolved Hide resolved
app/components/user-avatar.hbs Outdated Show resolved Hide resolved
app/components/user-avatar.hbs Outdated Show resolved Hide resolved
app/services/session.js Outdated Show resolved Hide resolved
app/components/privileged-action.js Outdated Show resolved Hide resolved
app/components/header.hbs Show resolved Hide resolved
@LawnGnome
Copy link
Contributor Author

I've pushed new commits in response to the review. (Thanks!)

To simplify re-review, I haven't rebased them into the previous set of commits, but (if approved) I'll reorder and squash them down as appropriate before merging.

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.56%. Comparing base (3044bbf) to head (78c0f77).
Report is 15 commits behind head on main.

❗ Current head 78c0f77 differs from pull request most recent head a5c3387. Consider uploading reports for the commit a5c3387 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8210   +/-   ##
=======================================
  Coverage   87.56%   87.56%           
=======================================
  Files         271      271           
  Lines       26983    26983           
=======================================
  Hits        23628    23628           
  Misses       3355     3355           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Turbo87 Turbo87 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 we should add for this change is an acceptance/application test that asserts that the full flow with the sudo mode is working correctly. This can be added in a follow-up PR though and this seems useful enough that I'll merge it without the test already :)

@Turbo87 Turbo87 merged commit fb16b46 into rust-lang:main Mar 4, 2024
7 checks passed
LawnGnome added a commit to LawnGnome/crates.io that referenced this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend 🐹 C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants