8000 feat: Microsoft authentication by tommoor · Pull Request #1953 · outline/outline · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: Microsoft authentication #1953

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Apr 17, 2021
Merged

feat: Microsoft authentication #1953

merged 12 commits into from
Apr 17, 2021

Conversation

tommoor
Copy link
Member
@tommoor tommoor commented Mar 13, 2021 •
edited
Loading

>> Setup Guide <<

If anyone wants to follow the guide and try this branch out on their own instance it would be much appreciated, I only have one Microsoft team to test with and it would be good to get some broader coverage before merging

closes #755

tommoor
tommoor commented Mar 13, 2021
View reviewed changes
tommoor
tommoor commented Mar 14, 2021
View reviewed changes
ulentini
ulentini reviewed Mar 15, 2021
View reviewed changes
@tommoor
Copy link
Member Author
tommoor commented Mar 23, 2021

Waiting on Microsoft Partner approval before moving this forward

This was referenced Mar 28, 2021
Closed
Closed
Closed
@tommoor
Copy link
Member Author
tommoor commented Mar 30, 2021

Thank you for your inquiry about your Microsoft Partner Network account MPNID XXXX in Partner Center. We are pleased to let you know this account is now authorized. Please, note that it may take up to 10 business days for the changes to be reflected in your account. If the issue persists after this time, please re-open this ticket.

🙌 one step closer.

@tommoor tommoor marked this pull request as ready for review April 9, 2021 04:42
@tommoor tommoor changed the title feat: Microsoft/Azure OAuth strategy feat: Microsoft authentication Apr 9, 2021
bglidwell
bglidwell approved these changes Apr 10, 2021
View reviewed changes
Copy link
@bglidwell bglidwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Successfully authenticated for me

@bglidwell
Copy link

Any idea when this might be merged in?

@ulentini
Copy link

Hi, I have 2 issues with this branch (but maybe it's just me)

  1. If I try to setup the azure oauth with one of my company azure apps, I cannot login since I get the "Invalid audience" error. This could depend on the different endpoints used to perform the initial authentication (from the passport strategy) VS the following requests to MS Graph api
  2. Even after I fixed the first problem, only the first login after the server starts has been successful. After that it fails with a "state mismatch" error. I was able to fix this changing this line in the passport azure package. It's like there's some sort of closure on the options object and once the first user generate the state cookie, it gets used for all following users. So, basically, changing that return statement to return { ...options } solved the issue for me.

@tommoor
Copy link
Member Author
tommoor commented Apr 16, 2021 •
edited
Loading

RE 2, weird but it could be the case – other packages do produce a new object, eg:

https://github.com/jaredhanson/passport-google-oauth2/blob/2399ef47f00b47ac587f056f2f100a2c4db81928/lib/strategy.js#L134

I'll fork the dependency now – it's clearly not maintained and a poor thing to rely on for such a key aspect of the application

@tommoor
Copy link
Member Author
tommoor commented Apr 17, 2021

@ulentini moved to a fork, and included your fix for the state and the updated authentication domain. I checked that the state is not retained between sign-ins, so looking good now.

tommoor added 2 commits April 17, 2021 11:36
@tommoor
Update .env.sample
13b8aec
@tommoor
Changing "Microsoft 365" -> "Microsoft"
1e6769b 
Based on the branding guidelines for sign-in here:
https://docs.microsoft.com/en-us/azure/active-directory/develop/howto-add-branding-in-azure-ad-apps#branding-dos-and-donts

> DON’T use “Microsoft 365 ID” or “Azure ID.” Microsoft 365 is also the name of a consumer offering from Microsoft, which doesn’t use Azure AD for authentication.
@tommoor tommoor merged commit 7a8ccdb into main Apr 17, 2021
@delete-merged-branch delete-merged-branch bot deleted the feat/azure-ad branch April 17, 2021 20:22
@tommoor
Copy link
Member Author
tommoor commented Apr 17, 2021

@bglidwell this is now deployed to the cloud hosted version of Outline
8000

@fooness
Copy link
fooness commented Apr 23, 2021

That’s a good step forward!

When might we get authentication methods which are not externalized to Google, Microsoft, or Slack?

Local accounts with Postgres? LDAP? SAML?

#1183

#1881

@tommoor
Copy link
Member Author
tommoor commented Apr 23, 2021

LDAP and SAML are included in the enterprise edition and will not be included in the open source codebase – you (or anyone in the future that reads this) can contact hello@getoutline.com to arrange an enterprise license key.

I have detailed what is required for local accounts in the ticket you linked, if the community wants to build that feature now that the authentication providers are pluggable they are welcome to do so!

@outline outline locked as off-topic and limited conversation to collaborators Apr 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ulentini ulentini ulentini left review comments

@bglidwell bglidwell bglidwell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Microsoft Teams Authentication
4 participants
@tommoor @bglidwell @ulentini @fooness
0