[GitHub] [couchdb] nickva commented on a diff in pull request #4070: Fix #3960

[GitBox <gi...@apache.org>]

nickva commented on code in PR #4070:
URL: https://github.com/apache/couchdb/pull/4070#discussion_r901828803


##########
src/couch/src/couch_db.erl:
##########
@@ -742,18 +742,16 @@ is_authorized(#user_ctx{name = UserName, roles = UserRoles}, Security) ->
         false -> check_security(names, UserName, Names)
     end.
 
-check_security(roles, [], _) ->
-    false;
-check_security(roles, UserRoles, Roles) ->
+check_security(roles, [_ | _] = UserRoles, [_ | _] = Roles) ->
     UserRolesSet = ordsets:from_list(UserRoles),
     RolesSet = ordsets:from_list(Roles),
     not ordsets:is_disjoint(UserRolesSet, RolesSet);
-check_security(names, _, []) ->
-    false;
-check_security(names, null, _) ->
+check_security(roles, _, _) ->
     false;
-check_security(names, UserName, Names) ->
-    lists:member(UserName, Names).
+check_security(names, UserName, [_ | _] = Names) ->
+    lists:member(UserName, Names);
+check_security(names, _, _) ->

Review Comment:
   One behavior difference it seems is previously `check_security(names, null, [<<"foo">>, null])` would have returned `false` but with the new code it would return `true`. However, I don't know how realistic is to have `null` in the list of `Names`, perhaps it's even impossible but since it's the authorization code we should triple check that. (cc:  @janl / @rnewson what do you think?)



-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org