Re: [PATCH] ddns.cgi: Drop static provider list for token based auth.

[Stefan Schantl <stefan.schantl@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 6664 bytes --]

Hi,
> Hi,
> 
> Alternatives I could come up with are:
> 
> * Create a command to have ddns list all those providers. That way
> that list is in ddns where it probably belongs best.

This probably would be the best solution.

So I'm thinking about to introduce something like "supports_token_auth
= True" similar to "can_remove_records = True" in the source file which
handles the providers.

After that, adding a new command option, which outputs the providers
with this flag similar to the "ddns list-providers" option.

In this case we will get a dynamically generated list of what we want
and need. 

> 
> * Update the UI and add a third field for the token. That is however
> very bad UI because either the token field or the username/password
> field are a waste of space.

I think a checkbox for "Use token based auth" or a dropdown for
choosing the Auth type and some nice JS code could avoid from this.

> 
> Actually, we need to work towards having token only. Anything else is
> just dangerous because the whole account can be managed if this data
> leaks.

You are true, but sadly this cannot be done by us - it'ss the job of
the providers out there.

- Stefan

> 
> -Michael
> 
> > On 2 Dec 2020, at 16:02, Stefan Schantl <stefan.schantl(a)ipfire.org>
> > wrote:
> > 
> > Mhm,
> > 
> > you are right this will only move the problem from the developers
> > (us)
> > to the users.
> > 
> > This is a really hard problem because there is no easy solution for
> > this issue.
> > 
> > We simple can take care about the users input and verify if the
> > providers supports what has been configured or we even do not.
> > 
> > If we decide to safe the users from themself, we need something to
> > verify against and that would be something like such a crappy
> > static
> > list.
> > 
> > Do I miss something or think to narrow?
> > 
> > @List followers, what are your opinions?
> > 
> > Best regards,
> > 
> > -Stefan
> > > You are moving the problem from the developer to the user.
> > > 
> > > I think that might be more messy.
> > > 
> > > I am also not sure how this will affect existing setups?
> > > 
> > > -Michael
> > > 
> > > > On 2 Dec 2020, at 15:35, Stefan Schantl <
> > > > stefan.schantl(a)ipfire.org>
> > > > wrote:
> > > > 
> > > > Hello Michael,
> > > > 
> > > > the intension of this patch was, that a supported provider of
> > > > DDNS
> > > > has
> > > > changed it's API to support now token based authentication. The
> > > > code
> > > > already has been changed so here we are safe by now.
> > > > 
> > > > Also in case a new dynamic DNS provider will be added to DDNS,
> > > > we
> > > > do
> > > > not have to do anything, because the GUI automatically will be
> > > > filled
> > > > with the new provider.
> > > > 
> > > > But there is one big exception in this:
> > > > 
> > > > If a new or existing provider has supports authentication
> > > > tokens,
> > > > this
> > > > provider has to be added to this list, which is not very
> > > > intuitive.
> > > > 
> > > > We also have to ship the GUI each time for new, if this
> > > > happened.
> > > > 
> > > > (This was the former reason, why we created the "ddns list-
> > > > providers"
> > > > command to prevent from this.)
> > > > 
> > > > My change simplifies, the check if token based authentication
> > > > should be
> > > > used or not. It removes the check against the static list of
> > > > providers
> > > > which supports token based authentications. (This is the list,
> > > > which
> > > > needs to be keep up to date by hand.)
> > > > 
> > > > For the user nothing changes, because if he speciefies "token"
> > > > as
> > > > username, still the token base auth will be used for the
> > > > choosen
> > > > provider.
> > > > 
> > > > The only way an user may be affected is, if he configures a
> > > > token
> > > > based
> > > > auth for a provider which does not supports it. This now would
> > > > be
> > > > possible and will result in an authentication error against the
> > > > provider - But Hey, if he fills in incorrect data the same will
> > > > be
> > > > happen.
> > > > 
> > > > The only bigger problem would be if the valid username of a
> > > > user is
> > > > "token". In this case he will be affected in the same way and
> > > > this
> > > > could not be fixed in an easy way.
> > > > 
> > > > IMHO, the benefits are bigger for us, because the maintain work
> > > > will
> > > > become much easier and one stupid error source will get
> > > > eliminated.
> > > > 
> > > > Best regards,
> > > > 
> > > > -Stefan 
> > > > > Hello,
> > > > > 
> > > > > I do not understand exactly what this patch is trying to
> > > > > achieve.
> > > > > 
> > > > > Unfortunately we have no choice than doing it this way with
> > > > > the
> > > > > current UI.
> > > > > 
> > > > > I do not think it is worth altering the UI for this, and I do
> > > > > not
> > > > > know how we could do it without having a list again?
> > > > > 
> > > > > -Michael
> > > > > 
> > > > > > On 2 Dec 2020, at 11:30, Stefan Schantl <
> > > > > > stefan.schantl(a)ipfire.org>
> > > > > > wrote:
> > > > > > 
> > > > > > This is really hard to maintain when adding new or altering
> > > > > > existing
> > > > > > providers.
> > > > > > 
> > > > > > Reference #12415.
> > > > > > 
> > > > > > Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
> > > > > > ---
> > > > > > html/cgi-bin/ddns.cgi | 8 ++++----
> > > > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/html/cgi-bin/ddns.cgi b/html/cgi-bin/ddns.cgi
> > > > > > index 715c37290..024eaf7f6 100644
> > > > > > --- a/html/cgi-bin/ddns.cgi
> > > > > > +++ b/html/cgi-bin/ddns.cgi
> > > > > > @@ -665,13 +665,13 @@ sub GenerateDDNSConfigFile {
> > > > > > 
> > > > > > 		my $use_token = 0;
> > > > > > 
> > > > > > -		# Handle token based auth for various
> > > > > > providers.
> > > > > > -		if ($provider ~~ ["dns.lightningwirelabs.com",
> > > > > > "entrydns.net", "regfish.com",
> > > > > > -				  "spdns.de", "zzzz.io"] &&
> > > > > > $username
> > > > > > eq "token") {
> > > > > > +		# Check if token based auth is configured.
> > > > > > +		if ($username eq "token") {
> > > > > > 			$use_token = 1;
> > > > > > +		}
> > > > > > 
> > > > > > 		# Handle token auth for freedns.afraid.org and
> > > > > > regfish.com.
> > > > > > -		} elsif ($provider ~~ ["freedns.afraid.org",
> > > > > > "regfish.com"] && $password eq "") {
> > > > > > +		if ($provider ~~ ["freedns.afraid.org",
> > > > > > "regfish.com"]
> > > > > > && $password eq "") {
> > > > > > 			$use_token = 1;
> > > > > > 			$password = $username;
> > > > > > 
> > > > > > -- 
> > > > > > 2.20.1
> > > > > >