-
-
Notifications
You must be signed in to change notification settings - Fork 714
Docs: Clarify behavior of API keys #3733
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
base: master
Are you sure you want to change the base?
Conversation
@orblivion Some initial feedback would be appreciated. Considering the definition of roleAssignment is tucked into a bullet list, I didn't want to get extensive, but I did want to give an example of the roleId pattern that was clear on the counting method of the integer. |
This might be part of your future plan for a big loud warning, but I think that we should make sure that it's clear that the default value is |
So maybe list the valid values, put (default) for allAccess, and (recommended) for roleId? |
The nice thing about setting a roleId, and perhaps your reason for recommending it, is that (if memory serves) the roleId per se is what stays connected to the token. The permissions attached to the role can change. So it gives the developer some flexibility for future versions. May be worth mentioning, but I understand if that's too much. In general, the document is getting kind of long. |
I'd worry about newcomers coming in and seeing a long doc and skipping our warnings and going straight to the template parameters. I could think about streamlining the doc, but that's a big investment which is not worth it. But we pretty much know they won't skip the parameters since at least some of them are required to make it work. So maybe for Though, if your plan was to put the big scary warning inside "Key Security Consideration" that would probably be fine to do instead. Also, I wonder if that section could be called "Security and Permissions"? Maybe get a third opinion. Okay that's all I'll say for now. |
We discussed on the call that I should clarify the default is effectively |
@orblivion I think we summarized on a call this change would be enough... I really think I shouldn't have left this one unmerged, so putting it back out here! |
Work in progress as we need to better define roleAssignment as well. I would prefer this to be like a boxed warning type of thing, but at a glance I don't see any on other pages. A small Note seemed too subtle for this.