craft-oa / gdpr-invitation

0 stars 0 forks source link

How to handle future users, which exist in the database ? #30

Closed withanage closed 4 weeks ago

withanage commented 2 months ago

If the user is in future,

when an invitation for a future date exists, we cannot search this user by email.

https://craft-test.online/9658/index.php/publicknowledge/api/v1/users?searchPhrase=coauthor@mailinator.com+&status=all

Screencast from 25.09.2024 17:32:36.webm

https://github.com/pkp/pkp-lib/blob/main/classes/user/Collector.php#L508

defstat commented 2 months ago

@withanage I am wondering if here the question is whether we should consider the existing invitations when we are searching for user accounts from the search tool of the invitation creation tool?

withanage commented 2 months ago

@defstat

The reason is that the collector here checks the date.

Therfore I think, we have to define exactly what should happen, before we decide implmentation

defstat commented 2 months ago

@withanage, is this related to the invitations back office, or it is a broader question regarding user management?

withanage commented 2 months ago

@defstat , i think it is a little general.

I would may be expect e.g. we can geta all status , whether we want future roles in the API.

Currently you only get active users.

https://craft-test.online/9658/index.php/publicknowledge/api/v1/users?searchPhrase=coauthor@mailinator.com+&status=all

asmecher commented 2 months ago

The API is behaving as it was designed, with [contextPath]/api/v1/users only returning users who are involved in the current journal:

In my opinion the query builder / API is behaving like it should, and changing it to start returning data for users who are active in other journals will be breaking GDPR by exposing their data. What's needed instead, I think, is a way to check user existence by email address without returning the data. This should be done using a HTTP HEAD request to the API.

ewhanson commented 2 months ago

@asmecher I think this sounds like a fine approach. I would assume there is a global constraint on emails in the system, not limited to a context, so this would need to be a site-wide endpoint, correct? Something like HEAD [SITE_URL]/api/v1/users/exists?email=test@example.com which could return 200 or 404 depending on the situation (or another error if something else was wrong).

Edit: As a bonus the endpoint could also check by username as well if that were useful.

asmecher commented 2 months ago

Yes, there's a site-wide constraint on emails -- but we won't be able to use the site-wide URL to do this, because it's not necessarily going to be on the same domain, and accordingly, the manager might not be logged into it.

ewhanson commented 2 months ago

@asmecher, right, that makes sense. The same type of endpoint for a context-based endpoint should work the same. But this approach sounds good and doesn't expose any more information about a user than if a user tried to register for the site with an email that was already in the system, so šŸ‘.

ewhanson commented 2 months ago

To bypass any issues around doing queries that are scoped to a context, a common Laravel way of doing this is kind of pretending like you're submitting a form in the first placeā€”using the user Validator to see if the passed email would pass the validation rules for creating a user and returning a 200 or 404 based on the results of that. I think a similar approach could work here as well.

asmecher commented 2 months ago

@ewhanson, I think HEAD and GET are supposed to be kept parallel, so that HEAD behaves just like GET only doesn't return any content. If we follow that lead, then I think it makes sense to have the existing getMany handler also handle HEAD requests -- with the only differences being that

ewhanson commented 2 months ago

@asmecher, that sounds good to me. For the additional parameter, I'd recommend we explicitly pass in an email or username rather than a more generic search query.

asmecher commented 2 months ago

@ewhanson, on further thought, I think you're right -- this is effectively a "get user by email" endpoint, which is different than the existing and complex search endpoint. We might want to just create it as a new single-purpose endpoint and not confuse the getMany function with more concerns. (We might want to add a "by username" endpoint for the same purpose too.)

ewhanson commented 2 months ago

@asmecher, that sounds good to me!