none
Web API 2 : правильный подход с возвратом ошибок RRS feed

  • Вопрос

  • пусть у меня есть API метод, который получает имя пользователями и проверяет, есть ли вообще такой пользователь и если да, то имеет ли он нужную роль (и только после этого продолжается выполнение метода)


            public async Task<HttpResponseMessage> SetNewPasswordAsync(SetNewPasswordApiModel model)
            {
                var user = await UserManager.FindByNameAsync(model.Username);
                if (user == null)
                    return Request.CreateErrorResponse(HttpStatusCode.NotFound, "User with such username is not found");
    
                if (user.AspNetRoles.Where(p => p.Name == Models.Roles.SmartphonePhotographer).Count() == 0)
                    return Request.CreateErrorResponse(HttpStatusCode.NotFound, "User does not have Smartphone Photographer role");
    
    
    
                var result = await UserManager.ResetPasswordAsync(user.Id, model.Code, model.NewPassword);
                if (result.Succeeded)
                {
                    return Request.CreateResponse<string>(HttpStatusCode.OK, "Password is changed");
                }
                else
                {
                    return Request.CreateResponse<List<string>>(HttpStatusCode.BadRequest, result.Errors.ToList());
                }
            }

    это работает, но и в случае с неправильным именем пользователя, и в случае, когда этот пользователь не имеет подходящей роли клиенту возвращается ошибка 404. У меня есть идея возвращать для каждого конкретного случая ошибки из диапазона 452-499, чтобы по номеру можно было определить тип ошибки. Хорошая это идея? И если да, то как это реализовать, т.к. CreateErrorMessage понимает только перечисление HttpStatusCode...

    PS. Я нашёл, что можно int привести к типу HttpStatusCode:

    return Request.CreateResponse((HttpStatusCode)452, new { Message = "User with such username is not found" } );
    но хорошая ли это идея, и если плохая - то почему? :)
    14 марта 2016 г. 0:00

Ответы

  • "Прежде всего, создавать дружественное сообщение на сервере в данном случае сродни жёсткому кодированию клиентских текстовых строк прямо в бизнес-логике/слое данных. Т.к. может быть тысяча причин, почему строка на клиенте должна отличаться от отданной сервером. Это может быть локализация, это может быть разный текст для версий андроида и айфона и т.д. Переданный уникальный ключ (будь то придуманный кастомный код, похожий на код ответа HTTP или какой-то другой ключ, даже текстовый)" – никто и не предлагает вам клиентскую логику реализовать на серверевы немножко не так поняли, я подразумевал статус коды для HTTP, а кастомных у вас может быть хоть миллион :) делайте всё, что хотите.

    "позволит по нему составить необходимый текст на клиенте. И это не исключает серверную обработку ошибок, это только специфицирует клиентскую обработку ошибок" - если нужно составить только текст и показать пользователю, то тут лучше получать его с сервера готовый, так как не нужно будет дублировать код на всех клиентах. Если вам нужен статус код на основании которого нужно менять бизнес-правила на клиенте, то тут дело другое, тогда подойдут костюмные коды.

    Если вы посмотрите код приведенный мною выше то AppHttpMessage это самое кастомное сообщение которое может содержать и коды и сообщения и данные, но главная фишка в том, что он не зависит от кодов HTTP.


    Сделаем содержимое сообщества лучше, вместе!

    17 марта 2016 г. 6:37
    Модератор
  • "Что значит простые? Если мне нужно проверить какое-то соответствие и дёрнуть внешний ресурс (который соответственно внедрён через абстракции) - то как быть?" – система авторизации, аутентификации у вас должна быть реализована на группах, правах и ролях. Пользователь может вызвать метод API если он есть в группе или у него есть соответствующее право/разрешение (permission). В фильтре писать логику не нужно, всё должно быть просто. Долго объяснять и писать, можете начать отсюда, но это на будущее, сейчас думаю вряд ли у вас получится изменить всё, так как нужны кардинальные перемены.

    "А по поводу передачи объектов через локатор - прежде всего, в книге, которую Вы мне советовали, локатор вообще в антипаттернах :)" – знаю, но потому что, по другому никак. Вот в ASP.NET Core это исправили.


    Сделаем содержимое сообщества лучше, вместе!

    17 марта 2016 г. 7:22
    Модератор

Все ответы

  • Про глобальную обработку ошибок очень хорошо написано здесь. Если у пользователя нет соответствующей роли, то для этого есть стандартное значение HttpStatusCode.Unauthorized, не нужно ничего дополнительного. Плюс ко всему к этому в ASP.NET есть фильтры авторизации всю логику лучше реализовать в них. Если реализовать свой собственный провайдер ролей (https://msdn.microsoft.com/ru-ru/library/8fw7xh74(v=vs.100).aspx), то писать фильтры тоже не нужно.

    Сделаем содержимое сообщества лучше, вместе!

    14 марта 2016 г. 5:50
    Модератор
  • Про глобальную обработку ошибок очень хорошо написано здесь. Если у пользователя нет соответствующей роли, то для этого есть стандартное значение HttpStatusCode.Unauthorized, не нужно ничего дополнительного. Плюс ко всему к этому в ASP.NET есть фильтры авторизации всю логику лучше реализовать в них. Если реализовать свой собственный провайдер ролей (https://msdn.microsoft.com/ru-ru/library/8fw7xh74(v=vs.100).aspx), то писать фильтры тоже не нужно.

    Сделаем содержимое сообщества лучше, вместе!

    ок, с авторизацией вроде всё понятно (хотя опять же, хотелось бы сообщить клиентской программе пользователь вообще не авторизирован или нет подходящей роли), а вот с кучей клиентских ошибок. Часто хочется специфицировать какая конкретно ошибка (даже с тем же NotFound, это может быть NotFound как одного ресурса, так и другого внутри конкретного АПИ метода). Я понимаю, что ещё есть сообщение об ошибке, но хотелось бы ещё и код, по которому клиентская программа может конкретно знать, что не так, не сравнивая строки и т.д. Я это пробую реализовать вот так:

    return request.CreateErrorResponse(HttpStatusCode.BadRequest, new HttpError("Something wrong") { { "CustomStatusCode", 478 } });

    т.е. получив 400 ошибку клиент может по 478 узнать что не так и соответственно это обработать, не парся "Something wrong". Правильно ли я рассуждаю?

    По поводу фильтров - я их всегда недолюбливал из-за невозможности передать в них параметры и теперь это вылезло во всей красе. Например, у меня есть сейчас такой вот фильтр авторизации:

        public class AuthorizeApiFilter : AuthorizeAttribute
        {
            public override void OnAuthorization(HttpActionContext actionContext)
            {
                string token = string.Empty;
                AuthenticationTicket ticket;
    
                token = (actionContext.Request.Headers.Any(x => x.Key == "Authorization")) ? actionContext.Request.Headers.Where(x => x.Key == "Authorization").FirstOrDefault().Value.SingleOrDefault().Replace("Bearer ", "") : "";
    
                if (token == string.Empty)
                {
                    actionContext.Response = actionContext.Request.CreateResponse(HttpStatusCode.Unauthorized, "Missing 'Authorization' header. Access denied.");
                    return;
                }
    
                //your OAuth startup class may be called something else...
                ticket = Startup.OAuthOptions.AccessTokenFormat.Unprotect(token);
    
                if (ticket == null)
                {
                    actionContext.Response = Models.Utils.CreateClientErrorResponse(actionContext.Request, 490, "Invalid token decrypted.");
                    return;
                }
    
                if (!bool.Parse(ticket.Properties.Dictionary["IsSmartphonePhotographerRole"]))
                {
                    actionContext.Response = actionContext.Request.CreateErrorResponse(HttpStatusCode.Unauthorized, "User does not have Smartphone Photographer role");
                    return;
                }
    
                if (!bool.Parse(ticket.Properties.Dictionary["IsEmailConfirmed"]))
                {
                    actionContext.Response = Models.Utils.CreateClientErrorResponse(actionContext.Request, 454, "User is not confirmed");
                    return;
                }
    
                //you could perform some logic on the ticket here...
    
                //you will be able to retrieve the ticket in all controllers by quering Properties and looking for "Ticket"... 
                //actionContext.Request.Properties.Add(new KeyValuePair<string, object>("Ticket", ticket));
                var UserID = int.Parse(ticket.Properties.Dictionary["UserID"]);
                actionContext.Request.Properties.Add("UserID", UserID);
                base.OnAuthorization(actionContext);
            }
        }

    Он работает, но мне хотелось бы логгировать ошибки. Для этого мне нужно передать ему объект, реализующий интерфейс ILog, т.к. приложение построено по принципу DI. Как это сделать?




    15 марта 2016 г. 23:20
  • "ок, с авторизацией вроде всё понятно (хотя опять же, хотелось бы сообщить клиентской программе пользователь вообще не авторизирован или нет подходящей роли), а вот с кучей клиентских ошибок. Часто хочется специфицировать какая конкретно ошибка (даже с тем же NotFound, это может быть NotFound как одного ресурса, так и другого внутри конкретного АПИ метода)." – поверьте практической пользы для клиента ноль, если бы это было наоборот, то давно бы реализовали подобную возможность из коробки. Зачем клиенту лишние детали, нет доступа и всё. К тому же детальное описание ошибок доступа чревато брешами в системе безопасности.

    "т.е. получив 400 ошибку клиент может по 478 узнать что не так и соответственно это обработать, не парся "Something wrong". Правильно ли я рассуждаю?" – конечно же, нужно разделять ошибки типа 404 и 401 но не более, если нужно выводить дружественное сообщение (user friendly) для клиента, и без подробный деталей, то просто меняйте текст ошибки на сервере,, не более и не менее, вот там то (на сервере) у вас должна быть централизованная система обработки ошибок.

    "По поводу фильтров - я их всегда недолюбливал из-за невозможности передать в них параметры и теперь это вылезло во всей красе. Например, у меня есть сейчас такой вот фильтр авторизации:" – зачем изобретать велосипед, и усложнять жизнь. Пишите простые фильтры. Вот вам пример из собственного приложения:

    namespace XXX.WebApp.Security
    {
        internal class XXXWebAppRoleProvider : RoleProvider
        {
            private readonly ISecurityService securityService;
    
            public ManageSuppliesWebAppRoleProvider()
            {
                this.securityService = AppServiceLocator.Current.GetInstance<ISecurityService>();
            }
    
            public override string ApplicationName
            {
                get
                {
                    throw new NotImplementedException();
                }
    
                set
                {
                    throw new NotImplementedException();
                }
            }
    
            public override bool IsUserInRole(string userName, string roleName)
            {
                if (string.IsNullOrEmpty(userName))
                {
                    throw new ArgumentException($"{nameof(userName)} Cannot be null or empty.");
                }
                if (string.IsNullOrEmpty(userName))
                {
                    throw new ArgumentException($"{nameof(roleName)} Cannot be null or empty.");
                }
    
                return this.securityService.IsUserInRole(userName, roleName);
            }
    
            public override string[] GetRolesForUser(string userName)
            {
                if (string.IsNullOrEmpty(userName))
                {
                    throw new ArgumentException($"{nameof(userName)} Cannot be null or empty.");
                }
                var securityToken = AuthenticationManager.GetSecurityToken(userName);
                var roles = this.securityService.GetUserRolesByUserId(securityToken.UserId);
    
                return roles.ToArray();
            }
    
            public override string[] GetAllRoles()
            {
                throw new NotImplementedException();
            }
    
            public override void AddUsersToRoles(string[] usernames, string[] roleNames)
            {
                throw new NotImplementedException();
            }
    
            public override void CreateRole(string roleName)
            {
                throw new NotImplementedException();
            }
    
            public override bool DeleteRole(string roleName, bool throwOnPopulatedRole)
            {
                throw new NotImplementedException();
            }
    
            public override string[] FindUsersInRole(string roleName, string usernameToMatch)
            {
                throw new NotImplementedException();
            }
    
            public override string[] GetUsersInRole(string roleName)
            {
                throw new NotImplementedException();
            }
    
            public override void RemoveUsersFromRoles(string[] usernames, string[] roleNames)
            {
                throw new NotImplementedException();
            }
    
            public override bool RoleExists(string roleName)
            {
                throw new NotImplementedException();
            }
        }
    }

    namespace XXX.WebApp.Api
    {
        [AntiForgeryValidate]
        public class ItemController : ApiController
        {
            private readonly IItemsService itemsService;
    
            public ItemController(IItemsService itemsService)
            {
                this.itemsService = itemsService;
            }
    
            [HttpGet, Authorize(Roles = AspRole.ItemsCreate)]
            public AppHttpMessage GetItemsCreateDataSource()
            {
                var appHttpMessage = new AppHttpMessage
                {
                    Data = itemsService.GetItemDataSource(false)
                };
                return appHttpMessage;
            }
    
            [HttpGet, Authorize(Roles = AspRole.ItemsUpdate)]
            public AppHttpMessage GetItemsUpdateDataSource(string itemCode)
            {
                var appHttpMessage = new AppHttpMessage
                {
                    Data = itemsService.GetItemDataSource(true, itemCode)
                };
                return appHttpMessage;
            }
    
            [HttpGet]
            public AppHttpMessage EnsureUniqueItemCode(string itemCode)
            {
                var appHttpMessage = new AppHttpMessage();
                var isUnique = itemsService.IsItemCodeExists(itemCode);
                appHttpMessage.Data = isUnique;
                return appHttpMessage;
            }
    }

    namespace XXX.WebApp.Api
    {
        public class WebApiExceptionHandler : ExceptionHandler
        {
            public override void Handle(ExceptionHandlerContext context)
            {
                var ipAddress = HttpContext.Current.Request.UserHostName;
                var userId = context.RequestContext.Principal.Identity.Name;
    
                if (string.IsNullOrEmpty(userId))
                {
                    userId = "Anonymous user";
                }
                else
                {
                    var securityToken = Security.AuthenticationManager.GetSecurityToken(userId);
                    userId = securityToken.CompanyId + ": " + securityToken.UserId;
                }
    
                var exceptionFullInfo = new ExceptionFullInfo
                {
                    UserId = userId,
                    RequestUri = context.Request.RequestUri.ToString(),
                    OriginalException = context.Exception,
                    IpAddress = ipAddress
                };
    
                var logger = AppServiceLocator.Current.GetInstance<ILogger>();
                logger.Write(exceptionFullInfo.ToString());
    
                // Temporarily disabled, need to implement.
                // AppExceptionManager.Current.HandleException(context.Exception, ExceptionPolicyName.NotifyPolicy);
    
                context.Result = new TextPlainExceptionResult
                {
                    AppHttpMessage = new AppHttpMessage
                    {
                        ServerResponse = "An unexpected error occurred! Please contact the server administrator."
                    },
    
                    Request = context.Request
                };
    
                // TODO: Add exceptions message resolver.
                
            private class TextPlainExceptionResult : IHttpActionResult
            {
                public AppHttpMessage AppHttpMessage { get; set; }
    
                public HttpRequestMessage Request { private get; set; }
    
                public Task<HttpResponseMessage> ExecuteAsync(CancellationToken cancellationToken)
                { 
                    if (Request != null)
                    {
                        var response = Request.CreateResponse(HttpStatusCode.OK, AppHttpMessage);
                        response.Headers.Add("X-Requested-AppHttpMessage", "X-Requested-AppHttpMessage");
    
                        return Task.FromResult(response);
                    }
    
                    using (var httpResponseMessage = new HttpResponseMessage
                    {
                        Content = new ObjectContent(AppHttpMessage.GetType(), AppHttpMessage, new JsonMediaTypeFormatter())
                    })
                    {
                        httpResponseMessage.Headers.Add("X-Requested-AppHttpMessage", "X-Requested-AppHttpMessage");
                        return Task.FromResult(httpResponseMessage);
                    }
                }
            }}
    "Он работает, но мне хотелось бы логгировать ошибки. Для этого мне нужно передать ему объект, реализующий интерфейс ILog, т.к. приложение построено по принципу DI. Как это сделать?" – используйте локатор сервисов (считается что он антипаттерн и т.п.....), но другого способа я не знаю.
    var logger = AppServiceLocator.Current.GetInstance<ILogger>();
    logger.Write(exceptionFullInfo.ToString());


    Сделаем содержимое сообщества лучше, вместе!

    16 марта 2016 г. 11:02
    Модератор
  • "ок, с авторизацией вроде всё понятно (хотя опять же, хотелось бы сообщить клиентской программе пользователь вообще не авторизирован или нет подходящей роли), а вот с кучей клиентских ошибок. Часто хочется специфицировать какая конкретно ошибка (даже с тем же NotFound, это может быть NotFound как одного ресурса, так и другого внутри конкретного АПИ метода)." – поверьте практической пользы для клиента ноль, если бы это было наоборот, то давно бы реализовали подобную возможность из коробки. Зачем клиенту лишние детали, нет доступа и всё. К тому же детальное описание ошибок доступа чревато брешами в системе безопасности.

    "т.е. получив 400 ошибку клиент может по 478 узнать что не так и соответственно это обработать, не парся "Something wrong". Правильно ли я рассуждаю?" – конечно же, нужно разделять ошибки типа 404 и 401 но не более, если нужно выводить дружественное сообщение (user friendly) для клиента, и без подробный деталей, то просто меняйте текст ошибки на сервере,, не более и не менее, вот там то (на сервере) у вас должна быть централизованная система обработки ошибок.


    Можно я здесь с Вами не соглашусь? :)

    Прежде всего, создавать дружественное сообщение на сервере в данном случае сродни жёсткому кодированию клиентских текстовых строк прямо в бизнес-логике/слое данных. Т.к. может быть тысяча причин, почему строка на клиенте должна отличаться от отданной сервером. Это может быть локализация, это может быть разный текст для версий андроида и айфона и т.д. Переданный уникальный ключ (будь то придуманный кастомный код, похожий на код ответа HTTP или какой-то другой ключ, даже текстовый) позволит по нему составить необходимый текст на клиенте. И это не исключает серверную обработку ошибок, это только специфицирует клиентскую обработку ошибок

    Второй нюанс - собственно сама клиентская обработка ошибок. Практическая польза в этом - вот у меня реальная ситуация сейчас :

    Пользователь запрашивает через АПИ список запросов. Но список запросов он может получить только в случае, если у него есть одна или более установленная локация. Т.е. если у него нет локаций, то нечего лохматить бабушку и дёргать этот АПИ метод, а нужно сначала дёргнуть другой, который добавляет локацию. И среди возможных ответов от метода, возвращающего список запросов, есть один ответ, который говорит "No Location". Т.е. клиентская программа должна этот ответ обработать особым образом, в данном случае открыть форму с добавление локации и соответствующим описанием почему так. Как Вы предлагаете отделить этот ответ от остальных? Это такой же 400-й (Bad Client Request), только со своими особенностями. Сравнивать переданное сообщение об ошибке? А если вдруг там будет синтаксическая ошибка (запятую пропустили) и разработчик без задней мысли её туда поставит? А вот какой-нибудь кастомный код 478 (в придачу к ответу 400) в теле сообщения спасёт отца русской демократии :)

    16 марта 2016 г. 21:26
  • "По поводу фильтров - я их всегда недолюбливал из-за невозможности передать в них параметры и теперь это вылезло во всей красе. Например, у меня есть сейчас такой вот фильтр авторизации:" – зачем изобретать велосипед, и усложнять жизнь. Пишите простые фильтры. Вот вам пример из собственного приложения:

    Что значит простые? Если мне нужно проверить какое-то соответствие и дёрнуть внешний ресурс (который соответственно внедрён через абстракции) - то как быть?

    А по поводу передачи объектов через локатор - прежде всего, в книге, которую Вы мне советовали, локатор вообще в антипаттернах :)

    Если у меня без сторонних резолверов и сделано по рабоче-крестьянски - через передачу параметров конструктора, то как быть? :)

    16 марта 2016 г. 21:31
  • "Прежде всего, создавать дружественное сообщение на сервере в данном случае сродни жёсткому кодированию клиентских текстовых строк прямо в бизнес-логике/слое данных. Т.к. может быть тысяча причин, почему строка на клиенте должна отличаться от отданной сервером. Это может быть локализация, это может быть разный текст для версий андроида и айфона и т.д. Переданный уникальный ключ (будь то придуманный кастомный код, похожий на код ответа HTTP или какой-то другой ключ, даже текстовый)" – никто и не предлагает вам клиентскую логику реализовать на серверевы немножко не так поняли, я подразумевал статус коды для HTTP, а кастомных у вас может быть хоть миллион :) делайте всё, что хотите.

    "позволит по нему составить необходимый текст на клиенте. И это не исключает серверную обработку ошибок, это только специфицирует клиентскую обработку ошибок" - если нужно составить только текст и показать пользователю, то тут лучше получать его с сервера готовый, так как не нужно будет дублировать код на всех клиентах. Если вам нужен статус код на основании которого нужно менять бизнес-правила на клиенте, то тут дело другое, тогда подойдут костюмные коды.

    Если вы посмотрите код приведенный мною выше то AppHttpMessage это самое кастомное сообщение которое может содержать и коды и сообщения и данные, но главная фишка в том, что он не зависит от кодов HTTP.


    Сделаем содержимое сообщества лучше, вместе!

    17 марта 2016 г. 6:37
    Модератор
  • "Что значит простые? Если мне нужно проверить какое-то соответствие и дёрнуть внешний ресурс (который соответственно внедрён через абстракции) - то как быть?" – система авторизации, аутентификации у вас должна быть реализована на группах, правах и ролях. Пользователь может вызвать метод API если он есть в группе или у него есть соответствующее право/разрешение (permission). В фильтре писать логику не нужно, всё должно быть просто. Долго объяснять и писать, можете начать отсюда, но это на будущее, сейчас думаю вряд ли у вас получится изменить всё, так как нужны кардинальные перемены.

    "А по поводу передачи объектов через локатор - прежде всего, в книге, которую Вы мне советовали, локатор вообще в антипаттернах :)" – знаю, но потому что, по другому никак. Вот в ASP.NET Core это исправили.


    Сделаем содержимое сообщества лучше, вместе!

    17 марта 2016 г. 7:22
    Модератор
  • "Он работает, но мне хотелось бы логгировать ошибки. Для этого мне нужно передать ему объект, реализующий интерфейс ILog, т.к. приложение построено по принципу DI. Как это сделать?" – используйте локатор сервисов (считается что он антипаттерн и т.п.....), но другого способа я не знаю.
    var logger = AppServiceLocator.Current.GetInstance<ILogger>();
    logger.Write(exceptionFullInfo.ToString());


    Сделаем содержимое сообщества лучше, вместе!

    Никогда не использовал локатор. Извините за глупый вопрос, но где его собственно взять для использования? :) Или он вообще настолько прост, что можно рукописную версию из 10 строк без страха использовать в рабочем приложении?

    24 марта 2016 г. 0:28
  • Можно и так и так, т.е. это по сути обёртка экземпляра контейнера, ничего больше, стандартно я использую UnityServiceLocator, так как использую Unity, просто красиво так :)

    Сделаем содержимое сообщества лучше, вместе!

    24 марта 2016 г. 8:32
    Модератор