-
Couldn't load subscription status.
- Fork 3
Personal Access Tokens Management RFC #91
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
Conversation
a8b8aac to
4b97939
Compare
4b97939 to
cf5d4c0
Compare
f9025d4 to
d3f1a22
Compare
|
|
||
| While this approach works well for user-interactive scenarios as the web UI, it may not be ideal for programmatic access or third-party integrations because: | ||
|
|
||
| * to perform initial login, a username/password pair needs to be known/stored by the client application, which may not be feasible or secure in all cases |
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.
Question: How would this scenario apply to the case where SSO is enabled and configured in Trento? Would PAT still be enabled?
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.
good question.
Quick answer would be regardless of SSO, users in trento would be able to generate PATs.
Let me dig deeper into it and get back to you more precisely.
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.
Left some comments on some little things :)
d3f1a22 to
a5c24a1
Compare
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.
Hey man, from a technical stand point it is ready to be included in the docs page, when you are done please add the reference here
https://github.com/trento-project/docs/blob/main/trento-docs-site/modules/developer/nav_developer.adoc?plain=1#L34
*** xref:rfc/0002-personal-access-tokens-management.adoc[2. Personal Access Tokens Management and Authentication]
About the content it good to read but like @antgamdia mentioned some open question stay :)
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.
Thanks @nelsonkopliku for making room for the conversation with this document.
IMHO the overall intention is correct, yet some details need to be expanded in order to see if the solution sounds.
More specifically, to complete this RFC I think we must have clear:
- the characteristics of the token
- the structure of the storage for the token
- how this token I used by other services (or just by Wanda)
I left some comments on the way, but the main comment is that.
Let's iterate on it 💪🏼
|
@balanza thanks for the feedback! I addressed the immediately actionable ones in this commit 33000ba For the rest:
let's sort out these two comments
this comment might help
the idea is what gets depicted in this discussion, which shortly put is having wanda make requests to relevant web apis with regards to tokens |
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.
Hey @nelsonkopliku thanks for the rfc, for me it lgtm, it is included in antora and build properly double checked it.
Feel free to include content changes, but from my side feel free to merge!
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.
Really good content.
I personally like the usage of JWT with the current header.
It might include some additional logic in the backend validation part, but it is easier to use.
Regardless stateless tokens, we already need to check whether the coming user is disabled or not, so at the end, we check the database for information. I guess we will need to do the same with these new tokens.
Wanda authorization is the tricky part. Antonio's proposal looks a good idea. It will make things more complex, as wanda will need to know about web hosting information, but well, i don't have better ideas so far.
We could leave this Wanda thread "open" while we start working web part, so we can be open to other alternatives
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.
Thank you @nelsonkopliku
Great job
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.
@nelsonkopliku thank you for adding more detail about the intended solution, now I can understand the rationale behind the choices we are making.
The overall architecture sounds to me. I left some feedback focusing on the token revocation check
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.
Apologies for coming up with a second review, but it occurred to me that we have a problem we need to discuss before approving.
All PATs are invalidated at every signing key rotation, and that might be a problem with some of our users' policy. It might not as well, but we must be clear on the drawback imho.
More details in the comment below.
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 see that we have an ad-hoc spec/implementation of what an Oauth/Oidc spec compliant solution can offer out of the box. Most Oauth/Oidc compliant identity provider solutions/specs/libraries undergo a period of scrutiny in both conceptual and usage/implementation based scenarios, that allows for their limitations to be better understood and accounted for by user/client applications. The fact that we need to rediscover/re-invent what is common knowledge/practice in the Oauth/Oidc space, can now be seen e.g. with certain observed limitations as highlighted by others. Since this has been a conscious choice to favor something identity related that is bespoke/not an industry standard, we must try to enable the immediate use case for which this RFC has been written (for now), but also be prepared for a more well-discovered/industry standard solution when the time is more appropriate.
94ef311 to
7ba1396
Compare
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.
Thanks!
Here's a link to the visual version of it
We agreed on focusing only on token generation/revokation.
Edit/regeneration is left out and references removed in this commit 87b4306