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

Create abstraction for login related operations in authentication module #2491

Closed
6 tasks
jannyHou opened this issue Feb 26, 2019 · 3 comments
Closed
6 tasks

Comments

@jannyHou
Copy link
Contributor

jannyHou commented Feb 26, 2019

Description / Steps to reproduce / Feature proposal

Description

Create abstraction for login service.

  • Create abstraction for login service
// U stands for User model, in this story we can omit it and use `UserProfile` 
// C stands for Credentials type
export interface LoginService<U, C> {}
  • Login service contains
    • login function login(request: Request): Promise<U>
      • optional to break it down into 2 functions
      • extractCredentials(request: Request): Promise<C>
      • verifyCredentials(credentials: C): Promise<U>
    • logout function invalidateCredentials?(request: Request): Promise<boolean>
  • Create Markdown file with tests
@bajtos
Copy link
Member

bajtos commented Mar 5, 2019

Similarly to what I wrote in #2435 (comment), I see two distinctive services here:

  1. One service to deal with the business logic of credentials in a way that's independent from the transport

    • verifyCredentials(credentials: C): Promise<U>
  2. Another service to deal with transport specific details, e.g. where to extract username & password from.

    • extractCredentials(request: Request): Promise<C>
    • invalidateCredentials?(request: Request): Promise<boolean>

While it makes sense to me to have an interface describing a service that can convert credentials into a user profile (perform the login), I find it problematic to implement transport specific behavior in a service.

How do we envision that such service would be used from a controller? It's important to describe the login endpoint via OpenAPI and include specification of input parameters. Are we expecting the controller method to be decorated with @param decorators and then pass the entire request to the login service? That would make it too easy to introduce bugs where the controller is describing different parameters than the service extracts from the request.

IMO, extract & invalidate credentials should not be provided by any service, but should be implemented as controller actions leveraging the services described in #2435 under the hood.

class UserController {
  constructor (
    // inject response, AccessTokenService, TokenTransportStrategy and LoginService
  ) {}

  @post('/login')
  async login(
    @param() email: string,
    @param() password: string,
  ) {
    const user = await this.loginService.verifyCredentials({email, password});
    const token = await this.tokenService.generateAccessToken(user);
    await this.transportStrategy.serializeAccessToken(this.response);
   // uh oh, what is the return value of this method?
   // if the token is returned in response body, how are we going to describe response schema?
  }
    
  @post('/logout')
  async logout(
    @inject('current access token') token: string
  ) {
    await this.tokenService.invalidateAccessToken(token);
  }
}

Also the entire idea of verifyCredentials(credentials) is applicable only to applications using local authentication scheme (storing user name & password). It won't work for applications using OpenID or OAuth based flow, where the credentials are verified with a third party authority.

I am not sure how much value that leaves for the verifyCredentials service interface?

@jannyHou
Copy link
Contributor Author

@bajtos

I find it problematic to implement transport specific behavior in a service.
How do we envision that such service would be used from a controller?

This is true...I haven't figured out a good solution for providing the spec. In PR #2576 we moved the login logic from controller function to the auth action, which also may also affect the API definition.

May need more time to come up with a better plan.

@emonddr
Copy link
Contributor

emonddr commented Mar 28, 2019

Implemented as a user service instead of a login service in PR : #2576

@emonddr emonddr closed this as completed Mar 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants