TheThingsNetwork / lorawan-stack

The Things Stack, an Open Source LoRaWAN Network Server
https://www.thethingsindustries.com/stack/
Apache License 2.0
974 stars 306 forks source link

Replace ContactInfo with UserIdentifiers as Administrative/Technical/Security Contacts #4930

Closed htdvisser closed 1 year ago

htdvisser commented 2 years ago

Summary

I think we should remove the repeated ContactInfo from entities and instead add UserIdentifiers for administrative, technical and security contacts.

What is already there? What do you see now?

Users can configure repeated ContactInfo for applications, clients, gateways, organizations and users. For users we already have their contact info (primary_email_address). For the other entities it's practically unused. It's also very annoying to maintain email addresses on many entities.

What is missing? What do you want to see?

Instead, we should allow selecting a user (or perhaps organization) as Administrative / Technical / Security contact on these entities.

How do you propose to implement this?

Question: Should it be UserIdentifiers or should it be OrganizationOrUserIdentifiers, because sometimes you can't really assign a single user.

johanstokking commented 2 years ago

Question: Should it be UserIdentifiers or should it be OrganizationOrUserIdentifiers, because sometimes you can't really assign a single user.

I think it can be an organization too. The original reason to have contact information on gateways was twofold; to decouple it from collaborators (or a single owner like in V2) but also to decouple it from individuals. With OrganizationOrUserIdentifiers we address both original reasons and it's a lot simpler to manage.

kschiffer commented 2 years ago

One issue if have with this is that it allows only registered users to be listed as contacts which I'm afraid increases the burden to use this feature, especially for administrative/security contact. I get though that this is kind of the trade-off of this change in turn for easier handling of these contacts.

htdvisser commented 2 years ago

There is no point in having unregistered users as contacts on these entities. If there's an issue with the entity, requiring us (or another user) to contact this person, they also need to have access to the entity to resolve the issue, which they can only do if they have an account with rights on the entity.

johanstokking commented 2 years ago

I think @kschiffer's scenario is more like having a non-person and non-organization contact for certain things, like the company's security department. In that case, they'd have to create an organization now.

htdvisser commented 2 years ago

Still TODO:

johanstokking commented 2 years ago
  1. Can I already implement the PBA part if the fields aren't public yet? It looks like all preparatory work can be done, that is, use the new fields if they are set and add TODOs for removing usage of the old field
  2. Bit out of scope here but still: tenant is missing the new fields. PBA uses that for the registration (see https://github.com/TheThingsIndustries/lorawan-stack/blob/v3.19/pkg/packetbrokeragent/agent.tti.go#L118-L125)
  3. If we have OrganizationOrUserIdentifiers, PBA still needs the email address, because that is what Packet Broker recognizes. Alternatively, we'd have to add a contact method in PBA to contact an entity (user or organization) in a service (like TTS) and/or a public contact form URL
htdvisser commented 2 years ago

A little update here:

johanstokking commented 2 years ago
  • Making administrative_contact and technical_contact fields public was already done as part of
  • For PBA I think @johanstokking can get started with what we already have.

The part that I think I still miss, or at least don't know how to do is:

3. If we have OrganizationOrUserIdentifiers, PBA still needs the email address, because that is what Packet Broker recognizes. Alternatively, we'd have to add a contact method in PBA to contact an entity (user or organization) in a service (like TTS) and/or a public contact form URL

I understand that I can get as far as administrative contact admin, but I need their email address. Or, like suggested, we need to do something else in PB.

htdvisser commented 2 years ago

I don't know what's the best way to turn OrganizationOrUserIdentifiers into a single email address. For users we'd have to introduce some setting that allows them to make their email address public, for organizations we'd have to introduce that setting as well as an email address field in the first place.

The "something else in PB" was the ability to use an URL as contact info instead of an email address. Then that URL could point to some page where someone can send a message to a gateway (this could be implemented as a "message" notification type). Then of course we'd have to find a way to prevent spam...

johanstokking commented 2 years ago

Right. I'll get rid of the email address setting from TTSC to Packet Broker. We can think of a contact form with captcha in the future.

nicholaspcr commented 1 year ago

I almost forgot to create the issues regarding the usage of the new fields on the PBA and Console.

Reading this discussion again and there is the idea that Hylke introduced which implies in the creation of a option which makes the user's email email public and the organization admin/technical field public as well (or at least accessible through TTS). Link to comment

I'm not so sure as to why there is the necessity of making the user and organization public before sending the notification email, but if so, this is something that blocks the two new issues #6112 and #6113, is it not? Because the usage of the new fields would only work if said User/Organization allowed the email to be used in a public manner. I might have misunderstood the meaning of his comment and that is why I'm tagging @adriansmares.

adriansmares commented 1 year ago

I'm not so sure as to why there is the necessity of making the user and organization public before sending the notification email, but if so, this is something that blocks the two new issues #6112 and #6113, is it not? Because the usage of the new fields would only work if said User/Organization allowed the email to be used in a public manner. I might have misunderstood the meaning of his comment and that is why I'm tagging @adriansmares.

It's a matter of access - see my comment at https://github.com/TheThingsNetwork/lorawan-stack/issues/6113#issuecomment-1467729506 . The general comment is that user emails are not public information, but since the _contact fields do not contain them, it is hard to actually contact these contacts. The previous contact info was in-place, and contained all of the information. While the _contact fields are a layer of indirection, and the way in which users (be them stack users or stack components) access the email of these users/organizations is dubious.

The general problem is that there are no privacy settings here - the contact info had the advantage of being explicit - the user chose to put his name and email there. The indirection here does not have this concept available.

nicholaspcr commented 1 year ago

Thanks for the explanation on the new issue regarding some of the notations.

Regarding the points made:

The complication introduced by administrative_contact/technical_contact is that they are a layer of indirection - you no longer have access directly to names and emails. You need to actually retrieve the user / organization and find out what the email is. To make the matters even better, you can end up in a situation in which you need to follow up chains of administrative contains (entity -> organization -> organization -> ... -> organization -> user) until you reach an email.

IS doesn't allow for organizations to be nested, tried it real quick just to be sure and the following error occurs: "message": "error:pkg/identityserver:nested_organizations (organizations can not be nested)" So these fields introduce an indirection of a single jump within a tree.

Regarding the retrieval of the email, I think a RPC with cluster authentication seems appropriate in order to fetch just the email based on the ID attached to the specific admin/technical contact

adriansmares commented 1 year ago

IS doesn't allow for organizations to be nested, tried it real quick just to be sure and the following error occurs: "message": "error:pkg/identityserver:nested_organizations (organizations can not be nested)" So these fields introduce an indirection of a single jump within a tree.

You're right, good catch. Even though ttnpb.Organization marks the contact fields are ttnpb.UserOrOrganizationIdentifiers, since an organization cannot be member of an organization, it is not actually possible for these identifiers to be organization.

Regarding the retrieval of the email, I think a RPC with cluster authentication seems appropriate in order to fetch just the email based on the ID attached to the specific admin/technical contact

For our own usage inter-component this will suffice, but keep in mind that the migration itself hampered user experience - as a user right now, all I know is an identifier. We need to think about how to make these emails available, if the referenced user allows it, to other stack users too.

nicholaspcr commented 1 year ago

You're right, good catch. Even though ttnpb.Organization marks the contact fields are ttnpb.UserOrOrganizationIdentifiers, since an organization cannot be member of an organization, it is not actually possible for these identifiers to be organization.

This is only valid if the IDs applied to the field are from the Collaborators within the entity, if fetched from a general list then the introduction of cycles is possible. Probably important when defining how to attach these IDs on the front-end, also might be a validation within the update entity operation (enforcing it to be on the collaborators list).

For our own usage inter-component this will suffice, but keep in mind that the migration itself hampered user experience - as a user right now, all I know is an identifier. We need to think about how to make these emails available, if the referenced user allows it, to other stack users too.

This is something that I'm only really seeing it within the Community version, the act of making the user control if his email can be fetched by an internal component via ID is something that in my eyes only has an applicable use of stopping spam on the Community version. In the enterprise it would be a more controlled set of users and having to enable said email for each user and organization is an extra step that should be somewhat enabled by default. What you think?

adriansmares commented 1 year ago

This is only valid if the IDs applied to the field are from the Collaborators within the entity, if fetched from a general list then the introduction of cycles is possible. Probably important when defining how to attach these IDs on the front-end, also might be a validation within the update entity operation (enforcing it to be on the collaborators list).

There is a backend check for this already - see https://github.com/TheThingsNetwork/lorawan-stack/blob/799609f83e2cdf3171c79eb8a55b11981632c1e3/pkg/identityserver/organization_registry.go#L250-L255

This is something that I'm only really seeing it within the Community version, the act of making the user control if his email can be fetched by an internal component via ID is something that in my eyes only has an applicable use of stopping spam on the Community version. In the enterprise it would be a more controlled set of users and having to enable said email for each user and organization is an extra step that should be somewhat enabled by default. What you think?


There are two separate issues to be discussed:

  1. How do stack components retrieve the actual contact information ? As mentioned above, this can be solved via extra RPCs - you give it EntityIdentifiers and it gives you the actual user information.
  2. How do stack users retrieve the actual contact information ? This is not so straight forward, and not only a CE issue. Users currently add collaborators under the assumption that this does not reveal information about themselves, and in general there are privacy issues attached to providing the email everywhere. As things stand right now, I would avoid making the email addresses public at all for non-admin users (and admins can already retrieve the user based on the user ID).
adriansmares commented 1 year ago
  1. How do stack components retrieve the actual contact information ? As mentioned above, this can be solved via extra RPCs - you give it EntityIdentifiers and it gives you the actual user information.

For the record, I have revisited this. Components can retrieve the contact information even today due to the following behaviors.

  1. The contact fields (administrative_contact and technical_contact) are available to the components themselves in the same way the contact_info is available today.
  2. Cluster authentication implies READ rights over all entities, at least from UserRegistry and OrganizationRegistry. As such, it is possible already for a component like PBA to retrieve the name and email of the contact, when the contact is set.
  1. How do stack users retrieve the actual contact information ? This is not so straight forward, and not only a CE issue. Users currently add collaborators under the assumption that this does not reveal information about themselves, and in general there are privacy issues attached to providing the email everywhere. As things stand right now, I would avoid making the email addresses public at all for non-admin users (and admins can already retrieve the user based on the user ID).

This requires the concept of privacy settings to be more well defined in The Things Stack. This is still to be revisited.

nicholaspcr commented 1 year ago

My bad on the delay in writing in here.

What I have on my mind is the following:

The following are the possible scenarios that I can think of regarding how do users interact with the admin/tech contacts:

  1. The system component wants to have access to the admin/tech contact of an entity. a. As Adrian pointed out Cluster auth implies in READ rights over all entities, so there shouldn't be a problem in here.
  2. An Admin wants to see the email of an admin/tech contact in an entity, he should have sufficient rights to access the information even if the other admin who set the email specified to not be public.
  3. An standard user wants to see the admin/tech email attached to an entity a. The email is not public and he cannot access it, then there should be a restricted information as a placeholder b. The email is public and the user will see the email normally.

With these points in mind I'm wondering how complex the privacy settings should be. I also assume that 95% of current users have their emails as not public in the ContactInfo and their primary_email is also private information.

The first thing that comes to mind is to develop a bit more to who an user can share the email, the public option present in the ContactInfo is somewhat broad. What if we were to specify where the user's email would be shared? Meaning that if he sets his email as the admin/tech contact of an entity, he would also have to specify with whom he is sharing said email information. That way users can avoid sharing the email of contact with all the collaborators of the entity in question. So in short, when adding his email as admin/tech contact in the entity, the user can also specify with which collaborators the email is visible.

I don't think there is a reason for users who are not collaborators to have access to the email attached to the entity in which the collaborators are listed, so before we develop a bit more into how the rights to view the admin/tech contact will be defined I want to know if the suggestion is good enough @adriansmares .

johanstokking commented 1 year ago

Meaning that if he sets his email as the admin/tech contact of an entity, he would also have to specify with whom he is sharing said email information. That way users can avoid sharing the email of contact with all the collaborators of the entity in question. So in short, when adding his email as admin/tech contact in the entity, the user can also specify with which collaborators the email is visible.

I think this gets too complicated.

I would say that admins can see everything indeed, also from other admins.

And I would say that if a user decides to set themselves as admin/tech contact, other collaborators with read access can see that.

nicholaspcr commented 1 year ago

The process like this would indeed be easier, to the point where I'm wondering if we need privacy settings. The only case would be if a non collaborator would want to see the admin/tech contact, in that case we can keep it similar to how it was before where we have a public field which sets if people outside of the organization can see the contact or not.

adriansmares commented 1 year ago

And I would say that if a user decides to set themselves as admin/tech contact, other collaborators with read access can see that.

The challenge is that a user can be set as a admin/tech contact by someone else, without approval, and then you can just scrape the users by inviting all of them as collaborators and manually setting them as collaborators. We also don't have the concept of 'who created this collaborator' such that we could answer the question 'user revealed himself'.

I think that the privacy settings don't necessarily have to be too complicated - we could have 3 levels (private, other collaborators, public) inside the collaborator entity itself, and that would control what is revealed about the collaborator. The contacts would have to be public.

nicholaspcr commented 1 year ago

I'm wondering if this is a bit of overkill or not, because the scenario to make this a valid option has to assume that the admin has bad intentions which it could be case but is that applicable to other places in the Stack? Admins have access to the user management page on the console which shows the users's emails.

adriansmares commented 1 year ago

I'm wondering if this is a bit of overkill or not, because the scenario to make this a valid option has to assume that the admin has bad intentions which it could be case but is that applicable to other places in the Stack? Admins have access to the user management page on the console which shows the users's emails.

This is not about admins - admins already have access to everything, including user lists and emails. This is mostly about community members interacting one with another - we don't want people to accidentally have their email address leaked due to our bad designs.

nicholaspcr commented 1 year ago

True, misread it. I read it as only admins can set the admin/tech contacts but indeed anyone can do it. Then yeah it makes sense to have the mentioned privacy settings.

Will create an issue for specifying the privacy settings for the User's email then.

Having this established the pending things related to the replacement of the ContactInfo are:

Does this sound correct to everyone?

adriansmares commented 1 year ago

Use privacy settings (public, other collaborators, private) - to be created

More specifically, for the collaborator entity, which currently only tracks the rights.

Console front-end flows regarding adding/removing the admin/tech contacts

https://github.com/TheThingsNetwork/lorawan-stack/issues/6112 handles this.

removing the deprecated ContactInfo this can only be done on v4 as is a change in IS API.

This is correct.

johanstokking commented 1 year ago

The challenge is that a user can be set as a admin/tech contact by someone else, without approval, and then you can just scrape the users by inviting all of them as collaborators and manually setting them as collaborators. We also don't have the concept of 'who created this collaborator' such that we could answer the question 'user revealed himself'.

You mean "as admin/tech contacts" (where emphasized)?

I really like to keep things simple, especially if this only affects the community network in practice.

Can we circumvent collaborator privacy flags by only allowing to set yourself as admin/tech contact, or let an admin do that? I.e. avoid that anyone can make anyone else admin/tech contact?

adriansmares commented 1 year ago

You mean "as admin/tech contacts" (where emphasized)?

Yes.

Can we circumvent collaborator privacy flags by only allowing to set yourself as admin/tech contact, or let an admin do that? I.e. avoid that anyone can make anyone else admin/tech contact?

It could work, with a setting in the Identity Server which controls this behavior (that you can only set yourself as a contact).

johanstokking commented 1 year ago

with a setting in the Identity Server which controls this behavior (that you can only set yourself as a contact).

Or make this IS configuration?

adriansmares commented 1 year ago

Or make this IS configuration?

Indeed, that's what I meant.

nicholaspcr commented 1 year ago

I really like to keep things simple, especially if this only affects the community network in practice. Can we circumvent collaborator privacy flags by only allowing to set yourself as admin/tech contact, or let an admin do that? I.e. avoid that anyone can make anyone else admin/tech contact?

I like the idea.

Created the issue for that idea, if I'm not mistaken #6112 is blocked until #6323 is finished, since I imagine that it implies in different screens depending on the IS config value.

johanstokking commented 1 year ago

@nicholaspcr can you check the status here? Is the only thing left that Packet Broker takes the identifiers from the new fields?

nicholaspcr commented 1 year ago

@johanstokking with #6112 and #6323 we solve two of the three items listed in here. Since the removal of the deprecated field can only happen in the v4, the issue itself is blocked on that.

I double-checked on the notifyInternal method responsible for determining who are the receivers as well, everything seems okay implementation wise.

I'm thinking that we could leave this on quarterly milestone or create a dedicated milestone for issues related to removal of deprecated fields on a new major API version. @KrishnaIyer what you think its best for keeping track on this type of issue?

johanstokking commented 1 year ago

Thanks.

Still, functionally, Packet Broker relies on the old fields, and it should use the new fields, right?

I'm thinking that we could leave this on quarterly milestone or create a dedicated milestone for issues related to removal of deprecated fields on a new major API version. @KrishnaIyer what you think its best for keeping track on this type of issue?

We have bump/major for that. That would take a new issue that is specific about the breaking change though. We already have a couple of issues with that label.

nicholaspcr commented 1 year ago

Still, functionally, Packet Broker relies on the old fields, and it should use the new fields, right?

I'm a bit late responding to this but I'm failing to see where the PacketBroker is using the old contacts, looking at the code in pkg/packetbrokeragent there seems to be support for the admin/tech contact. I could be making a mistake and looking at the wrong place, let me know if that is the case

nicholaspcr commented 1 year ago

Still, functionally, Packet Broker relies on the old fields, and it should use the new fields, right?

Went through the files in PBA to better understand and indeed the methods use the ContactIInfo field which should be deprecated to juggle the admin/tech contacts around. I opened a PR to change some of the behaviors that I found while going through the files.

Initially I thought you were referencing the repositories present in https://github.com/packetbroker but that also seems to include the admin/tech contacts in the go-api v2 and v3. Regarding the contents of PBA, still needs changes to eliminate the usage of the ContactInfo, that includes the tenant.proto on enterprise and updating structs like the RegistrationInfo in the packetbrokeragent/agent.go.

nicholaspcr commented 1 year ago

Moving this to the next milestone as its not something urgent and the changes to PBA are still in review.

Regarding the contents of PBA, still needs changes to eliminate the usage of the ContactInfo, that includes the tenant.proto on enterprise and updating structs like the RegistrationInfo in the packetbrokeragent/agent.go.

After discussing a bit with Adrian I'm inclined to leave the removal of ContactInfo field in the tenant.proto and its usage for the bump/major issue. The reason for this change was to avoid updating the emails in many places when it changes, but a tenant is by definition singular.

Meaning that leaving as it currently is on the tenant proto doesn't impact performance, so the change would be only for setting the code standard on how to handle emails, which I believe is not something that should be prioritized in here but rather have it done when moving to v4.

In short, I think its appropriate to close this issue after #6430 is merged and leave the mentioned changes to a new issue describing what should be done on a major bump.