Add feature permission revoke for profiles#549
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
| id UUID PRIMARY KEY, | ||
| profile_id UUID NOT NULL REFERENCES profile(id) ON DELETE CASCADE, | ||
| feature VARCHAR(255) NOT NULL, | ||
| state permission_state NOT NULL, |
There was a problem hiding this comment.
this could just be a boolean?
There was a problem hiding this comment.
I prefer using an enum over a boolean, as it offers greater extensibility and simplifies future migrations.
|
|
||
| #[derive(Serialize, Deserialize, ToSchema, Clone, ToResponse)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum PermissionState { |
There was a problem hiding this comment.
this can also be a boolean? unless you anticipate this having more states soon?
| if let Some(permission) = permissions.get(&(profile_id, feature.clone())) { | ||
| Ok(permission.state == models::PermissionState::Enabled) | ||
| } else { | ||
| Ok(true) |
There was a problem hiding this comment.
i think cancel quote shouldn't be allowed if a permission hasn't been created? aka this should be whitelist, not blacklist
There was a problem hiding this comment.
in other words, you need to have Permission enabled in order to cancel quote. if permission is disabled, this fails. if the permission doesn't exist for this profile_id, it fails as well
There was a problem hiding this comment.
Why do you think whitelist is better than blacklist :-?
There was a problem hiding this comment.
i think cancellation should be a privileged action because it has costs on the system. if anyone abuses it, then that can cause bad impacts on downstream consumers. thus, it should be enabled for trusted parties and not for any anonymous integrator
There was a problem hiding this comment.
Yeah I agree with your point. For this reason the api is behind the authentication module. It means you can only access the API with API KEY. So by default it's enabled if you have API KEY. If you have API KEY and abuse the feature, then we'll revoke your access by adding a disabled row to your profile Id Privilege model.
There was a problem hiding this comment.
you're right that it is behind the authentication guard. but if an API key passes the authentication guard but doesn't have the Privilege, as it stands right now the cancellation will be allowed. I think that the default behavior should be that it is not allowed.
for example, right now, if there is an API key for a random entity that isn't a searcher, then that entity can make a bid and then cancel it.
There was a problem hiding this comment.
I'm still not fully convinced this is better. Using an API key is already a form of whitelisting, right? We only generate keys for trusted partners, and if any feature is misused, we can just revoke the privilege of that feature.
Introducing an additional layer feels like double whitelisting, which seems unnecessary to me.
Also, I’m not too keen on the idea of having to create a migration every time we add a new feature behind the privileged module—just to grant access to all existing profiles or API keys.
m30m
left a comment
m30m
left a comment
There was a problem hiding this comment.
I think this adds a double meaning to the term permission into the rep. Maybe use something like Privilege?
| feature VARCHAR(255) NOT NULL, | ||
| state permission_state NOT NULL, | ||
| created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP |
There was a problem hiding this comment.
what's the use of this column? As far as I understood, rows will not get updated.
There was a problem hiding this comment.
It's a normal practice. Here it may not be necessary but I prefer to have it in all tables.
3 participants
This PR adds the ability to revoke a specific feature permission for a given profile.