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

Rename blacklist #74

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

FinnLawrence
Copy link

Following the conventions set in Rails since 2018, replacing the use of the term 'blacklist'. I've proposed using the word 'revoke' ('revoked_tokens', 'revocation') to replace it in the context of this gem, since it's applicable to the effects on the token.

  • Replaced all instances of 'blacklist' with 'revoke' and so on.
  • Renamed files using the term.

@Gokul595 this should work from here for new adopters of the gem (such as myself), but obviously this will create conflicts for those already using it since they will already have a database table named blacklisted_tokens. Off the top of my head the Readme could be expanded to suggest:

  • Changing their configuration to point to the old blacklisted_tokens table.
  • Adding a migration to rename their blacklisted_tokens table to the new name.

Or perhaps we could add a fallback in the code where if the revoked_tokens table doesn't exist, it would look for the blacklisted_tokens table? This would make the upgrade transparent for existing users, and the Readme could note this

Do you have any thoughts on an approach for this (assuming you'd like to merge this in)?

@Gokul595
Copy link
Owner

@FinnLawrence Thanks for the PR. Allow me sometime to think about this change and the way we should handle the existing blacklist tables.

@Gokul595
Copy link
Owner

@FinnLawrence Changes looks good. It's better to support blacklisted_tokens related config and association after this change. We can remove the support in later versions.

I think below changes should be sufficient:

Config:

  • Create alias in lib/api_guard.rb file for blacklist_token_after_refreshing to use revoke_token_after_refreshing value

Association

  • api_guard_associations blacklisted_token: 'blacklisted_tokens' should assign the association to revoked_token attribute
  • blacklisted_token should use revoked_token value

Let me know if you need any help in doing this change.

@berkos
Copy link

berkos commented Feb 10, 2024

Great suggestion @FinnLawrence! It would be great if we can get this. 🙏

@FinnLawrence
Copy link
Author

FinnLawrence commented Feb 22, 2024

Hey @Gokul595 perhaps you could make those last few changes and merge in?
I'm not super sure on how to do the assigning of the associations sorry.

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

3 participants