583776 - The ISPDB should output v1.1 format files, to ease reviewer burden.

Status: RESOLVED FIXED

(Webtools :: ISPDB Server, defect, P1)

Description

[Blake Winton (:bwinton) (:☕️)]

Uh, I think the summary says it all.

Comment 1

[Ludovic Hirlimann [:Usul]]

Blake can you ping some of the students ?

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Uh, which students?

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to add v1.1 export to the ISPDB.

Sample XML file coming, but you should check that the RelaxNG schema validates what you think it should.

Thanks,
Blake.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Attachment: A sample XML file.

And here's the sample xml file.

Comment 5

[Ben Bucksch (:BenB)]

Thanks, Blake, for tackling this!

sample XML:

<username/>

That's wrong and one of the annoying things.
The schema should validate this, too.

Comment 6

[Ben Bucksch (:BenB)]

relaxng (as far as I can read it):

        <zeroOrMore>
          <element name="incomingServer">

should be <oneOrMore>

     <element name="outgoingServer">

That's also <oneOrMore> (we have configs with both SSL and STARTTLS smtp server configs)

What is <data type="ID"> as datatype? Should we use it for <hostname>, too, if it works for id=? Would it allow %?
Also, is there no datatype for URLs? A bunch of our attributes are URLs.

            <element name="authentication">
              <choice>
                <value/>
                <value>password-cleartext</value>

empty auth is not valid.

OTOH, a number of other values are valid, see <https://wiki.mozilla.org/Thunderbird:Autoconfiguration:ConfigFileFormat>

outgoing <port> should be unsignedInt, not text

<enable> is also <zeroOrMore>, not just one <optional>.

<descr> of <documentation>, as well as <instruction> of <enable>, can both have a lang= attribute, and therefore also appear several times <zeroOrMore>.

Why did you use a <define> for <username>, but not the rest? I'd remove the <define>.

Comment 7

[Ben Bucksch (:BenB)]

Additionally, a number of elements can appear. They are not used by TB3.1 yet, but are part of the v1.1 data format:
<inputField>, <restriction> (on <outgoingServer>), <checkInterval>, <clientConfigUpdate> are the important ones.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #5)
> <username/>
> That's wrong and one of the annoying things.
> The schema should validate this, too.

So we require usernames to have at least one character?

(In reply to comment #6)
>         <zeroOrMore>
>           <element name="incomingServer">
> should be <oneOrMore>
>      <element name="outgoingServer">
> That's also <oneOrMore> (we have configs with both SSL and STARTTLS smtp
> server configs)

So, I was wondering about that.  Couldn't we have sending-only or incoming-only servers?

> What is <data type="ID"> as datatype? Should we use it for <hostname>, too, if
> it works for id=? Would it allow %?

ID means that there can only be one attribute with this value in the document.  (It's like id's in HTML or XUL.)

> Also, is there no datatype for URLs?
> A bunch of our attributes are URLs.

Fixed.

>             <element name="authentication">
>               <choice>
>                 <value/>
> empty auth is not valid.

Fixed.

> OTOH, a number of other values are valid, see
> <https://wiki.mozilla.org/Thunderbird:Autoconfiguration:ConfigFileFormat>

Added.

> <enable> is also <zeroOrMore>, not just one <optional>.

Fixed.

> <descr> of <documentation>, as well as <instruction> of <enable>, can both have
> a lang= attribute, and therefore also appear several times <zeroOrMore>.

Fixed, although I don't ensure that there is only one per lang.

> Why did you use a <define> for <username>, but not the rest? I'd remove the
> <define>.

I've gone the other way, and refactored the common stuff into defines, precisely so that I can avoid this error:
> outgoing <port> should be unsignedInt, not text
:)

(In reply to comment #7)
> Additionally, a number of elements can appear. They are not used by TB3.1 yet,
> but are part of the v1.1 data format:
> <inputField>, <restriction> (on <outgoingServer>), <checkInterval>,
> <clientConfigUpdate> are the important ones.

So, I haven't added them, because we don't use them yet, but if you want to send me a file that has them in, I guess I could add them.

In the meantime, I've got another patch waiting for your answers to the above questions.

Later,
Blake.

Comment 9

[Ben Bucksch (:BenB)]

> So we require usernames to have at least one character?

Yes, an empty username is no username and invalid.

(There is one ISP, T-Online, which allows empty usernames and passwords on one POP server and authenticates via IP-Address and PPP implicitly, but knowing that a lot of software doesn't like an empty username, they also allow anything (including "any") as username and it works just as well, and they document that, too.)

In any case, that <username/> is the one error that annoyed me most in the configs exported from the ISPDB. I think that's just an invalid config.

I would enforce placeholders, and in the ISPDB we should do that, but for the config file in general, it's OK if e.g. the autoconfig server of the ISP generates a config file specifically for this user, with a literal username "bbucksch", given the email address.

> So, I was wondering about that.  Couldn't we have sending-only or
> incoming-only servers?

I don't consider that a whole config, you'd depend on 2 separate independent providers, one for incoming one for outgoing, and I think that's a real fringe case. The user has to do stuff manually anyways in this case, so I'd say that's not a case for autoconfig, but manual config. All ISPs provide both incoming and outgoing mail servers, because you can't participate in an exchange without both.

Only exception would be NNTP, but you don't allow that in the config yet anyway.

> ID means that there can only be one attribute with this value in the document.

Ah, thanks. I don't think that's true here, though.

> In the meantime, I've got another patch waiting

Thank you, will review later.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with Ben's comments.

(In reply to comment #9)
> > So we require usernames to have at least one character?
> Yes, an empty username is no username and invalid.

Fixed.

> > So, I was wondering about that.  Couldn't we have sending-only or
> > incoming-only servers?
> I don't consider that a whole config

Fixed.

> > ID means that there can only be one attribute with this value in the document.
> Ah, thanks. I don't think that's true here, though.

Well, since we've only got one element with an id, we should be safe.  ;)
But I've changed it to match "domain", since ideally its value should be one of the domains.

> > In the meantime, I've got another patch waiting
> Thank you, will review later.

Thanks!  Here's the patch, for when you're ready.

Later,
Blake.

Comment 11

[Ben Bucksch (:BenB)]

Comment on attachment 462184 [details]
A sample XML file.

> <username/>

Still broken

Comment 12

[Ben Bucksch (:BenB)]

- rename relaxng_schema.xml to relaxng_schema.1.0.xml, maybe
- we have a lot of values defined as <text/>. Is that non-empty text?
  If not, it probably should be, see e.g. all the empty IDs we get from
  old ISPDB server.
  (although that makes the schema much less nice :-( )
- why are socketType and username not directly in common-elements?
- GSSAPI and NTLM should be in common-auth-types.
  Same for none and client-IP-address, although they are most important for
  outgoing, not incoming (you have it the other way around)
- <restriction> is still missing

I'm reviewing only the relaxng, I can't really review the rest.

Comment 13

[Ben Bucksch (:BenB)]

Also missing:
- <inputField> (bug 564043, but it's already legal in files)
- <pop3> (used in web.de and others)
   <leaveMessagesOnServer>true</leaveMessagesOnServer>
   <checkInterval minutes="15"/>
  </pop3>

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with Ben's comments.

(In reply to comment #12)
> - rename relaxng_schema.xml to relaxng_schema.1.0.xml, maybe

Fixed.

> - we have a lot of values defined as <text/>. Is that non-empty text?
>   If not, it probably should be, see e.g. all the empty IDs we get from
>   old ISPDB server.
>   (although that makes the schema much less nice :-( )

Yeah, since this is just the tests, and just a schema for the file, I am okay with it not catching _everything_.  (I've added it to username because we seem to mess that one up more often.)

> - why are socketType and username not directly in common-elements?

Fixed.

> - GSSAPI and NTLM should be in common-auth-types.

Fixed.

>   Same for none and client-IP-address, although they are most important for
>   outgoing, not incoming (you have it the other way around)

Switched.

> - <restriction> is still missing

Who uses restriction?  (Added anyways.)

(In reply to comment #13)
> Also missing:
> - <inputField> (bug 564043, but it's already legal in files)

So, we don't currently do anything with them in our code, so we could rename them to "userinput" (which is what they're listed as in https://wiki.mozilla.org/Thunderbird:Autoconfiguration:ConfigFileFormat#User_input_fields), so I'm going to not add these until the code that uses them is checked in, just in case we change our minds.

> - <pop3> (used in web.de and others)
>    <leaveMessagesOnServer>true</leaveMessagesOnServer>
>    <checkInterval minutes="15"/>
>   </pop3>

Added.

Thanks,
Blake.

Comment 15

[Ben Bucksch (:BenB)]

>                <value>smtp-after-pop</value>

1. Not valid for incoming server, but outgoing. Just move it.

> leaveMessagesOnServer

2. There's no data type="boolean"? Well, WFM.

3. leaveMessagesOnServer and checkInterval are both optional within pop3. You don't need to add both.

4. It's client-IP-address, not client-ip-address.
   Both for auth and restriction.
Maybe we should call it "SMTP-after-POP" as well.

5. <enable><instruction> should be mandatory (oneOrMore, not zeroOrMore).
   Unlike <documentation><descr>, which is indeed optional.

(<data type="language"/>: nice! I like RelaxNG a lot.)

Sorry for going round on this.

Comment 16

[Ben Bucksch (:BenB)]

> You don't need to add both.

You = file author

Comment 17

[Blake Winton (:bwinton) (:☕️)]

Attachment: The hopefully-final patch.

(In reply to comment #15)
> >                <value>smtp-after-pop</value>
> 1. Not valid for incoming server, but outgoing. Just move it.

Moved.

> > leaveMessagesOnServer
> 2. There's no data type="boolean"? Well, WFM.

There is, but it also accepts 0/1, which we don't want to.

> 3. leaveMessagesOnServer and checkInterval are both optional within pop3. You
> don't need to add both.

Optionalized.

> 4. It's client-IP-address, not client-ip-address.
>    Both for auth and restriction.
> Maybe we should call it "SMTP-after-POP" as well.

Changed, and changed.

> 5. <enable><instruction> should be mandatory (oneOrMore, not zeroOrMore).
>    Unlike <documentation><descr>, which is indeed optional.

Mandatorized.

> Sorry for going round on this.

No worries.  It's good to get this right, since then other people can use it to validate their configs.  :)

Later,
Blake.

Comment 18

[Ben Bucksch (:BenB)]

Comment on attachment 475585 [details] [diff] [review]
The hopefully-final patch.

Thanks.
r=BenB - as said in the beginning, only applies to RelaxNG part, I can't review the rest.

Comment 19

[Ben Bucksch (:BenB)]

1. You're just copying the v1.0 output code to v1.1 output code, right?
Isn't that a lot of code duplication? Wouldn't it be better to work with |if| statements?
2. The v1.1 output code doesn't support several servers. The DB probably neither, currently, but it would be quite a change, from how I read the logic.
3. What about auth type name? That's coming from DB, no? Are you ensuring that we output "password-cleartext" and not "plain"?
4. |if not data.email_provider_id:| I think that's an invalid state. Shouldn't that be fixed in the DB and DB population code? I think the ID should be chosen explicitly (if there are several domains).

Comment 20

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #19)
> 1. You're just copying the v1.0 output code to v1.1 output code, right?
> Isn't that a lot of code duplication? Wouldn't it be better to work with |if|
> statements?

Well, I would like to keep them separate so that adding new versions of output doesn't have any possibility of messing up older versions.

> 2. The v1.1 output code doesn't support several servers. The DB probably
> neither, currently, but it would be quite a change, from how I read the logic.

Yeah, and I'm going to leave that for the poor soul who has to implement multiple servers in the DB.  (*cough* sancus *cough* ;)

> 3. What about auth type name? That's coming from DB, no? Are you ensuring that
> we output "password-cleartext" and not "plain"?

Nope, I'm outputting whatever's in the DB.  It's the DB's job to make sure that's correct.  (I also don't check the value of the hostname field. ;)

> 4. |if not data.email_provider_id:| I think that's an invalid state. Shouldn't
> that be fixed in the DB and DB population code? I think the ID should be
> chosen explicitly (if there are several domains).

Perhaps, although defaulting the id to the first domain in the list isn't a terrible default, imho.  Again, I'm doing my best to leave database changes out of this patch, although I wouldn't object to a followup bug about this.  ;)

Later,
Blake.

Comment 21

[Ben Bucksch (:BenB)]

> I would like to keep them separate so that adding new versions of output
> doesn't have any possibility of messing up older versions.

I understand, but I think it's more likely that we make bugfixes that apply to both versions. v1.1 and v1.0 are not *that* different, after all.

> > Are you ensuring that we output "password-cleartext" and not "plain"?
> Nope, I'm outputting whatever's in the DB.  It's the DB's job to make sure
> that's correct.

That needs to be fixed, as that's one of the differences between v1.0 and v1.1 (i.e. whatever is in the DB will be wrong for one of them), and also one of the annoying things that I have to say every time when I review config files exported from ISPDB.

> Perhaps, although defaulting the id to the first domain in the list isn't a
> terrible default, imho.  Again, I'm doing my best to leave database changes out
> of this patch, although I wouldn't object to a followup bug about this.  ;)

Followup bug is fine with me.

Comment 22

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 475585 [details] [diff] [review]
The hopefully-final patch.

And off for the second review.

Gozer, if you like this diff, could you please just check it in?  I'm on my NetBook this week, and so don't have any of my ssh keys set up.  :(

Thanks,
Blake.

Comment 23

[Philippe M. Chiasson (:gozer)]

Comment on attachment 475585 [details] [diff] [review]
The hopefully-final patch.

Committed revision 74797.