)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"fffae5fe4f7a9205c46defe41685066d24a7718f","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Improve log for forbidden access"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Improve the log indication action and address"},{"line_number":10,"context_line":"for an unauthorized request"},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"Change-Id: I395d91d80981fc948fd5476978b28ed9ecb7db5b"},{"line_number":13,"context_line":"Closes-Bug: #1266921"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"AAAAUH%2F%2B7E8%3D","line":10,"updated":"2014-01-20 18:00:01.000000000","message":"Sorry to be a stickler about the commit message, but it does not indicate what the improvement is. The detail simply repeats what the summary says. Instead, the commit message should describe the problem that the patch solves and the way that the patch solves it.\n\nFor example, something like this would be more appropriate:\n\n Add more detailed log output in case of 403 Forbidden responses\n\n When an API action is denied by the a policy, a 403 Forbidden is returned to\n the user. However, there is no indication in the log messages what policy\n condition failed, making it difficult for an admin to explain to a user why a\n particular request returned a 403. This patch adds a detailed debug message\n to the logs to allow an admin to investigate and determine the cause of the response.","commit_id":"d7b85a02072e86525283db3e8f9e30a72ddfe0c7"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"21d965384e7ca51b2dfaba0c3581ee6ab911b554","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Improve log for forbidden access"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Improve the log indication action and address"},{"line_number":10,"context_line":"for an unauthorized request"},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"Change-Id: I395d91d80981fc948fd5476978b28ed9ecb7db5b"},{"line_number":13,"context_line":"Closes-Bug: #1266921"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"AAAAUH%2F%2B5%2Fk%3D","line":10,"in_reply_to":"AAAAUH%2F%2B7Bs%3D","updated":"2014-01-20 21:24:07.000000000","message":"Of course you can, that\u0027s why I offered them :)","commit_id":"d7b85a02072e86525283db3e8f9e30a72ddfe0c7"},{"author":{"_account_id":9450,"name":"Juan Manuel Ollé","email":"juan.m.olle@intel.com","username":"jmolle"},"change_message_id":"ae3e56a0d65140f3b10edd68efd1220f04e29163","unresolved":false,"context_lines":[{"line_number":7,"context_line":"Improve log for forbidden access"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Improve the log indication action and address"},{"line_number":10,"context_line":"for an unauthorized request"},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"Change-Id: I395d91d80981fc948fd5476978b28ed9ecb7db5b"},{"line_number":13,"context_line":"Closes-Bug: #1266921"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"AAAAUH%2F%2B7Bs%3D","line":10,"in_reply_to":"AAAAUH%2F%2B7E8%3D","updated":"2014-01-20 18:06:26.000000000","message":"sounds good. may I take some of your words?","commit_id":"d7b85a02072e86525283db3e8f9e30a72ddfe0c7"}],"keystone/common/wsgi.py":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"fffae5fe4f7a9205c46defe41685066d24a7718f","unresolved":false,"context_lines":[{"line_number":219,"context_line":"            return render_exception(e, user_locale\u003dreq.best_match_language())"},{"line_number":220,"context_line":"        except exception.Forbidden as e:"},{"line_number":221,"context_line":"            address \u003d req.environ.get(\u0027REMOTE_ADDR\u0027, \u0027\u0027)"},{"line_number":222,"context_line":"            LOG.warning("},{"line_number":223,"context_line":"                _(\u0027User from: %(remote_addr)s is not authorized\u0027"},{"line_number":224,"context_line":"                  \u0027 to perform action: %(action)s\u0027),"},{"line_number":225,"context_line":"                {\u0027remote_addr\u0027: address, \u0027action\u0027: action})"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F%2B7EI%3D","line":222,"updated":"2014-01-20 18:00:01.000000000","message":"This should definitely not be a warning. It should be a debug message. A warning is for cases where the operator should be concerned about the inner workings of Keystone or that security has possibly been breached in some way.","commit_id":"d7b85a02072e86525283db3e8f9e30a72ddfe0c7"},{"author":{"_account_id":9450,"name":"Juan Manuel Ollé","email":"juan.m.olle@intel.com","username":"jmolle"},"change_message_id":"ae3e56a0d65140f3b10edd68efd1220f04e29163","unresolved":false,"context_lines":[{"line_number":219,"context_line":"            return render_exception(e, user_locale\u003dreq.best_match_language())"},{"line_number":220,"context_line":"        except exception.Forbidden as e:"},{"line_number":221,"context_line":"            address \u003d req.environ.get(\u0027REMOTE_ADDR\u0027, \u0027\u0027)"},{"line_number":222,"context_line":"            LOG.warning("},{"line_number":223,"context_line":"                _(\u0027User from: %(remote_addr)s is not authorized\u0027"},{"line_number":224,"context_line":"                  \u0027 to perform action: %(action)s\u0027),"},{"line_number":225,"context_line":"                {\u0027remote_addr\u0027: address, \u0027action\u0027: action})"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAUH%2F%2B7CM%3D","line":222,"in_reply_to":"AAAAUH%2F%2B7EI%3D","updated":"2014-01-20 18:06:26.000000000","message":"Jay, I could change it to debug, but the exception should remain in warning right?","commit_id":"d7b85a02072e86525283db3e8f9e30a72ddfe0c7"},{"author":{"_account_id":9412,"name":"Andres Buraschi","email":"andres.buraschi@intel.com","username":"aburaschi"},"change_message_id":"bd5330eeb4aba7bad257d98fec2fda52fac00783","unresolved":false,"context_lines":[{"line_number":219,"context_line":"            return render_exception(e, user_locale\u003dreq.best_match_language())"},{"line_number":220,"context_line":"        except exception.Forbidden as e:"},{"line_number":221,"context_line":"            address \u003d req.environ.get(\u0027REMOTE_ADDR\u0027, \u0027\u0027)"},{"line_number":222,"context_line":"            LOG.debug("},{"line_number":223,"context_line":"                _(\u0027User from: %(remote_addr)s is not authorized\u0027"},{"line_number":224,"context_line":"                  \u0027 to perform action: %(action)s\u0027),"},{"line_number":225,"context_line":"                {\u0027remote_addr\u0027: address, \u0027action\u0027: action})"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F%2B6v8%3D","line":222,"updated":"2014-01-20 19:02:46.000000000","message":"Jay, I\u0027m sorry to disagree... Log.warning seems more appropriate for this situation (and consistent with the Unauthorized exception above). However, as I consider myself a newbie here, can I ask which is the rational behind the change you suggest?\n\nThanks!","commit_id":"02f6dcce221cab4be1fe397319d513c11481e93b"},{"author":{"_account_id":9412,"name":"Andres Buraschi","email":"andres.buraschi@intel.com","username":"aburaschi"},"change_message_id":"9cc0c2670ed888dacb0eb744f1bd038d80a3664b","unresolved":false,"context_lines":[{"line_number":219,"context_line":"            return render_exception(e, user_locale\u003dreq.best_match_language())"},{"line_number":220,"context_line":"        except exception.Forbidden as e:"},{"line_number":221,"context_line":"            address \u003d req.environ.get(\u0027REMOTE_ADDR\u0027, \u0027\u0027)"},{"line_number":222,"context_line":"            LOG.debug("},{"line_number":223,"context_line":"                _(\u0027User from: %(remote_addr)s is not authorized\u0027"},{"line_number":224,"context_line":"                  \u0027 to perform action: %(action)s\u0027),"},{"line_number":225,"context_line":"                {\u0027remote_addr\u0027: address, \u0027action\u0027: action})"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F%2B2b8%3D","line":222,"in_reply_to":"AAAAUH%2F%2B59Y%3D","updated":"2014-01-21 13:01:24.000000000","message":"Hi Jay, I appreciate your explanation! Thanks! Seen that way, I understand the point, and agree with you.","commit_id":"02f6dcce221cab4be1fe397319d513c11481e93b"},{"author":{"_account_id":9450,"name":"Juan Manuel Ollé","email":"juan.m.olle@intel.com","username":"jmolle"},"change_message_id":"da17cebf840bef57c2a898664761036c5f73ed4c","unresolved":false,"context_lines":[{"line_number":219,"context_line":"            return render_exception(e, user_locale\u003dreq.best_match_language())"},{"line_number":220,"context_line":"        except exception.Forbidden as e:"},{"line_number":221,"context_line":"            address \u003d req.environ.get(\u0027REMOTE_ADDR\u0027, \u0027\u0027)"},{"line_number":222,"context_line":"            LOG.debug("},{"line_number":223,"context_line":"                _(\u0027User from: %(remote_addr)s is not authorized\u0027"},{"line_number":224,"context_line":"                  \u0027 to perform action: %(action)s\u0027),"},{"line_number":225,"context_line":"                {\u0027remote_addr\u0027: address, \u0027action\u0027: action})"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F%2B1%2Bs%3D","line":222,"in_reply_to":"AAAAUH%2F%2B59Y%3D","updated":"2014-01-21 14:29:20.000000000","message":"Hi Jay, for any reason If I set the log as debug it can\u0027t be shown\nIs it possible debug logs could not be included inside exception catch?  info log works ok","commit_id":"02f6dcce221cab4be1fe397319d513c11481e93b"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"7c55625c176c052e62191792074f3637aece00b1","unresolved":false,"context_lines":[{"line_number":219,"context_line":"            return render_exception(e, user_locale\u003dreq.best_match_language())"},{"line_number":220,"context_line":"        except exception.Forbidden as e:"},{"line_number":221,"context_line":"            address \u003d req.environ.get(\u0027REMOTE_ADDR\u0027, \u0027\u0027)"},{"line_number":222,"context_line":"            LOG.debug("},{"line_number":223,"context_line":"                _(\u0027User from: %(remote_addr)s is not authorized\u0027"},{"line_number":224,"context_line":"                  \u0027 to perform action: %(action)s\u0027),"},{"line_number":225,"context_line":"                {\u0027remote_addr\u0027: address, \u0027action\u0027: action})"}],"source_content_type":"text/x-python","patch_set":2,"id":"AAAAUH%2F%2B59Y%3D","line":222,"in_reply_to":"AAAAUH%2F%2B6v8%3D","updated":"2014-01-20 21:27:39.000000000","message":"Hi Andres :)\n\nSo, it\u0027s all about who is the one reading the log. An operator is the one going to be reading the log files, not a user or a keystone administrator. The operator is going to be looking at the log files when errors occur -- in order to identify the source of a failure.\n\nIn this case, there is no failure, nor is there any security risk. The action of returning a 403 is a normal part of the operation of Keystone, and therefore there should be no warning or error logged. Instead, the log message level should be info() -- since this log message is merely information and not meant to warn the operator that something is wrong -- or debug() -- a finer-grained level of information. I would argue that this information should be he debug() level of message.","commit_id":"02f6dcce221cab4be1fe397319d513c11481e93b"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"c41295f893327f2cad333aee62926797d8bbdf8b","unresolved":false,"context_lines":[{"line_number":223,"context_line":"                _(\u0027User from: %(remote_addr)s is not authorized\u0027"},{"line_number":224,"context_line":"                  \u0027 to perform action: %(action)s\u0027),"},{"line_number":225,"context_line":"                {\u0027remote_addr\u0027: address, \u0027action\u0027: action})"},{"line_number":226,"context_line":"            LOG.warning(e)"},{"line_number":227,"context_line":"            return render_exception(e, user_locale\u003dreq.best_match_language())"},{"line_number":228,"context_line":"        except exception.Error as e:"},{"line_number":229,"context_line":"            LOG.warning(e)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2BuC4%3D","line":226,"updated":"2014-01-22 20:26:23.000000000","message":"The exception should not be logged with LOG.warning() here. There is nothing in the exception class that is not already contained in the LOG.debug() message on line 222-225.","commit_id":"94bcbb93b6efbc587a4834f2b0c2efd7ede6190d"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"59af6249d58f75817d8e35592372a826dda0585a","unresolved":false,"context_lines":[{"line_number":223,"context_line":"                _(\u0027User from: %(remote_addr)s is not authorized\u0027"},{"line_number":224,"context_line":"                  \u0027 to perform action: %(action)s\u0027),"},{"line_number":225,"context_line":"                {\u0027remote_addr\u0027: address, \u0027action\u0027: action})"},{"line_number":226,"context_line":"            LOG.warning(e)"},{"line_number":227,"context_line":"            return render_exception(e, user_locale\u003dreq.best_match_language())"},{"line_number":228,"context_line":"        except exception.Error as e:"},{"line_number":229,"context_line":"            LOG.warning(e)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2BXao%3D","line":226,"in_reply_to":"AAAAUH%2F%2BXiM%3D","updated":"2014-01-27 17:19:15.000000000","message":"OK, so I understand now Juan\u0027s comment about a warning being previously logged on line 228, thx. However, I still maintain that line 226 should be removed and that there should be no warning logged at all for this particular condition (Forbidden). This is not a situation that is elevated to the warning log level, IMO. I would be +1 on this patch if line 226 was removed :)","commit_id":"94bcbb93b6efbc587a4834f2b0c2efd7ede6190d"},{"author":{"_account_id":5623,"name":"David Lyle","email":"dklyle0@gmail.com","username":"david-lyle"},"change_message_id":"da6accda27c20fb50a80fdb56a57ea85898e34a6","unresolved":false,"context_lines":[{"line_number":223,"context_line":"                _(\u0027User from: %(remote_addr)s is not authorized\u0027"},{"line_number":224,"context_line":"                  \u0027 to perform action: %(action)s\u0027),"},{"line_number":225,"context_line":"                {\u0027remote_addr\u0027: address, \u0027action\u0027: action})"},{"line_number":226,"context_line":"            LOG.warning(e)"},{"line_number":227,"context_line":"            return render_exception(e, user_locale\u003dreq.best_match_language())"},{"line_number":228,"context_line":"        except exception.Error as e:"},{"line_number":229,"context_line":"            LOG.warning(e)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2BXiM%3D","line":226,"in_reply_to":"AAAAUH%2F%2Bqho%3D","updated":"2014-01-27 16:58:32.000000000","message":"The base class for exception.Forbidden is exception.Error, by way of exception.SecurityError  So Juan\u0027s assertion that the current behavior is logging a warning seems accurate.  I could see adding the check for type Forbidden in the except block starting at 228 to add debug level logging info, or maintaining the new except block.","commit_id":"94bcbb93b6efbc587a4834f2b0c2efd7ede6190d"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"a4778beb6005371b30cc280cacff0a257a499f8e","unresolved":false,"context_lines":[{"line_number":223,"context_line":"                _(\u0027User from: %(remote_addr)s is not authorized\u0027"},{"line_number":224,"context_line":"                  \u0027 to perform action: %(action)s\u0027),"},{"line_number":225,"context_line":"                {\u0027remote_addr\u0027: address, \u0027action\u0027: action})"},{"line_number":226,"context_line":"            LOG.warning(e)"},{"line_number":227,"context_line":"            return render_exception(e, user_locale\u003dreq.best_match_language())"},{"line_number":228,"context_line":"        except exception.Error as e:"},{"line_number":229,"context_line":"            LOG.warning(e)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2Btyk%3D","line":226,"in_reply_to":"AAAAUH%2F%2Bt7Q%3D","updated":"2014-01-22 21:09:07.000000000","message":"What do you mean by \"now it is a warning\"? Currently, nothing is logged.","commit_id":"94bcbb93b6efbc587a4834f2b0c2efd7ede6190d"},{"author":{"_account_id":9450,"name":"Juan Manuel Ollé","email":"juan.m.olle@intel.com","username":"jmolle"},"change_message_id":"abdfa357e68b1034798b55c9caff265c74535e25","unresolved":false,"context_lines":[{"line_number":223,"context_line":"                _(\u0027User from: %(remote_addr)s is not authorized\u0027"},{"line_number":224,"context_line":"                  \u0027 to perform action: %(action)s\u0027),"},{"line_number":225,"context_line":"                {\u0027remote_addr\u0027: address, \u0027action\u0027: action})"},{"line_number":226,"context_line":"            LOG.warning(e)"},{"line_number":227,"context_line":"            return render_exception(e, user_locale\u003dreq.best_match_language())"},{"line_number":228,"context_line":"        except exception.Error as e:"},{"line_number":229,"context_line":"            LOG.warning(e)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2Bt7Q%3D","line":226,"in_reply_to":"AAAAUH%2F%2Bt8E%3D","updated":"2014-01-22 20:55:28.000000000","message":"Yes, but now it is a warning and if I change it from warning to debug or info, I\u0027m not only adding info but also changing the actual log level","commit_id":"94bcbb93b6efbc587a4834f2b0c2efd7ede6190d"},{"author":{"_account_id":9450,"name":"Juan Manuel Ollé","email":"juan.m.olle@intel.com","username":"jmolle"},"change_message_id":"27ee99eb5b076601684dd6ed4b1e8f446b1cf68a","unresolved":false,"context_lines":[{"line_number":223,"context_line":"                _(\u0027User from: %(remote_addr)s is not authorized\u0027"},{"line_number":224,"context_line":"                  \u0027 to perform action: %(action)s\u0027),"},{"line_number":225,"context_line":"                {\u0027remote_addr\u0027: address, \u0027action\u0027: action})"},{"line_number":226,"context_line":"            LOG.warning(e)"},{"line_number":227,"context_line":"            return render_exception(e, user_locale\u003dreq.best_match_language())"},{"line_number":228,"context_line":"        except exception.Error as e:"},{"line_number":229,"context_line":"            LOG.warning(e)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2Bqho%3D","line":226,"in_reply_to":"AAAAUH%2F%2Btyk%3D","updated":"2014-01-23 12:11:56.000000000","message":"the previous behavior was exiting in the line 228 through the base exception. In that case the exception was logged as warning.\n\nIf I keep the exception logging as warning, I agree to keep the extra information as debug. But If only the extra information is logged, I think it should be warning.\n\nI think that both logs should be keep, one warning (the exception like old behavior) and the other as debug (the extra info)","commit_id":"94bcbb93b6efbc587a4834f2b0c2efd7ede6190d"},{"author":{"_account_id":9450,"name":"Juan Manuel Ollé","email":"juan.m.olle@intel.com","username":"jmolle"},"change_message_id":"a5cd53fb141f5287953a25690573ae57b8e33f46","unresolved":false,"context_lines":[{"line_number":223,"context_line":"                _(\u0027User from: %(remote_addr)s is not authorized\u0027"},{"line_number":224,"context_line":"                  \u0027 to perform action: %(action)s\u0027),"},{"line_number":225,"context_line":"                {\u0027remote_addr\u0027: address, \u0027action\u0027: action})"},{"line_number":226,"context_line":"            LOG.warning(e)"},{"line_number":227,"context_line":"            return render_exception(e, user_locale\u003dreq.best_match_language())"},{"line_number":228,"context_line":"        except exception.Error as e:"},{"line_number":229,"context_line":"            LOG.warning(e)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2BuCU%3D","line":226,"in_reply_to":"AAAAUH%2F%2BuC4%3D","updated":"2014-01-22 20:30:04.000000000","message":"Ok, in that case I think I should change the other log from debug to warning.\n\nPerhaps adding the exception text to the same log like in line 218?","commit_id":"94bcbb93b6efbc587a4834f2b0c2efd7ede6190d"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e3aa80559c68c7c506899545af38ea3bde8b5abc","unresolved":false,"context_lines":[{"line_number":223,"context_line":"                _(\u0027User from: %(remote_addr)s is not authorized\u0027"},{"line_number":224,"context_line":"                  \u0027 to perform action: %(action)s\u0027),"},{"line_number":225,"context_line":"                {\u0027remote_addr\u0027: address, \u0027action\u0027: action})"},{"line_number":226,"context_line":"            LOG.warning(e)"},{"line_number":227,"context_line":"            return render_exception(e, user_locale\u003dreq.best_match_language())"},{"line_number":228,"context_line":"        except exception.Error as e:"},{"line_number":229,"context_line":"            LOG.warning(e)"}],"source_content_type":"text/x-python","patch_set":3,"id":"AAAAUH%2F%2Bt8E%3D","line":226,"in_reply_to":"AAAAUH%2F%2BuCU%3D","updated":"2014-01-22 20:53:04.000000000","message":"No. I thought we went over this already.... it\u0027s not something that should be a warning. It\u0027s debug or info-level information, not warning :)","commit_id":"94bcbb93b6efbc587a4834f2b0c2efd7ede6190d"},{"author":{"_account_id":220,"name":"Haneef Ali","email":"haneef.ali@hp.com","username":"haneef"},"change_message_id":"eb6a78a718df81d58ed9a9fe32e5d3b028d02b0f","unresolved":false,"context_lines":[{"line_number":217,"context_line":"                _(\u0027Authorization failed. %(exception)s from %(remote_addr)s\u0027),"},{"line_number":218,"context_line":"                {\u0027exception\u0027: e, \u0027remote_addr\u0027: req.environ[\u0027REMOTE_ADDR\u0027]})"},{"line_number":219,"context_line":"            return render_exception(e, user_locale\u003dreq.best_match_language())"},{"line_number":220,"context_line":"        except exception.Forbidden as e:"},{"line_number":221,"context_line":"            address \u003d req.environ.get(\u0027REMOTE_ADDR\u0027, \u0027\u0027)"},{"line_number":222,"context_line":"            LOG.info("},{"line_number":223,"context_line":"                _(\u0027User from: %(remote_addr)s is not authorized\u0027"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2BWG4%3D","line":220,"updated":"2014-01-27 20:29:31.000000000","message":"I\u0027m not sure what value does this change add?  This information is already  available  in  access.log.  If you add caller\u0027s id or name, then it may be useful","commit_id":"d83119c4270350b36f587fbad1bd4a1c3dc52b1f"}]}