-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add sign-in request for authentication token retrieval #216
base: 3.0
Are you sure you want to change the base?
Conversation
PR Review ChecklistDo not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed. Trivial Change
Code
Architecture
|
|
||
message SignIn { | ||
message Req { | ||
message Password { |
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.
Sometimes it's called UserPassword
in APIs, maybe it's better to signal that it's not just a password.
@@ -16,16 +17,18 @@ message Connection { | |||
Version version = 1; | |||
string driver_lang = 2; | |||
string driver_version = 3; | |||
|
|||
Authentication.SignIn.Req authentication = 4; |
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.
Idk how to name it, please share ideas.
|
||
package typedb.protocol; | ||
|
||
message Authentication { |
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.
Doesn't feel like a part of the User
API, so I declared a new package. In the future, there can be RefreshToken
or whatever, so it won't be totally empty, and the scope is clearer.
|
||
message Authentication { | ||
|
||
message SignIn { |
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.
Maybe this is GetToken
? Because you're sending username/pass -> token? Not specifically to do with signing in right 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.
Or just Token
, then you have Req
and Res
subtypes!
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.
It's quite a #: you # with credentials, and you get your access token for further authentication/authorization. Just like in REST, you # into a website, and then your token is cached in your cookies, so it is used for authentication/authorization for further requests. I don't mind renaming it to GetToken here though, I'd still name it a "#" in HTTP, so I decided to add some symmetry.
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.
Definitely not Token
as we can Refresh
it in the future.
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.
Well Token.Refresh makes sense too right?
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.
Token.Req
, Token.Res
, Token.Refresh.Req
, Token.Refresh.Res
? Is it a common approach?
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.
what does refresh do differently from the regular one?
otherwise we can have Token.Create.Req/Res
and Token.Refresh.Req/Res
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.
Oh, okay, this is a good option. I thought you wanted Token.Req
and Token.Res
, and then append Token.Refresh
to it.
But I'd still want it to be a part of Authentication
, because it's the general scope, and we could have some other entities of authentication in the future.
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.
ok, that works for me :)
Release notes: usage and product changes
A new mechanism of authentication tokens has been introduced to replace the old way of sending usernames and passwords through the network with every request.
Instead, all user credentials (currently, it's usernames and passwords) are sent only:
connection_open
request for authentication and authorization, with a temporary token returned;sign_in
request for #s within an established connection (to change the user or to get a new authentication token).Then, all further requests are expected to be authenticated only by temporary, less sensitive tokens.
The approach is extensible to other credential types that can be introduced in the future.
Implementation