-
Notifications
You must be signed in to change notification settings - Fork 23
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
Additional token patch fields #53
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
# Token value (only sudo tokens can use this) | ||
token: Optional[str] | ||
# Datetime to expire the token | ||
expire_at: Optional[datetime] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plutopulp and I had a chat about this and concluded that letting this field be patched defeats the purpose of having it. If a token has an expiry date but gets leaked the attacker can just set the expiry date to far in the future using the leaked token.
GitHub provides a feature to create a new token with an identical feature set but with a new expiry and value. Going down that route feels nicer to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do all tokens have the same permissions for users?
A reasonable use case here would be if someone needs to extend a production token expiry date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it's possible that the expire_at field can only be set by dashboard token
@@ -40,6 +40,10 @@ class TokenPatch(Patchable): | |||
name: Optional[str] | |||
# Boolean specifying if token can be used | |||
is_enabled: Optional[bool] | |||
# Token value (only sudo tokens can use this) | |||
token: Optional[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a corresponding pipeline-top PR for this? What's the use case here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APIv1 migration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to be able to set user's tokens to what they were in APIv1
No description provided.