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

Allow TOTP keys to be filterable #545

Closed
wants to merge 1 commit into from
Closed

Conversation

pkevan
Copy link
Contributor

@pkevan pkevan commented Mar 21, 2023

What?

Allow TOTP keys to be filtered on save/retrieval.

Why?

Since the discussion in #389 seems to be on-going, the following should allow users of the plugin to decide how they would like to approach the encryption part.

How?

Adds filters on save and retrieval.

Testing Instructions

Test adding filters, and making sure they are saved.

Changelog Entry

Added - filters to TOTP keys.

Since the discussion in WordPress#389 seems to be on-going, the following should allow users of the plugin to decide how they would like to approach the encryption part.

Perhaps we can follow up with solutions after they've been implemented and debate further?
@jeffpaul jeffpaul added this to the 0.9.0 milestone Mar 21, 2023
@iandunn iandunn requested a review from dd32 March 21, 2023 18:29
Copy link
Member

@dd32 dd32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm not against this change, I don't personally see the direct need for it.

I understand the intention of why, but it seems to me that this isn't something that needs to be filtered except in rare circumstances, and is working around other limitations.

A plugin can add a second provider, which overloads those methods and saves them elsewhere. What is not be filterable is being able to tell two-factor that For the provider "Two_Factor_Totp" use the classname "CUSTOMPLUGIN_Factor_Totp" as the plugin is hard-coded with the notion that provider_name === php_classname, so I'd probably be saying the filter needs to be on Two_Factor_Core:: get_providers()'s resultset or in it's loop that creates the provider objects.

(Created #546 for this, which I don't think necessarily supersedes this, but rather, offers a much more flexible future approach for dealing with individual provider customizations)

These filters as-is are even a bit limiting, the set_user_totp_key filter can't be used to save it outside of user_meta for example.

There's also the generic metadata filters, which admittedly probably shouldn't be used to filter this value if you're using it for encryption purposes.

@pkevan
Copy link
Contributor Author

pkevan commented Mar 29, 2023

Closing in favour of the more flexible approach in #546.

@pkevan pkevan closed this Mar 29, 2023
@pkevan pkevan deleted the patch-1 branch March 29, 2023 10:14
@jeffpaul jeffpaul removed this from the 0.9.0 milestone Apr 4, 2023
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