Skip to content
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

Allowed usage of custom AuthToken based on knox.AbstractAuthToken #275

Merged
merged 2 commits into from Aug 16, 2022
Merged

Allowed usage of custom AuthToken based on knox.AbstractAuthToken #275

merged 2 commits into from Aug 16, 2022

Conversation

Khalidm98
Copy link
Contributor

@Khalidm98 Khalidm98 commented Aug 6, 2022

Hi guys,
I really love this package and I regularly use it in projects that require token authentication.
I recently came across a situation in which I needed to support multiple profiles for the user in a multi-platform project. So, it is required to link the token with both the user profile and the platform client. Hence, I came up with this solution and I hope that it would be of good use to the package users.
Please, inform me if there is any unclear change and point out any change that needs further modifications.
Thanks.

@Khalidm98
Copy link
Contributor Author

Hi @belugame , @James1345

I can imagine how busy you must be, I also would really appreciate it if you could have a look on the PR and give feedback...

Thanks.

@belugame
Copy link
Collaborator

@Khalidm98 hey, sorry, but i m not using or maintaining this library since years. I think it is the time to make a fork and you being the new admin.

@Khalidm98
Copy link
Contributor Author

@belugame Thanks for your reply.
May be @James1345 could help, and I can also see that @johnraz is actively reviewing the PRs.
I am sure that you have a lot going on guys, and I don't mean to be pushy or anything. Of course if you would you can have a look whenever you are free.

But I think adding this feature will be of great help to many users of this package. And it is just a slight added feature, so in my opinion it should be added to your original repo not a fork of mine.

Here are some related issues and also PRs that I found related to this feature:
Issues: #159 #189 #196 #251 #263
PRs: #184 #252

Thanks guys...

@James1345
Copy link
Member

Hi @Khalidm98
Sorry for the slow response, It's try I don't have a huge amount of time for this project any more but I try to keep on top of the little requests.
This looks mostly good - can get_token_model be put somewhere better though? I don't like utils files generally - it suggests something isn't in the right place, or is trying to do too many things (there are some good reasons). I think this function would be fine to live in models though - it is a models method
If you can get that done I'll try to get a merge and check the releases work (which I've not checked in some time)

P.S. @belugame Is it possible to set you so people don't tag you any more? do you want me to do that?

@belugame
Copy link
Collaborator

@James1345 it would be good, but I don't think it is possible. People see that I did the releases/merges in the past, so they tag me :P

@Khalidm98
Copy link
Contributor Author

@James1345 thanks for your response, really appreciate it.
I have moved get_token_model() to models as you suggested, I also took the liberty to add the new TOKEN_MODEL settings in the docs.
Looking forward for it to be merged soon if everything is ok ✌️

@James1345 James1345 merged commit dc870df into jazzband:develop Aug 16, 2022
@RyanHope
Copy link

Can anyone provide an example of how to use this feature. I can't for the life of me get this working.

@Khalidm98
Copy link
Contributor Author

app_label.models:

from knox.models import AbstractAuthToken

class AuthToken(AbstractAuthToken):
    # add extra fields

settings:

KNOX_TOKEN_MODEL = "app_label.AuthToken"

@RyanHope
Copy link

RyanHope commented Nov 21, 2022

I have tried creating a new class based off that abstract class and I get errors like this:

ERRORS:
knox.AuthToken.user: (fields.E304) Reverse accessor for 'knox.AuthToken.user' clashes with reverse accessor for 'website.SecureToken.user'.
HINT: Add or change a related_name argument to the definition for 'knox.AuthToken.user' or 'website.SecureToken.user'.
knox.AuthToken.user: (fields.E305) Reverse query name for 'knox.AuthToken.user' clashes with reverse query name for 'website.SecureToken.user'.
HINT: Add or change a related_name argument to the definition for 'knox.AuthToken.user' or 'website.SecureToken.user'.

EDIT: These were errors from last night. Revisited things today and I got it working.

@Khalidm98
Copy link
Contributor Author

Glad it worked ✌️

@geemang2022
Copy link

@Khalidm98 I'm trying to use your changes (which are very cool BTW). Based on these docs: https://docs.djangoproject.com/en/4.2/topics/db/models/#abstract-related-name once we extend the AbstractAuthToken these clashes are expected. I see auth.py & view.py are using the auth_token_set accessor. We need a way to make this dynamic to use the correct accessor.

api.AuthToken.user: (fields.E304) Reverse accessor for 'api.AuthToken.user' clashes with reverse accessor for 'knox.AuthToken.user'. HINT: Add or change a related_name argument to the definition for 'api.AuthToken.user' or 'knox.AuthToken.user'. api.AuthToken.user: (fields.E305) Reverse query name for 'api.AuthToken.user' clashes with reverse query name for 'knox.AuthToken.user'. HINT: Add or change a related_name argument to the definition for 'api.AuthToken.user' or 'knox.AuthToken.user'. knox.AuthToken.user: (fields.E304) Reverse accessor for 'knox.AuthToken.user' clashes with reverse accessor for 'api.AuthToken.user'. HINT: Add or change a related_name argument to the definition for 'knox.AuthToken.user' or 'api.AuthToken.user'. knox.AuthToken.user: (fields.E305) Reverse query name for 'knox.AuthToken.user' clashes with reverse query name for 'api.AuthToken.user'. HINT: Add or change a related_name argument to the definition for 'knox.AuthToken.user' or 'api.AuthToken.user'.

@Khalidm98
Copy link
Contributor Author

Khalidm98 commented Jun 16, 2023

@geemang2022 thanks for your feedback,
I think this is same issue that @RyanHope had, you can fix this by not adding knox in settings.INSTALLED_APPS.
Once we extend the AbstractAuthToken model inside a local django project app, we should register this app and remove knox from installed apps.
registering knox will add the AuthToken model from knox models, which will not be used in that case.

@hosseinmp76
Copy link

any plan for a new realse which contains this?

@last-ikun
Copy link

Here is my solution for adding extra fields: Define a new Model that has a OneToOneField to the original AuthToken model, and add your extra fields in this new model:

class AuthTokenExtras(models.Model):
  token = models.OneToOneField(AuthToken, on_delete=models.CASCADE)
  # define your extra fields below:
  ...
  ...

you can look up these extra fields given a token

dontexit pushed a commit to dontexit/django-rest-knox that referenced this pull request Jan 24, 2024
…zzband#275)

* Allowed usage of custom AuthToken based on knox.AbstractAuthToken

* moved get_token_model() to models, updated the docs

Co-authored-by: Khalidm98 <khalid.refaat@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants