Support open deployments and related realtime events #175

dennari · 2017-04-04T11:57:08Z

If a raw deployment is considered 'open', it's served without authentication or authorization.

Adds a new endpoint at /events/deployment/{projectId}-{deploymentId} which returns COMMENT_ADDED and COMMENT_DELETED events related to the specified open deployment.

NOTE: change the base to master after #174 has been merged.

vsaarinen · 2017-04-04T15:41:30Z

If the user is not logged in, they don't have a teamId. Unless there's something that I'm missing, the path can't have teamId in it.

vsaarinen

A few comments.

vsaarinen · 2017-04-04T15:43:16Z

src/realtime/realtime-hapi-plugin.ts

+  private requestHandlerFactory(streamFactory: StreamFactory) {
+    return async (request: Hapi.Request, reply: Hapi.IReply) => {
+      try {
+        // const observable = await this.getObservable(teamId, since ? parseInt(since, 10) : undefined);


vsaarinen · 2017-04-04T15:43:50Z

src/realtime/realtime-hapi-plugin.ts

@@ -304,7 +324,7 @@ export class RealtimeHapiPlugin {
    try {
      const deployment = event.payload.deployment;
      const apiResponse = await this.jsonApiPlugin.getEntity(
-          'deployment', api => api.toApiDeployment(deployment.projectId, deployment));
+        'deployment', api => api.toApiDeployment(deployment.projectId, deployment));


The last ) should be on the following line.

vsaarinen · 2017-04-04T15:47:19Z

src/realtime/realtime-hapi-plugin.ts

+    super({
+        name: 'realtime-plugin',
+        version: '1.0.0',
+    });
    this.persistedEvents = this.eventBus.getStream()
      .filter(isPersistedEvent)
      .map(event => <PersistedEvent<any>> event)


Since isPersistedEvent is a type guard, isn't this map unnecessary?

It's purpose is only to make the compiler agree that this.persistedEvents has the desired narrowed type.

I tested it without the map and that seemed to be the case.

vsaarinen · 2017-04-04T15:51:02Z

src/realtime/realtime-hapi-plugin.ts

-        }
-        observable = Observable.concat(Observable.from(existing), observable);
+  private async getStream(request: Hapi.Request) {
+    const {teamId, projectId, deploymentId} = request.params;


Should have spaces inside curly brackets.

Are we missing some linter rule?

It's currently not a supported rule in tslint AFAIK.

palantir/tslint#1044

vsaarinen · 2017-04-04T15:54:11Z

src/realtime/realtime-hapi-plugin.ts

+      this.persistedEvents.filter(predicate),
+    );
+    if (since && !isNaN(since)) {
+      const existing = await this.eventBus.getEvents(_teamId, since);


Will this pass on missed events that are not related to this deployment if projectId and deploymentId exist?

A nice catch indeed 👍

dennari · 2017-04-05T01:20:26Z

Changed to not require the teamId. This PR now also includes the related changes for serving open raw deployments.

vsaarinen

Good idea splitting up the realtime stuff into a separate module.

I haven't finished going over all the code, but one bigger thing comes to mind: in the future, it's likely that we want to configure deployment "openness" on a per-project basis. Because of this, making the API ready for this type of check is likely the best route since it's logically clear and we can easily implement our current situation in the "is this project open?" check.

In the realtime API, you now do some checks related to an "open team ID", but I feel it would be better to have some sort of "is this project open?" check instead. That check can then be implemented as "is this project an open team's project?" in our current situation. That also allows us to have multiple "open" teams with minimal changes in the future, if needed.

vsaarinen · 2017-04-05T06:57:35Z

src/deployment/deployment-hapi-plugin.ts

    } else {
+      server.route(this.traceRoute());
+      server.route(this.rawDeploymentRoutes(false));
+      server.route(this.rawDeploymentRoutes(true));


Are both of these here on purpose?

Yes, the one with false is for the open deployments

vsaarinen · 2017-04-05T07:14:08Z

src/deployment/deployment-hapi-plugin.ts

+    };
+  }
+
+  private rawDeploymentRoutes(needsAuthorization: boolean): Hapi.IRouteConfiguration[] {


Much clearer now. 👍

vsaarinen · 2017-04-05T07:18:33Z

src/integration-test/system-integration-tests.ts

@@ -39,6 +39,7 @@ interface SSE {

 const flowToken = process.env.FLOWDOCK_FLOW_TOKEN;
 const projectFolder = process.env.SYSTEM_TEST_PROJECT ? process.env.SYSTEM_TEST_PROJECT : 'blank';
+const openProjectFolder = process.env.SYSTEM_TEST_PROJECT_OPEN ? process.env.SYSTEM_TEST_PROJECT_OPEN : 'blank-open';


This (and other adjacent rows) could be simplified to

const openProjectFolder = process.env.SYSTEM_TEST_PROJECT_OPEN || 'blank-open';

vsaarinen · 2017-04-05T07:56:11Z

src/realtime/realtime-module.ts

+    // listens to SSEEvents and shares them
+    this.sseStream = this.eventBus.getStream()
+      .filter(isPersistedEvent)
+      .map(event => event as PersistedEvent<any>)


As mentioned, it seems that removing the map still makes this.sseStream of the type Observable<PersistedEvent<any>> because of the type guard, making it unnecessary.

Right. I'd like to say the typings for filter have improved, making the map unnecessary.

vsaarinen · 2017-04-05T08:07:49Z

src/realtime/realtime-hapi-plugin.ts

+        cors: true,
+        validate: {
+          params: {
+            teamId: Joi.number().required(),


Should this be here?

vsaarinen · 2017-04-05T08:11:21Z

src/realtime/realtime-hapi-plugin.ts

-      this.logger.error(msg, { error, event });
-      throw Error(msg);
+      const { projectId, deploymentId } = request.params;
+      const teamId = await request.getOpenTeamId();


Doesn't this now assume that whenever we fetch events from this endpoint that they belong to the open team? This is not the case if I'm a logged in user from another team and I use a Deployment View as a landing page, at which point this endpoint will be used.

A higher-level comment about this handler in the main review comment.

vsaarinen · 2017-04-05T08:12:24Z

src/server/hapi.ts

@@ -20,6 +20,8 @@ declare module 'hapi' {
  interface RequestDecorators {
    userHasAccessToProject: (projectId: number) => Promise<boolean>;
    userHasAccessToTeam: (teamId: number) => Promise<boolean>;
+    isOpenDeployment: (projectId: number, deploymentId: number) => Promise<boolean>;
+    getOpenTeamId: () => Promise<number | undefined>;


What if we have multiple teams that want open deployments?

I'd maybe get rid of this function altogether. More info in the main review comment.

dennari · 2017-04-05T12:00:38Z

Good points. The initial implementation left much to be desired. I've now rehauled the way the 'open' check works and feel this is much closer to what it should've been.

vsaarinen · 2017-04-06T07:45:40Z

src/authentication/authentication-hapi-plugin.ts

    return async (
      payload: AccessToken,
      request: Hapi.Request,
      callback: (err: any, valid: boolean, credentials?: any) => void,
    ) => {
-      let isAuthorized = false;
+      let isAuthorized: boolean | undefined = false;


Since this can now have three values, I'd prefer e.g. an enum to make it explicit what each state means.

Refactor DeploymentHapiPlugin to be clearer

Extract a separate module from RealtimeHapiPlugin

Also make it more clear when the request has been authenticated, but not yet authorized

… requests

Also make better use of subclassing in the private version

Add typings and include authorizationStatus as an enum

vsaarinen

Very nice changes! Two very minor things.

vsaarinen · 2017-04-06T13:46:05Z

src/deployment/deployment-hapi-plugin.ts

      // Store the original hostname
      request.app.hostname = hostname;
      // Overwrite the hostname to use virtual host routing
+      // Each vhost (including the default one) has a separate routing table


Is this still relevant?

Yes, we still use two separate ones

vsaarinen · 2017-04-06T13:49:27Z

src/authentication/authentication-hapi-plugin.ts

+      return callback(
+        undefined,
+        authorizationStatus === AuthorizationStatus.AUTHORIZED ||
+        authorizationStatus === AuthorizationStatus.NOT_CHECKED,


Should be indented a bit more.

vsaarinen · 2017-04-06T13:53:31Z

🎉

vsaarinen suggested changes Apr 4, 2017

View reviewed changes

dennari force-pushed the deployment-subdomain branch from 63155a4 to 3543baa Compare April 4, 2017 21:38

dennari mentioned this pull request Apr 5, 2017

Redirect to the UI login page #172

Merged

dennari force-pushed the deployment-realtime branch from 5fad3be to e247cf8 Compare April 5, 2017 01:12

dennari changed the title ~~Add an endpoint for deployment specific comment events~~ Support open deployments and related realtime events Apr 5, 2017

dennari mentioned this pull request Apr 5, 2017

Allow having open-access deployments #176

Closed

vsaarinen suggested changes Apr 5, 2017

View reviewed changes

dennari mentioned this pull request Apr 5, 2017

Deployment subdomain #174

Merged

dennari force-pushed the deployment-realtime branch from 36649b6 to 4e7d6db Compare April 5, 2017 12:41

vsaarinen changed the base branch from deployment-subdomain to master April 5, 2017 13:00

vsaarinen reviewed Apr 6, 2017

View reviewed changes

dennari added 16 commits April 6, 2017 12:42

Add an endpoint for deployment specific events

922a0eb

Require authorization

afa8366

Fix tests and bind 'getStream' properly

511ec3d

Fix a crucial oversight in getStream

efa4093

Add OPEN_TEAM_NAME configuration option

36f5754

Add the methods for checking if the deployment is 'open'

60133e8

Allow open access to open deployments

61b2acc

Refactor DeploymentHapiPlugin to be clearer

Add integration tests for open deployments

0681a07

Remove empty line

30d6618

Add a request decorator for fetching the open team id

26cb958

Add support for comment events for open deployments

05d1b43

Extract a separate module from RealtimeHapiPlugin

Add getProjectTeam

6d6e10b

Also make it more clear when the request has been authenticated, but not yet authorized

Allow deploymentHandler to also serve events for regularly authorized…

18af506

… requests

Remove empty line

c13c353

Remove unnecessary map

4f14874

Cleanup

6f2739f

dennari added 9 commits April 6, 2017 12:42

Remove accidentally added submodule

c9dcbbe

Add another helper project for integration tests

38d443c

Fix route validation

e43192e

Make comparison case insensitive

97ff248

Improve wording

29b8678

Simplify by using the Hapi's authentication modes

48d5f32

Also make better use of subclassing in the private version

Don't follow redirects by default

d0b60ba

Ensure that that deployment is not openly accessible

50979b2

Remove blank lines

3aa3188

dennari force-pushed the deployment-realtime branch from 4e7d6db to 3aa3188 Compare April 6, 2017 09:42

dennari added 2 commits April 6, 2017 15:10

Loosen email validation

e0f01e7

Improve request credentials

008ecf6

Add typings and include authorizationStatus as an enum

vsaarinen reviewed Apr 6, 2017

View reviewed changes

vsaarinen approved these changes Apr 6, 2017

View reviewed changes

Fix indentation

e18557c

vsaarinen merged commit b098dc7 into master Apr 6, 2017

vsaarinen deleted the deployment-realtime branch April 6, 2017 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support open deployments and related realtime events #175

Support open deployments and related realtime events #175

dennari commented Apr 4, 2017 •

edited

Loading

vsaarinen commented Apr 4, 2017 •

edited

Loading

vsaarinen left a comment

vsaarinen Apr 4, 2017

vsaarinen Apr 4, 2017

vsaarinen Apr 4, 2017

dennari Apr 4, 2017

vsaarinen Apr 5, 2017

vsaarinen Apr 4, 2017

dennari Apr 4, 2017

vsaarinen Apr 5, 2017

vsaarinen Apr 5, 2017

vsaarinen Apr 4, 2017

dennari Apr 4, 2017

dennari commented Apr 5, 2017

vsaarinen left a comment •

edited

Loading

vsaarinen Apr 5, 2017

dennari Apr 5, 2017

vsaarinen Apr 5, 2017

vsaarinen Apr 5, 2017

dennari Apr 5, 2017

vsaarinen Apr 5, 2017

dennari Apr 5, 2017

vsaarinen Apr 5, 2017

vsaarinen Apr 5, 2017

vsaarinen Apr 5, 2017

vsaarinen Apr 5, 2017

vsaarinen Apr 5, 2017

dennari commented Apr 5, 2017

vsaarinen Apr 6, 2017

vsaarinen left a comment

vsaarinen Apr 6, 2017

dennari Apr 6, 2017

vsaarinen Apr 6, 2017

vsaarinen commented Apr 6, 2017

Support open deployments and related realtime events #175

Support open deployments and related realtime events #175

Conversation

dennari commented Apr 4, 2017 • edited Loading

vsaarinen commented Apr 4, 2017 • edited Loading

vsaarinen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennari commented Apr 5, 2017

vsaarinen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennari commented Apr 5, 2017

Choose a reason for hiding this comment

vsaarinen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsaarinen commented Apr 6, 2017

dennari commented Apr 4, 2017 •

edited

Loading

vsaarinen commented Apr 4, 2017 •

edited

Loading

vsaarinen left a comment •

edited

Loading