Skip to content
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

Update Office365 / Microsoft provider #376

Closed
freakinside opened this issue Jul 24, 2018 · 13 comments
Closed

Update Office365 / Microsoft provider #376

freakinside opened this issue Jul 24, 2018 · 13 comments
Assignees
Labels
bug
Milestone

Comments

@freakinside
Copy link

@freakinside freakinside commented Jul 24, 2018

Im using bell version 8.8.0

I have github integration working but i have problems with office365. After i select a user it seems there is a problem obtaining the profile.

I checked the code on the latest version and seems none of the specific urls for office365 changed, so i assume version 8.8.0 should still work as other providers do.

{ Error: Failed obtaining office365 user profile
    at Wreck.(anonymous function) (/var/app/current/node_modules/bell/lib/oauth.js:298:43)
    at read (/var/app/current/node_modules/wreck/lib/index.js:523:20)
    at finish (/var/app/current/node_modules/wreck/lib/index.js:344:20)
    at wrapped (/var/app/current/node_modules/hoek/lib/index.js:879:20)
    at onReaderFinish (/var/app/current/node_modules/wreck/lib/index.js:415:16)
    at g (events.js:292:16)
    at emitNone (events.js:91:20)
    at emit (events.js:185:7)
    at finishMaybe (_stream_writable.js:512:14)
    at endWritable (_stream_writable.js:522:3)
    at Writable.end (_stream_writable.js:487:5)
    at IncomingMessage.onend (_stream_readable.js:511:10)
    at IncomingMessage.g (events.js:292:16)
    at emitNone (events.js:91:20)
    at IncomingMessage.emit (events.js:185:7)
    at endReadableNT (_stream_readable.js:974:12)
    at _combinedTickCallback (internal/process/next_tick.js:80:11)
    at process._tickDomainCallback (internal/process/next_tick.js:128:9)
  data: <Buffer >,
  isBoom: true,
  isServer: true,
  output: 
   { statusCode: 500,
     payload: 
      { statusCode: 500,
        error: 'Internal Server Error',
        message: 'An internal server error occurred' },
     headers: {} },
  reformat: [Function] }
@freakinside

This comment has been minimized.

Copy link
Author

@freakinside freakinside commented Jul 24, 2018

In /var/app/current/node_modules/bell/lib/oauth.js

Wreck.post(settings.provider.token, requestOptions, (err, tokenRes, payload) => {

It assumes that payload object has access_token and refresh_token, but i have id_token and refresh_token instead when using office365 provider. Not sure why i dont get an access_token as response code is 200.

@freakinside

This comment has been minimized.

Copy link
Author

@freakinside freakinside commented Jul 25, 2018

I already get the access_token. The problem is that there the default scopes are note enough to get an access_token. I solved it adding 'User.Read'.

    server.auth.strategy('office365', 'bell', {
        provider: 'office365',
        password: Config.auth.password,
        clientId: Config.auth.external.office365.clientId,
        clientSecret: Config.auth.external.office365.clientSecret,
        isSecure: Config.auth.isSecure,
        location: Config.api.protocol + '://' + Config.api.domain + ((Config.api.port != 80 && Config.api.port != 443) ? ':' + Config.api.port : ''),
        scope: ['openid', 'offline_access', 'profile', 'User.Read']
    });

Now i get a 401 error while trying to retrieve the profile. I see the following error on the response headers.

2000010;reason="ErrorCode: \'PP_E_RPS_CERT_NOT_FOUND\'. Message: \'Certificate cannot be found. Certificate required for the operation cannot be found.%0d%0a Internal error: spRPSTicket->ProcessToken failed. Failed to call CRPSDataCryptImpl::UnpackData:Certificate cannot be found. Certificate required for the operation cannot be found.%0d%0a Internal error: Failed to decrypt data. :Failed to get session key. RecipientId=293577. spCache->GetCacheItem returns error.:Cert Name: (null). SKI: 45237f1479435b9c4def8b7a1b36edb0105e0546...\'";error_category="invalid_msa_ticket"

Does this work only with paid office365 subscriptions or should it also work with free microsoft accounts?

@freakinside

This comment has been minimized.

Copy link
Author

@freakinside freakinside commented Jul 25, 2018

If i make an overkill scope declaration i access profile data. Im trying to find which are the minimum scopes to keep it working.

@freakinside

This comment has been minimized.

Copy link
Author

@freakinside freakinside commented Jul 25, 2018

It works using this scopes:

scope: ['openid', 'offline_access', 'profile', 'https://outlook.office.com/mailboxsettings.read']

Ill try to test it with a paid account a bit later to check if there are any differences.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Jul 30, 2018

Microsoft/Office oauth is a weird thing, I believe they have 3 versions now and it is super confusing which one to use :D
We found that https://github.com/Salesflare/bell/blob/dc5bc6b40b6e533ae5854aa2987364da9689d1c2/lib/providers/microsoft.js covers both Office365 profiles and Outlook.com profiles and I believe even msn.com profiles.
Also note that the scope notation is different for graph api then for the old api.
If you want profile and mail access I believe you need something like scope: ['User.Read', 'Mail.ReadWrite', 'Mail.Send', 'offline_access']
Do note that the graph api mail part is a weird api with a bunch of it own quirks (see comments in here https://github.com/Salesflare/unimail/blob/93aedab1e4bb0c347515ee983923ffa62464c452/lib/unimail-office365.js :D)

@ItalyPaleAle

This comment has been minimized.

Copy link

@ItalyPaleAle ItalyPaleAle commented Sep 19, 2018

Hi @AdriVanHoudt it actually took me a while to figure out how Bell works.

There are two main protocols in the Microsoft cloud for authentication:

  • Azure AD "v1", which Bell implements as azuread
  • Azure AD v2, which Bell implements as office365, but it should more correctly be called azureadv2

You can read about the difference here: https://docs.microsoft.com/en-us/azure/active-directory/develop/azure-ad-endpoint-comparison

The way apps are registered for v1 and v2 is very different too, with separate portals.

// EDIT
Just to make things more complicated, some other documentation (on the same topic) is here: https://developer.microsoft.com/en-us/graph/docs/concepts/auth_overview

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Sep 19, 2018

Yeah, note that Bell atm doesn't have a provider that uses Azure AD v2 + MS graph api.
If you want I can make a new provider that does this. And then maybe mark the others as deprecated since afaik the graph api one should support everything (?)
Then we have

@ItalyPaleAle

This comment has been minimized.

Copy link

@ItalyPaleAle ItalyPaleAle commented Sep 19, 2018

I've been playing around with a PR, but I'm getting this issue: Authentication failed due to: Missing (provider) request token cookie. Not sure how to fix that, in my test app.

By the way, I would recommend deprecating office365 and live, which are old and/or not implemented correctly. We should have, instead:

  • azuread for v1
  • azureadv2 for v2 (this fully replaces office365, but getting the email address "the right way" - currently it can't get the email if the user doesn't have an Office365 subscription)

We could have "microsoft" as an alias of "azureadv2"

The https://graph.microsoft.com/v1.0/me/ and https://outlook.office.com/api/v2.0/me endpoints don't include an email address unless the user has an active Office365 subscription, or if the account is sourced from an on-prem AD that includes the "mail" field. Workaround is to read the id_token.

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Nov 10, 2018

@ItalyPaleAle any progress? If you need help with a PR, open one so we can see where it is failing.

@hueniverse hueniverse changed the title Failed obtaining office365 user profile. v8.8.0 Update Office365 / Microsoft provider Nov 10, 2018
@hueniverse hueniverse added the bug label Nov 10, 2018
@ItalyPaleAle

This comment has been minimized.

Copy link

@ItalyPaleAle ItalyPaleAle commented Nov 11, 2018

@hueniverse I encountered a lot of issues, some with Bell itself (like above) and some with Azure, and ended up writing my own code that didn't use Bell.

In particular, the issues with Azure are worth a discussion, as I believe there are some choices to be made:

  1. Bell currently supports many different providers, which could be unified: azuread (uses v1 APIs), live (v1) and office365 (v2 APIs). They all return different data too, as they use different endpoints. Might make sense to simplify them to two: azuread and azureadv2. Having multiple providers, doing similar things, was bugging me a lot (I have a sort of OCD for things that aren't "in order" ;) ).
  2. Providers should support 4 different (kinds of) "tenants": a single tenant ID or domain name (for an Azure AD / Office365 tenant), consumers to support only MSA's (Microsoft Accounts - former Windows Live ID), organizations to support only Azure AD directories/tenants (but all of them), and common to support all of the above. This choice isn't secondary, as there are situations where an user account can belong to multiple tenants, or when a MSA is added to an Azure AD directory, and specifying the tenant or directory type is important.
  3. There is currently an issue with returning the user's email address using any Microsoft Graph API. In fact, calling the MS Graph endpoints (linked above) does not always return an email address. This happens when the user account belongs to an Azure AD tenant, and either does not have an Office365 license, or it's not sourced from an on-prem Active Directory (in which case the "email" field would be set). Reading the UPN could provide the email address, but this wouldn't work if the account is a Microsoft Account added to an Azure AD tenant (granted, it's an edge case, but...). Seems that the only reliable workaround here is to request an id_token (using OpenID Connect) and not just an OAuth 2.0 token. (Note: I've reported the issue to the Azure AD team, but can't speak for when it will be fixed)
  4. Corollary to points 1 and 3: we should figure out if we want a common format for the user profile returned, and what that format should be.
@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Nov 28, 2018

Hmm nice and complex.
I think at this point it would be easier if you opened up a PR (even if it doesn't work but just shows the intent and where you would see this going).
And then we can discuss from there.
I know we use this config in prod with success but maybe there are some enterprise/edge cases we missed.

  1. so you propose to merge azuread and live and move office365 to azureadv2? Is this how Microsoft refers to them?
  2. Not super following but how would bell support these tenants?
  3. This feels like an azure ad issue indeed and fairly edge casey. I also don't see how bell would fix this.
  4. We can do this for MS configs but is difficult to stretch to all providers.
@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Jan 11, 2019

@ItalyPaleAle are you still up to champion this issue?

@ItalyPaleAle

This comment has been minimized.

Copy link

@ItalyPaleAle ItalyPaleAle commented Jan 16, 2019

I'm very low on bandwidth for writing code for this at the moment, but I can provide some guidance if anyone is willing to take this on. If you want a PR, I am not sure when I can find time to do this.

I originally looked into this for a side project of mine, but ended up writing my own code as I was having too many issues with Bell.

@hueniverse hueniverse added this to the 10.1.2 milestone Sep 13, 2019
@hueniverse hueniverse self-assigned this Sep 13, 2019
@hueniverse hueniverse closed this Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.