-
Notifications
You must be signed in to change notification settings - Fork 32
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
Feature: provide getting auth token and user profile api for client-server used. #47
Conversation
0ef2ab9
to
048040c
Compare
Codecov ReportBase: 92.52% // Head: 92.77% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #47 +/- ##
===========================================
+ Coverage 92.52% 92.77% +0.24%
===========================================
Files 224 229 +5
Lines 3117 3223 +106
Branches 370 391 +21
===========================================
+ Hits 2884 2990 +106
+ Misses 169 168 -1
- Partials 64 65 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
048040c
to
69b7f59
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.
Other part LGTM.
public async authIdentity(ctx: KoaContext) { | ||
const username = ctx.request.query['username'] as string; | ||
const password = ctx.request.query['password'] as string; | ||
if (!username || !password) | ||
throw new Error('please provide "username" and "password".'); | ||
|
||
if ( | ||
!(username in this.usersCredentials) || | ||
!(md5(password) === this.usersCredentials[username].md5Password) | ||
) | ||
throw new Error('authenticate user identity failed.'); | ||
|
||
const token = Buffer.from(`${username}:${password}`).toString('base64'); | ||
|
||
return { | ||
token: token, | ||
}; | ||
} |
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 think this function ( and the /auth endpoint) only provide the information of authenticators, e.g. token, redirect URL ...etc. But we don't need to do auth check here, it is the job of authCredential
function, we don't need to duplicate the logic here. So do other authenticators.
public async authIdentity(ctx: KoaContext) {
const username = ctx.request.query['username'] as string;
const password = ctx.request.query['password'] as string;
if (!username || !password)
throw new Error('please provide "username" and "password".');
// encode it anyway, let users fail at authCredential function
const token = Buffer.from(`${username}:${password}`).toString('base64');
return {
token: token,
};
}
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 @oscar60310 the for suggestion, after discussion I have made it only generate the token, and provide another API /auth/user-profile
to get the user data with the token which got from /auth/token
if (isEmpty(options)) | ||
throw new Error( | ||
'please set at least one auth type and user credential when you enable the "auth" options.' | ||
); |
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.
NIT: Can we check the config in onActivate function?
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 for @oscar60310 suggestion. it has been fixed.
if (isEmpty(this.options)) | ||
throw new Error( | ||
'please set at least one auth type and user credential when you enable the "auth" options.' | ||
); |
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.
Can we check the option at onActivate
function too?
if (this.enable && isEmpty(xxxxx
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 for @oscar60310 suggestion. it has been fixed.
if (authenticator.activate) await authenticator.activate(); | ||
} | ||
// setup route | ||
this.setAuthRoute(); |
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.
Can we only mount our routes only when extension is enabled?
if(this.enable) this.setAuthRoute();
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 for @oscar60310 suggestion, it has been fixed.
} | ||
|
||
private setAuthRoute() { | ||
this.router.get(`/auth`, async (context: KoaContext, next) => { |
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'd prefer using POST instead of GET to send our credentials.
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.
Ought this route to be public? For now, it's protected by authCredentialMiddleware
, that is, we have to send credentials before knowing how to get one.
❌
curl --location --request GET 'http://localhost:3000/auth?type=basic&username=ivan&password=123'
✅
curl --location --request GET 'http://localhost:3000/auth?type=basic&username=ivan&password=123'
--header 'Authorization: Basic aXZhbjoxMjM='
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 for @oscar60310 suggestion and finding the issue. The API has changed to POST and rename it to /auth/token
. Otherwise, the original calling the /auth
also needs the --header 'Authorization: Basic aXZhbjoxMjM='
issue also has been fixed, thanks a lot!
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 also add more test cases related to authentication in the integration-testing
package and change the structure.
// Assert | ||
expect(response.statusCode).toEqual(400); | ||
expect(response.body).toEqual(expected); | ||
server.close(); |
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.
We should close server in afterEach hooks, or they become dangling if test cases failed.
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 for @oscar60310 suggestion. it has been fixed.
- add "AuthRouteMiddleware" for creating each route by indicated auth options. - add "authIdentity" for authenticate user identity in authenticator. - add test cases for auth identity and "AuthRouteMiddleware". - add disable options of "auth" in influenced test cases.
…tic to "onActivate" method
…vate" method of auth route middleware
…der issue - add the /auth route checking in auth credential middleware. - refactor integration-testing example structure and add authenticate example
…e token - change "/auth" to "/auth/token" and rename "authIdentity" method to "getTokenInfo". - remove checking user credential logistic in "getTokenInfo" method. - update all related test cases
- Update the get method of "/auth/token" to post method. - Update getting request fields source in "basic", "password-files" and "token" in "getTokenInfo" method of authenticator. - Update related test cases.
6c77597
to
8a3ad94
Compare
…tring source auth data - add "authSourceNormalizerMiddleware" and its options to support client providing auth data from query string or payload. - rename "authRouteMiddleware" to "authRouterMiddleware". - create "BaseAuthMiddleware" to collect same logistic part for activate method in auth credential and auth router middleware. - remove "is-base64" package. - add "authSourceNormalizerMiddleware" test cases. - add new test cases for testing auth api with providing query string or paylod auth data source.
8a3ad94
to
ac2d8d5
Compare
… jest run correct. - import 'koa-bodyparser' in each authenticator because koa-bodyparser has define it under the koa types in node_module. - remove defined 'BodyRequest'
Hi @oscar60310, it has been fixed and provide new API for getting user profile, thanks for discussing with me and giving suggestion 😃 |
…n options, and set disable for auth in "vulcan.yaml" template in labs project
Description
Provide an getting token info POST API
/auth/token
and get user profile GET API/auth/user-profile
for client-server to authenticate and get user information. Otherwise, we also support providing auth credentials data ( e.g: token ) from query string or payload. So it not only has one way to provide byAuthorization
in the header.We will talk about it in two sections.
How to use
/auth/token
and/auth/user-profile
APIThe
/auth/token
POST API is used to get tokens by passing user identity information, e.g:username
orpassword
. After you get the token, you could continue to get the current user's basic information by GET/auth/user-profile
with the above got token.Also if you would like to query the data endpoint API, you also need the token.
Set
auth
options to make API work first.The
type
value should provide satisfy the key name underauth
options, e.g:basic
,simple-token
,password-file
.. and so on.Sample 1
Then you could access auth and get a token with API
/auth/token
endpoint.Sample 2
If you also added
password-file
auth type options in settings:Then the auth API
/auth/token
withtype
could usesimple-token
andbasic
.Error message
If you do not provide
type
query string, it will show400
status code with the message, like below:Other payloads like username, password
Current built-in authenticators:
simple-token
,password-file
andbasic
both need to be givenusername
andpassword
fields in the payload.If the client missed some field e.g:
username
orpassword
it will throw an error message with status400
:After you get the token e.g:
dXNlcjE6dGVzdDE=
, then you could get the user profile following the above/auth/user-profile
GET API we talked.Just call the GET API
/auth/user-profile
and set theAuthorization
with token value e.g:dXNlcjE6dGVzdDE=
in request header.Customize your Authenticator
If the above built-in
basic
,password-files
,simple-token
authenticator could not satisfy, we could define theauthenticator
, you should inheritBaseAuthenticator
and implement thegetTokenInfo
andauthCredential
method.Support providing token or other related auth credentials data from query string or payload
We also define another middleware called
authSourceNormalizerMiddleware
to support clients who could pass the auth credentials data query string or payload.Set the
auth-source
optionsDefault, the client could through
auth
key and base64 with JSON stringify flow to encode yourAuthroizartion
key with value. like below:Then we will find the
auth
in the query string and check if the format is Base64 or not, if yes, we will try to decode it and move it to the header, but, currently, we only accept theAuthorization
key name in the object. ( We will provide a whitelist feature to make user could define the acceptable key name in the object )If you would like to change the key name like
x-auth
to replace theauth
key, or you would like to setx-auth
by payload, the you could define it inauth-source
options:like above, then when you send a query to the data endpoint e.g:
/orders
, it looks like below:Commit Message
server.close
to after each function in test casesonActivate
methodonActivate
method of auth route middlewareAuthorization
header issue/auth/token
endpoint to post method/auth/user-profile
api to get user profile.vulcan.yaml
template in labs project