[Spike]Make model UserProfile more flexible

[jannyHou]

Description / Steps to reproduce / Feature proposal

See the discussion in comment, the UserProfile interface in @loopback/authentication should be more flexible to allow user defined custom properties.

Current Behavior

Other than the required id property, there are 2 optional properties name and email, while not all the models being authenticated have them. For example, the user model has firstName and secondName instead of name.

Expected Behavior

Other than the required id property, UserProfile should honour custom properties defined in the authenticated model.

Acceptance Criteria

• Explorer the best approach to allow developers provide additional/custom properties in UserProfile
• Since the authenticate function and the authenticated method return a user profile instance, the solution should make generating OpenAPI schema for the User model easy enough.
• Potential suggestions:
  • Turn UserProfile into a LoopBack 4 model
  • Custom User implements UserProfile
  • Others(for the story owner to explorer more)

See Reporting Issues for more tips on writing good issues

[osmanmesutozcan]

It feels to me @loopback/authentication should not even define a UserProfile interface. It should let the user define their own interfaces and promote that in usage example. It is simply not possible to guess what every user would like to have in their user profile.

Or maybe I am missing something ?

[rohit-khanna]

Hello @jannyHou , @osmanmesutozcan
Cant we create a custom class, implement UserProfile into it and then use the new one?

I implemented the "authentication" with username and password using MongoDB where in instead of returning "UserProfile" instance (once user is Authenticated), i returned my custom class (which implements UserProfile), and it worked.

Or Am i misinterpreting the question?

[jannyHou]

@osmanmesutozcan
The reason why I would like to have a built-in UserProfile model/interface/type is this would simplify the authorization system we are going to build.

While I also understand your concern:

It feels to me @loopback/authentication should not even define a UserProfile interface. It should let the user define their own interfaces and promote that in usage example. It is simply not possible to guess what every user would like to have in their user profile.

The idea of turning UserProfile to a class is to generate the corresponding OpenAPI schema easier. I had a PR on local(haven't published to remote) to create the class and also have the same feeling as yours.

We should explorer more approaches in this issue.

[jannyHou]

Interesting, @rohit-khanna do you mind share your solution here?

Did you also change the UserProfile returned in the strategy-adapter to your custom user class?

[rohit-khanna]

Hello @jannyHou ,
The approach i followed was this:

Custom User Profile Model

@model()
export class User extends Entity implements UserProfile {
  @property({
    type: 'string',
    id: true,
    required: true,
  })
  id: string;

  @property({
    type: 'string',
    required: true,
  })
  username: string;

  @property({
    type: 'string',
    required: true,
  })
  password: string;

  @property({
    type: 'string',
  })
  email?: string;

  @property({
    type: 'boolean',
  })
  employed?: boolean;

  @property({
    type: 'string',
  })
  someDesc?: string;

  constructor(data?: Partial<User>) {
    super(data);
  }
}

Basic Verify Function

It is defined inside my CustomAuthProvider which implements Provider<Strategy | undefined>.

Invoked from value() like this

 if (name === 'BasicStrategy') {
      let self = this;
      return new BasicStrategy(this.basicVerify.bind(self));
    } 

Code:

async basicVerify(
    username: string,
    password: string,
    cb: (err: Error | null, user?: UserProfile | false) => void,
  ) {
    // find user by name & password
    const filterBuilder = new FilterBuilder<User>();
    let whereBuilder = new WhereBuilder<User>();
    filterBuilder
      .where(
        whereBuilder
          .eq('username', `${username}`)
          .eq('password', `${password}`)
          .build(),
      )
      .fields('id', 'email');

    let user: User | null = await this.userRepo.findOne(filterBuilder.build());
    if (user) {
// fetch these fields from DB, or modify or do whatever
      user.employed = true;
      user.someDesc = 'some description goes here';

      cb(null, user);
    } else {
      cb(null, false);
    }
  }

Response received at POSTMAN

{
    "id": "1",
    "email": "[email protected]",
    "employed": true,
    "someDesc": "some description goes here"
}

[jannyHou]

@rohit-khanna Thanks!

When you specify the type of user to be UserProfile | false , the editor should complain about unknown property employed and someDesc IMO...

[rohit-khanna]

@jannyHou , actually it should not complain.
My intention here was Programming to interfaces and since in the current implementation of code, UserProfile is the interface and my user model implements it, it allows me to return any reference of UserProfile.

what do you think ?

[lygstate]

@rohit-khanna Looks good for me.

[rohit-khanna]

@lygstate Cheers

[bajtos]

Other than the required id property, UserProfile should honour custom properties defined in the authenticated model.

@jannyHou I have two comments:

• Why is the id property required?
• What if the database schema uses a different property name for the primary column instead of id? For example pk, uid? It is also possible to use the email or username as the primary key (not that I would recommend such schema).

In this light, I tend to agree with osmanmesutozcan who wrote

It feels to me @loopback/authentication should not even define a UserProfile interface. It should let the user define their own interfaces and promote that in usage example. It is simply not possible to guess what every user would like to have in their user profile.

Example usage:

@model()
class MyUserProfile extends Entity {
 // ...
}

class MyController {
  @get('/users/me', {
    responses: {
      '200': {
        description: 'The current user profile',
        content: {
          'application/json': {
            schema: {'x-ts-type': MyUserProfile}},
          },
        },
      },
    },
  })
  @authenticate('jwt')
  async printCurrentUser(
    @inject('authentication.currentUser') currentUser: MyUserProfile,
  ): Promise<MyUserProfile> {
    return currentUser;
  }
}

I am not sure what would this mean for our implementation. Maybe we should simply change UserProfile into an empty interface with no properties? Maybe we can leverage generic arguments instead? IDK.

[dhmlau]

Discussion with @raymondfeng @jannyHou @emonddr:
this will eventually move to the common layer @raymondfeng is adding. See #2900.

[canmustu]

My solution is delete all properties of UserProfile interface and implement CustomUser to UserProfile. This is more flexible and easy to do.

Define this interface in your project :

export interface CustomUser {
    id?:string;
    username? : string;
    firstName? : string;
    lastName? : string;
    gender? : boolean;
    // etc....
}

And change the UserProfile like this :

export interface UserProfile implements CustomUser { }

[dougal83]

Nitpick: If we're following the Openid Standard claims would it be a good idea to rename 'id' to 'sub'? If anyone is confused, they can refer to the referenced documentation.

[JesusTheHun]

@dougal83 this will be confusing for most developers.

The public interface should indeed only have getters to access semantic values. A getId(): string method will retrieve a unique identifier for the user, whatever it is. The connection with the application will simply be the implementation of this method. You can stack the interfaces to enable some security features such as RBAC with an additional interface describing a getRoles(): string[]method.
That's my two cents

[dhmlau]

@jannyHou, based on what we've accomplished in the @loopback/security PR, could you please see what's left needs to be done and add the acceptance criteria accordingly? Thanks!

[jannyHou]

@dhmlau I am refactoring @loopback/authentication and @loopback/authorization to depend on @loopback/security, see PR #3590, will figure out what to do with UserProfile and update it here.

[jannyHou]

See PoC #3771 and follow-up stories created in

#3846
#3845