You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2020/05/14 16:24:21 UTC
[GitHub] [couchdb] rnewson opened a new pull request #2888: allow configurability of JWT claims that require a value
rnewson opened a new pull request #2888:
URL: https://github.com/apache/couchdb/pull/2888
e.g;
[jwt]
required_claims = {iss, <<"hello">>}
required_claims is now a comma-separated list of claims in erlang language
format.
fixes https://github.com/apache/couchdb/issues/2886
----------------------------------------------------------------
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
[GitHub] [couchdb] davisp commented on a change in pull request #2888: allow configurability of JWT claims that require a value
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2888:
URL: https://github.com/apache/couchdb/pull/2888#discussion_r425430870
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -209,11 +209,12 @@ jwt_authentication_handler(Req) ->
get_configured_claims() ->
Claims = config:get("jwt_auth", "required_claims", ""),
- case re:split(Claims, "\s*,\s*", [{return, list}]) of
- [[]] ->
- []; %% if required_claims is the empty string.
- List ->
- [list_to_existing_atom(C) || C <- List]
+ case Claims of
+ "" ->
+ [];
+ Claims ->
+ {ok, Parsed} = couch_util:parse_term("[" ++ Claims ++ "]"),
Review comment:
Not sure how much you care, but we don't usually mutate the value that we pass to parse_term (i.e., we require useres to add the square brackets in the config. Not sure if we care or not?
----------------------------------------------------------------
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
[GitHub] [couchdb] rnewson commented on a change in pull request #2888: allow configurability of JWT claims that require a value
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #2888:
URL: https://github.com/apache/couchdb/pull/2888#discussion_r425609279
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -209,11 +209,12 @@ jwt_authentication_handler(Req) ->
get_configured_claims() ->
Claims = config:get("jwt_auth", "required_claims", ""),
- case re:split(Claims, "\s*,\s*", [{return, list}]) of
- [[]] ->
- []; %% if required_claims is the empty string.
- List ->
- [list_to_existing_atom(C) || C <- List]
+ case Claims of
+ "" ->
+ [];
+ Claims ->
+ {ok, Parsed} = couch_util:parse_term("[" ++ Claims ++ "]"),
Review comment:
the problem is 3.1.0 released with it being a comma-separate list of strings.
----------------------------------------------------------------
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
[GitHub] [couchdb] gbshhennsi commented on pull request #2888: allow configurability of JWT claims that require a value
Posted by GitBox <gi...@apache.org>.
gbshhennsi commented on pull request #2888:
URL: https://github.com/apache/couchdb/pull/2888#issuecomment-709937667
Hey guys, in which version of couchdb will you release this fix or when will I be able to use it?
----------------------------------------------------------------
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
[GitHub] [couchdb] rnewson merged pull request #2888: allow configurability of JWT claims that require a value
Posted by GitBox <gi...@apache.org>.
rnewson merged pull request #2888:
URL: https://github.com/apache/couchdb/pull/2888
----------------------------------------------------------------
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
[GitHub] [couchdb] janl commented on a change in pull request #2888: allow configurability of JWT claims that require a value
Posted by GitBox <gi...@apache.org>.
janl commented on a change in pull request #2888:
URL: https://github.com/apache/couchdb/pull/2888#discussion_r426800112
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -209,13 +209,19 @@ jwt_authentication_handler(Req) ->
get_configured_claims() ->
Claims = config:get("jwt_auth", "required_claims", ""),
- case re:split(Claims, "\s*,\s*", [{return, list}]) of
- [[]] ->
- []; %% if required_claims is the empty string.
- List ->
- [list_to_existing_atom(C) || C <- List]
+ Re = "((?<key1>[a-z]+)|{(?<key2>[a-z]+)\s*,\s*\"(?<val>[^\"]+)\"})",
+ case re:run(Claims, Re, [global, {capture, [key1, key2, val], binary}]) of
+ nomatch ->
Review comment:
fair, thanks for explaining!
----------------------------------------------------------------
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
[GitHub] [couchdb] davisp commented on a change in pull request #2888: allow configurability of JWT claims that require a value
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2888:
URL: https://github.com/apache/couchdb/pull/2888#discussion_r426737761
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -209,13 +209,19 @@ jwt_authentication_handler(Req) ->
get_configured_claims() ->
Claims = config:get("jwt_auth", "required_claims", ""),
- case re:split(Claims, "\s*,\s*", [{return, list}]) of
- [[]] ->
- []; %% if required_claims is the empty string.
- List ->
- [list_to_existing_atom(C) || C <- List]
+ Re = "((?<key1>[a-z]+)|{(?<key2>[a-z]+)\s*,\s*\"(?<val>[^\"]+)\"})",
+ case re:run(Claims, Re, [global, {capture, [key1, key2, val], binary}]) of
+ nomatch ->
Review comment:
Seems like it'd be a good idea here to have a clause that notices when we are unable to match a configured value that's non-empty. Just something like `nomatch when Claims /= "" -> log thing`.
----------------------------------------------------------------
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
[GitHub] [couchdb] rnewson commented on a change in pull request #2888: allow configurability of JWT claims that require a value
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #2888:
URL: https://github.com/apache/couchdb/pull/2888#discussion_r426799196
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -209,13 +209,19 @@ jwt_authentication_handler(Req) ->
get_configured_claims() ->
Claims = config:get("jwt_auth", "required_claims", ""),
- case re:split(Claims, "\s*,\s*", [{return, list}]) of
- [[]] ->
- []; %% if required_claims is the empty string.
- List ->
- [list_to_existing_atom(C) || C <- List]
+ Re = "((?<key1>[a-z]+)|{(?<key2>[a-z]+)\s*,\s*\"(?<val>[^\"]+)\"})",
+ case re:run(Claims, Re, [global, {capture, [key1, key2, val], binary}]) of
+ nomatch ->
Review comment:
the user gets the http response, the admin gets the log message (who might also be the user). I didn't want to point at the specific misconfiguration to the user (who in this situation could be anyone, they might not have valid credentials). But I did want to mention that it's specific to JWT in case the user can use a different method, and so the admin has something more meaningful to report to user@ / github / etc.
----------------------------------------------------------------
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
[GitHub] [couchdb] rnewson commented on a change in pull request #2888: allow configurability of JWT claims that require a value
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #2888:
URL: https://github.com/apache/couchdb/pull/2888#discussion_r425682328
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -209,11 +209,12 @@ jwt_authentication_handler(Req) ->
get_configured_claims() ->
Claims = config:get("jwt_auth", "required_claims", ""),
- case re:split(Claims, "\s*,\s*", [{return, list}]) of
- [[]] ->
- []; %% if required_claims is the empty string.
- List ->
- [list_to_existing_atom(C) || C <- List]
+ case Claims of
+ "" ->
+ [];
+ Claims ->
+ {ok, Parsed} = couch_util:parse_term("[" ++ Claims ++ "]"),
Review comment:
I switched to a better format.
----------------------------------------------------------------
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
[GitHub] [couchdb] janl commented on a change in pull request #2888: allow configurability of JWT claims that require a value
Posted by GitBox <gi...@apache.org>.
janl commented on a change in pull request #2888:
URL: https://github.com/apache/couchdb/pull/2888#discussion_r426790732
##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -209,13 +209,19 @@ jwt_authentication_handler(Req) ->
get_configured_claims() ->
Claims = config:get("jwt_auth", "required_claims", ""),
- case re:split(Claims, "\s*,\s*", [{return, list}]) of
- [[]] ->
- []; %% if required_claims is the empty string.
- List ->
- [list_to_existing_atom(C) || C <- List]
+ Re = "((?<key1>[a-z]+)|{(?<key2>[a-z]+)\s*,\s*\"(?<val>[^\"]+)\"})",
+ case re:run(Claims, Re, [global, {capture, [key1, key2, val], binary}]) of
+ nomatch ->
Review comment:
Why not mention claims in the http response? Seems odd to require folks to look in two places.
----------------------------------------------------------------
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