You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by Andreas Joseph Krogh <an...@visena.com> on 2021/06/08 12:14:54 UTC

What's the reason for having Username's constructor private?

We have an environment where usernames can be any character thus the rules 
enforced by Username does not apply to us. We were thinking about subclassing 
it, until we saw it has a private constructor signaling that it isn't meant for 
extending. 

I propose to change this constructor to be protected so one may extend this 
class with custom validation-logic for the userName. How are others handling 
this problem? 



-- 
Andreas Joseph Krogh 
CTO / Partner - Visena AS 

Re: What's the reason for having Username's constructor private?

Posted by Andreas Joseph Krogh <an...@visena.com>.
Great, we' craft a PR.

Den 10. juni 2021 16:48:02 CEST, skrev "btellier@apache.org" <bt...@apache.org>:
>
>On 10/06/2021 21:33, Andreas Joseph Krogh wrote:
>> [...]
>>  
>> So - again; What about having a Username-class which is just
>> that, /any/ username?
>Again, this would violates quite a lot of logic inside James itself.
>
>I have personnally  no problem with contribution:
>
> - Allow subclassing of Username if that can help.
>
> - Introduce a (say) LoginName POJO within /protocols/imap
>
> - Anything helping modularizing protocols/imap
>
>But changing such a central class for a specific use case, in the scope
>of the entire James server, do not seem acceptable to me.
>
>Benoit
>
>>  
>> --
>> *Andreas Joseph Krogh*
>> CTO / Partner - Visena AS
>> Mobile: +47 909 56 963
>> andreas@visena.com <ma...@visena.com>
>> www.visena.com <https://www.visena.com>
>> <https://www.visena.com>
>>  
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
>> For additional commands, e-mail: server-dev-help@james.apache.org

Re: What's the reason for having Username's constructor private?

Posted by "btellier@apache.org" <bt...@apache.org>.
On 10/06/2021 21:33, Andreas Joseph Krogh wrote:
> [...]
>  
> So - again; What about having a Username-class which is just
> that, /any/ username?
Again, this would violates quite a lot of logic inside James itself.

I have personnally  no problem with contribution:

 - Allow subclassing of Username if that can help.

 - Introduce a (say) LoginName POJO within /protocols/imap

 - Anything helping modularizing protocols/imap

But changing such a central class for a specific use case, in the scope
of the entire James server, do not seem acceptable to me.

Benoit

>  
> --
> *Andreas Joseph Krogh*
> CTO / Partner - Visena AS
> Mobile: +47 909 56 963
> andreas@visena.com <ma...@visena.com>
> www.visena.com <https://www.visena.com>
> <https://www.visena.com>
>  
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
> For additional commands, e-mail: server-dev-help@james.apache.org

Re: What's the reason for having Username's constructor private?

Posted by Andreas Joseph Krogh <an...@visena.com>.
På torsdag 10. juni 2021 kl. 15:44:17, skrev btellier@apache.org 
<ma...@apache.org> <btellier@apache.org <ma...@apache.org>>:



[...] 
 We only use JAMES for IMAP. 
So you only use the content of /protocols/imap, correct?

This means that you had been implementing / will be implementing some 
mailbox-api implementation of your own, correct?



Yes, we have a working implementation based on a custom, extended, version on 
James-3.0.4-beta. We've used that for a long time and are now looking at 
upgrading to v3.6. But the usage of the Username-class, with its current 
"non-POJO restrictions", in the API makes things a little more difficult for us 
that we'd like....






[...] @Test void arbitraryCharInLocalPart() { assertThatCode(() -> Username.of(
"Whatever_I- want3$$$###_! as lons I do not give a domain")) 
.doesNotThrowAnyException(); } 


The only constraint if a domain part is unspecified, is the size of 256 
character and the absence of @.



To come back to your original problem, what prevents, for instance the user 
scheme to be: <username>_<customer-key> ?

In short; What prevents us from using Username is (we only user "userid", ie. 
Username.localPart, we don't use "domain"): 
1. The "userid"-part must allow any character, including '@'. Many of our 
customers have '@' in their usernames, for various reasons. (well, \n and other 
control-characters are really not needed). 
2. The "userid" should be case-sensitive. 


Both of these are according to the IMAP-spec, so it's sad that James prevents 
this. As said, using Base32 encoding lets us get around this, but it doesn't 
"feel right". 

The fact that Username uses toLowercase(Locale.US) actually lets one use other 
interesting encodings likeecoji <https://github.com/keith-turner/ecoji>, as it 
doesn't touch multibyte-characters/codepoints when lowercasing. 

So - again; What about having a Username-class which is just that, any 
username? 

-- 
Andreas Joseph Krogh 
CTO / Partner - Visena AS 
Mobile: +47 909 56 963 
andreas@visena.com <ma...@visena.com> 
www.visena.com <https://www.visena.com> 
 <https://www.visena.com> 

Re: What's the reason for having Username's constructor private?

Posted by Andreas Joseph Krogh <an...@visena.com>.
Forgot to respond to your last paragraph, great to hear that you're open to make Username protected, we'll make a PR.

Den 10. juni 2021 15:44:17 CEST, skrev "btellier@apache.org" <bt...@apache.org>:
>
>On 10/06/2021 18:43, Andreas Joseph Krogh wrote:
>> På torsdag 10. juni 2021 kl. 05:09:50, skrev btellier@apache.org
>> <ma...@apache.org> <btellier@apache.org
>> <ma...@apache.org>>:
>>
>>     [...]
>>     > 1. The login-parser should do just that parse the login-command and
>>     > provide the Strings /as //is/, not try to do anything meaningful og
>>     > enforce constraints on it, as Username does.
>>     Having login-parser agnostic of James internal logic would seem like a
>>     fair point to me, I can agree with that.
>>
>>     (We could introduce a new POJO that do not enforce constraints in
>>     order
>>     to preverve strong typing.)
>>     >  
>>     > 2. The logic in Username seems flawed:
>>     > - Why a limit on length? Nothing in the protocol dictates this.
>>     James has to be working with several protocols, not just IMAP.
>>
>>     Implementation details (JPA) might have been pushing these kind of
>>     concerns to the Username POJO.
>>
>>  
>>
>> Username is not really a POJO, by definition, as it does lots more
>> than just holding on to the "iserid". I agree that having a
>> Username-type is cool for type-safety, but it should only wrap the
>> "userid"-String, nothing else.
>>  
>>  
>>  
>>
>>     > - The username used for IMAP-LOGIN is completely decoupled from
>>     > whatever <local-part> is in an email-address. Yes, they /could/ be
>>     > related but are often not, even if modern MUAs like to think so.
>>     > Username should make no asumptions about local-part/domain-part
>>     in the
>>     > provider userid.
>>     Here likely lies the root cause: contrary to (say) SMTP, IMAP so
>>     far is
>>     hard coupled with the James implementation, with hard dependencies on
>>     things like mailbox-api.
>>
>>  
>>  
>> We only use JAMES for IMAP.
>
>So you only use the content of /protocols/imap, correct?
>
>This means that you had been implementing / will be implementing some
>mailbox-api implementation of your own, correct?
>
>>  
>>
>>     > - According to the spec the LOGIN-command is:
>>     > login           = "LOGIN" SP userid SP password
>>     > userid          = astring
>>     > astring         = 1*ASTRING-CHAR / string
>>     > string          = quoted / literal
>>     >  
>>     > So any string will be valid for "userid", and it's up to the site
>>     > (where the IMAP-server is used) to interpret the userid
>>     case-sensitive
>>     > or insensitive, so JAMES show not enforce anything and just
>>     "leave it
>>     > as is".
>>     I understand that James is free to do whatever it wants with userId
>>     validation.
>>
>>     I understand that tooling is missing to allow people to override that
>>     behaviour in James / James IMAP stack.
>>     [...]
>>     > So - I'm proposing to refactor so that Username only wraps the
>>     parsed
>>     > "userid"-String, nothing else. If validation or other stuff is
>>     needed
>>     > I think that should be handled by some other /interface/ which the
>>     > JAMES-project is free to provide a default-impl of, but it must be
>>     > possible to implement custom logic. Also proposing to pull out
>>     all the
>>     > from*-factory-methods.
>>     >  
>>     > What do you think?
>>     Personally I am reluctant to weaken data validation , that is already
>>     reasonably permissive, without understanding the impacts on the
>>     code base.
>>
>>     I do think that there is other alternatives than hard-coding logic
>>     into
>>     usernames.
>>
>>     You could rely on custom API / classes to make the bridge between
>>     usernames and your custom logic.
>>
>>  
>>  
>> Do you mean "we can, as of James-3.6" or in "som future version".I
>> don't understand how we could accomplish that in v-3.6 due to the
>> limitations of the Username-type, which I'm describing in this thread,
>> and that Username is used "all over" in the IMAP-API. It's an integral
>> part of UserRepository and lots of other classes mentioned in previous
>> emails, so it's impossible to implement an IMAP-server using
>> James-3.6-API without using it, unless I'm missing something.
>>  
>>  
>>  
>>
>>     Turning off virtual-hosting will also remove the limitation of the
>>     domains that as I understand is the point limiting you.
>>
>>  
>>  
>> The only thing which is limiting us is that Username is enforcing
>> constraints not specified by the IMAP-standard for "userid". We don't
>> use virtual-hosting or other JAMES-stuff, only the IMAP-stack for
>> being able to act as an IMAP-server. We have implemented custom stuff
>> to retrieve/store data in our custom-modelled database, and everything
>> works well in that regards.
>
>@Test void arbitraryCharInLocalPart() {
>    assertThatCode(() -> Username.of("Whatever_I- want3$$$###_! as lons I do not give a domain"))
>        .doesNotThrowAnyException(); }
>
>
>The only constraint if a domain part is unspecified, is the size of 256
>character and the absence of @.
>
>
>To come back to your original problem, what prevents, for instance the
>user scheme to be:  <username>_<customer-key> ?
>
>>  
>> As I've mentioned earlier, we get around this by using Base32-encoding
>> for the "localPart" in Username and decode it wherever we need the
>> value, but we'd really like the restrictions enforced by it to be
>> removed, to comply with the IMAP-standard.
>
>The truth is that the IMAP stack is not independant of the James server
>and as such, in another thread propose to move it back into
>server/protocols folder to make this clear. While generifying the IMAP
>code is noble, we will end up finding out that Username usage is
>enforced at mailbox-api usage and we start getting quite inside James guts.
>
>That being said, I think having more generic CommandParsers  (that can
>thus be used outside of James context) and modular IMAP Processor/
>command processor would allow for greater power for people reusing this
>code. This should allow people to choose precisely which part of the
>IMAP stack they wish to use.
>
>Regarding your immediate problem, I personally no problems having
>Username protected to allow one to subclass it if need be. Likely this
>is the simplest option here. Feel free to open a pull request.
>
>Regards,
>
>Benoit
>
>
>>  
>> Thanks.
>>  
>> --
>> *Andreas Joseph Krogh*
>> CTO / Partner - Visena AS
>> Mobile: +47 909 56 963
>> andreas@visena.com <ma...@visena.com>
>> www.visena.com <https://www.visena.com>
>> <https://www.visena.com>
>>  

Re: What's the reason for having Username's constructor private?

Posted by "btellier@apache.org" <bt...@apache.org>.
On 10/06/2021 18:43, Andreas Joseph Krogh wrote:
> På torsdag 10. juni 2021 kl. 05:09:50, skrev btellier@apache.org
> <ma...@apache.org> <btellier@apache.org
> <ma...@apache.org>>:
>
>     [...]
>     > 1. The login-parser should do just that parse the login-command and
>     > provide the Strings /as //is/, not try to do anything meaningful og
>     > enforce constraints on it, as Username does.
>     Having login-parser agnostic of James internal logic would seem like a
>     fair point to me, I can agree with that.
>
>     (We could introduce a new POJO that do not enforce constraints in
>     order
>     to preverve strong typing.)
>     >  
>     > 2. The logic in Username seems flawed:
>     > - Why a limit on length? Nothing in the protocol dictates this.
>     James has to be working with several protocols, not just IMAP.
>
>     Implementation details (JPA) might have been pushing these kind of
>     concerns to the Username POJO.
>
>  
>
> Username is not really a POJO, by definition, as it does lots more
> than just holding on to the "iserid". I agree that having a
> Username-type is cool for type-safety, but it should only wrap the
> "userid"-String, nothing else.
>  
>  
>  
>
>     > - The username used for IMAP-LOGIN is completely decoupled from
>     > whatever <local-part> is in an email-address. Yes, they /could/ be
>     > related but are often not, even if modern MUAs like to think so.
>     > Username should make no asumptions about local-part/domain-part
>     in the
>     > provider userid.
>     Here likely lies the root cause: contrary to (say) SMTP, IMAP so
>     far is
>     hard coupled with the James implementation, with hard dependencies on
>     things like mailbox-api.
>
>  
>  
> We only use JAMES for IMAP.

So you only use the content of /protocols/imap, correct?

This means that you had been implementing / will be implementing some
mailbox-api implementation of your own, correct?

>  
>
>     > - According to the spec the LOGIN-command is:
>     > login           = "LOGIN" SP userid SP password
>     > userid          = astring
>     > astring         = 1*ASTRING-CHAR / string
>     > string          = quoted / literal
>     >  
>     > So any string will be valid for "userid", and it's up to the site
>     > (where the IMAP-server is used) to interpret the userid
>     case-sensitive
>     > or insensitive, so JAMES show not enforce anything and just
>     "leave it
>     > as is".
>     I understand that James is free to do whatever it wants with userId
>     validation.
>
>     I understand that tooling is missing to allow people to override that
>     behaviour in James / James IMAP stack.
>     [...]
>     > So - I'm proposing to refactor so that Username only wraps the
>     parsed
>     > "userid"-String, nothing else. If validation or other stuff is
>     needed
>     > I think that should be handled by some other /interface/ which the
>     > JAMES-project is free to provide a default-impl of, but it must be
>     > possible to implement custom logic. Also proposing to pull out
>     all the
>     > from*-factory-methods.
>     >  
>     > What do you think?
>     Personally I am reluctant to weaken data validation , that is already
>     reasonably permissive, without understanding the impacts on the
>     code base.
>
>     I do think that there is other alternatives than hard-coding logic
>     into
>     usernames.
>
>     You could rely on custom API / classes to make the bridge between
>     usernames and your custom logic.
>
>  
>  
> Do you mean "we can, as of James-3.6" or in "som future version".I
> don't understand how we could accomplish that in v-3.6 due to the
> limitations of the Username-type, which I'm describing in this thread,
> and that Username is used "all over" in the IMAP-API. It's an integral
> part of UserRepository and lots of other classes mentioned in previous
> emails, so it's impossible to implement an IMAP-server using
> James-3.6-API without using it, unless I'm missing something.
>  
>  
>  
>
>     Turning off virtual-hosting will also remove the limitation of the
>     domains that as I understand is the point limiting you.
>
>  
>  
> The only thing which is limiting us is that Username is enforcing
> constraints not specified by the IMAP-standard for "userid". We don't
> use virtual-hosting or other JAMES-stuff, only the IMAP-stack for
> being able to act as an IMAP-server. We have implemented custom stuff
> to retrieve/store data in our custom-modelled database, and everything
> works well in that regards.

@Test void arbitraryCharInLocalPart() {
    assertThatCode(() -> Username.of("Whatever_I- want3$$$###_! as lons I do not give a domain"))
        .doesNotThrowAnyException(); }


The only constraint if a domain part is unspecified, is the size of 256
character and the absence of @.


To come back to your original problem, what prevents, for instance the
user scheme to be:  <username>_<customer-key> ?

>  
> As I've mentioned earlier, we get around this by using Base32-encoding
> for the "localPart" in Username and decode it wherever we need the
> value, but we'd really like the restrictions enforced by it to be
> removed, to comply with the IMAP-standard.

The truth is that the IMAP stack is not independant of the James server
and as such, in another thread propose to move it back into
server/protocols folder to make this clear. While generifying the IMAP
code is noble, we will end up finding out that Username usage is
enforced at mailbox-api usage and we start getting quite inside James guts.

That being said, I think having more generic CommandParsers  (that can
thus be used outside of James context) and modular IMAP Processor/
command processor would allow for greater power for people reusing this
code. This should allow people to choose precisely which part of the
IMAP stack they wish to use.

Regarding your immediate problem, I personally no problems having
Username protected to allow one to subclass it if need be. Likely this
is the simplest option here. Feel free to open a pull request.

Regards,

Benoit


>  
> Thanks.
>  
> --
> *Andreas Joseph Krogh*
> CTO / Partner - Visena AS
> Mobile: +47 909 56 963
> andreas@visena.com <ma...@visena.com>
> www.visena.com <https://www.visena.com>
> <https://www.visena.com>
>  

Re: What's the reason for having Username's constructor private?

Posted by Andreas Joseph Krogh <an...@visena.com>.
På torsdag 10. juni 2021 kl. 05:09:50, skrev btellier@apache.org 
<ma...@apache.org> <btellier@apache.org <ma...@apache.org>>:
[...]
 > 1. The login-parser should do just that parse the login-command and
 > provide the Strings /as //is/, not try to do anything meaningful og
 > enforce constraints on it, as Username does.
 Having login-parser agnostic of James internal logic would seem like a
 fair point to me, I can agree with that.

 (We could introduce a new POJO that do not enforce constraints in order
 to preverve strong typing.)
 > 
 > 2. The logic in Username seems flawed:
 > - Why a limit on length? Nothing in the protocol dictates this.
 James has to be working with several protocols, not just IMAP.

 Implementation details (JPA) might have been pushing these kind of
 concerns to the Username POJO. 


 Username is not really a POJO, by definition, as it does lots more than just 
holding on to the "iserid". I agree that having a Username-type is cool for 
type-safety, but it should only wrap the "userid"-String, nothing else. 




> - The username used for IMAP-LOGIN is completely decoupled from
 > whatever <local-part> is in an email-address. Yes, they /could/ be
 > related but are often not, even if modern MUAs like to think so.
 > Username should make no asumptions about local-part/domain-part in the
 > provider userid.
 Here likely lies the root cause: contrary to (say) SMTP, IMAP so far is
 hard coupled with the James implementation, with hard dependencies on
 things like mailbox-api. 


 We only use JAMES for IMAP. 




> - According to the spec the LOGIN-command is:
 > login = "LOGIN" SP userid SP password
 > userid = astring
 > astring = 1*ASTRING-CHAR / string
 > string = quoted / literal
 > 
 > So any string will be valid for "userid", and it's up to the site
 > (where the IMAP-server is used) to interpret the userid case-sensitive
 > or insensitive, so JAMES show not enforce anything and just "leave it
 > as is".
 I understand that James is free to do whatever it wants with userId
 validation.

 I understand that tooling is missing to allow people to override that
 behaviour in James / James IMAP stack.
 [...]
 > So - I'm proposing to refactor so that Username only wraps the parsed
 > "userid"-String, nothing else. If validation or other stuff is needed
 > I think that should be handled by some other /interface/ which the
 > JAMES-project is free to provide a default-impl of, but it must be
 > possible to implement custom logic. Also proposing to pull out all the
 > from*-factory-methods.
 > 
 > What do you think?
 Personally I am reluctant to weaken data validation , that is already
 reasonably permissive, without understanding the impacts on the code base.

 I do think that there is other alternatives than hard-coding logic into
 usernames.

 You could rely on custom API / classes to make the bridge between
 usernames and your custom logic. 



Do you mean "we can, as of James-3.6" or in "som future version".I don't 
understand how we could accomplish that in v-3.6 due to the limitations of the 
Username-type, which I'm describing in this thread, and that Username is used 
"all over" in the IMAP-API. It's an integral part of UserRepository and lots of 
other classes mentioned in previous emails, so it's impossible to implement an 
IMAP-server using James-3.6-API without using it, unless I'm missing something. 




Turning off virtual-hosting will also remove the limitation of the
 domains that as I understand is the point limiting you. 



The only thing which is limiting us is that Username is enforcing constraints 
not specified by the IMAP-standard for "userid". We don't use virtual-hosting 
or other JAMES-stuff, only the IMAP-stack for being able to act as an 
IMAP-server. We have implemented custom stuff to retrieve/store data in our 
custom-modelled database, and everything works well in that regards. 



As I've mentioned earlier, we get around this by using Base32-encoding for the 
"localPart" in Username, and decode it wherever we need the value, but we'd 
really like the restrictions enforced by it to be removed, to comply with the 
IMAP-standard. 

Thanks. 

-- 
Andreas Joseph Krogh 
CTO / Partner - Visena AS 
Mobile: +47 909 56 963 
andreas@visena.com <ma...@visena.com> 
www.visena.com <https://www.visena.com> 
 <https://www.visena.com> 

Re: What's the reason for having Username's constructor private?

Posted by "btellier@apache.org" <bt...@apache.org>.
On 09/06/2021 19:45, Andreas Joseph Krogh wrote:
> På onsdag 09. juni 2021 kl. 06:07:46, skrev btellier@apache.org
> <ma...@apache.org> <btellier@apache.org
> <ma...@apache.org>>:
>
>     Hello Andreas,
>     [...]
>     I do not get why you need to rely on the Username POJO in your custom
>     IMAP stack.
>
>     Maybe you can introduce some abstractions that suites your needs and
>     have the logic to convert it to the James POJOs in places that are
>     needed.
>
>     Also turning off virtual hosting enables to pass any string
>     (except `@`)
>     in the local part. Maybe this could be sufficient to easily solve your
>     issue?
>
>  
>  
> The reason we "have to" rely on Username is, AFAIU, that that's the
> type being used by many classes which make up much of the login- and
> other user-related code. We're thinking about
> replacing LoginCommandParser, but the Username-class is what's being
> passed around so I don't see any way around having to rely on it. Am I
> missing something?
>  
> There are several things I find strange about the current logic in,
> and usages of, Username:
>  
> 1. The login-parser should do just that parse the login-command and
> provide the Strings /as //is/, not try to do anything meaningful og
> enforce constraints on it, as Username does.
Having login-parser agnostic of James internal logic would seem like a
fair point to me, I can agree with that.

(We could introduce a new POJO that do not enforce constraints in order
to preverve strong typing.)
>  
> 2. The logic in Username seems flawed:
> - Why a limit on length? Nothing in the protocol dictates this.
James has to be working with several protocols, not just IMAP.

Implementation details (JPA) might have been pushing these kind of
concerns to the Username POJO.

> - The username used for IMAP-LOGIN is completely decoupled from
> whatever <local-part> is in an email-address. Yes, they /could/ be
> related but are often not, even if modern MUAs like to think so.
> Username should make no asumptions about local-part/domain-part in the
> provider userid.
Here likely lies the root cause: contrary to (say) SMTP, IMAP so far is
hard coupled with the James implementation, with hard dependencies on
things like mailbox-api.

> - According to the spec the LOGIN-command is:
> login           = "LOGIN" SP userid SP password
> userid          = astring
> astring         = 1*ASTRING-CHAR / string
> string          = quoted / literal
>  
> So any string will be valid for "userid", and it's up to the site
> (where the IMAP-server is used) to interpret the userid case-sensitive
> or insensitive, so JAMES show not enforce anything and just "leave it
> as is".
I understand that James is free to do whatever it wants with userId
validation.

I understand that tooling is missing to allow people to override that
behaviour in James / James IMAP stack.
>  
>  
> We're getting around this by base32-encoding the username before
> constructing the Username-object, and decoding it where we use it for
> retreiving our custom User-objects, which we stuff in the ImapSession.
>  
> Username is used/leaking in many places (e.g. UserRepository,
> MailboxSession), which whould be fine if the Username-class didn't
> enforce any rules on "userid", so as it is now we have no choise to
> not rely on it.
>  
> So - I'm proposing to refactor so that Username only wraps the parsed
> "userid"-String, nothing else. If validation or other stuff is needed
> I think that should be handled by some other /interface/ which the
> JAMES-project is free to provide a default-impl of, but it must be
> possible to implement custom logic. Also proposing to pull out all the
> from*-factory-methods.
>  
> What do you think?
Personally I am reluctant to weaken data validation , that is already
reasonably permissive, without understanding the impacts on the code base.

I do think that there is other alternatives than hard-coding logic into
usernames.

You could rely on custom API / classes to make the bridge between
usernames and your custom logic.

Turning off virtual-hosting will also remove the limitation of the
domains that as I understand is the point limiting you.

Cheers,

Benoit
> --
> *Andreas Joseph Krogh*
> CTO / Partner - Visena AS
>  

Re: What's the reason for having Username's constructor private?

Posted by Andreas Joseph Krogh <an...@visena.com>.
På onsdag 09. juni 2021 kl. 06:07:46, skrev btellier@apache.org 
<ma...@apache.org> <btellier@apache.org <ma...@apache.org>>:
Hello Andreas,
 [...]
 I do not get why you need to rely on the Username POJO in your custom
 IMAP stack.

 Maybe you can introduce some abstractions that suites your needs and
 have the logic to convert it to the James POJOs in places that are needed.

 Also turning off virtual hosting enables to pass any string (except `@`)
 in the local part. Maybe this could be sufficient to easily solve your
 issue? 


The reason we "have to" rely on Username is, AFAIU, that that's the type being 
used by many classes which make up much of the login- and other user-related 
code. We're thinking about replacing LoginCommandParser, but the Username-class 
is what's being passed around so I don't see any way around having to rely on 
it. Am I missing something? 

There are several things I find strange about the current logic in, and usages 
of, Username: 

1. The login-parser should do just that parse the login-command and provide 
the Stringsas is, not try to do anything meaningful og enforce constraints on 
it, as Username does. 

2. The logic in Username seems flawed: 
- Why a limit on length? Nothing in the protocol dictates this. 
- The username used for IMAP-LOGIN is completely decoupled from whatever 
<local-part> is in an email-address. Yes, theycould be related but are often 
not, even if modern MUAs like to think so. Username should make no asumptions 
about local-part/domain-part in the provider userid. 
- According to the spec the LOGIN-command is: 
 login = "LOGIN" SP userid SP password userid = astring astring = 
1*ASTRING-CHAR / string string = quoted / literal 


So any string will be valid for "userid", and it's up to the site (where the 
IMAP-server is used) to interpret the userid case-sensitive or insensitive, so 
JAMES show not enforce anything and just "leave it as is". 


We're getting around this by base32-encoding the username before constructing 
the Username-object, and decoding it where we use it for retreiving our custom 
User-objects, which we stuff in the ImapSession. 

Username is used/leaking in many places (e.g. UserRepository, MailboxSession), 
which whould be fine if the Username-class didn't enforce any rules on 
"userid", so as it is now we have no choise to not rely on it. 

So - I'm proposing to refactor so that Username only wraps the parsed 
"userid"-String, nothing else. If validation or other stuff is needed I think 
that should be handled by some otherinterface which the JAMES-project is free 
to provide a default-impl of, but it must be possible to implement custom 
logic. Also proposing to pull out all the from*-factory-methods. 

What do you think? 

-- 
Andreas Joseph Krogh 
CTO / Partner - Visena AS 

Re: What's the reason for having Username's constructor private?

Posted by "btellier@apache.org" <bt...@apache.org>.
Hello Andreas,

On 08/06/2021 21:56, Andreas Joseph Krogh wrote:
> På tirsdag 08. juni 2021 kl. 16:02:22, skrev Jean Helou
> <jean.helou@gmail.com <ma...@gmail.com>>:
>
>     Hello Andreas,
>
>     We have an environment where usernames can be *any* character thus the
>     > rules enforced by Username does not apply to us. We were
>     thinking about
>     > subclassing it, until we saw it has a private constructor
>     signaling that it
>     > isn't meant for extending.
>     >
>
>     Do you mean org.apache.james.core.Username ?
>
>  
>
> Yes.
>  
>  
>
>     If I understand correctly, the only character restriction there is you
>     can't put an @ in the local part ...  even
>     org.apache.james.core.Username#fromLocalPartWithoutDomain[1] won't
>     bypass
>     that ( and of course the usual not null, not empty)
>     As you can see in
>     https://github.com/apache/james-project/blob/master/core/src/main/java/org/apache/james/core/Username.java#L132,
>     a Username has a mapping to a org.apache.james.core.MailAddress
>     and this
>     mapping is used in multiple places. Allowing @ in the username
>     would break
>     that conversion.
>
>     Can you provide a more detailed picture of what you are trying to
>     achieve
>     and why you would want @ as part of the username ?
>
>  
>  
> We're building a custom IMAP-server, based on JAMES, where users
> provide their IMAP-username on the format <username>@<customer-key>,
> where the username may be anything (except blank, ofcourse). The
> <customer-key> (provided as part of the USERNAME as far as JAMES/IMAP
> knows, parsed as "anything after the last '@'-character") is used for
> knowing which database-mapping to use. It seems JAMES is hardcoded
> to the concept of usernames being an email-address, or part of it, and
> that is not always the case for us/our users. In that sense it seems
> JAMES is intended for use as an application, not a library, is this
> correct?
>  
> In other words; We need the username to be "whatever", and
> case-sensitive, so it would be preferable if Username was less
> restrictive, or an interface. Without this we'll have to base32-encode
> (because of case-insensitivity in Username) the username parsed with
> our custom LoginCommandParser, and decode where needed.
>  
>  
> Thanks.
>  
I do not get why you need to rely on the Username POJO in your custom
IMAP stack.

Maybe you can introduce some abstractions that suites your needs and
have the logic to convert it to the James POJOs in places that are needed.

Also turning off virtual hosting enables to pass any string (except `@`)
in the local part. Maybe this could be sufficient to easily solve your
issue?

Benoit
> --
> *Andreas Joseph Krogh*
> CTO / Partner - Visena AS
>  
>  
>
>     (I'll admit that the current check is overly restrictive as it doesn't
>     account for quoting in the local part [2] which allows using @ in
>     a quoted
>     string or escaping @ using a \)
>
>     [1]
>     https://github.com/apache/james-project/blob/master/core/src/main/java/org/apache/james/core/Username.java#L71
>     [2] https://datatracker.ietf.org/doc/html/rfc3696#section-3
>
>     Cheers,
>     Jean
>
>  
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
> For additional commands, e-mail: server-dev-help@james.apache.org

Re: What's the reason for having Username's constructor private?

Posted by Andreas Joseph Krogh <an...@visena.com>.
På tirsdag 08. juni 2021 kl. 16:02:22, skrev Jean Helou <jean.helou@gmail.com 
<ma...@gmail.com>>: 
Hello Andreas,

 We have an environment where usernames can be *any* character thus the
 > rules enforced by Username does not apply to us. We were thinking about
 > subclassing it, until we saw it has a private constructor signaling that it
 > isn't meant for extending.
 >

 Do you mean org.apache.james.core.Username ? 



 Yes. 


If I understand correctly, the only character restriction there is you
 can't put an @ in the local part ... even
 org.apache.james.core.Username#fromLocalPartWithoutDomain[1] won't bypass
 that ( and of course the usual not null, not empty)
 As you can see in
 
https://github.com/apache/james-project/blob/master/core/src/main/java/org/apache/james/core/Username.java#L132,
 a Username has a mapping to a org.apache.james.core.MailAddress and this
 mapping is used in multiple places. Allowing @ in the username would break
 that conversion.

 Can you provide a more detailed picture of what you are trying to achieve
 and why you would want @ as part of the username ? 



We're building a custom IMAP-server, based on JAMES, where users provide their 
IMAP-username on the format <username>@<customer-key>, where the username may 
be anything (except blank, ofcourse). The <customer-key> (provided as part of 
the USERNAME as far as JAMES/IMAP knows, parsed as "anything after the last 
'@'-character") is used for knowing which database-mapping to use. It seems 
JAMES is hardcoded to the concept of usernames being an email-address, or part 
of it, and that is not always the case for us/our users. In that sense it seems 
JAMES is intended for use as an application, not a library, is this correct? 

In other words; We need the username to be "whatever", and case-sensitive, so 
it would be preferable if Username was less restrictive, or an interface. 
Without this we'll have to base32-encode (because of case-insensitivity in 
Username) the username parsed with our custom LoginCommandParser, and decode 
where needed. 


Thanks. 


-- 
Andreas Joseph Krogh 
CTO / Partner - Visena AS 


(I'll admit that the current check is overly restrictive as it doesn't
 account for quoting in the local part [2] which allows using @ in a quoted
 string or escaping @ using a \)

 [1]
 
https://github.com/apache/james-project/blob/master/core/src/main/java/org/apache/james/core/Username.java#L71
 [2] https://datatracker.ietf.org/doc/html/rfc3696#section-3

 Cheers,
 Jean 

Re: What's the reason for having Username's constructor private?

Posted by Jean Helou <je...@gmail.com>.
Hello Andreas,

We have an environment where usernames can be *any* character thus the
> rules enforced by Username does not apply to us. We were thinking about
> subclassing it, until we saw it has a private constructor signaling that it
> isn't meant for extending.
>

Do you mean org.apache.james.core.Username ?

If I understand correctly, the only character restriction there is you
can't put an @ in the local part ...  even
org.apache.james.core.Username#fromLocalPartWithoutDomain[1] won't bypass
that ( and of course the usual not null, not empty)
As you can see in
https://github.com/apache/james-project/blob/master/core/src/main/java/org/apache/james/core/Username.java#L132,
a Username has a mapping to a org.apache.james.core.MailAddress and this
mapping is used in multiple places. Allowing @ in the username would break
that conversion.

Can you provide a more detailed picture of what you are trying to achieve
and why you would want @ as part of the username ?

(I'll admit that the current check is overly restrictive as it doesn't
account for quoting in the local part [2] which allows using @ in a quoted
string or escaping @ using a \)

[1]
https://github.com/apache/james-project/blob/master/core/src/main/java/org/apache/james/core/Username.java#L71
[2] https://datatracker.ietf.org/doc/html/rfc3696#section-3

Cheers,
Jean


>
>

> I propose to change this constructor to be protected so one may extend
> this class with custom validation-logic for the userName. How are others
> handling this problem?
>
> --
> *Andreas Joseph Krogh*
> CTO / Partner - Visena AS
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
> For additional commands, e-mail: server-dev-help@james.apache.org