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

Image support for FreeOTP #52

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jmoleiro
Copy link

Feature added

Added Image support for FreeOPT as explained on this StackOverflow answer: https://stackoverflow.com/a/50410063

Feature test

Feature tested on FreeOTP and works as expected. Also tested on Microsoft Authenticator, Authy and Google Authenticator to check that still works on authenticators that do not suport the image parameter.

Test were made on Android versions only

Copy link
Owner

@RobThree RobThree left a comment

Choose a reason for hiding this comment

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

@willpower232 What do you think? Do we want a (from what I can gather) vendor-specific "extension" in this library? Especially since this will also muddy the waters with #56

I'm not strictly against, just also not a real fan I guess...

@willpower232
Copy link
Collaborator

This seems to be an application-specific deviation from the official spec so I'd vote against including it here, pending any official change to the spec.

The implementation of #56 provides the capability to the individual developers application without affecting the core of the library so is less responsibility to the library as a whole than this change.

@RobThree
Copy link
Owner

RobThree commented Mar 9, 2021

An option would be to refactor out something like an IUriGenerator or something with a default implementation that could be extended by inheritance or even completely replaced with another implementation. But, to be totally honest, I think that's waaaay overengineering a solution. OTOH I do understand the desire for a logo/image. I'm on the fence about this one.

Thing is; even if the extra parameter is added, it doesn't break any other authenticators (does it? - we'll need to verify this at least for the top 5 or so most popular ones). And as long as no image is set the parameter won't be added to the url. So I also don't see any harm other than 'promoting' use of non-standard extensions. Then again, that's what made the internet the internet; if everyone had stuck with the spec we'd still be in the early 2000's by web-standards...

For now I vote to be on the conservative reserved side and keep it out. For now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants