-
Notifications
You must be signed in to change notification settings - Fork 110
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
feat: added Microsoft as oauth provider #8
Conversation
ab03f1d
to
5d13544
Compare
Some Microsoft logins for government instances use https://login.microsoftonline.us https://graph.microsoft.us/v1.0/me maybe having a domain .env variable would make this work with government and non government Microsoft instances. |
Never heard of it maybe because it is only available for US GOV I would prefer to do this via optional env variable with a boolean like |
Yea US Gov instance uses all .us domains. same goes for their graph api yep, having that optional .env would work great |
Agree by adding |
Added |
Thanks! |
Conflicts resolved should be good to go :) |
From my testing, I believe Microsoft forwards to the correct domain based on the tenant id that is supplied. But I still think how @jfrelik implemented this is good to ensure we explicitly define which instance to use. I didn't get to test all the way through since its asking me to grant access permission but that could be due to me reusing an existing client id.
UPDATE: was able to test global and US instance after creating a new app registration. |
Just want to point out there's more instances than just global and US gov. From what I can briefly see, there's global, US Gov, China, Germany and potentially more. https://learn.microsoft.com/en-us/entra/identity-platform/authentication-national-cloud I think we should default to global, and have an environment variable to override instance (which is what Microsoft's auth SDKs call this). |
Then what about letting the user configure the authorization url and tokens url like for GitHub? |
That can be the solution, although looking at some other projects like Appwrite they only support global .com domain Therefore if we want to support all Entra instances i would suggest 3 optional env variables, if not set the module would assume global instance Proposed envs: |
I suggest we follow what Microsoft's SDKs does - https://learn.microsoft.com/en-us/entra/identity-platform/tutorial-v2-nodejs-webapp-msal#add-app-registration-details
The token and and authorization URL path remain the same, the with the only part changing is the instance and tenant IDs, so I don't think it should be configurable.
|
I don't think we need an env variable each time since it can configure it with the
Then we could give for configuration: export default oauth.microsoftEventHandler({
config: {
authorizationURL: '', // custom url
tokenURL: '', // custom url
userURL: '', // custom url
scope: []
},
// ...
}) |
Added Microsoft as an oauth provider