Improve Transport-Auth relationship

[Aetherall]

trafficstars

In order to achieve this goal, I will have no other choice than make a big PR over those packages :

@accounts/server @accounts/rest-express @accounts/password @accounts/oauth

I would be pleased to have your opinions on this !

AccountsServer

• [ ] Change the way authenticationServices are registered

// Link Authentication Services to the AccountsServer
// [{name:'a'},{name:'b'}] => { a:{name:'a'}, b:{name:'b'} }
this.authenticationServices = config.authenticationServices.reduce(
	( acc: AuthenticationServices, authenticationService: AuthenticationService ) =>
	({ ...acc, [authenticationService.name]: authenticationService.link(this) })
,{})

• [ ] Add a method for accessing the authenticationService

// => Access a specific AuthenticationService to execute a specific method
public useAuthenticationService(target: any, params: any, connectionInfo: ConnectionInformations): any {
	const { service, ...serviceParams } = target;
	const authenticationService: AuthenticationService = this.authenticationServices[ service ];
	if(!authenticationService){
		throw new AccountsError('Server', 'useAuthenticationService', `Service ${service} not found`);
	}
	return authenticationService.useService(serviceParams, params, connectionInfo);
}

I intend to use the same pattern for the notification Services so from the accounts package we can do

// Access the Email NS then use the verification template from the password email plugin
await this.accountsServer.useNotificationService('email').notify('password', 'verification', { email, user: dbUser, token })

TransportExpress

• [ ] Refactor the package into a class
• [ ] Get rid of the conditional routes in favor of a dynamic route

this.router.use(`/${this.path}/:service/:provider?/:action`, this.useAuthenticationService)

private useService = async ( req: Request, res: Response, next: NextFunction) : Promise <void> => {
	// Get the connection Informations
	const connectionInfo: ConnectionInformations = getConnectionInfo(req);
	// Identify the target function to hit
	const target: any = req.params;
	// Extract the action parameters
	const params: any = req.body;
	try {
		const { tokens, ...response } : any = await this.accountsServer.useService(target, params, connectionInfo)
		if(tokens) this.tokenTransport.setTokens(tokens, { req, res });
		this.send(res, response);
	} catch(err) {
		this.sendError(res, err)
	}
}

PasswordService & AuthService

• [ ] Add a useService function in order to use the methods of the package

public useService( target: any, params: any, connectionInfo: ConnectionInformations ): Promise<object> | AccountsError {
	const actionName: string = target.action;
	const action = this[actionName];
	if(!action) {
		throw new AccountsError(`[ Accounts - Password ] useService : No action matches ${actionName} `)
	}
	return action( params, connectionInfo )
}

• [ ] Add a firewall so from the transport we can't access other methods than the ones we should

// Maybe something like V on the exemple above
const action = this[this.firewall[actionName]];

• [ ] Add link function so when the accountsServer instanciate an AuthService, it became aware of the database and the future notification providers

public link( accountsServer: AccountsServer ) : this {
	this.accountsServer = accountsServer;
	// I tend to want to remove those and access this.accountsServer.databaseInterface each time so it's easier to see where those come from
	this.databaseInterface = this.accountsServer.databaseInterface;
	this.tokenManager = this.accountsServer.tokenManager;
	return this;
}

[pradel]

AccountsServer

• Change the way authenticationServices are registered Why not keep the way that actual services are instantiated (object) rather than an array, will be the same no?
• Add a method for accessing the authenticationService 👍 This will be used only by the transport package right?

TransportExpress

• Refactor the package into a class 👍
• Get rid of the conditional routes in favor of a dynamic route This will reduce the code and will be indeed better just need to be really careful about the methods we expose.

PasswordService & AuthService

• Add a useService function in order to use the methods of the package This will be used only by the transport package right?
• Add a firewall so from the transport we can't access other methods than the ones we should Can you add an example of how do you think this will be stored?
• Add link function so when the accountsServer instanciate an AuthService, it became aware of the database and the future notification providers // I tend to want to remove those and access this.accountsServer.databaseInterface each time so it's easier to see where those come from I indeed think this would be better, we will be able to use the hook service in all other packages also. I just don't like the databaseInterface name, maybe more db or database.

Looks like changes make sense, this will allow to add services without having to modify the other packages. We just need to be really careful about the methods we expose.

[Aetherall]

AccountsServer

Why not keep the way that actual services are instantiated (object) rather than an array, will be the same no?

It will be exactly the same, this is because we are giving the ability to the user to set the serviceName, and this behaviour is not wanted

new AccountsServer({
    services: {
        oauth: new PasswordService(),
        passwod: new PasswordService()
    }
})

Also, IMO this is more easy to understand for new users

new AccountsServer({
    services: [ new PasswordService(), new OAuthService()]
})

TransportExpress

Add a method for accessing the authenticationService This will be used only by the transport package right?

Yes

Get rid of the conditional routes in favor of a dynamic route This will reduce the code and will be indeed better just need to be really careful about the methods we expose

This is for sure ! I recommend you to check the #183 issue where I am proposing to go a step further ;)

PasswordService & AuthService

Add a useService function in order to use the methods of the package This will be used only by the transport package right?

Yes but more precisely, this will be used by the useAuthenticationService of AccountsServer

// from within the ExpressTransport
this.accountsServer.useAuthenticationService(target)

// The accountsServer will redirect to the right AuthenticationService.useService

Add a firewall so from the transport we can't access other methods than the ones we should Can you add an example of how do you think this will be stored?

class PasswordService implements AuthenticationService {
	static firewall = {
		authenticate: 'authenticate',
		verifyEmail: 'verifyEmail'
	}

	public useService( target, params, connectionInfo ): Promise<object> {
			const actionName: string = target.action;
			const action = this[this.firewall[actionName]];
			if(!action) {
				throw new AccountsError(`[ Accounts - Password ] useService : No action matches ${actionName} `)
			}
			return action( params, connectionInfo )
		}
}

I indeed think this would be better, we will be able to use the hook service in all other packages also

This would be great for the developer experience !

I just don't like the databaseInterface name, maybe more db or database.

I tend to prefer using long explicit names, but database is cool to me too :+1:

---

Looks like changes make sense, this will allow to add services without having to modify the other packages. We just need to be really careful about the methods we expose.

Yes this is the goal, adding new packages should be as simple as writing a class :) Keeping all of this secure will require some sort of firewall or tricks