Skip to content

Audit trail for more owner actions #1548

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

Open
5 of 8 tasks
carols10cents opened this issue Nov 1, 2018 · 4 comments
Open
5 of 8 tasks

Audit trail for more owner actions #1548

carols10cents opened this issue Nov 1, 2018 · 4 comments
Labels
A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works E-has-mentor E-help-wanted

Comments

@carols10cents
Copy link
Member

carols10cents commented Nov 1, 2018

Further steps beyond #1478. These changes wouldn't be exposed in the UI but would be in the database for us to use in determining who took what actions when.

  • Add a column to the api_tokens table named revoked that's a BOOLEAN NOT NULL DEFAULT 'f'. Then instead of deleting tokens, mark them as revoked.

  • Create a migration (using diesel migration generate, as explained in this guide) that adds a new table named version_owner_actions with the following columns:

    • id SERIAL PRIMARY KEY
    • version_id FOREIGN KEY REFERENCES versions (id)
    • owner_id FOREIGN KEY REFERENCES users (id)
    • owner_token_id FOREIGN KEY REFERENCES api_tokens (id)
    • action INTEGER NOT NULL (that would map to a Rust enum with variants publish, yank, unyank)
    • time TIMESTAMP NOT NULL DEFAULT now()
  • Keep the headers[0] authorization header value accessible by holding onto it in the AuthenticationSource::ApiToken variant

  • Within the crate publish transaction, after the new version record is created, create a new version owner action record with action = "publish", version_id = version.id, owner_id = user.id, owner_token = req.authentication_source() (and then extract the
    token value)

  • Make similar changes to add records to the activity table in yank and unyank

  • Add a field to EncodableVersion that's a Vec of all the actions, owner ids, and times that this version has had an action taken on it so that this info, minus the api token value, is returned in the API response

  • Add a table crate_owners_actions that records who adds and removes other owners from a crate

  • Add tests that this information is being recorded and returned as expected

@hinchley2018
Copy link

hinchley2018 commented Dec 3, 2018

@carols10cents This sounds like a fun issue to sink my teeth into.
Would you mind if I pick this up?
Is there a deadline for completing this? (might take me a few weeks)

@carols10cents
Copy link
Member Author

I don't mind at all, and no, there's no deadline!

bors-voyager bot added a commit that referenced this issue Dec 3, 2018
1567: Mark API tokens as revoked r=carols10cents a=joshleeb

With this PR, when a user deletes an API token, rather than deleting it from the database, it will instead be marked as `revoked`.

That means that this PR also adds a migration to add the column `revoked` to the `api_tokens` table.

Ref. #1548 (Task 1)

Co-authored-by: Josh Leeb-du Toit <[email protected]>
@carols10cents carols10cents changed the title Audit trail for more actions Audit trail for more owner actions Feb 14, 2019
bors added a commit that referenced this issue Oct 4, 2019
Hold onto authorization header with ApiToken source

Modify the `AuthenticationSource::ApiToken` variant to hold onto the authorization header that may be used to fetch the current user.

Ref. #1548 (Task 4)
bors added a commit that referenced this issue Oct 5, 2019
Hold onto authorization header with ApiToken source

Modify the `AuthenticationSource::ApiToken` variant to hold onto the authorization header that may be used to fetch the current user.

Ref. #1548 (Task 4)
bors added a commit that referenced this issue Oct 6, 2019
Add migration and model for version_owner_actions table

Add a migration to create the `version_owner_actions` table, as well as a `VersionOwnerAction` struct in `models/actions.rs`.

Ref. #1548 (Task 2)
bors added a commit that referenced this issue Dec 2, 2019
Add audit trail to the publish, yank and unyank transactions.

Contributes to [Issue 1548](#1548)
Based on the commits from [PR 1604](#1604)
markcatley added a commit to markcatley/crates.io that referenced this issue Dec 13, 2019
markcatley added a commit to markcatley/crates.io that referenced this issue Dec 15, 2019
Including the following audit actions:
- Invite User
- Remove User

Closes rust-lang#1548
markcatley added a commit to markcatley/crates.io that referenced this issue Dec 18, 2019
markcatley added a commit to markcatley/crates.io that referenced this issue Dec 18, 2019
Including the following audit actions:
- Invite User
- Remove User

Closes rust-lang#1548
bors added a commit that referenced this issue Dec 19, 2019
…10cents

Add Verions Owner Actions to the API

Contributes to Issue #1548
markcatley added a commit to markcatley/crates.io that referenced this issue Dec 20, 2019
Including the following audit actions:
- Invite User
- Remove User

Closes rust-lang#1548
markcatley added a commit to markcatley/crates.io that referenced this issue Jan 2, 2020
Including the following audit actions:
- Invite User
- Remove User

Closes rust-lang#1548
@Turbo87 Turbo87 added the C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works label Feb 11, 2021
@carols10cents
Copy link
Member Author

There's some discussion that happened in #2025 that's relevant to implementation:

I was imagining information like "user x added user y as an owner at datetime" being available publicly so that anyone could audit the risk of activities being malicious or not.

I feel like knowing when an invitation is sent and when an invitation is rejected are not useful pieces of information; it's only when an invitation is accepted and a crate has a new owner that it's interesting. What do you think?

we do need to think about cases when a person might want to remove all association they have with a crate, past and present. So user_a, who was owner of crate_b at some point, now wants nothing to do with crate_b and doesn't want crates.io to show that they were ever owner of crate_b.

@carols10cents
Copy link
Member Author

Just had a thought-- because we sometimes use emails sent from the same address as the verified email address in crates.io, it might be useful to know if an account's verified email address has been changed recently.

Sometimes we say "hey can you send us an email from the verified address" and for whatever reason the person needs to change the verified address to something else and that's totally legitimate. But if it's been set to [email protected] for years, then it gets set to [email protected] and then we get a request to do something with a crate, that might be a sign that something suspicious is going on.

Right now, I think we just overwrite the old verified email address if it's changed; it would be nice to keep those historical records around marked as old/invalid/superseded but viewable in the database and/or admin web UI someday.

@Turbo87 Turbo87 removed the E-medium label Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works E-has-mentor E-help-wanted
Projects
None yet
Development

No branches or pull requests

3 participants