-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce login with Microsoft #539
base: main
Are you sure you want to change the base?
Conversation
4c24f94
to
8f5052f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great addition. You’ve been the first person willing to dig into my credentials code since I wrote it all. :) I was pretty pleased with how this turned out when I did the initial refactor that added a bunch of different methods. I’m curious you’re take now that you’ve made sense of it all.
I do think it’s the right call to simply duplicate some of the parallel logic with google oauth. If we pulled out similarities I would worry that they may diverge in the future. I think it’s easier to maintain these separately even though there is some duplication.
On this specific PR, what you did looks really good. One overall question: should we call it microsoft_*
or microsoft_graph_*
? My instinct would have been to just use the single word, microsoft, but googling I do see people refer to it both ways. Or maybe these are two different systems? I’m not as familiar with the Microsoft ecosystem but I do clearly see references to Microsoft Graph so I trust your context more than mine and am happy to follow your lead on this. I just wanted to raise this question for reflection since it’ll be hard to change all the references later.
The main thing I’m flagging is just a couple things on tests. I haven’t tried setting this up for myself yet on my machine. I’ll try to do that later today and I’ll give the README a close review as I’m doing this to make sure all the steps work.
30ec7e0
to
d8c76e5
Compare
e651afe
to
a3a8041
Compare
a3a8041
to
07eba7a
Compare
72eba22
to
3dfd08d
Compare
@drnic I hoped to finish reviewing today but I didn't make it. I should be able do this tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finished giving it a careful review and flagged a few final cleanup items but they're all minor.
Unfortunately, I had trouble getting into my Azure account. I haven't used it for a long time. So, I didn't actually try initializing the whole thing on my account but I'm trusting that you tried this out. :) Aside from that, I looked closely at all the code and it's all sane.
Take a look at my final comments and then I think we should be ready to merge.
User#microsoft_graph_credential.oauth_token
to interact with Microsoft Graph APISequence flow: