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

[MU] List inactive users #1298

Open
quantranhong1999 opened this issue Nov 12, 2024 · 13 comments
Open

[MU] List inactive users #1298

quantranhong1999 opened this issue Nov 12, 2024 · 13 comments
Assignees
Labels

Comments

@quantranhong1999
Copy link
Member

Why

MU requires us the ability to report the inactive users list for example the users who have not used our services in the last 3 months.

We need to record the users's login activity.

Today we have user login activity using AuditTrail with login logs stored in Loki. However upon large data size (TMail produces a lot of logs), Loki strips a part of the logs to give an in-time response, which makes the users activity analysis not reliable.

We need to store that piece of information somewhere else rather than Loki.

How

Storage

Redis seems to be the suitable storage for the need: famous for session data use case, fast write, AOF seems reasonable for data backup (could accept a bit of data lag for user activity use case).

Code change

We likely could override Authenticator e.g. write a TMailUserRepositoryAuthenticator that injects UserRepositoryAuthenticator with the Redis part to publish the user login information.

For Redis data structure, we can likely use SET:

  • key could be the username (check for sure it does not conflict with the rate limit key,...).
  • value could be login timestamp.

We would need a webadmin route to list:

  • never active users
  • inactive users for a duration e.g. the last 3 months

We would need to have a module chooser to enable the user login recording module.

DoD

Integration tests + docs.

Could be another task to deploy the work on MU after careful testing.

@chibenwa
Copy link
Member

key could be the lastConnectionDate/{username}

@chibenwa
Copy link
Member

=> We would need a webadmin route

Please detail the webadmin route

@chibenwa
Copy link
Member

Cc @guimard

@quantranhong1999
Copy link
Member Author

Please detail the webadmin route

Could be something like:

curl -XGET /reports/users/usernameToBeUsed?status=inactive
would list the users who have never accessed TMail service

curl -XGET /reports/users/usernameToBeUsed?status=inactive&duration=3months
would list the users who have not accessed TMail service for the last 3 months

@chibenwa
Copy link
Member

chibenwa commented Nov 13, 2024

No lets actually just expose what is stored.

We can add further per user info as: ip, mail user agent

Often that's very usefull for audit.

We shall separate IMAP and SMTP

How about:

curl -XGET /reports/users/connection
[
 {
   "username": "[email protected]",
   "imap": {
       "lastConnectionDate": "XXXX",
       "ipAddress": "XXX",
       "userAgent": "XXX"
   },
   "smtp": {
       "lastConnectionDate": "XXXX",
       "ipAddress": "XXX",
       "userAgent": "XXX"
   }
  },
...
]

Then filters:

curl -XGET /reports/users/connection?activitySince=3month

would return a list of user active during this duration

And of course per user queries:

curl -XGET /reports/users/connection/btellier@linagora

 {
   "username": "[email protected]",
   "imap": {
       "lastConnectionDate": "XXXX",
       "ipAddress": "XXX",
       "userAgent": "XXX"
   },
   "smtp": {
       "lastConnectionDate": "XXXX",
       "ipAddress": "XXX",
       "userAgent": "XXX"
   }
  }

@chibenwa
Copy link
Member

I move this into TODO as this is needed for MU

Cc @guimard for review.

Cc @PatrickPereiraLinagora for review.

@hungphan227
Copy link
Contributor

hungphan227 commented Nov 28, 2024

If the data structure is just key-value, redis could be good option. However, the json document show that it is quite more complicated. Therefore, I suggest we save data in a new cassandra table.

@Arsnael
Copy link
Member

Arsnael commented Nov 28, 2024

Nope no Cassandra table for this, it's the worst option really. We dont want to create yet an other table.

The description of the ticket seems clear enough to me about Redis choice.

@Arsnael Arsnael self-assigned this Nov 28, 2024
@Arsnael Arsnael removed the grooming label Nov 28, 2024
@Arsnael
Copy link
Member

Arsnael commented Nov 28, 2024

For Redis data structure, we can likely use SET:

With the fact that we need to store different kind of infos (protocol, last connection date, ip, user agent), better to use either I think HSET https://redis.io/docs/latest/commands/hset/ or JSON.SET https://redis.io/docs/latest/commands/json.set/

@chibenwa
Copy link
Member

I prefer json.set : will look less hacky at describing an object

@Arsnael
Copy link
Member

Arsnael commented Nov 28, 2024

Alright I reviewed the ticket again as the initial proposition was relying on storing only the last connection date, and not extra data like protocol, ip address or user agent. So I feel like by overriding the Authenticator we would not have those info, thus the description is outdated IMO.

However this is what I believe we could do (and if I'm wrong please correct me):

  • IMAP => in TMail we have TMailAuthenticateProcessor, that is here to replace imap AuthenticateProcessor for TMail, where we could just add the logic of storing in redis that data? We know it's imap, and we have the imap session for the other data
  • SMTP => TMailUsersRepositoryAuthHook , replacing UsersRepositoryAuthHook in TMailCmdHandlerLoader could do the trick too? We know it's smtp, have the smtp session for other info
  • JMAP (cause I guess we need to track that too?) => trickier, I'm less sure... Could maybe do something similar and duplicate the Jmap Autneticator class to TmailAuthenticator class and redo the guice bindings? (bit of messy work here) Then we could add the login info to Redis from there (we have the HttpServer request for extra info and we know it's jmpa protocol)

WDYT?

Also for storing last login info on Redis, do we make it mandatory (of course if we use Redis), or configurable (enable/disable)?

@chibenwa
Copy link
Member

JMAP (cause I guess we need to track that too?)

No lemon do.

@chibenwa
Copy link
Member

chibenwa commented Nov 28, 2024

I'm against code specific to Tmail code.

    1. I'd like to maintain the delta of Tmail auth code as small as possible compared to community. This proposal goes against this goal.
    1. The need already did arise twice from very different context. My feeling is that it would not be the only time.

Proposal 1

What's wrong with the proposal of getting those infos through the MDC / context?

I bet it would be doable. A bit hacky but we coult still rely on Authenticator

Code snippet:

org.slf4j.MDC.get("ip")
clock.instant()
org.slf4j.MDC.get("protocol")
org.slf4j.MDC.get("user-agent")

James side i might just cost os to propagate the MDC upon line countinuations and login.


Proposal 2

How about defining an API triggered upon authentication succees / failures?

We could then make it configuration-loadable with user defined code as an extension mechanism.

We could then

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

No branches or pull requests

4 participants