Define `createBrandedValue` method #11

Pl217 · 2021-08-13T11:16:50Z

~~Move (and adapt) authentication and logging related code to the core repository, where both new (https://github.com/UN-OCHA/hpc-api) and old (https://github.com/UN-OCHA/hpc_service) APIs can use it.~~

This PR is repurposed to do some basic codebase maintenance and define createBrandedValue method needed for https://github.com/UN-OCHA/hpc_service/pull/2356

Because it can lead to inconsistencies when two package managers are used. From `yarn` log: "package-lock.json found. Your project contains lock files generated by tools other than Yarn. It is advised not to mix package managers in order to avoid resolution inconsistencies caused by unsynchronized lock files. To clear this warning, remove package-lock.json"

s0 · 2021-08-16T10:50:54Z

@Pl217 Have you got an associated PR for hpc_service in the works? Rather than reviewing this in isolation, i'd like to test it with the old API to make sure that everything still works

Pl217 · 2021-08-16T11:28:38Z

@Pl217 Have you got an associated PR for hpc_service in the works? Rather than reviewing this in isolation, i'd like to test it with the old API to make sure that everything still works

I'm still working on that one. Since it requires much more changes and diligence, I pushed this PR and hpc-api related PR, which were already done - UN-OCHA/hpc-api#21

s0

There seem to be quite a few things that you've moved over here that I would have preferred for us to keep out of this codebase completely, in particular:

legacy auth related stuff
restify/hapi-specific implementation details / request objects, where i think we can do a much better job of abstracting out the concrete interfaces that the library actually needs to use.

Let's have a call to discuss what I have in mind.

s0 · 2021-09-01T15:57:08Z

src/auth/role.data.ts

+ *
+ * @deprecated
+ */
+export const ROLES = [


hmm.

While I appreciate why you moved this module over, it' very much an artifact of the previous authentication system, and we probably don't want to move it over at all. (I probably named it in a confusing way, apologies about that). The way it handles the concept of roles is very different from roles.ts where the roles are expressed as code rather than data in the database. This file is effectively a hard-coded snapshot of the expected roles from the old auth system from the database.

s0 · 2021-09-01T15:59:06Z

src/db/index.ts

+  participantRole: participantRole(conn),
+  role: role(conn),
+  rolePermittedAction: rolePermittedAction(conn),


we don't want to port these models as they are from the legacy authentication system that we're phasing out

s0 · 2021-09-01T16:03:10Z

src/Request.ts

+import * as hapi from '@hapi/hapi';
+import * as restify from 'restify';
+
+interface ExpandedRequest {


Hmm this class also feels like something that's a little too specific to the way that we expect requests to be constructed for restify using our middlewear in hpc_service only, and not something that feels generally applicable for the legacy and new API.

The general library should not need to be aware of specific implementations / libraries that we're using to implement the rest API servers (and therefore should not need to have any restify / hapi types included at all). Instead, any interfaces should only really include stuff that's common across both usages.

s0 · 2021-09-01T16:06:08Z

src/Context.ts

+export default interface Context {
+  connection: Knex<any, unknown[]>;
+  config: Config;
+  request: Request;


i would not pass the raw request as part of the context. for the boundary of this library (i.e. the data that we'll be giving to this library in any functions that we call), we'd need the data to already be abstracted in a way that is general and useful for the library to use. the library shouldn't need to know that it's supporting a request for restify, hapi, whatever etc... all it should know is, this is how i log stuff, this is my database connection, etc...

s0 · 2021-09-01T16:07:01Z

src/auth/index.ts

+ * they can be added to this file and avoid the need for a migration or
+ * modifying roles in any way.
+ */
+type ExtendedPermittedAction = PermittedActionIdString | perms.GlobalPermission;


we don't want to include any of the legacy auth stuff in what we port over.

s0 · 2021-09-01T16:08:43Z

src/auth/index.ts

+  const addLegacyPermissions = async () => {
+    const globallyPermitted = await getLegacyGloballyPermittedActions(context);
+    if (globallyPermitted.size === 0) {
+      return;
+    }
+    allowed.global = allowed.global || new Set();
+    for (const permission of globallyPermitted) {
+      allowed.global.add(permission);
+    }
+  };


this we will want to keep in hpc_service only, to support the endpoints that haven't been updated to use the new auth system only.

s0 · 2021-09-01T16:11:18Z

src/db/util/index.ts

+ * Take a collection of objects,
+ * and create a map of them, using aparticular property as the key
+ */
+export const organizeObjectsByUniqueProperty = <I, P extends keyof I>(


this function can apply more generally than jsut db related actions, so should probably live in a module in src/util instead.

s0 · 2021-09-01T16:11:39Z

src/db/util/async.ts

+ * This function takes advantage of the NodeJS single-threaded concurrency model
+ * to group multiple requests together using `setImmediate`.
+ */
+export const createGroupableAsyncFunction = <


this function can apply more generally than jsut db related actions, so should probably live in a module in src/util/async instead.

s0 · 2021-09-01T16:15:04Z

src/logging/log.ts

+/**
+ * Use of `any` in this module is generally deliberate to help with generics
+ */
+import bunyan = require('bunyan');


It may be smarter to just expose an interface that can be served by bunyan, rather than adding bunyan as a direct dependency. i.e. port over LogContext etc... but the actual implementation for the logging can still be specific to the environment (i.e. hpc_service, hpc-api) etc... Which will give a bit more flexibility in terms of dependencies on these libraries, where hpc-api will likely be a bit more flexible.

s0 · 2021-09-01T16:16:44Z

src/util/error.ts

+  private readonly body: ErrorBody;
+  private readonly statusCode: StatusCode;
+
+  constructor(message: string, namespace?: string, originalError?: Error) {


let's completely get rid of the concept of namespace while we're at it. It causes our error + endpoint code to be so messy, and should be completely done for us automatically by the way of including stacktraces in our error logging.

Since `husky` got updated to v7 major version, `.husky/.gitignore` can be removed now.

Pl217 · 2021-09-06T13:28:07Z

Let's still keep the useful bits of this PR. There are some maintenance commits which I would like to keep and also define createBrandedValue which is needed for https://github.com/UN-OCHA/hpc_service/pull/2356.

Pl217 assigned s0 Aug 13, 2021

Pl217 requested a review from a team as a code owner August 13, 2021 11:16

Pl217 mentioned this pull request Aug 13, 2021

HPC-7904: 🛂 Setup authentication for resolvers UN-OCHA/hpc-api#21

Merged

Pl217 force-pushed the HPC-7904 branch 2 times, most recently from 50195d2 to 46988da Compare August 23, 2021 09:45

s0 suggested changes Sep 1, 2021

View reviewed changes

s0 added the needs major changes There are review or issue comments to address that will result in big changes label Sep 1, 2021

Pl217 added 5 commits September 6, 2021 15:08

Update dev dependencies

c2c079b

Since `husky` got updated to v7 major version, `.husky/.gitignore` can be removed now.

Fix typo in CONTRIBUTING.md

2a01b6d

Update Typescript version

ebab717

Define participant DB model

1dcbb24

Define createBrandedValue method

aa15deb

Pl217 force-pushed the HPC-7904 branch from 46988da to aa15deb Compare September 6, 2021 13:25

Fix linting errors coming from require-await rule

376be1f

Pl217 added ready for review All comments have been addressed, and the Pull Request is ready for review and removed needs major changes There are review or issue comments to address that will result in big changes labels Sep 6, 2021

Pl217 changed the title ~~Port authentication and logging related code~~ Define createBrandedValue method Sep 6, 2021

s0 approved these changes Sep 6, 2021

View reviewed changes

s0 merged commit 77e2a97 into develop Sep 6, 2021

s0 deleted the HPC-7904 branch September 6, 2021 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define `createBrandedValue` method #11

Define `createBrandedValue` method #11

Pl217 commented Aug 13, 2021 •

edited

Loading

s0 commented Aug 16, 2021 •

edited

Loading

Pl217 commented Aug 16, 2021

s0 left a comment

s0 Sep 1, 2021

s0 Sep 1, 2021

s0 Sep 1, 2021

s0 Sep 1, 2021

s0 Sep 1, 2021

s0 Sep 1, 2021

s0 Sep 1, 2021

s0 Sep 1, 2021

s0 Sep 1, 2021

s0 Sep 1, 2021

Pl217 commented Sep 6, 2021

Define createBrandedValue method #11

Define createBrandedValue method #11

Conversation

Pl217 commented Aug 13, 2021 • edited Loading

s0 commented Aug 16, 2021 • edited Loading

Pl217 commented Aug 16, 2021

s0 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

Pl217 commented Sep 6, 2021

Define `createBrandedValue` method #11

Define `createBrandedValue` method #11

Pl217 commented Aug 13, 2021 •

edited

Loading

s0 commented Aug 16, 2021 •

edited

Loading