Skip to content

chore: Force license uuids to not be null #6012

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

Merged
merged 5 commits into from
Feb 14, 2023
Merged

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Feb 3, 2023

What this does

All new licenses have uuids, old licenses might not. So this PR gives old licenses a random uuid to support a non-null database schema.

Future work

We should require licenses to have uuids or fail. Breaking old licenses.

@Emyrk
Copy link
Member Author

Emyrk commented Feb 3, 2023

@spikecurtis
Copy link
Contributor

spikecurtis commented Feb 3, 2023

It's unclear to me why we needed a UUID in the first place.

We had just an id index --- is it just to normalize with other DB objects where we use UUIDs for them? I don't like the idea of having two IDs for the same thing.

@Emyrk
Copy link
Member Author

Emyrk commented Feb 3, 2023

@spikecurtis I am not sure. I do not think this PR is the fix, but something should be done.

@spikecurtis
Copy link
Contributor

Looks like UUIDs were added in #5110

@kylecarbs can you bring us up to speed on why UUIDs were added and whether we can drop one or other of id uuid?

@Emyrk
Copy link
Member Author

Emyrk commented Feb 8, 2023

We could make the code accept invalid uuids if the license is generated before Feb 8, 2023 for example. Any new licenses have to have valid uuids.

@Kira-Pilot
Copy link
Member

@kylecarbs bump
This is blocking #6125. I would prefer it if we could stick with the UUID because the Audit Log needs that type and seems like every other model has it.

@spikecurtis
Copy link
Contributor

@Emyrk Is it now the case that the UUID is embedded in the signed license itself, or its it just a DB identifier local to the deployment it is installed on?

@Emyrk
Copy link
Member Author

Emyrk commented Feb 9, 2023

It is embedded to the license claims. We insert a uuid string in the JWT ID field: https://github.com/coder/license/blob/2ae8cabd512f05b58ab871d41e6327bbc405a00d/cmd/licensor/issue.go#L79-L79

@Emyrk
Copy link
Member Author

Emyrk commented Feb 9, 2023

@spikecurtis uuids were added to licenses 4 months ago: https://github.com/coder/license/commit/2d26cbd6f689b566a2e84b47d98f94748e2c7fa5

I am not sure if licenses exist without uuids. It is possible. However I think we could just force the error if the license is made after a certain date to handle any old formats.

@kylecarbs
Copy link
Member

Would this stop old licenses from working @Emyrk?

@Emyrk
Copy link
Member Author

Emyrk commented Feb 10, 2023

@kylecarbs To support old licenses that fail uuid parse, I think we would need to check the issue date (if it's in there) and allow uuid to fail for licenses before a certain date.

This PR does not do that yet.

@Emyrk
Copy link
Member Author

Emyrk commented Feb 13, 2023

I am going to generate a random uuid for licenses that do not have uuids.

This will make old and new licenses work. A followup should be to require uuids, potentially breaking old licenses.

@Emyrk Emyrk merged commit 733f58c into main Feb 14, 2023
@Emyrk Emyrk deleted the stevenmasley/license_id branch February 14, 2023 00:22
@github-actions github-actions bot locked and limited conversation to collaborators Feb 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants