[NEW] Api provider by rodrigok · Pull Request #61 · RocketChat/Rocket.Chat.Apps-engine

rodrigok · 2018-09-03T18:41:21Z

Closes #12

Rocket.Chat bridge implementation RocketChat/Rocket.Chat#11938

TODO

List provided webhooks on app's details page to be easy to copy
Add property to define hook as private or public
Add property to define hook security level
Property to provide curl samples for each hook

IMPROVEMENTS

Implement security level TOKEN_AUTH
Implement security level SHARED_SECRET

Webhook admin UI

Implementation example

// index.ts
import {
    IConfigurationExtend,
    ILogger,
} from '@rocket.chat/apps-engine/definition/accessors';
import { ApiSecurity, ApiVisibility } from '@rocket.chat/apps-engine/definition/api/IApi';
import { App } from '@rocket.chat/apps-engine/definition/App';
import { IAppInfo } from '@rocket.chat/apps-engine/definition/metadata';

import { MessageEndpoint } from './endpoints';

export class JitsiApp extends App {
    constructor(info: IAppInfo, logger: ILogger) {
        super(info, logger);
    }

    protected async extendConfiguration(configuration: IConfigurationExtend): Promise<void> {
        await configuration.api.provideApi({
            visibility: ApiVisibility.PRIVATE,
            security: ApiSecurity.UNSECURE,
            endpoints: [
                new MessageEndpoint(),
            ],
        });
    }
}

// endpoints.ts
import { HttpStatusCode, IHttp, IModify, IPersistence, IRead } from '@rocket.chat/apps-engine/definition/accessors';
import { IApiRequest, IApiResponse } from '@rocket.chat/apps-engine/definition/api';
import { IApiEndpoint } from '@rocket.chat/apps-engine/definition/api/IApi';
import { example } from '@rocket.chat/apps-engine/definition/api/IApiExample';
import { IApiEndpointInfo } from '@rocket.chat/apps-engine/definition/api/IApiEndpointInfo';

export class MessageEndpoint implements IApiEndpoint {
    public path: string = 'message/:id';

    @example({
        params: {
            id: 'message_id',
        },
        query: {
            asd: '123',
        },
        headers: {
            'X-Auth': 'asd: 123',
        },
    })
    public async get(request: IApiRequest, endpoint: IApiEndpointInfo, read: IRead, modify: IModify, http: IHttp, persis: IPersistence): Promise<IApiResponse> {
        return {
            status: HttpStatusCode.OK,
            headers: {
                'X-Auth': 'asd: 123',
            },
            content: {
                test: request.query.asd,
            },
        };
    }

    @example({
        params: {
            id: 'message_id',
        },
        query: {
            asd: '123',
        },
        content: {
            content: 1,
        },
    })
    public async post(request: IApiRequest, endpoint: IApiEndpointInfo, read: IRead, modify: IModify, http: IHttp, persis: IPersistence): Promise<IApiResponse> {
        return {
            status: HttpStatusCode.OK,
            content: request.content,
        };
    }
}

codecov · 2018-09-03T18:43:53Z

Codecov Report

Merging #61 into master will increase coverage by 0.44%.
The diff coverage is 63.26%.

@@            Coverage Diff             @@
##           master      #61      +/-   ##
==========================================
+ Coverage   60.58%   61.03%   +0.44%     
==========================================
  Files          51       55       +4     
  Lines        1799     1935     +136     
  Branches      278      299      +21     
==========================================
+ Hits         1090     1181      +91     
- Misses        709      754      +45

Impacted Files	Coverage Δ
src/server/bridges/AppBridges.ts	`100% <ø> (ø)`	⬆️
src/server/compiler/AppCompiler.ts	`22.98% <0%> (-0.89%)`	⬇️
src/server/accessors/ConfigurationExtend.ts	`100% <100%> (ø)`	⬆️
src/server/managers/AppAccessorManager.ts	`100% <100%> (ø)`	⬆️
src/server/errors/PathAlreadyExistsError.ts	`100% <100%> (ø)`
src/server/AppManager.ts	`16.3% <33.33%> (+0.48%)`	⬆️
src/server/misc/Utilities.ts	`46.34% <35.71%> (-0.33%)`	⬇️
src/server/accessors/ApiExtend.ts	`50% <50%> (ø)`
src/server/managers/AppApi.ts	`69.38% <69.38%> (ø)`
src/server/managers/AppApiManager.ts	`80% <80%> (ø)`
... and 4 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 d3521ea...23d2768. Read the comment docs.

graywolf336

I'll review this in more detail later, but I honestly think if we switch the IWebhook to be a class it will be extremely helpful. Like you described to me, it will allow us to provide generic utility/helper functions inside the class that are common things done by developers.

rodrigok · 2018-09-08T16:51:47Z

@graywolf336 can you provide some examples?

piotrkochan · 2018-09-13T13:10:32Z

awesome!

@graywolf336 for me Interfaces are fine

graywolf336 · 2018-09-20T19:17:59Z

src/server/managers/AppWebhook.ts

+    'patch',
+];
+
+export class AppWebhook {


What's the purpose of having two AppWebhook items within the managers folder? I think should combine them into one and keep the name of it to be AppWebhookManager, unless there's a valid reason I am overlooking.

I'll take a look ASAP.

I'm planing to change the naming to API instead of Webhooks, what do you think?

@rodrigok @graywolf336 renamed from webhook to api.

rodrigok · 2018-09-20T20:19:47Z

@geekgonecrazy awesome, can you remove the docs changes? I'm planning to move docs to a dedicated branch to prevent this mess on PRs as Bradley suggested some time ago.

This reverts commit fe17e36.

geekgonecrazy · 2018-09-20T21:48:09Z

@rodrigok done :)

package.json

src/definition/api/IApi.ts

graywolf336 · 2018-09-21T21:23:31Z

src/definition/api/IApi.ts

+     * will be generated automatically.
+     * @param `X-Auth-Token`
+     */
+    // AUTH_TOKEN,


What's your plans for this? I see they're commented out, but might want to add some documentation in the code for App developers to let them know that more will be coming in the near future.

Well, the AUTH_TOKEN is already documented there, the idea is to have options to handle authentication automatically for the apps. But it's not clear the options for me now.

It's commented out, so it won't show up in the generated docs if I'm not mistaken..yeah I'm not sure the best route to go yet without putting forth much effort in thoughts.

I don't want to have it in docs since we don't have it implemented yet, it's basically a reminder that we will have more options there.

graywolf336 · 2018-09-21T21:38:29Z

src/definition/api/IApi.ts

+     * Path to complete the api URL. Example: https://{your-server-address}/api/apps/public/{your-app-id}/{path}
+     * or https://{your-server-address}/api/apps/private/{your-app-id}/{private-hash}/{path}
+     */
+    path: string;


What would you think about allowing param style route definitions? For example: /contacts/:contactId/numbers would result in the passed in contactId being a param. Maybe this can be a future plan for improvements

This already works 😄

@rodrigok sweet! Maybe you can add some documentation so other developers are aware of it and can make usage of it 😃

graywolf336 · 2018-09-21T21:45:52Z

src/definition/accessors/IApiExtend.ts

+export interface IApiExtend {
+    /**
+     * Adds an api which can be called by external services lateron.
+     * Should an api already exists an error will be thrown.


Hmm, to be honest I'm not a fan fo this. If we are allowing them to provide the api, then we need to allow more than one api to be passed in..which in a sense we are, via the apiEndpoints. However, I think this goes against the style of the provideSlashCommand and provideSetting, both of which allow more than one being provided. In my opinion, we need to stick with the same style to ensure a standard is kept and to reduce the amount of possible confusion to the App developers. Maybe we revert back to an interface that the App implements which gets called to return the apiEndpoint informations. Which would work well when installing an App, as the person installing the App will see that the App provides an apiEndpoint which external services will be able to call. And since an App can only implement it once, it would solve the issue along with it could be called after settings which the App developer could read a setting to allow the administrator to configure how it works.

Idk if I understood your point, but you are able to provide more than one API, mainly if you need different properties like visibility and security. Is that what you were meaning?

So, you currently can provide more than one? The code documentation states that you can not: "Should an api already exists an error will be thrown."

By the API we mean an endpoint, you can configure more than one API but containing different endpoints

So, you can have more than one API but different endpoints? If I call this method more than once, each time containing two different endpoints, will it error out or will it succeed?

Will succeed

src/definition/api/IEndpoint.ts

src/definition/api/IApi.ts

src/server/managers/AppApiManager.ts

graywolf336 · 2018-09-24T16:32:18Z

src/server/managers/AppApi.ts

+
+        switch (this.api.visibility) {
+            case ApiVisibility.PUBLIC:
+                this.basePath = `/api/apps/public/${app.getID()}`;


@rodrigok what would you think about us making the nameSlug of the Apps to be unique and use it instead of the App's ID? This would help with the transition over to #56 (package.json) to slowly get rid of the usage of the id. As a result, and where the idea came to me from, the "endpoints" and urls generated/provided will be easier to use (and shorter) instead of having a long id in the url.

I wan't to move the ID to be a unique ID based on name as NPM does, that way we keep using the ID here, just change how the ID would be generated.

Right, so if we go with the name slug or a special smaller ID such as author:app-name then we could generate shorter urls and that would be a unique ID since the author couldn't have two of the same app names but two people could have the same App name as their author name would be different.

Yes, but that will be in the future

cuonghuunguyen · 2020-08-26T06:17:37Z

HI @graywolf336 @rodrigok, does your team have a plan to implement the other security level for the API in the next releases?

rodrigok · 2020-08-26T11:52:48Z

@meomay503, unfortunately, no, it's not in our short roadmap.

[NEW] Webhook provider

e2b5130

rodrigok force-pushed the webhooks branch from 839d7b2 to e2b5130 Compare September 3, 2018 18:54

rodrigok mentioned this pull request Sep 3, 2018

[NEW] Apps: API provider RocketChat/Rocket.Chat#11938

Merged

rodrigok changed the title ~~[WIP ][NEW] Webhook provider~~ [WIP] [NEW] Webhook provider Sep 3, 2018

rodrigok added 3 commits September 3, 2018 16:55

More tests for webhooks

74ec5d3

Implement webhook security and visibility

b2a4bda

Handle webhook errors

36cb048

rodrigok force-pushed the webhooks branch from 2f55573 to 36cb048 Compare September 7, 2018 15:41

rodrigok added 2 commits September 7, 2018 19:15

Provide webhook examples api as a decorator

bfc8dc6

Add webhook api to list webhooks

ab3761b

rodrigok changed the title ~~[WIP] [NEW] Webhook provider~~ [NEW] Webhook provider Sep 7, 2018

rodrigok requested review from Hudell, geekgonecrazy, graywolf336 and sampaiodiego September 7, 2018 23:05

graywolf336 suggested changes Sep 8, 2018

View reviewed changes

graywolf336 reviewed Sep 20, 2018

View reviewed changes

rodrigok changed the title ~~[NEW] Webhook provider~~ [WIP] [NEW] Webhook provider Sep 20, 2018

geekgonecrazy added 2 commits September 20, 2018 15:05

rename from webhooks to api

c5e9097

update docs

fe17e36

Revert "update docs"

061ffce

This reverts commit fe17e36.

geekgonecrazy changed the title ~~[WIP] [NEW] Webhook provider~~ [NEW] Api provider Sep 20, 2018

rodrigok added 3 commits September 21, 2018 11:56

Allow access to node modules {path, url, crypto, buffer}

848d75d

Pass endpoint information to api execution

15f2e53

Provide a new api format

221a1b4

graywolf336 reviewed Sep 21, 2018

View reviewed changes

package.json Outdated Show resolved Hide resolved

Remove unnecessary dependency @types/node

e6e71ef

graywolf336 reviewed Sep 21, 2018

View reviewed changes

src/definition/api/IApi.ts Outdated Show resolved Hide resolved

graywolf336 reviewed Sep 21, 2018

View reviewed changes

src/definition/api/IApi.ts Outdated Show resolved Hide resolved

graywolf336 reviewed Sep 21, 2018

View reviewed changes

graywolf336 suggested changes Sep 21, 2018

View reviewed changes

rodrigok added 3 commits September 21, 2018 23:09

Fix reviews

15134ad

Implements abstract class ApiEndpoint

a01d79e

Fix enpoints to start with /api

23d2768

graywolf336 reviewed Sep 24, 2018

View reviewed changes

graywolf336 approved these changes Sep 24, 2018

View reviewed changes

rodrigok added this to the 1.1.0 milestone Sep 25, 2018

rodrigok merged commit d92a98d into master Sep 25, 2018

rodrigok deleted the webhooks branch September 25, 2018 19:11

Comments

Conversation

rodrigok commented Sep 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

graywolf336 left a comment

Choose a reason for hiding this comment

Uh oh!

rodrigok commented Sep 8, 2018

Uh oh!

piotrkochan commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graywolf336 Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodrigok commented Sep 20, 2018

Uh oh!

geekgonecrazy commented Sep 20, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cuonghuunguyen commented Aug 26, 2020

Uh oh!

rodrigok commented Aug 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

rodrigok commented Sep 3, 2018 •

edited

Loading

codecov bot commented Sep 3, 2018 •

edited

Loading

piotrkochan commented Sep 13, 2018 •

edited

Loading

graywolf336 Sep 20, 2018 •

edited

Loading