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

Removes privileges enum. #55

Merged
merged 5 commits into from
Feb 9, 2024
Merged

Removes privileges enum. #55

merged 5 commits into from
Feb 9, 2024

Conversation

KevinJoiner
Copy link
Contributor

Switched types to int64 to match the values used in claims. Updated names to have vehicle prefixe since we have multiple sets of privileges

Copy link
Member

@elffjs elffjs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the prefixing is good.

@elffjs
Copy link
Member

elffjs commented Feb 9, 2024

Ohh is it just the cast from the type to int64? I had assumed it was int vs int64. Maybe we change the middleware signature instead? I'm open to that. You get the veneer of type safety.

@KevinJoiner
Copy link
Contributor Author

@elffjs I would prefer to change the middleware as long as the blast radius isn't too bad. I don't believe the JWT decoding will be affected.

@elffjs
Copy link
Member

elffjs commented Feb 9, 2024

Yeah go for it. It will be a minor pain when someone upgrades the shared library.

Comment on lines 10 to 14
ManufacureMintDevice Privilege = 1
// ManufactureDistributeDevice provides access to distributing a device.
ManufactureDistributeDevice Privilege = 2
// ManufactureFactoryReset provides access to factory resetting a device.
ManufactureFactoryReset Privilege = 3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manufacturer? (misspelled in the first line)

switch p {
case NonLocationData:
// String returns the string representation of a VehiclePrivilege.
func (v Privilege) String() string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add cases for the manufacturer ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I am going to get rid of it since we can't distinguish ManufacturerMintDevice and VehicleNonLocationData in any meaningful way.

@KevinJoiner KevinJoiner merged commit ca20a1c into main Feb 9, 2024
2 checks passed
@elffjs elffjs deleted the remove-priv-enum branch February 17, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants