osu-tournament-rating / otr-api

API powering osu! Tournament Rating
https://otr.stagec.xyz/
4 stars 2 forks source link

Hash OAuthClient secrets #317

Closed myssto closed 1 month ago

myssto commented 1 month ago

Instead of client secrets being stored in the database as plaintext, they are now hashed and validated using Identity Framework's IPasswordHasher<T>. This pull includes some light refactoring of the OAuthHandler, as I feel delegating the creation and updating of clients to the client service and repository is more proper.

Now that we do not have access to the plaintext client secrets, the secret for a client will only ever be returned once when a client is created. If a user loses access to the secret, they will have to reset it.

/me should have client access and manipulation endpoints, but I will implement that in a new pull. I don't believe web has implemented features regarding clients at all, so this should be safe to push through.

Deployment Note: oauth_clients table needs to be truncated. All existing clients should be re-created (or secrets reset) as they will no longer be able to be authorized after this pull.

New Endpoints

Breaking Changes

Schema Changes

Closes #146

hburn7 commented 1 month ago

I realize we can easily store an encryption key secret and be able to keep the same flow (e.g. user can view their secret as many times as they want). We can reference the secret and securely decrypt the secret before returning it.

I think this is a good way forward, what do you think?

myssto commented 1 month ago

I realize we can easily store an encryption key secret and be able to keep the same flow (e.g. user can view their secret as many times as they want). We can reference the secret and securely decrypt the secret before returning it.

I think this is a good way forward, what do you think?

Hard disagree... Encryption sucks, and hashing is the industry standard for password-like authentication. If we were to go the encryption route, we'd have to consider key rotation and a million other things. I'd give this SO answer and the linked material a read, it gives a good bit of insight into what I'm talking about. This is simple, secure, works with minimal effort, and realistically if a user has to reset their secret it would not be effecting much. Shouldn't be misplacing it in the first place :P

hburn7 commented 1 month ago

Alright, that's fine then. We can stick to hashing.