Skip to content

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.

3 participants