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

feat: resolve authentication strategy registered via extension point #2763

Merged
merged 1 commit into from
May 3, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 196 additions & 0 deletions packages/authentication/docs/authentication-system.md
Original file line number Diff line number Diff line change
Expand Up @@ -163,3 +163,199 @@ And the abstractions for:
- return user
- controller function:
- process the injected user

## Registering an authentication strategy via an extension point
Copy link
Contributor

Choose a reason for hiding this comment

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

I hope in follow-up PRs we can move this awesome doc to the README.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@b-admike , yes. @jannyHou 's PR : #2822 overlaps with mine, and we have discussed that we will be moving all separate .md files in the authentication package into a comprehensive readme.md file. And this readme file will also be the source of the loopback.io docs that are generated by our doc build process.


Authentication strategies register themselves to an authentication strategy
provider using an
[Extension Point and Extensions](https://loopback.io/doc/en/lb4/Extension-point-and-extensions.html)
pattern.

The `AuthenticationStrategyProvider` class in
`src/providers/auth-strategy.provider.ts` (shown below) declares an
`extension point` named
`AuthenticationBindings.AUTHENTICATION_STRATEGY_EXTENSION_POINT_NAME` via the
`@extensionPoint` decorator. The binding scope is set to **transient** because
an authentication strategy **may** differ with each request.
`AuthenticationStrategyProvider` is responsible for finding (with the aid of the
`@extensions()` **getter** decorator) and returning an authentication strategy
which has a specific **name** and has been registered as an **extension** of the
aforementioned **extension point**.

```ts
@extensionPoint(
AuthenticationBindings.AUTHENTICATION_STRATEGY_EXTENSION_POINT_NAME,
{scope: BindingScope.TRANSIENT},
)
export class AuthenticationStrategyProvider
implements Provider<AuthenticationStrategy | undefined> {
constructor(
@extensions()
private authenticationStrategies: Getter<AuthenticationStrategy[]>,
@inject(AuthenticationBindings.METADATA)
private metadata?: AuthenticationMetadata,
) {}
async value(): Promise<AuthenticationStrategy | undefined> {
if (!this.metadata) {
return undefined;
}
const name = this.metadata.strategy;
const strategy = await this.findAuthenticationStrategy(name);
if (!strategy) {
// important not to throw a non-protocol-specific error here
let error = new Error(`The strategy '${name}' is not available.`);
Object.assign(error, {
code: AUTHENTICATION_STRATEGY_NOT_FOUND,
});
throw error;
}
return strategy;
}

async findAuthenticationStrategy(name: string) {
const strategies = await this.authenticationStrategies();
const matchingAuthStrategy = strategies.find(a => a.name === name);
return matchingAuthStrategy;
}
}
```

The **name** of the strategy is specified in the `@authenticate` decorator that
is added to a controller method when authentication is desired for a specific
endpoint.

```ts
class UserController {
constructor() {}
@get('/whoAmI')
@authenticate('basic')
whoAmI()
{
...
}
}
```

An authentication strategy must implement the `AuthenticationStrategy` interface
defined in `src/types.ts`.

```ts
export interface BasicAuthenticationStrategyCredentials {
email: string;
password: string;
}

export class BasicAuthenticationStrategy implements AuthenticationStrategy {
name: string = 'basic';

constructor(
@inject(BasicAuthenticationStrategyBindings.USER_SERVICE)
private userService: BasicAuthenticationUserService,
) {}

async authenticate(request: Request): Promise<UserProfile | undefined> {
const credentials: BasicAuthenticationStrategyCredentials = this.extractCredentals(
request,
);
const user = await this.userService.verifyCredentials(credentials);
const userProfile = this.userService.convertToUserProfile(user);

return userProfile;
}
```

A custom sequence must be created to insert the
`AuthenticationBindings.AUTH_ACTION` action. The `AuthenticateFn` function
interface is implemented by the `value()` function of
`AuthenticateActionProvider` class in `/src/providers/auth-action.provider.ts`.

```ts
class SequenceIncludingAuthentication implements SequenceHandler {
constructor(
@inject(SequenceActions.FIND_ROUTE) protected findRoute: FindRoute,
@inject(SequenceActions.PARSE_PARAMS)
protected parseParams: ParseParams,
@inject(SequenceActions.INVOKE_METHOD) protected invoke: InvokeMethod,
@inject(SequenceActions.SEND) protected send: Send,
@inject(SequenceActions.REJECT) protected reject: Reject,
@inject(AuthenticationBindings.AUTH_ACTION)
protected authenticateRequest: AuthenticateFn,
) {}

async handle(context: RequestContext) {
try {
const {request, response} = context;
const route = this.findRoute(request);

//
// The authentication action utilizes a strategy resolver to find
// an authentication strategy by name, and then it calls
// strategy.authenticate(request).
//
// The strategy resolver throws a non-http error if it cannot
// resolve the strategy. When the strategy resolver obtains
// a strategy, it calls strategy.authentication(request) which
// is expected to return a user profile. If the user profile
// is undefined, then it throws a non-http error.
//
// It is necessary to catch these errors
// and rethrow them as http errors (in our REST application example)
//
// Errors thrown by the strategy implementations are http errors
// (in our REST application example). We simply rethrow them.
//
try {
//call authentication action
await this.authenticateRequest(request);
} catch (e) {
// strategy not found error, or user profile undefined
if (
e.code === AUTHENTICATION_STRATEGY_NOT_FOUND ||
e.code === USER_PROFILE_NOT_FOUND
) {
throw new HttpErrors.Unauthorized(e.message);
Copy link
Member

Choose a reason for hiding this comment

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

I feel this is adding too much friction for application developers. The current proposal is is good enough for now (incremental baby steps FTW!), but I'd like you to create a follow-up SPIKE issue to look into ways how to allow @loopback/authentication (and any other 3rd party extensions) to contribute custom error-code to status-code mapping. See the current hard-coded map here:
https://github.com/strongloop/loopback-next/blob/22400fe9a40b17042a5df684300f8a007dd50edd/packages/rest/src/providers/reject.provider.ts#L12-L16

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bajtos :

I'd like you to create a follow-up SPIKE issue to look into ways how to allow @loopback/authentication (and any other 3rd party extensions)

@jannyHou mentioned there was a task to work on something like this.

Do you mean Enable custom reject implementation to leverage the built-in "code to statusCode" mapping ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bajtos

I feel this is adding too much friction for application developers

I am not sure what you mean. The code you mention is inside a custom sequence that user must write... we are not shipping the sequence which includes the authentication action. The two agnostic errors I am specifying here are thrown from code that cannot throw protocol specific errors since that code may support protocols other than http in the future. My mock (not production) implementations for strategies, token service and user service need to throw some kind of errors. I decided for my test bucket to have them throw http errors because my test bucket runs a rest server that handles http requests. The custom sequence rethrows these http exceptions as is, and throws the captured agnostic errors as http errors .

Since users will be writing their own production-ready implementations for sequence, token service, user service, and strategies , I am having a hard time understanding how the code above is painting the user into a corner. If you could clarify. thanks :)

Copy link
Member

Choose a reason for hiding this comment

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

@emonddr

Do you mean Enable custom reject implementation to leverage the built-in "code to statusCode" mapping ?

Not really. The goal of #1942 is to allow custom implementations of reject to read data from the mapping table. I.e. if the application developer decides to write a different error handling routine, they should have a way how to determine the HTTP status code to return based on error code.

In this discussion, I am asking for a mechanism that will allow extension developers to contribute new mapping entries, so that reject implementations can pick them up.

I am not sure what you mean. The code you mention is inside a custom sequence that user must write...

I find the custom sequence you are proposing as too complex. I would like the custom sequence to look like this:

class SequenceIncludingAuthentication implements SequenceHandler {
  // ...
  async handle(context: RequestContext) {
    try {
      const {request, response} = context;
      const route = this.findRoute(request);
      
      await this.authenticateRequest(request);	

      const args = await this.parseParams(request, route);
      const result = await this.invoke(route, args);
      this.send(response, result);
    } catch (error) {
      this.reject(context, error);
      return;
    }
  }
}

Under the hood:

  • When @loopback/authentication is registered as a component via app.component, it contributes new mappings to codeToStatusCodeMap.
  • The built-in reject action picks up the mapping contributed by the authentication extension and converts AUTHENTICATION_STRATEGY_NOT_FOUND to 401 Unauthorized.

BTW the current implementation is problematic because it discards the original error stack trace pointing to the place where the error was triggered (e.g. in the authentication strategy) and replaces it with a stack trace pointing to the custom sequence. This makes troubleshooting rather difficult.

Here is a better solution:

try {
  await this.authenticateRequest(request);
} catch (e) {
  if (
    e.code === AUTHENTICATION_STRATEGY_NOT_FOUND ||
    e.code === USER_PROFILE_NOT_FOUND
  ) {
    err.statusCode = 401;
  }
  throw err;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point about the stack trace. I will look into it. :)

} else {
// strategy error
throw e;
}
}

// Authentication successful, proceed to invoke controller
const args = await this.parseParams(request, route);
const result = await this.invoke(route, args);
this.send(response, result);
} catch (error) {
this.reject(context, error);
return;
}
}
}
```

Then custom sequence must be bound to the application, and the authentication
strategy must be added as an **extension** of the **extension point** using the
`addExtension` function.

```ts
export class MyApplication extends BootMixin(
ServiceMixin(RepositoryMixin(RestApplication)),
) {
constructor(options?: ApplicationConfig) {
super(options);

this.component(AuthenticationComponent);

this.sequence(SequenceIncludingAuthentication);

addExtension(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should add a sugar method such as app.extension().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@raymondfeng is this something you will add?

this,
AuthenticationBindings.AUTHENTICATION_STRATEGY_EXTENSION_POINT_NAME,
BasicAuthenticationStrategy,
{
namespace:
AuthenticationBindings.AUTHENTICATION_STRATEGY_EXTENSION_POINT_NAME,
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, the namespace is in plural form, such as authentication.strategies

},
);
}
}
```
114 changes: 114 additions & 0 deletions packages/authentication/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion packages/authentication/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@
"@types/node": "^10.11.2",
"@types/passport": "^1.0.0",
"@types/passport-http": "^0.3.6",
"passport-http": "^0.3.0"
"passport-http": "^0.3.0",
"jsonwebtoken": "^8.5.1"
},
"keywords": [
"LoopBack",
Expand Down
Loading