-
Notifications
You must be signed in to change notification settings - Fork 916
feat: adds azure AD as oauth2 provider #855
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
base: dev
Are you sure you want to change the base?
feat: adds azure AD as oauth2 provider #855
Conversation
7cdc33d
to
96fc221
Compare
Took me a while to come back to this one :) It looks good, and is pretty similar to the setup that I have. I'm not logging in to a particular tenant, so my tenantId is "common", and maybe that should be the default? But I think otherwise this is good, and I can add any fixes as I adopt it myself. @JoaoPedroAS51 thoughts? |
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.
@vinayakkulkarni @noobling Hey! Nice work! 😃
I think we should set tenantId
to common
by default, as @bmulholland suggested.
And we can also add a scheme that modifies the logout
, as suggested by @Dashboard-Community-Center in #1012
authorization: `https://login.microsoftonline.com/${strategy.tenantId}/oauth2/v2.0/authorize`, | ||
userInfo: 'https://graph.microsoft.com/v1.0/me', | ||
token: `https://login.microsoftonline.com/${strategy.tenantId}/oauth2/v2.0/token` |
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.
authorization: `https://login.microsoftonline.com/${strategy.tenantId}/oauth2/v2.0/authorize`, | |
userInfo: 'https://graph.microsoft.com/v1.0/me', | |
token: `https://login.microsoftonline.com/${strategy.tenantId}/oauth2/v2.0/token` | |
authorization: `https://login.microsoftonline.com/${strategy.tenantId || 'common'}/oauth2/v2.0/authorize`, | |
userInfo: 'https://graph.microsoft.com/v1.0/me', | |
token: `https://login.microsoftonline.com/${strategy.tenantId || 'common'}/oauth2/v2.0/token` |
Does this make sense @JoaoPedroAS51 @bmulholland ?
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 think should be like this:
const tenantId = strategy.tenantId || 'common'
assignDefaults(strategy, {
scheme: 'oauth2',
endpoints: {
authorization: `https://login.microsoftonline.com/${tenantId}/oauth2/v2.0/authorize`,
userInfo: 'https://graph.microsoft.com/v1.0/me',
token: `https://login.microsoftonline.com/${tenantId}/oauth2/v2.0/token`
},
codeChallengeMethod: 'S256',
scope: ['openid', 'profile'],
autoLogout: true
})
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.
Agreed, @JoaoPedroAS51's approach is much easier to read. I'd suggest a comment above the || "common"
line that says this:
// Allows users with both personal Microsoft accounts and work/school accounts from Azure AD to sign into the application.
// https://docs.microsoft.com/en-us/azure/active-directory/develop/active-directory-v2-protocols#endpoints
96fc221
to
032ce67
Compare
The logout method should be something like this. But need to test.
import { encodeQuery, removeTokenPrefix } from '../utils'
import { Oauth2Scheme } from '../schemes/oauth2'
export class AzureADScheme extends Oauth2Scheme {
logout() {
if (this.options.endpoints.logout) {
const token = removeTokenPrefix(this.token.get())
const opts = {
post_logout_redirect_uri: this.logoutRedirectURI,
id_token_hint: token
};
const url = this.options.endpoints.logout + "?" + encodeQuery(opts)
window.location.replace(url)
}
return this.$auth.reset()
}
} |
@@ -0,0 +1,17 @@ | |||
import { assignDefaults, addAuthorize } from '../../utils/provider' |
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.
Providers are not inside folders anymore. So you can move the file to providers
dir, and rename to ad.ts
Any update on the PR progress? |
// cc @bmulholland