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

Remove storage of the contact_info in most entities and make it read-only on User storage #6515

Closed
4 tasks done
nicholaspcr opened this issue Sep 4, 2023 · 4 comments
Closed
4 tasks done
Assignees
Labels
c/identity server This is related to the Identity Server compat/db This could affect Database compatibility in progress We're working on it
Milestone

Comments

@nicholaspcr
Copy link
Contributor

Summary

With the replacement of the ContactInfo field for (Admin|Tech)Contacts we should work to remove the field from most entities in the store.

The exception would be the user_store since its ContactInfo list was not affected by the migration done in #4996 and it might have useful information for some users. That being said the restriction on the usage of the field should still be imposed here, making it a read-only field.

Current Situation

The ContactInfo field is deprecated and a user receives a warning every time a operation involves the field.

Why do we need this? Who uses it, and when?

To restrict users who are still trying to use a deprecated field and to allow us to remove sections of the code before the major bump, making it easier down the road when we try to release a v4.

Proposed Implementation

LIst of propose changes and implementation:

  1. Remove the field from storage.
    1.1. Remove ContactInfo from the code of all entities with the exception of User
    1.2. Make migration to remove ContactInfo rows that have an entity_type other than user
  2. Restrict the ContactInfo creation on user_registry
    2.1. Make it read-only.

Contributing

  • I can help by doing more research.
  • I can help by implementing the feature after the proposal above is approved.
  • I can help by testing the feature before it's released.

Code of Conduct

@nicholaspcr nicholaspcr added the needs/triage We still need to triage this label Sep 4, 2023
@nicholaspcr nicholaspcr self-assigned this Sep 4, 2023
@KrishnaIyer
Copy link
Member

This requires a DB migration right?

@KrishnaIyer KrishnaIyer added c/identity server This is related to the Identity Server compat/db This could affect Database compatibility and removed needs/triage We still need to triage this labels Sep 6, 2023
@KrishnaIyer KrishnaIyer added this to the v3.28.0 milestone Sep 6, 2023
@nicholaspcr
Copy link
Contributor Author

Indeed it does

@nicholaspcr
Copy link
Contributor Author

@adriansmares Indeed there is one change related to this that wasn't listed on the v3.28 milestone.

Updating primary_email_address as a non admin puts primary_email_address_validated_at as nil (followed by sending a validation email to the new email address). It was my mistake as I didn't create a issue for it last week but most of the done is already done on the branch fix/user-update-email-validation.

Will create a issue for it and link it here.

@adriansmares
Copy link
Contributor

My view here is that the contact_info field itself should, for all entity types including users, become a read only view of the technical_contact and administrative_contact fields.

How this should work in my view is that given that:

  1. We have an application app1 and two users - user1 and user2.
  2. user1 is the technical contact of app1, and user2 is the administrative contact of app1.

When I get/list contact_info, I get a slice of two contacts which contain the details (i.e. emails) of user1 and user2, with public set to false, as long as I have the required access over the entity. If I don't have access, the PublicSafe translation will remove the contact info and that is fine, and we can even skip retrieving them from the storage layer.

For updates, we should just ignore the contact_info field, discard it from the field mask, and return an RPC warning (via warnings.Add) that the field update is unsupported.


Implementation wise in my view this means that basically after we Get/List/Search the entities, we do another pass in order to retrieve the emails of the users/organizations, and do follow the chain in the case the contact is an organization. As before, we should do this retrieval only if the caller really can access this information.


The contents of the table itself can be fully deleted and discarded after we have settled the email validation saga for the users, in order to decouple it from the current contact info usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/identity server This is related to the Identity Server compat/db This could affect Database compatibility in progress We're working on it
Projects
None yet
Development

No branches or pull requests

3 participants