-
Notifications
You must be signed in to change notification settings - Fork 108
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
1041 allo endpoint v2 #240
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the permissions for the API key, here is what I think:
A. we can either include the permission flags (submit_passports
, ...) into the ApiKey model, and then we maintain that for each ApiKey individually
B. we can keep them in a separate model like it is now (APIKeyPermissions
), but then allow this to be re-used (so change the
permissions = models.OneToOneField(
"APIKeyPermissions", on_delete=models.CASCADE, null=True, blank=True
)
to a ForeignKey
My thinking is that this would allow us to set up different permission combinations that we might want to enable / disable at some point for users (similar to the rate limit tiers that we have). If we make one change to the permissions, this will automatically apply to all users.
I think solution B. might be better long-term. But then we are not there yet to make use of this.
So I would just stick to A. because it is simpler and (we have 1 less table, 1 less relation to follow). Ant maintenance of this in the admin is also more straight forward (which is important for support staff).
A. Wouldn't this be the same result but make queries to the ApiKey model more expensive? B. I thought about this, but it seems like this might be alittle confusing to maintain. For example what if we wanted to edit permissions for a single api key? By to editing permissions for one key you would be editing permissions for any that reference it. You could always create another permissions record, but that might not be initially obvious.
|
b7e3a7d
to
b2ac5f8
Compare
fixes: passportxyz/passport#1041