Write encrypted tags permission #486
Conversation
- Addition of canWriteEncryptedTags - Renaming of canSeePasswords to canSeeEncryptedTags Code PR: #486
| @@ -18,6 +18,7 @@ features { | |||
| useWhitelistOnRepurpose = true | |||
| keepSomeMetaOnRepurpose = [ attr1, attr2 ] | |||
| deleteSomeMetaOnRepurpose = [ attr4, attr3 ] | |||
| syslogAsset = tumblrtag1 | |||
There was a problem hiding this comment.
This isn't directly related to this right? Just general clean up?
There was a problem hiding this comment.
Well, I needed this to be able to see errors I was getting. so it is technically general cleanup, but since it was a one liner, I decided to sneak it in.
| @@ -20,7 +20,29 @@ abstract class User(val username: String, val password: String) { | |||
| case false => None | |||
| } | |||
| def isEmpty(): Boolean = username.isEmpty && password.isEmpty && roles.isEmpty | |||
| def canSeePasswords(): Boolean = Permissions.please(this, Permissions.Feature.CanSeePasswords) | |||
| def canSeeEncryptedTags(): Boolean = { | |||
| // want to rename canSeePasswords to canSeeEncryptedTags | |||
There was a problem hiding this comment.
Picky, but this comment is a bit hard to decipher at first glance. Maybe something like
// We want to rename canSeePasswords to canSeeEncryptedTags but want to keep
// backward compatibility. So we use canSeeEncryptedTags if it is set, else fall
// back to canSeePasswords.
//
// Permissions default to true, so we also need to test if the permission is
// configured, and only use it if it is.
Maintain backwards compatibility with feature.canSeePasswords though Also, make this permission default to false if it is not set
william-richard
force-pushed
the
will-write-encrypted-tags-permission
branch
from
95d6938 to
88dface
Compare
|
Nice, LGTM. This should include some messaging in future release notes about backwards compatibility (or lack thereof) and provide a clear upgrade path. |
| @@ -123,6 +125,13 @@ class AssetLifecycle(user: Option[User], tattler: Tattler) { | |||
| opts.find(kv => restricted(kv._1)).map(kv => | |||
| return Left(new Exception("Attribute %s is restricted".format(kv._1)))) | |||
| } | |||
|
|
|||
| opts.find(kv => Feature.encryptedTags.contains(kv._1)).map(kv => | |||
There was a problem hiding this comment.
map really seems like the wrong since you don't care about the results only the side effect. perhaps foreach would more closely express what is happening
You might even put the writable and restricted tests into a single method since you use it in a few places
I realized while doing some work on encrypted tags that, while we restrict which users are able to read encrypted tags, we do not restrict which users can write to them.
Depending on how you have built your automation around collins, this could have very bad consequences.
For example, if you automatically make server passwords match the password set in collins, using a configuration management tool that runs regularly like puppet or chef, an attacker that could not read the password could set it to a value they know, and then gain access or elevated permissions to your server.
This PR aims to fix this vulnerability by adding a
canWriteEncryptedTagspermission, that restricts which users are able to write encrypted tags.It also renames
canSeePasswordstocanSeeEncryptedTagsto better reflect what the permission does - you may have reasons to encrypt non-password attributes. It does this in a backwards compatible manner, so your existing permissions configs that usecanSeePasswordsshould continue to function.@tumblr/collins RFR