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

[Question] Passport - Azure AD Integration #559

Closed
oehm-smith opened this issue May 28, 2019 · 21 comments
Closed

[Question] Passport - Azure AD Integration #559

oehm-smith opened this issue May 28, 2019 · 21 comments

Comments

@oehm-smith
Copy link

oehm-smith commented May 28, 2019

Information

Maybe this will be answered in the new #552 though I'm thinking the differences are enough that this is an independent passport case.

  • Version: 5.15
  • Type: Story

Description

We use Azure and I've been trying to integrate https://www.npmjs.com/package/passport-azure-ad

I have setup the Azure side and the request has a bearer token in the header. The TSED server log shows that the Azure-passport is starting (as a service):

 {"name":"AzureAD: Bearer Strategy","hostname":"Droid.gateway","pid":62602,"level":30,"msg":"In BearerStrategy constructor: strategy created","time":"2019-05-28T06:32:16.485Z","v":0} 

However my BearerStrategy is not firing. I think the problem may be where I place the call for it. In the doco they say to :

   server.get('/api/tasks', passport.authenticate('oauth-bearer', { session: false }), listTasks);

But nothing I've tried seems to call my callback defined just like they have at https://www.npmjs.com/package/passport-azure-ad#5211-sample-using-the-bearerstrategy

And from this I can't understand what @Authenticate() is for in https://tsed.io/tutorials/passport.html#local-strategy. I'm new to passport and there must be stuff happening in the background that hasn't yet been made clear to me. I cloned and followed your code at https://github.com/TypedProject/example-ts-express-decorator/tree/4.0.0/example-passport also.

Update - This is how I think it works (I thought about this overnight).

  • Passport updates the Express namespace and adds to Express.request in particular.
  • /login which in the example-passport code performs Passport.authenticate(..) ... request.login(user) will (if successful login) place the user object in the login
  • The @Authenticate middleware has access to the request object and determine if the login was successful. Hence the if (request.isAuthenticated()). It also has access to any roles defined in the annotation on the endpoint. And it can check these against the user object added on the request during login.

(My confusion over @Authenticate() was that I thought it was responsible for the login also. That was because i was thinking of the use case of Azure AD where the login has already taken place. Now I realise that the example-passport is a different use case (the normal one it seems) where the login happens as part of the application. Phew!)

Is the 'normal' way of adding middleware in Express as they used:

 server.get('/api/tasks', passport.authenticate('oauth-bearer', { session: false }), listTasks);

The same as TSED Middleware (https://tsed.io/docs/middlewares.html#configuration)?

Reading the page again it seems so.

I tried implementing an Endpoint Middleware and using it with @UseBefore on my endpoint, thinking that I need to get in before the @Authenticate. However my passport-azure-ad callback is still not running.

Maybe I'm missing something? I will keep on looking at this, however if you have any ideas I'm all ears.

@oehm-smith oehm-smith changed the title Passport- Passport - Azure AD May 29, 2019
@oehm-smith oehm-smith changed the title Passport - Azure AD Passport - Azure AD Integration May 29, 2019
@oehm-smith
Copy link
Author

oehm-smith commented May 29, 2019

The @UseBefore middleware seems be correct since is triggering before the @Authenticated one is. However it appears that it isn't firing. Atleast i know where the problem lies now.

@oehm-smith
Copy link
Author

oehm-smith commented May 29, 2019

Looking through the code, i had to use the following (see below). I also seemed to be able to consolidate my changes in to the @Authenticated middleware before request.isAuthenticated().

The changes I've made now gets past the problem I had (the callback is being called), however it is still not working due what appears to be an issue with passport-azure-ad, which I am continuing to work on.

TranslationAPIController.ts

@OverrideMiddleware(AuthenticatedMiddleware)
export class UserAuthMiddleware implements IMiddleware {
    constructor(@Inject() private authService: AuthService) {}

    public use(
        @EndpointInfo() endpoint: EndpointMetadata,
        @Request() request: express.Request,
        @Response() response: express.Response,
        @Next() next: express.NextFunction
    ) {
        const options = endpoint.get(AuthenticatedMiddleware) || {};
        this.authService.authenticate(request);   <-- Had to add this
        if (!request.isAuthenticated()) {
            throw new Forbidden('Forbidden');
        }
        next();
    }
}

AuthService.ts

    setup() {
        this.bearerStrategy = new BearerStrategy(jwtOptions,
            (token: ITokenPayload, done: VerifyCallback) => { 
        ....
    }

    authenticate(request: express.Request) {
        this.bearerStrategy.authenticate(request, {session: false}); <-- This is what i had to use
    }

Its a bit annoying when the doco is not updated at the same time as the code.... Mind you, this is probably something that will become obvious once I've got my head (further) around Passport!

For the record, the error I am now receiving is:

TypeError: self.success is not a function
    at verified (/Users/bbos/dev/dhs/translate/translateboard/node_modules/passport-azure-ad/lib/bearerstrategy.js:565:21)

AzureAD/passport-azure-ad#427

@oehm-smith oehm-smith changed the title Passport - Azure AD Integration [Question] Passport - Azure AD Integration May 30, 2019
@oehm-smith
Copy link
Author

I found the solution and was wrong above. Please see https://stackoverflow.com/questions/56372349/azure-ad-open-bearerstrategy-typeerror-self-success-is-not-a-function/56389805#56389805.

I will write up a sample project for passport-azure-ad in TSED when the problem is fully solved (I have an outstanding issue as discussed in the question at https://stackoverflow.com/questions/56389077/passport-azure-ad-in-tsed-framework-seems-to-run-asynchronously). I'm happy for anyone here to look at that!

@Romakita
Copy link
Collaborator

Romakita commented May 31, 2019

Sorry @oehm-smith. I forgot to answer you. I know the documentation over passport is only correct for local strategy. For other strategy it's a bit complicated. This is why I work on @tsed/passport package #552
It should simplify the passport integration.

To answers you, the simpliest way to doing that:

server.get('/api/tasks', passport.authenticate('oauth-bearer', { session: false }), listTasks);

with Tsed, it should be something like that:

class MyCtrl {
   @Get('/api/tasks')
   @UseBefore(passport.authenticate('oauth-bearer', { session: false }))
   get() {
     
   } 
}

Since v5.16.0, use @Authenticated decorator isn't the right way to protected you route. @Authenticated is used for a local strategy with passport (my bad) and it will be deprecated in future in favor of @UseAuth which is more generic.

Declare custom middleware/decorator is the right way in your case.

In your case, declaring a OAuthBearer decorator could be something like that:

import {applyDecorators} from  "@tsed/core";
import {UseAuth} from "@tsed/common";
import {Security, Operation, Responses} from "@tsed/swagger";

export function CustomAuth(options = {}): Function {
  return applyDecorators(
    UseAuth(passport.authenticate('oauth-bearer', { session: false, ...options })),

    // Metadata for swagger
    Security("oauth", ...(options.scopes || [])),
    Operation({
      "parameters": [
        {
          "in": "header",
          "name": "Authorization",
          "type": "string",
          "required": true
        }
      ]
    }),
    Responses(401, {description: "Unauthorized"}),
    Responses(403, {description: "Forbidden"})
  );
}

Then you have to configure Passport.js with oauth-bearer.

If I can help you to provide example, tell me. I can add you to official tsed example repository. Your help are welcome ;)

See you
Romain

@oehm-smith
Copy link
Author

oehm-smith commented Jun 1, 2019

Thanks @Romakita, I will try out your suggested changes. What I did seems to work though as per the stackoverflow link I included, there is a problem with the async nature of passport-azure-ad. I created a hack using setTimeout() to prove that everything else works.

I will include example(s) for the project.

@oehm-smith
Copy link
Author

Hi @Romakita I think the @UseAuth(CustomAuthMiddleware) mentioned at https://tsed.io/docs/authentication.html#usage is going to suffer the same problem I talked about in https://stackoverflow.com/questions/56389077/passport-azure-ad-seems-to-run-asynchronously. The solution of @UseBefore(passport.authenticate('oauth-bearer', { session: false })) works but doesn't allow for any coding.

I see your response above didn't talk about the CustomAuthMiddleware as per https://tsed.io/docs/authentication.html#usage. Did you have insight knowing that this is a problem?

ps. I have to clean up that SO post. jaredhanson/passport#536 (comment) is a bit more succinct.

pps. The simple 'normal' version works, but doesn't allow me to get in with code to check the authInfo.

@oehm-smith oehm-smith reopened this Jun 4, 2019
@Romakita
Copy link
Collaborator

Romakita commented Jun 4, 2019

Hello @oehm-smith,
Maybe I don't unterstand what you want. But Passport use strategies and if you want customize strategy and verify information before connecting a user, you have to configure correctly your stragegy.

The first step is to declare your strategy and append it to Passport. Factory could be the right recipe to doing that.

export const BearerStrategy = Symbol.for("BearerStrategy");

registerFactory(BearerStrategy, {
  deps: [TokenService, ServerSettingsService],
  useFactory: (tokenService: TokenService, settings: ServerSettingsService) => {
    const verify = async (token: ITokenPayload, done: VerifyCallback) => {
      // Verify is the right place to check given token and return userinfo
      try {
        const user = await tokenService.find(token);

        if (!user) {
          tokenService.add(token);
          return done(null, token);
        }

        return done(null, user, token);
      } catch (er) {
        return done(er);
      }
    };

    const strategy = new BearerStrategy(settings.get("jwtOptions"), verify);

    Passport.use(strategy);

    return strategy;
  }
});

If you have to handle authenticate flow, you can override authenticate BearerStrategy method as following:

export class CustomBearerStrategy extends BearerStrategy {
   authenticate(...args) {
      // Do something
      return super.authenticate(...args)
   }
}

Now we can create custom decorator for oauth-bearer:

import {applyDecorators} from  "@tsed/core";
import {UseAuth} from "@tsed/common";
import {Security, Operation, Responses} from "@tsed/swagger";

export function OAuthBearer(options = {}): Function {
  return applyDecorators(
    UseAuth(passport.authenticate('oauth-bearer', { session: false, ...options })),

    // Metadata for swagger
    Security("oauth", ...(options.scopes || [])),
    Operation({
      "parameters": [
        {
          "in": "header",
          "name": "Authorization",
          "type": "string",
          "required": true
        }
      ]
    }),
    Responses(401, {description: "Unauthorized"}),
    Responses(403, {description: "Forbidden"})
  );
}

Then you can use this decorator to protect your routes:

@Controller("/")
exports class MyController {
    @Get("/")
    @OAuthBearer()
    getSomething() {

    }
}

Tell if these examples are aligned with your needs and if it works :)
See you
Romain

@oehm-smith
Copy link
Author

Thanks @Romakita for the attempt at understanding the problem. Perhaps rewriting the passport-azure-ad bearerStrategy would be a solution though that is a level of complexity I don't want to go to (since then I'd have to maintain it).

My main need to do this (as described in my new SO Post - https://stackoverflow.com/questions/56453040/passport-azure-ad-seems-to-run-asynchronously-still) is so that I can get access to the roles and scopes defined on the endpoint. But with this async issue I can't.

I will have to let this go now as I've been working on it for far too long. The desire was to have fine-grained auth. I'll just have to report back that it wasn't possible.

Thanks again!

@Romakita
Copy link
Collaborator

Romakita commented Jun 5, 2019

Hoo ok I see now :).
Given option to OAuthBearer decorator can be retrieved at any time.
You have just update OAuthBearer code:

export const OAuthBearerOptions = Symbol.for("OAuthBearerOptions")
import {applyDecorators} from  "@tsed/core";
import {UseAuth, AuthOptions} from "@tsed/common";
import {Security, Operation, Responses} from "@tsed/swagger";
import {OAuthBearerOptions} from "./OAuthBearerOptions";

export function OAuthBearer(options = {}): Function {
  return applyDecorators(
    AuthOptions(OAuthBearerOptions, options), // Add this to store all options and retrieve it in verify function
    UseAuth(passport.authenticate('oauth-bearer', { session: false, ...options })),
   
    // Metadata for swagger
    Security("oauth", ...(options.scopes || [])),
    Operation({
      "parameters": [
        {
          "in": "header",
          "name": "Authorization",
          "type": "string",
          "required": true
        }
      ]
    }),
    Responses(401, {description: "Unauthorized"}),
    Responses(403, {description: "Forbidden"})
  );
}

Then you can retrieve stored options in verify function (which is the right place to handle authentication and check precondition)

import {Req, }
import {OAuthBearerOptions} from "./OAuthBearerOptions"

export const BearerStrategy = Symbol.for("BearerStrategy");

registerFactory({
  provide: BearerStrategy,
  deps: [AuthService, ServerSettingsService],
  useFactory: (authService: AuthService, settings: ServerSettingsService) => {
    const verify = async (req: Req, token: ITokenPayload, done: VerifyCallback) => {
      // Verify is the right place to check given token and return userinfo
      try {
        const options = req.ctx.endpoint.get(OAuthBearerOptions) // retrieve options configured for the endpoint 
      
        // check precondition and authenticate user by his token and given options
        const user = await AuthService.authenticate(token, options);

        if (!user) {
          tokenService.add(token);
          return done(null, token);
        }

        return done(null, user, token);
      } catch (er) {
        return done(er);
      }
    };

    const strategy = new BearerStrategy({
      ...settings.get("jwtOptions"),
       passReqToCallback: true  // !!!! IMPORTANT
    }, verify);

    Passport.use(strategy);

    return strategy;
  }
});

I think this solution will work ;)
See you @oehm-smith
Romain

@oehm-smith
Copy link
Author

oehm-smith commented Jun 6, 2019

I see. My bad for thinking you meant re-write Passport-Azure-AD.

I started simple and first added AuthOptions to OAuthBearer (and didn't implemented the registerFactory you suggested). It doesn't appear to work.

I could see that items were added to the store but in the request.ctx.endpoint is nothing unexpected:

Request.ctx.endpoint: {"_target":{},"_propertyKey":"search","_name":"search",
"_store":{"_map":{}},"_methodClassName":"search","beforeMiddlewares":[null],
"middlewares":[],"afterMiddlewares":[],"pathsMethods":
[{"method":"post","path":"/search","isFinal":true}]}

There is a _store but it is empty.

I put a breakpoint in AuthOptions and after the last call (*) to store.merge (https://github.com/TypedProject/ts-express-decorators/blob/bdf3c1418fb236224b633f4954aacbe6ddd48400/packages/common/src/mvc/decorators/method/authOptions.ts#L53) it has the content:

> store.entries()
    [[Entries]] = Array(2)
    0 = {"OAuthBearerOptions" => Object} {key: "OAuthBearerOptions", value: }
    1 = {"authenticate" => Object} {key: "authenticate", value: }
    length = 2
    ...

> store.forEach(x => console.log(JSON.stringify(x)))
    {"role":"admin","scope":["admin"]}
    {}

(*) - it is called 11 times. I have 8 endpoints - I suppose a few extra calls for backend endpoints such as for swagger. I only have 4 endpoints using @CustomAuth() - i guess this AuthOptions is called by default implicitly.

Given this first attempt didn't work, I thought that maybe the registerFactory that I hadn't written is important (although as far as I understand it, the purpose of registerFactory is to provide different implementations of an interface or similar (constant also maybe ...)). So I put it in place ...

I couldn't get exactly what you had written working and instead I wrote the following in my existing AuthService:

type BearerStrategyFactory = BearerStrategy
const BearerStrategyFactory = Symbol.for('BearerStrategy');

const verifyCallback = async (request: express.Request, token: ITokenPayload, done: VerifyCallback) => { 
    ... 
    console.log(`* Request.ctx.endpoint: ${JSON.stringify(request.ctx.endpoint)}`);
}

registerFactory(BearerStrategyFactory, {
    deps: [],
    useFactory: () => {
        console.log(`BearerStrategyFactory`);
        const strategy = new BearerStrategy(jwtOptions, verifyCallback);
    
        Passport.use('oauth-bearer', strategy);
    
        return strategy;
    }
});

@Service()
export class AuthService implements BeforeRoutesInit, AfterRoutesInit {
    constructor(private serverSettings: ServerSettingsService,
                @Inject(ExpressApplication) private  expressApplication: ExpressApplication),
                @Inject(BearerStrategyFactory) bearerStrategy: BearerStrategyFactory) {
    }

The problem is that the BearerStrategyFactory doesn't seem to be instantiated. I thought that simply @Injecting the registerFactory would be all that is needed. Is there something else I need to do?

And what about the Store being empty as per the first problem I worked on?

@oehm-smith oehm-smith reopened this Jun 6, 2019
@oehm-smith
Copy link
Author

oehm-smith commented Jun 6, 2019

I also tried this but doesn't work either:

class BearerStrategyFactoryClass implements AfterRoutesInit {
    constructor() {
        console.log(`BearerStrategyFactory`);
        const strategy = new BearerStrategy(jwtOptions, verifyCallback);

        Passport.use('oauth-bearer', strategy);

        return strategy;
    }
}

registerFactory(BearerStrategyFactory, BearerStrategyFactoryClass);

Oh, you can't return from a constructor so this would have never worked.

@Romakita
Copy link
Collaborator

Romakita commented Jun 6, 2019

What is you tsed/di version ? I'll try to give you a complete project example ;)

See you

@oehm-smith
Copy link
Author

oehm-smith commented Jun 6, 2019

5.15.0. I see 5.18.1 is available and will upgrade to this (regardless of this problem).

It didn't fix the problem with the registerFactory starting.

@oehm-smith
Copy link
Author

Hi @Romakita did you work out what's missing?

@Romakita
Copy link
Collaborator

I work on example ;)

@Romakita
Copy link
Collaborator

Romakita commented Jun 11, 2019

Here the example: https://github.com/TypedProject/tsed-example-passport-azure-ad
I sent you an invit to this repository ;)

Context is correctly attached to request object, so you can get options from endpoint configuration and use it.
Sorry, some given examples on github are wrong. It explain why your code doesn't work as expected.

Tell me if the example works with your configuration ;)

See you
Romain

@oehm-smith
Copy link
Author

oehm-smith commented Jun 11, 2019

Ace! I will work on that later today.
...
Nope its a day later. Passport-Azure-Ad with scopes was a bit tricky to implement - had to get configuration just right. See https://github.com/AzureAD/passport-azure-ad/issues/396.

That is fixed so I'll move on to look at your code @Romakita .

@oehm-smith
Copy link
Author

oehm-smith commented Jun 14, 2019

I just wanted to clarify the direction I have taken - I implemented Single Page App (SPA) Auth. https://docs.microsoft.com/en-us/azure/active-directory/develop/scenario-spa-app-registration. By virtue of this, the auth I'm putting in place won't work without a client. For my customer's app, I have used Angular 7. Would you like an example using this or do you want your repository to be only about TSED? If the former then great, I can put in place what I have. If not I'll need to work on a B2B or B2C example. https://docs.microsoft.com/en-us/azure/active-directory-b2c/active-directory-b2c-overview, https://docs.microsoft.com/en-us/azure/active-directory/b2b/what-is-b2b

Perhaps the best thing is to rename this example to tsed-example-passport-azure-ad-spa. And leave room to create an example for each alternative.

Your thoughts?

@Romakita
Copy link
Collaborator

Romakita commented Jun 14, 2019

@oehm-smith No problem, right now, the repository is only here to solve your problem with azure-ad. If you want adding a spa, you can. If you think adding more examples will help other developers to implement azure-ad with Ts.ED framework, you're welcome to work on ;). I'll help as it as possible to doing that.

I'll rename the repository by tsed-example-passport-azure-ad-spa when the work on it is down.

And leave room to create an example for each alternative.

If you want another repository to create other example, tell me ^^

See you
Romain

@oehm-smith
Copy link
Author

oehm-smith commented Jun 25, 2019

OMG this has been one of the worst projects I have worked on. Azure AD, much like any Microsoft developer technology is painful to work with if you want to do things a little differently.

My latest issue can be read at https://stackoverflow.com/questions/56746473/azure-ad-a-scope-is-always-required-but-need-a-default-one.

I decided the simplest thing for now is to define an application level scope that, unfortunately, all users will have to belong to. Perhaps it is an oversight by Azure and they will correct it in time and in which case this project can be fixed.

@Romakita Romakita closed this as completed Mar 1, 2020
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

No branches or pull requests

2 participants