Change the PK field

[eagle-r]

Is your feature request related to a problem? Please describe.
The PK on the APIKey model is the string containing the prefix and hash. This makes it challenging to work with when referencing it in URLs in many REST-based frameworks. eg. 2TothlA4.pbkdf2_sha256$36000$dfPh6LIy7NB3$FTA61Pvuq3gres5ZsclXimNCRz70jvDRl/ygYOjEiLc=. This contains a slash and other special chars that would typically be URL encoded.

Describe the solution you'd like
Either convert the model to use a standard integer PK or provide some other means to reference it in such a way

Describe alternatives you've considered
I'm not sure, but could this be handled through the solution being considered for #34?

[florimondmanca]

Hey, thanks for this.

I’m trying to understand the use case here. Are you trying to build an API to manage API keys? This package wasn’t originally intended for this kind of use case — more for private cross-service identification — so this explains why the PK is not friendly to being passed in URLs.

If migrating to a good old integer PK can make this easier for you, I’m open to the idea, though we need to make sure this doesn’t introduce any security risks.

[eagle-r]

Thanks for getting back to me. Our use case is very similar to what you describe. We're using it to allow another service to consume our public API. The issue we have is in the administration of our keys. We don't use django admin in our system. Instead we have a React-based frontend that communicates with the backend via a different API secured with standard Django authentication. The issue we have is that to retrieve the relevant API Key data for the frontend (or mark it as revoked, etc), we need to identify it for the backend based on an ID because we're using a RESTful API. However, due to the characters turning up in the hash, its not recognised by the backend as a valid identifier that can be retrieved from the database. Does that make sense?

…

On Fri, 7 Jun 2019 at 18:51, Florimond Manca ***@***.***> wrote: Hey, thanks for this. I’m trying to understand the use case here. Are you trying to build an API to manage API keys? This package wasn’t originally intended for this kind of use case — more for private cross-service identification — so this explains why the PK is not friendly to being passed in URLs. If migrating to a good old integer PK can make this easier for you, I’m open to the idea, though we need to make sure this doesn’t introduce any security risks. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#40?email_source=notifications&email_token=ACAVC3ADV3JHKCZHQTLO7NDPZIOSNA5CNFSM4HVR7OS2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXFHXKI#issuecomment-499809193>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACAVC3BT7I5IFWIMG4LTKF3PZIOSNANCNFSM4HVR7OSQ> .

[florimondmanca]

Yes, makes sense!

Reading your comment, I actually think being able to manage API keys through REST endpoints secures behind authentication could be in the scope of this package. I suppose you built serializers and views for API keys to create the endpoint? Are they generic enough to be included built-in?

Anyway, as for the PK, I think you’ve got a fair point. We can create a data migration that moves the current PK (prefix + hash) to another field (actually probably two separate fields), and use a standard integer PK.

I’m a bit short on time at the moment (and also need to take a look at the abstract API key feature), so if you feel like giving this a shot, let me know!

[curtismorte]

@eagle-r,
It's a bad idea to pass an API Key in a URL. You're essentially exposing your API Key.

With your reference to #34, that's a solution. However, I think it's actually a poor fix for the actual problem.

@florimondmanca, we need to normalize the table to have a generic based integer PK. With the completion of #34, those who need to change the PK to some other form of ID, say a UUID, will have the flexibility to do so.

[curtismorte]

@eagle-r and @florimondmanca,
In my opinion, while this is technically not a bug and is more of a feature enhancement, the issue should be considered with the same level of priority as a bug.

Using a Model View Set requires a PK. The current PK we have is not a valid form of PK.

[florimondmanca]

Thanks for your input @curtismorte, and agreed on the fact that this fits the bug label better.

[eagle-r]

If I have time this weekend I'll have a go at submitting a PR.

@curtismorte I agree that exposing the key in the URL is a security risk.

@florimondmanca I assume that when you mentioned splitting the existing key into two fields you were talking about the prefix and hash being two separate fields? I actually feel this is even better than just replacing the PK by itself because it means that I can pass the prefix in the ModelVewSet if I need to without ever exposing the hash.

Thanks for all the input.

[florimondmanca]

Yes @eagle-r, I was referring to the prefix and hash. :-)

I think having the prefix used as a PK could make sense, but since it is tightly coupled to the generation/validation mechanism I would rather not have clients use it all over the place, at least for the sake of separation of concerns.

And storing the prefix separately while keeping the PK as is would induce data duplication.

So I think that to enable building CRUD on top of API keys, a simple integer PK (or anything else via customization) is enough. We don’t need to be fancy. :)

I agree that exposing the key in the URL is a security risk.

I don’t think you were exposing the API key, because it’s actual value can’t be retrieved from the prefix and hash.

The fact that we need to ask this question (can I use this PK in URLs?) is another sign that things need to change. :)