Re: Session refactoring discussion

[Rene Cordier <rc...@apache.org>]

Hi Benoit,

Thank you for your feedback.

Answers are inline as well but overall it looks good feedback to me.

Best regards,
Rene.

On 22/01/2020 12:10, Tellier Benoit wrote:
> Hi René,
> 
> My answers are inlined...
> 
> Best regard,
> 
> Benoit
> 
> On 22/01/2020 10:21, Rene Cordier wrote:
>> Hi guys,
>>
>> I would like to bring on the table some proposition to refactor our
>> session usage within James.
>>
>> # Context
>> [...]
>> Then I tried to use a `MailboxMapper` defined in the mailbox-store
>> module to be able to access to the mailbox containing that message
>> without having the mailbox id. However, we should not rely on this
>> outside of the mailbox modules, and rely more on the abstract API
>> defined in mailbox-api module.
> 
> In my opinion we would need another architecture decision record on this
> consensual topic as this is the heart of our core APIs.
> 
> Could you take care about it?

Two ADRs in total then. Ok it makes sense to me, I will try to check all 
of that.

> 
> I would add in the "context" that as part of an upcoming effort to port
> our current JMAP implementation to the lastest JMAP specification, we
> will have to support JMAP account delegation. Thus being able to
> audit/represent these concept efficiently when interacting with the
> mailbox store is also related to this work
> 
>> # Current API
>> [...]
>>
>> # What we need
>>
>> Well we can agree that the actual session is not enough to cover all the
>> cases that we have at the moment. I think we can have probably 4 types
>> of accesses within James to do operations on mailboxes :
>>
>> * User session : user is logged and authenticated explicitly, does
>> operations on mailboxes he has rights on.
>> * User delegation session : James is doing user-related operations
>> internally, it's more of an implicit user session (mailets, listeners,
>> ...?)
> 
> I think you are mixing two concepts here...
> 
> We have 'implicit' user actions, taken by the system on the behalf of
> the user (eg: listener, mailets). These system action originated from a
> user action.
> 
> Which is different to 'delegation': Bob gives Alice the right to
> interact with Bob's mailbox as if she was Bob.
> 
>   - This can be revoked
>   - We need to track that an action originated from Alice, and not from
> Bob (audit)

Thanks for the clarification, that was not clear on my side indeed. Noted !

> 
>> * Impersonation session : Admin is logged and authenticated, but has the
>> authorization to act on behalf of an other user
>> * System session : internal operations not directly related to a user (a
>> good example would be the recomputation of a jmap preview of a single
>> message)
>>
>> We would need then to refactor the code to ensure we have different
>> types of sessions regarding those cases and review the session usage to
>> not introduce security vulnerabilities.
>>
>>
>> # Proposed solution
>>
>> We could start by having a different session type for each of those
>> cases above, by also modifying the API in `SessionProvider` to have the
>> required methods achieved by this :
>>
>> * `login` => user session : TypeSession.User
>> * `loginAsOtherUser` => impersonation session :
>> TypeSession.ImpersonatedUser
>> * `createDelegatedUserSession` => user delegation session :
>> TypeSession.DelegatedUser
>> * `createSystemSession` => system session : TypeSession.System
>>
>> Those grained differentiations would also help better for auditing and
>> logging purposes.
>>
>> We need as well to be careful and refactor the code regarding those new
>> types of sessions and sets of rights :
>>
>> * `TypeSession.User` and `TypeSession.ImpersonatedUser` should have the
>> rights defined for the user
>> * `TypeSession.ImpersonatedUser` should have the rights of the user that
>> is being impersonated, not the logged in admin
>> * `TypeSession.System` should have bigger rights, to allow more global
>> operations on mailboxes in necessary cases (need to use carefully though)
> 
> Define "(need to use carefully though)"... How would you achieve
> carefulness here?

Could be adding some more related tests, or having more granularity in 
the rights for system session, depending what we need at that place, 
instead of giving full rights. I'm not exactly sure yet though.

> 
>> We need to define what kind of session is suitable for which parts of
>> the code and also add/update tests suites accordingly.
>>
>> We probably should propose an ADR for this too...
> 
> That, is necessary, yes.
>>
>>
>> What do you think of it? Any feedback / discussion is welcomed of course
>> ! Thank you for reading this.
> 
> I believe MailboxSession need to cary along different information
> depending of the 'SessionType'.
> 
> User type just need to cary along the username
> In case of delegation we need to have both users
> In case of a system session (as defined above) have the name of the
> service taking an action is more relevant than a username
> 
> Maybe we could have an object representing the Entity using this session
> to interact with the mailbox store?

Like an Actor perhaps? UserActor, SystemActor, DelegatedActor, 
ImpersonatedActor

> 
>>
>> Best regards,
>> Rene.
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
>> For additional commands, e-mail: server-dev-help@james.apache.org
>>
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
> For additional commands, e-mail: server-dev-help@james.apache.org
> 
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org