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

[Feature Request]: Change ProfileId defaulting to be an invalid profile #5440

Open
BenHenning opened this issue Jun 22, 2024 · 0 comments · May be fixed by #5482
Open

[Feature Request]: Change ProfileId defaulting to be an invalid profile #5440

BenHenning opened this issue Jun 22, 2024 · 0 comments · May be fixed by #5482
Assignees
Labels
enhancement End user-perceivable enhancements. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet.

Comments

@BenHenning
Copy link
Sponsor Member

BenHenning commented Jun 22, 2024

Is your feature request related to a problem? Please describe.

Currently, a default (unspecified) proto3 proto message will default its integers to 0. In the case of ProfileId, this means it defaults to an internal profile ID of 0.

An oversight when designing the profile management system is that the admin profile (first profile created on the device) is also initialized to 0, despite the default logged out ID being -1. This creates a potentially buggy inconsistency where code may assume a default, uninitialized ProfileId corresponds to an invalid profile when, in reality, it probably corresponds to the admin profile.

Describe the solution you'd like

ProfileId needs to be refactored to have a proper uninitialized state, which can probably be best done by introduce a logged out value:

message ProfileId {
  oneof type {
    int32 logged_in_internal_profile_id = 1;
    bool logged_out = 2; // Can be true or false; presence is sufficient.
  }
}

This should be binary compatible with the existing ProfileId, but it changes the semantics of default ProfileId to be an unspecified type, rather than a type that's logged in.

However, this requires substantial changes still throughout the codebase, namely:

  • All assumptions (including in ProfileManagementController) that '-1' means 'logged out' should be changed to use an actual logged-out state ProfileId.
  • All cases where internalProfileId is extracted from a ProfileId need to be checked to ensure that ProfileId's type is the correct logged-in case.

Describe alternatives you've considered

Haven't considered alternatives, but there are likely many approaches to this problem.

Additional context

Came up during review of #5248 but I was aware of it prior to that PR since it's existed as an issue since the introduction of profiles support.

@BenHenning BenHenning added enhancement End user-perceivable enhancements. triage needed labels Jun 22, 2024
@adhiamboperes adhiamboperes added Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. and removed triage needed labels Jun 26, 2024
@adhiamboperes adhiamboperes self-assigned this Jul 11, 2024
@MohitGupta121 MohitGupta121 linked a pull request Aug 13, 2024 that will close this issue
6 tasks
@BenHenning BenHenning added this to the 1.0 Global availability milestone Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement End user-perceivable enhancements. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet.
Development

Successfully merging a pull request may close this issue.

3 participants