[IMPROVE] Refactor Bridges

[d-gubert]

Using a Proxy to check for permissions in the bridges ended up being too loosely coupled with the bridges themselves. For instance, to create a new bridge, one is required to:

• Create a new interface in the bridges folder,
• Add that bridge to the Bridge union type in the AppBridges file (so it is recognised to be a bridge),
• Then create a new permission checker for that bridge in the permissions folder, noting that both have different names;
• and those three components will be brought together dynamically by the AppPermissionManager, which wraps the bridge with a Proxy at runtime when the engine gets it via the getNewBridge() method.

Another example of this is that the Proxy used in the AppPermissionManager relies on the name of the bridge to be exactly the same both in the host implementation and the permission checker, which is very loosely coupled and error-prone.
Conversely, the AppPermissionManager is too tied up to the concept of bridges by providing a method to wrap only bridge objects.

Basically, there is no mechanism in the code linking these components, even though they are conceptually linked.

Solution Proposal

One possible way to tie up these knots is by refactoring the bridges to be classes instead of interfaces. This will allow us to require the host system to extend these classes when implementing the bridges, which will provide us with a more reliable way of asserting that the host's implementation is correct and will also help the host with this implementation. Another benefit will be the possibility of colocating the permission checking with the actual bridge definition, by employing the Template Method pattern.

For example, let's take the message bridge: instead of having an IMessageBridge interface, an AppMessageBridge permission checker and the AppPermissionManager tying them up with a Proxy, we could have:

abstract class AppMessageBridge extends BridgeBase {
    /**
     * Here we defined the template methods for each one of the current
     * ones defined by the bridge. These template methods are going to do
     * the actual permission checking just before running the methods implemented
     * by the host system.
     *
     * For the sake of brevity, I'll introduce the example of just one template
     * method below.
     */
    public doCreate(message: IMessage, appId: string): Promise<string> {
        if (!AppPermissionManager.hasPermission(appId, AppPermissions.message.write)) {
            throw new PermissionDeniedError({
                appId,
                missingPermissions: [AppPermissions.message.write],
            });
        }


        return this.create(message, appId);
    }
    /**
     * These are the same methods defined in the IMessageBridge interface today
     * and these are the ones that should be implemented by the host system
     */
    abstract protected create(message: IMessage, appId: string): Promise<string>;
    abstract protected getById(messageId: string, appId: string): Promise<IMessage>;
    abstract protected update(message: IMessage, appId: string): Promise<void>;
    abstract protected notifyUser(user: IUser, message: IMessage, appId: string): Promise<void>;
    abstract protected notifyRoom(room: IRoom, message: IMessage, appId: string): Promise<void>;
    abstract protected typing(options: ITypingDescriptor, appId: string): Promise<void>;

[codecov]

Codecov Report

Merging #399 (3afeae2) into alpha (7ad5ea9) will decrease coverage by 2.52%.
The diff coverage is n/a.

❗ Current head 3afeae2 differs from pull request most recent head 696279b. Consider uploading reports for the commit 696279b to get more accurate results

@@            Coverage Diff             @@
##            alpha     #399      +/-   ##
==========================================
- Coverage   51.68%   49.16%   -2.53%     
==========================================
  Files          74      109      +35     
  Lines        2790     3513     +723     
  Branches      421      532     +111     
==========================================
+ Hits         1442     1727     +285     
- Misses       1348     1786     +438     

Impacted Files	Coverage Δ
src/server/accessors/Notifier.ts	56.00% <0.00%> (-20.48%)	⬇️
src/server/misc/Utilities.ts	34.88% <0.00%> (-13.96%)	⬇️
src/server/accessors/LivechatUpdater.ts	22.22% <0.00%> (-11.12%)	⬇️
src/server/accessors/Modify.ts	80.00% <0.00%> (-10.91%)	⬇️
src/server/accessors/Reader.ts	92.85% <0.00%> (-7.15%)	⬇️
src/server/compiler/AppFabricationFulfillment.ts	54.83% <0.00%> (-6.28%)	⬇️
src/server/accessors/Http.ts	94.44% <0.00%> (-5.56%)	⬇️
src/server/managers/AppListenerManager.ts	6.52% <0.00%> (-0.92%)	⬇️
src/server/accessors/LivechatRead.ts	8.69% <0.00%> (-0.83%)	⬇️
src/server/AppManager.ts	13.65% <0.00%> (-0.74%)	⬇️
... and 47 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 063b0d7...696279b. Read the comment docs.

[d-gubert]

• Do we still need the bridge interfaces? For instance IUserBridge or ILivechatBridge. Considering now we have other definitions of the bridges in the form of abstract classes, it looks to me that we can safely remove their interface counterparts.

• Some bridges don't require permissions to execute their actions (AppActivationBridge for instance). Do they still need the doStuff() and stuff() method definitions?

• With the new changes, no error is thrown when an app tries to do something it doesn't have permission to. This can make troubleshooting much harder.

@thassiov what do you think?

[felipetomm]

@d-gubert , that's an necessary console.log or we can remove them in a future PR?

[d-gubert]

Oops. Yeah, I guess we can remove it in the next release. Thanks for the heads up :)