search

Azure / ms-rest-nodeauth

node.js based authentication library for Azure with type definitions
MIT License
33 stars 33 forks source link

Fix a bunch of type related issues #75

Closed arthurschreiber closed 3 years ago

arthurschreiber commented 5 years ago

This fixes a whole bunch of type related issues in this library.

I tried to make the commits self-contained.

I'll add some comments explaining some of the changes that I made. 🙇

daviwil commented 5 years ago

Hi @arthurschreiber, thanks for sending this great PR!

We're currently in the process of winding down development on this library in favor of the new @azure/identity library that's being developed for our next wave of TypeScript SDKs so we are less likely to accept PRs for ms-rest-nodeauth at this point. Were any of these changes fixes for actual bugs that blocked your ability to use the library?

Out of curiosity, where have you been using ms-rest-nodeauth so far?

arthurschreiber commented 5 years ago

@daviwil Oh wow, I did not know this library is getting deprecated. 🙇

I'm one of the maintainers of tedious, the Microsoft recommended Node.js package for connecting to SQL Server and Azure SQL databases. Azure AD and MSI authentication support was implemented via @azure/ms-rest-nodeauth. And the fixes I've provided so far were all things I've found that didn't seem right.

I guess migrating to @azure/identity would probably be a better use of my time? 😅

daviwil commented 5 years ago

It's slowly being deprecated, but yeah, it'd be better to focus on @azure/identity. How are you currently using these credentials in tedious? I could give you some pointers on how you might be able to switch over to @azure/identity, I don't think it'd be a significant amount of work depending on what you're doing currently.

arthurschreiber commented 5 years ago

@daviwil https://github.com/tediousjs/tedious/blob/7721dac034ded633e9fa8d85155a311243fbdd45/src/connection.js#L2076-L2117

Yeah, I took a quick glance over @azure/identity, and it should be pretty straightforward to switch. 👍

daviwil commented 5 years ago

Please let me know if you have any questions while trying it out! That library is still in preview so your feedback will be very valuable.

funkydev commented 5 years ago

@daviwil I have a question about InteractiveBrowserCredential in node.js app.

This authorization method is supported by the current library but by @azure/identity is not. Is there any info about implementation of the interactive authentication?

daviwil commented 5 years ago

@funkydev as of @azure/identity 1.0.0-preview.2, we now provide an InteractiveBrowserCredential for the in-browser interactive auth flow, but we don't have such an implementation for Node.js yet. We've got it on our internal roadmap for a future preview but we don't know when it will arrive.

funkydev commented 5 years ago

@daviwil So I have opened the issue #4774 which requests that.

People can't move to @azure/identity library, when it doesn't support the same functionality. tedious library needs support for Interactive login.

daviwil commented 5 years ago

Thank you for filing that! We will let you know when that gets added.

arthurschreiber commented 3 years ago

@ramya-rao-a Based on https://github.com/tediousjs/tedious/issues/1146#issuecomment-696241577, would it make sense to get these changes merged and the type related issues resolved? 🙇‍♂️

ramya-rao-a commented 3 years ago

Hey @arthurschreiber

Thanks for your patience here

Would you consider splitting this into two separate PRs?

ramya-rao-a commented 3 years ago

@arthurschreiber Created #121 for the AuthResponse type changes

ramya-rao-a commented 3 years ago

Thanks for the work here @arthurschreiber

I have pulled out the changes for strengthening the types around AuthResponse in #121 which is now merged and an update will be released soon

Closing this PR as we havent seen much activity here. If there are any more improvements that can be made in a non breaking way, please send a new PR