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

Restore ability to specify token secret for API token (#800) #806

Merged
merged 4 commits into from
Jan 20, 2022

Conversation

BruceMacD
Copy link
Collaborator

@BruceMacD BruceMacD commented Jan 17, 2022

When standing up Infra it is possible to specify what the API tokens for connecting destinations and root access are as code. This requires creating/updating an API token and its associated access token.

If a token has an empty key or secret then they are generated in the token creation logic not changed by this PR.

  • Wrote appropriate unit tests
  • Considered security implications of the change
  • Updated associated docs where necessary
  • Updated associated configuration where necessary
  • Change is backwards compatible if it needs to be (user can upgrade without manual steps?)
  • Nothing sensitive logged

Resolves #800

Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

Can we avoid passing around the empty objects by instead splitting this into two functions? This behavior exists for the CreateOrUpdate function (which is probably going away?), and it sort of muddies the implementation of the non-create-or-update version. If the client had to do all this create-or-update functionality on the client-side, it seems like this workflow would break anyway.

@BruceMacD
Copy link
Collaborator Author

Can we avoid passing around the empty objects by instead splitting this into two functions? This behavior exists for the CreateOrUpdate function (which is probably going away?), and it sort of muddies the implementation of the non-create-or-update version. If the client had to do all this create-or-update functionality on the client-side, it seems like this workflow would break anyway.

Good call @ssoroka, I'll update with this suggestion.

@BruceMacD BruceMacD mentioned this pull request Jan 19, 2022
Merged
15 tasks
Copy link
Contributor

@jmorganca jmorganca left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for fixing this 💯

@BruceMacD BruceMacD merged commit 3317bcf into main Jan 20, 2022
@BruceMacD BruceMacD deleted the valid-api-token branch January 20, 2022 12:06
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.

Generated API token is not valid
3 participants