fabric8-services / fabric8-auth

Identity and Access Management for fabric8 services
https://auth.openshift.io/api/status
Apache License 2.0
14 stars 26 forks source link

[User deactivation] expose endpoint to list the users to deactivate #791

Closed xcoulon closed 5 years ago

xcoulon commented 5 years ago

Part of the User Deactivation Epic

This endpoint will return a list of usernames that did not log on the platform for a given time (could be a checked query param in the request, with a default value if not specified) The number of returned usernames will be limited to a value (could be a checked query param in the request, with a default value if not specified).

alexeykazakov commented 5 years ago

I still believe that the goroutine which monitors and initiates deactivation should be in auth service. Admin console which is for manual tasks initiated by admins/humans. And this deactivation is a fully automated service. Having proper logging is important though and we need to thing about that aspect. So, should we really expose any endpoint to list users to deactivate?

cc: @sbryzak

sbryzak commented 5 years ago

I tend to agree with @alexeykazakov , the admin console is intended to be a tool for admin users to perform various management functions, not a platform for hosting automated business processes. The auth service is already responsible for performing asynchronous workflows (token cleanup for example) so it makes sense to expand on this existing functionality to drive the user deactivation process. With this in mind, it seems there is no purpose in having an endpoint to list users to deactivate, as it becomes a moot point in light of the fact that auth service will be driving the process internally.

On the note of logging, does sentry support such a feature as tags or labels that may be used to flag certain logging events for the purpose of easily identifying log messages generated by the deactivation workflow?

xcoulon commented 5 years ago

Ok, it makes sense not to use the admin console for such an operation, indeed. I also suggested that we could look at batch processes, so we wouldn't need to use table locks to prevent concurrent triggers of such operations (token cleanup, user deactivations, etc.). In that case, the batch process could call auth to get a paginated list of users to deactivate, and proceed with "batch" of accounts to deactivate.

Which also leads to another question: do you think we may need to include some "tempo" while deactivating users, so that tenants can be deleted, but at a not-too-fast pace ? cc: @MatousJobanek

alexeykazakov commented 5 years ago

Batch job would require the separated CI/CD and other infra stuff for the batch thing. I think a goroutine with some table locking functionality would be an easier solution. Similar to what we have in tenant update.

MatousJobanek commented 5 years ago

@xcoulon what do you mean by "tempo"? Some timeout when you would wait until the tenant namespaces are removed? If you don't make the call in separated goroutine but you wait for the response returned from the tenant service then there is no need for waiting - the tenant service removes the namespaces and tenant from the DB and then replies (it's not executed in separated goroutine). If there is some problem with the deletion, then it replies with the error.

xcoulon commented 5 years ago

@xcoulon what do you mean by "tempo"? Some timeout when you would wait until the tenant namespaces are removed? If you don't make the call in separated goroutine but you wait for the response returned from the tenant service then there is no need for waiting - the tenant service removes the namespaces and tenant from the DB and then replies (it's not executed in separated goroutine). If there is some problem with the deletion, then it replies with the error.

@MatousJobanek ok, thanks for confirming. I was worried that under load, the tenant removal could start having issues

xcoulon commented 5 years ago

closing this issue, there is no need for an endpoint to expose the list of users to deactivate for now: after discussion, we will use a go routine in the auth service to trigger the deactivation