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

Adds Auth0 as IdP #1611

Merged
merged 4 commits into from Aug 17, 2020
Merged

Adds Auth0 as IdP #1611

merged 4 commits into from Aug 17, 2020

Conversation

jaime-talkdesk
Copy link
Contributor

@jaime-talkdesk jaime-talkdesk commented Jul 13, 2020

Hey, I just made a Pull Request!

Adds Auth0 for as Identity Provider to Sign In.

I ended up not using passport-auth0 since it's quite opinionated in the sense they have requirements around using request.session which is not being used in the other passport implementations, as well it's own nonce implementation. This would require to add express-session which I'm not sure if this is the way we want to go.

image

image

image

image

✔️ Checklist

  • All tests are passing yarn test
  • Screenshots attached (for UI changes)
  • Relevant documentation updated
  • Prettier run on changed files
  • Tests added for new functionality
  • Regression tests added for bug fixes

@jaime-talkdesk jaime-talkdesk requested a review from a team as a code owner July 13, 2020 23:18
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

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

👍 to basing it off the passport-oauth2 strategy instead of the auth0 one.

As mentioned in a comment I'm wondering whether it makes sense to include OAuth here at all?

A future thing coming up is how to actually map this to users, but leaving that for later


constructor(private readonly sessionManager: SessionManager<Auth0Session>) {}

async getAccessToken(
Copy link
Member

Choose a reason for hiding this comment

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

Does it actually make sense to implement OAuth for auth0? Do they have their own APIs that you'd want to talk to? Otherwise I'm thinking we just stick to using it as a IdP only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have that use case. It was more for someone to use if they wished so.

What are the bare minimum for the Idp? I would need OpenIdConnectApi but ProfileInfoApi, BackstageIdentityApi, SessionStateApi as well?

Copy link
Member

Choose a reason for hiding this comment

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

Bare minimum for providing a Backstage Identity for now would be ProfileInfoApi, BackstageIdentityApi, SessionStateApi. I think it might make sense to go with only that tbh, as afaik the access and ID tokens would be from the the actual provider used to log in to auth0, and at that point we likely want to stick to the purpose-built APIs we already have for that, e.g. googleAuthApi etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Rugvip UserSettings's OIDCProviderSettings requires for the API Ref to extend OpenIdConnectApi. I think we should support at least OpenID/ID token since we might not have a dedicated API for some Idp that uses Auth0 supports.

If you prefer to remove it anyway, does it make sense to have it as a SignIn provider? I could me mixing up the concepts of Backstage identity and Open ID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed OAuthAPI.getaccessToken() in the meantime

Copy link
Member

Choose a reason for hiding this comment

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

The OIDCProviderSettings isn't part of the sign-in flow though, that's for OAuth, i.e. access delegation. A bit more info that here: https://github.com/spotify/backstage/blob/master/docs/auth/index.md#accessing-third-party-services.

Backstage Identity isn't fully settled yet, right now it's mostly there so that we can add some constraints to the implementation of auth providers that want to be used for sign-in. Either way the Backstage identity is a partial implementation of Open ID Connect. Looking at for example the Google provider, which implements both Open ID and Backstage Identity, you get two different ID Tokens, one that proves your Google identity, and one that proves your Backstage identity.

import { Observable } from '../../../../types';

type CreateOptions = {
// TODO(Following the words of Rugvip): These two should be grabbed from global config when available, they're not unique to Auth0Auth
Copy link
Member

Choose a reason for hiding this comment

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

Gonna try to get around to this after these PRs have landed 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JK :D, I can follow up this to clean it up

tokenParams(): object {
return {
'type': 'web_server',
'grant_type': 'client_credentials'
Copy link
Member

Choose a reason for hiding this comment

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

This confuses me, I assume we would've wanted authorization_code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I assumed the same but they get the access token with that grant type 🤔 Example in their lib: https://github.com/auth0/passport-auth0/blob/096f789bea36a45a18d1a06accdd73decfb65131/lib/index.js#L143

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it's hardcoded to authorization_code in the base strategy, so you could just leave it out here: https://github.com/jaredhanson/passport-oauth2/blob/e20f26aad60ed54f0e7952928cbb64979ef8da2b/lib/strategy.js#L167

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it makes sense. I can test with authorization code to see if behaves according to the RFC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works as expected. Wonder why their lib has client_credentials 🤷

OAuthApi & ProfileInfoApi & BackstageIdentityApi & SessionStateApi & OpenIdConnectApi
>({
id: 'core.auth.auth0',
description: 'Provides authentication towards Gitlab APIs',
Copy link
Collaborator

Choose a reason for hiding this comment

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

the description needs to be updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

) {
if (process.env.NODE_ENV !== 'development') {
throw new Error(
'Failed to initialize OAuth2 auth provider, set AUTH_OAUTH2_CLIENT_ID, AUTH_OAUTH2_CLIENT_SECRET, AUTH_OAUTH2_AUTH_URL, and AUTH_OAUTH2_TOKEN_URL env vars',
Copy link
Collaborator

@soapraj soapraj Jul 14, 2020

Choose a reason for hiding this comment

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

The env vars in the error and warning messages need to be updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

} from '../types';
import OAuth2Strategy from 'passport-oauth2';

interface OAuth0ProviderConfig {
Copy link
Collaborator

@soapraj soapraj Jul 14, 2020

Choose a reason for hiding this comment

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

rename OAuth0ProviderConfig -> Auth0ProviderConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

refreshToken: string;
};

export class OAuth2AuthProvider implements OAuthProviderHandlers {
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename OAuth2AuthProvider -> Auth0Provider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@jaime-talkdesk jaime-talkdesk force-pushed the adds_auth0 branch 5 times, most recently from 0bf1895 to 21afcf8 Compare July 19, 2020 22:41
@Rugvip
Copy link
Member

Rugvip commented Aug 12, 2020

Resolved conflicts with master. The auth config implementation has changed a bit since this was submitted, so we'll need to add auth0 back in the app-config.yaml

@soapraj
Copy link
Collaborator

soapraj commented Aug 12, 2020

Hey @jaime-talkdesk - looks like the backend parts need more fixes before this is good to go. We'll take a stab at it in the next couple of days.

Let us know if you want to take it up?

@nikek nikek merged commit c5c2c2c into backstage:master Aug 17, 2020
@jaime-talkdesk jaime-talkdesk deleted the adds_auth0 branch August 20, 2020 08:42
@jaime-talkdesk
Copy link
Contributor Author

Hey @soapraj, thanks for completing the rest 🙏

@adamdmharvey adamdmharvey mentioned this pull request Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants