You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/01/06 07:53:59 UTC
[GitHub] [incubator-apisix] lilien1010 opened a new pull request #1027: Jwt
token optmization
lilien1010 opened a new pull request #1027: Jwt token optmization
URL: https://github.com/apache/incubator-apisix/pull/1027
### Summary
Jwt authentication plugin comoatible that token without Bearer prefix
### Full changelog
1. t/plugin/jwt-auth.t
2. plugins/jwt-auth.lua
### Issues resolved
Fix https://github.com/apache/incubator-apisix/issues/1026
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-apisix] lilien1010 commented on a change in pull request
#1027: Jwt authentication plugin compatible with Bearer prefix
Posted by GitBox <gi...@apache.org>.
lilien1010 commented on a change in pull request #1027: Jwt authentication plugin compatible with Bearer prefix
URL: https://github.com/apache/incubator-apisix/pull/1027#discussion_r363566591
##########
File path: lua/apisix/plugins/jwt-auth.lua
##########
@@ -89,15 +90,19 @@ function _M.check_schema(conf)
end
-local function fetch_jwt_token()
- local args = ngx.req.get_uri_args()
- if args and args.jwt then
- return args.jwt
+local function fetch_jwt_token(ctx)
+ local token = core.request.header(ctx, "authorization")
+ if token then
+ local prefix = sub_str(token, 1, 7)
+ if prefix == 'Bearer ' or prefix == 'bearer ' then
+ return sub_str(token, 8)
+ end
+ return token
end
- local headers = ngx.req.get_headers()
- if headers.Authorization then
- return headers.Authorization
+ token = ngx.ctx.api_ctx.var.arg_jwt
Review comment:
ok,changed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-apisix] moonming commented on a change in pull request
#1027: Jwt authentication plugin compatible with Bearer prefix
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #1027: Jwt authentication plugin compatible with Bearer prefix
URL: https://github.com/apache/incubator-apisix/pull/1027#discussion_r363198533
##########
File path: lua/apisix/plugins/jwt-auth.lua
##########
@@ -90,16 +91,19 @@ end
local function fetch_jwt_token()
- local args = ngx.req.get_uri_args()
- if args and args.jwt then
- return args.jwt
- end
-
local headers = ngx.req.get_headers()
Review comment:
please use `core.request.header(ctx, "authorization")`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-apisix] lilien1010 commented on a change in pull request
#1027: Jwt authentication plugin compatible with Bearer prefix
Posted by GitBox <gi...@apache.org>.
lilien1010 commented on a change in pull request #1027: Jwt authentication plugin compatible with Bearer prefix
URL: https://github.com/apache/incubator-apisix/pull/1027#discussion_r363215859
##########
File path: lua/apisix/plugins/jwt-auth.lua
##########
@@ -90,16 +91,19 @@ end
local function fetch_jwt_token()
- local args = ngx.req.get_uri_args()
- if args and args.jwt then
- return args.jwt
- end
-
local headers = ngx.req.get_headers()
if headers.Authorization then
+ if sub_str(headers.Authorization,1,7) == 'Bearer ' then
Review comment:
sure , I think it's better handle `bearer `
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-apisix] lilien1010 commented on a change in pull request
#1027: Jwt authentication plugin compatible with Bearer prefix
Posted by GitBox <gi...@apache.org>.
lilien1010 commented on a change in pull request #1027: Jwt authentication plugin compatible with Bearer prefix
URL: https://github.com/apache/incubator-apisix/pull/1027#discussion_r363224464
##########
File path: lua/apisix/plugins/jwt-auth.lua
##########
@@ -90,16 +91,19 @@ end
local function fetch_jwt_token()
- local args = ngx.req.get_uri_args()
- if args and args.jwt then
- return args.jwt
- end
-
local headers = ngx.req.get_headers()
if headers.Authorization then
+ if sub_str(headers.Authorization,1,7) == 'Bearer ' then
+ return sub_str(headers.Authorization,8)
+ end
return headers.Authorization
end
+ local args = ngx.req.get_uri_args()
+ if args and args.jwt then
Review comment:
if change to ngx.ctx.api_ctx.jwt , it won't pass the test case,
I think it should be `ngx.ctx.api_ctx.var.arg_jwt`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-apisix] moonming commented on a change in pull request
#1027: Jwt authentication plugin compatible with Bearer prefix
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #1027: Jwt authentication plugin compatible with Bearer prefix
URL: https://github.com/apache/incubator-apisix/pull/1027#discussion_r363198899
##########
File path: lua/apisix/plugins/jwt-auth.lua
##########
@@ -90,16 +91,19 @@ end
local function fetch_jwt_token()
- local args = ngx.req.get_uri_args()
- if args and args.jwt then
- return args.jwt
- end
-
local headers = ngx.req.get_headers()
if headers.Authorization then
+ if sub_str(headers.Authorization,1,7) == 'Bearer ' then
+ return sub_str(headers.Authorization,8)
Review comment:
ditto
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-apisix] membphis commented on a change in pull request
#1027: Jwt authentication plugin compatible with Bearer prefix
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #1027: Jwt authentication plugin compatible with Bearer prefix
URL: https://github.com/apache/incubator-apisix/pull/1027#discussion_r363326515
##########
File path: lua/apisix/plugins/jwt-auth.lua
##########
@@ -89,15 +90,19 @@ function _M.check_schema(conf)
end
-local function fetch_jwt_token()
- local args = ngx.req.get_uri_args()
- if args and args.jwt then
- return args.jwt
+local function fetch_jwt_token(ctx)
+ local token = core.request.header(ctx, "authorization")
+ if token then
+ local prefix = sub_str(token, 1, 7)
+ if prefix == 'Bearer ' or prefix == 'bearer ' then
+ return sub_str(token, 8)
+ end
+ return token
end
- local headers = ngx.req.get_headers()
- if headers.Authorization then
- return headers.Authorization
+ token = ngx.ctx.api_ctx.var.arg_jwt
Review comment:
`ngx.ctx. api_ctx` equals to `ctx`.
So this line should be `token = ctx.var.arg_jwt` which is simpler.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-apisix] moonming commented on a change in pull request
#1027: Jwt authentication plugin compatible with Bearer prefix
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #1027: Jwt authentication plugin compatible with Bearer prefix
URL: https://github.com/apache/incubator-apisix/pull/1027#discussion_r363205726
##########
File path: lua/apisix/plugins/jwt-auth.lua
##########
@@ -90,16 +91,19 @@ end
local function fetch_jwt_token()
- local args = ngx.req.get_uri_args()
- if args and args.jwt then
- return args.jwt
- end
-
local headers = ngx.req.get_headers()
if headers.Authorization then
+ if sub_str(headers.Authorization,1,7) == 'Bearer ' then
+ return sub_str(headers.Authorization,8)
+ end
return headers.Authorization
end
+ local args = ngx.req.get_uri_args()
+ if args and args.jwt then
Review comment:
please use `ngx.ctx.api_ctx.jwt`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-apisix] moonming commented on a change in pull request
#1027: Jwt authentication plugin compatible with Bearer prefix
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #1027: Jwt authentication plugin compatible with Bearer prefix
URL: https://github.com/apache/incubator-apisix/pull/1027#discussion_r363198859
##########
File path: lua/apisix/plugins/jwt-auth.lua
##########
@@ -90,16 +91,19 @@ end
local function fetch_jwt_token()
- local args = ngx.req.get_uri_args()
- if args and args.jwt then
- return args.jwt
- end
-
local headers = ngx.req.get_headers()
if headers.Authorization then
+ if sub_str(headers.Authorization,1,7) == 'Bearer ' then
Review comment:
do we need to deal with `bearer`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-apisix] membphis merged pull request #1027: Jwt
authentication plugin compatible with Bearer prefix
Posted by GitBox <gi...@apache.org>.
membphis merged pull request #1027: Jwt authentication plugin compatible with Bearer prefix
URL: https://github.com/apache/incubator-apisix/pull/1027
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-apisix] moonming commented on a change in pull request
#1027: Jwt authentication plugin compatible with Bearer prefix
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #1027: Jwt authentication plugin compatible with Bearer prefix
URL: https://github.com/apache/incubator-apisix/pull/1027#discussion_r363206173
##########
File path: t/plugin/jwt-auth.t
##########
@@ -218,7 +218,7 @@ hello world
--- request
GET /hello
--- more_headers
-Authorization: eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJrZXkiOiJ1c2VyLWtleSIsImV4cCI6MTg3OTMxODU0MX0.fNtFJnNmJgzbiYmGB0Yjvm-l6A6M4jRV1l4mnVFSYjs
+Authorization: Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJrZXkiOiJ1c2VyLWtleSIsImV4cCI6MTg3OTMxODU0MX0.fNtFJnNmJgzbiYmGB0Yjvm-l6A6M4jRV1l4mnVFSYjs
Review comment:
please add test case for wrong value of `Authorization Bearer`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-apisix] moonming commented on a change in pull request
#1027: Jwt authentication plugin compatible with Bearer prefix
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #1027: Jwt authentication plugin compatible with Bearer prefix
URL: https://github.com/apache/incubator-apisix/pull/1027#discussion_r363198693
##########
File path: lua/apisix/plugins/jwt-auth.lua
##########
@@ -90,16 +91,19 @@ end
local function fetch_jwt_token()
- local args = ngx.req.get_uri_args()
- if args and args.jwt then
- return args.jwt
- end
-
local headers = ngx.req.get_headers()
if headers.Authorization then
+ if sub_str(headers.Authorization,1,7) == 'Bearer ' then
Review comment:
need to add space after `,`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services