search

Brightspace / D2L.Security.OAuth2

Brightspace OAuth 2.0 for C#
Apache License 2.0
7 stars 16 forks source link

Use service exception #9

Closed mpharoah-d2l closed 8 years ago

mpharoah-d2l commented 8 years ago

Finally got around to this: Made the auth service token provisioning client throw an exception deriving from ServiceException. Once the LMS is updated to pull in the newest LOReS and Auth client packages, this should make identifying auth service connection errors easier.

mpharoah-d2l commented 8 years ago

!set-reviewers @dylanjacobsd2l @j3parker

mpharoah-d2l commented 8 years ago

(This is a low priority PR. It does not need to be merged this sprint.)

j3parker commented 8 years ago

Opps I'm sorry, I missed this and made a conflict headache for you :(

j3parker commented 8 years ago

Thanks a lot, fixing this up would be great. CC @omsmith

j3parker commented 8 years ago

Looks good! Thanks for picking this one up.

mpharoah-d2l commented 8 years ago

Are there any security concerns with relaying all or part of the auth service's response when it can't be serialized to an access token or error object? (So the LMS makes a call to the auth service, doesn't understand the response, and relays the response in it's error message to the user)

j3parker commented 8 years ago

Nope!

mpharoah-d2l commented 8 years ago

Pulled in the latest changes, refactored code to work with the latest changes, and made the last 2 changes you mentioned.

Re-ran all unit and integration tests (all passed), but I haven't done manual testing yet.

mpharoah-d2l commented 8 years ago

Looks like the AppVeyor build is failing because I'm referencing version 1.0.0 of the D2L.Services.Core.Exceptions NuGet package, which is present in our internal D2L NuGet repository, but the public NuGet repo uses version 1.0.0.27289. The code should be identical, but the LMS and other services may need to be updated to use the same package version.

j3parker commented 8 years ago

Ok, let me know when you want me to click merge.

mpharoah-d2l commented 8 years ago

I'm good with this being merged now.