Makes configuration uploadable thru API #240

jonathanbataire · 2025-01-14T07:10:45Z

Description

Current app requires rebuilding and re-deploying a app in order to change config
This approach introduces uploadable config making it easier to change configuration
The app defaults to the prebuilt config, once a new configuration is uploaded it overrides any pre-built config
Configuration can only be updated by CHT admins
Fixes #194

changes

introduces new api end point /api/config used with basic auth to upload config

curl -X POST https://username:password@user-management.com/api/config?domain=cht-instance.com \
     -H "Content-Type: application/json" \
     -d @config.json

jonathanbataire · 2025-01-16T07:30:30Z

src/config/index.ts

    const parentMatch = contactType.hierarchy.find(c => c.level === 1);
    if (!parentMatch) {
-      throw new Error(`hierarchy at level 1 is required: "${contactType.name}"`);
+      throw new Error(`hierarchy at level 1 is required: "${contactType?.name}"`);


jonathanbataire · 2025-01-16T07:30:46Z

src/config/index.ts

-    const contactMatch = config.contact_types.find(c => c.name === name);
+  public static async getContactType(name: string) : Promise<ContactType> {
+    const {config} = await init();
+    const contactMatch = config.contact_types.find(c => c?.name === name);


freddieptf · 2025-02-24T15:26:34Z

src/config/config-factory.ts

 };

-export default function getConfigByKey(key: string = 'CHIS-KE'): PartnerConfig {
+export default async function getConfigByKey(key: string = 'CHIS-KE'): Promise<PartnerConfig> {


In what scenario would the config be invalid after upload? Isn't validity checked at upload time?

freddieptf · 2025-02-24T15:30:00Z

src/config/index.ts


-const partnerConfig = getConfigByKey(CONFIG_NAME);
-const { config } = partnerConfig;
+async function init(): Promise<PartnerConfig> {


Why not use getConfigByKey directly?

freddieptf · 2025-02-24T15:31:15Z

src/lib/cht-session.ts

Is this change necessary?

yes, isAdmin is more of a chtSession property than an action on chtSession

freddieptf · 2025-02-24T15:37:16Z

src/server.ts


    try {
      const chtSession = Auth.createCookieSession(cookieToken);
+      if (req.routeOptions.url === '/api/config' && !chtSession?.isAdmin) {


I think you should add this check to the route's request handler itself

I think authorization should be handled in the auth hook/middleware
but i have no strong opinion about this though i can move it
LMK

src/server.ts

freddieptf · 2025-03-11T13:56:16Z

src/config/config-factory.ts

+
+export async function readConfig(): Promise<PartnerConfig> {
+  try {
+    const fileContent = await fs.promises.readFile(uploadedConfigFilePath, 'utf-8');


I see alot of calls had to be converted to async so i have to ask, Is there a synchronous equivalent to this?

yeah sure there is but its asynchronous mainly for performance.
since this is an api/server you'd want to support concurrency with multiple requests without blocking
also config is read almost every time you make a request to the server for a page

If It's for performance, wouldn't caching the config be more performant than reading from disk for every request?

maybe maybe not
Its just a single small file we just read from
but generally i wouldn't advice reading file synchronously in a server cos of its blocking nature
It more like reading db data synchronously, not advisable

…management into configurable-app

freddieptf · 2025-03-12T21:56:23Z

src/config/chis-ke/gross.ts

  };

-  const contactType = Config.getContactType(payload.contact_type);
+  const contactType = await Config.getContactType(payload.contact_type);


Doesn't look like this is still an async function

freddieptf · 2025-03-12T22:17:41Z

src/server.ts


+    const authHeader = req.headers.authorization as string;
+    if (authHeader && authHeader.startsWith('Basic ')) {
+      if (!req.routeOptions.url?.startsWith('/api')) {


When i POST the config without credentials it will try to redirect me to the /login endpoint which i think is awkward for an API. Is there any way this can be improved?

freddieptf · 2025-03-12T22:22:13Z

src/config/index.ts

+
+    //if config has default config i.e a new install
+    //return passed domain for api config upload authentication
+    if (availableDomains.some(c => c.domain === 'localhost')) {


I don't understand the comment here, why are you explicitly checking for localhost?

freddieptf · 2025-03-12T22:23:29Z

src/config/index.ts

  }

-  public static getLogoBase64() : string {
+  public static async getLogoBase64(): Promise<string> {


Doesn't need to be async

freddieptf · 2025-03-12T22:26:28Z

src/routes/add-place.ts

  fastify.post('/place/dob', async (req, resp) => {
    const { place_type, prefix, prop_type } = req.query as any;
-    const contactType = Config.getContactType(place_type).contact_properties.find(prop => prop.type === prop_type);
+    const config = await Config.getContactType(place_type);


Could be reverted, doesn't look relevant to the changes in the PR

freddieptf · 2025-03-12T22:31:48Z

src/routes/api.ts

+
+  });
+
+  fastify.post('/api/config', async (req: FastifyRequest, reply: FastifyReply) => {


I was able to post a config without any domains, which crashed the app after a logout. I was unable to post a new config after the crash happened

gj finding this case but there will be others. i'd bet on it.
this points to a deeper problem - how do you recover in production if an invalid config crashes the app? just another reason this pattern is concerning

freddieptf · 2025-03-12T22:38:31Z

src/config/config-factory.ts

+    if (!key) {
+      console.log('no configuration found. Setting to default');
+      const defaultConfig = kenyaConfig;
+      defaultConfig.config.domains = [{


Wouldn't this override the domains in this config? Would it be better to just crash the app if the key isn't set in the environment variables since that would be a deployment config issue?

The crash scenario is just below this code block which i think is a better way of handling this

i have added default config in case env variable is not provided this can then be overridden via api

environment variables should always be provided and checked at start up. We also shouldn't be changing the config values in code

thats not what im doing
i simply making the env variable optional
if not provided the app falls back to a default config which allows overwriting via api with any auth domain
other configs ie env and uploaded, will validate authenticate domain before overwriting
chck it out

freddieptf · 2025-03-12T22:47:47Z

src/config/config-factory.ts

+    try {
+      const jsonString: string = JSON.stringify(jsonConfigData, null, 2);
+      fs.writeFileSync(this.filePath, jsonString);
+      this.refreshConfig();


since refreshConfig is only used here, maybe you can just directly set the config here after writing it. Saves you a trip to the filesystem.

nitpick: the parameter name jsonConfigData is kinda confusing, maybe just config is fine?

lets leave it there keeps it organized the writeconfig will be doing more than it describes
besides its a great action to have on the class

freddieptf · 2025-03-12T22:50:23Z

src/routes/app.ts

 export default async function sessionCache(fastify: FastifyInstance) {
  fastify.get('/', async (req, resp) => {
-    const contactTypes = Config.contactTypes();
+    const contactTypes = await Config.contactTypes();


No longer async

freddieptf

Good progress, there are some changes around having a "default config" that i feel should not be a part of this PR and require further discourse. Please create another issue for this and lets have this PR only address adding the API

freddieptf · 2025-03-14T11:10:57Z

src/config/default-config/config.json

I am against having a default config simply because you'd be making some assumptions of one's setup. The dev should have their environment set up correctly and provide a valid config for their setup before starting the service and if none is provided the app shouldn't start

its not meant to be used it wouldnt work its just a place holder to allow uploading config on setting up the tool without breaking the tool
its there to allow some upload config
not having it leaves a gap on how config upload workflow would go

if you leave ke(or other) config as default i cant update config in my set up if i dont have admin access to ke instance
also remember config is not set only in env variable

not having it leaves a gap on how config upload workflow would go

Isn't the workflow simply overwriting the app's current config?

if you leave ke(or other) config as default i cant update config in my set up if i dont have admin access to ke instance
also remember config is not set only in env variable

The config to load is already set via the CONFIG_NAME env variable. If no part of this api touches the env variables then how would that value change? For your case wouldn't It always be chis-ug?

built in config file is never overwritten
a new file is created for the uploaded config
NOTE as of last week chis-ug is no longer a built in config it was removed by some PR

How would the UG app be deployed with these changes?

no env variable
deploy app
upload config from project repo

freddieptf · 2025-03-14T12:08:52Z

src/config/index.ts

  // TODO: Joi? Chai?
-  public static assertValid({ config }: PartnerConfig = partnerConfig) {
-    for (const contactType of config.contact_types) {
+  public static assertValid(config?: PartnerConfig) {


If the config is undefined isn't it invalid by default?

not sure i understand this fully

You made the parameter config optional. It shouldn't be

It was optional before

It had a default value, that doesn't mean it was optional. I don't think you should be able to pass an undefined value to this function

thats exaclty what it means :)
check this out

cht-user-management/src/index.ts

Line 10 in 6547f53

Config.assertValid();

It had a default value. The default value was set outside the function, you are setting the default value inside the function and changing the function signature to allow for a null value. That's my issue here. The function is meant to validate a config that's passed to it via the parameters.

that function was being called with a undefined parameter im not changing the signiture
have you looked at the link?
it literally accepts undefined param

also the default param value was being set in the function not outside

let me know if you want to hope on a call tomorrow so we can finalize this

freddieptf · 2025-03-14T12:11:31Z

src/routes/api.ts

+  fastify.post('/api/config', async (req: FastifyRequest, reply: FastifyReply) => {
+    try {
+      const config = await req.body as ConfigSystem;
+      await Config.assertValid({ config });


assertValid is not async

freddieptf · 2025-03-14T12:13:29Z

src/config/index.ts

-  public static assertValid({ config }: PartnerConfig = partnerConfig) {
-    for (const contactType of config.contact_types) {
+  public static assertValid(config?: PartnerConfig) {
+    const { config: assertionConfig } = config || ConfigFactory.getConfigFactory().loadConfig(CONFIG_NAME);


We should be validating the passed config, not trying to load it in the function.

i did this cause the way it was built initially
in some cases assertValid is called without any config passed
so to minimized changes i had to cater for these scenarios
it not doing anything different from what was there only how its implemented

some in some cases assertValid is called with any config passed

and that's exactly how it's supposed to work, to validate the config passed to the function. Please revert.

i dont understand before this PR this function is called without passing config into it
its what im trying to maintain here

It had a default value that was the config loaded at startup. The equivalent behavior now would be calling loadConfig and passing that value to this function

similar to what is being discussed here #240 (comment)

freddieptf · 2025-03-14T12:21:38Z

src/routes/api.ts

+      reply.send({ message: 'configuration updated' });
+      return;
+    } catch (error) {
+      console.error('Route api/config: ', error);


Validation errors should return status code 400

freddieptf · 2025-03-14T12:26:44Z

src/config/index.ts

+    const newConfigHasExistingDomain = _.some(availableDomains,
+      existingDomain => _.some(assertionConfig.domains, newDomain => _.isMatch(existingDomain, newDomain)));
+    if (!this.isDefaultConfig() && !newConfigHasExistingDomain) {
+      throw Error(`Invalid configuration: 'domains' property doesn't contains any existing domain`);


Any reason for not allowing the domain list to be fully updated?

if you have domains a,b, and c in the tool
when updated you dont want to overwrite them with x,y and z without atleast having any of a,b and c in the new config
avoid locking yourself out accidentally/intentionally

What if you only had credentials for domain d and it gets overwritten leaving domains a, b, x, y, z? Wouldn't you still be locked out?

That doesn't make the config invalid and i don't think it's something we should be checking for. Admins have access to the deployments (cht and this tool) and should be able to handle such cases so i don't see how that would be a problem

dont you think a person with credentials to a domain not part of the UMT domains shouldnt be able to overwrite domains
scenario: if i had admin access to ug instance should i be able to use the same admin access to upload config in a ke instance

This doesn't prevent that though. If that's what you're trying to prevent then shouldn't you be checking that the domain passed in the api call exists in the config's domain list?

let me explain better
if i had admin access to ug i can overwrite all domains and change them to all ke domains
this check tries to avoid this such that at least one of the new domains exited in the previous config
Overwriting all is NOT impossible this just makes it harder so its not done by accident
Overwriting all would be at least a 2 step operation

hence avoid admin locking them selves out by uploading all ke domains in a ug app and locking themselves out because they dont have ke credentials

if i had admin access to ug i can overwrite all domains and change them to all ke domains

That's up-to the user and it's out of our hands. The user is an admin, they have ways to fix that.

scenario: if i had admin access to ug instance should i be able to use the same admin access to upload config in a ke instance

This is the scenario you should be fixing as it's a valid bug here

honestly i dont see any problem with that check its just a precaution
that scenario is handled

freddieptf · 2025-03-14T13:27:38Z

src/config/index.ts

+      throw Error(`invalid configuration: 'contact_types' property is empty`);
+    }
+
+    for (const contactType of assertionConfig.contact_types) {


We need to update this and make validation very thorough making sure all required fields on the ContactType object have values. Use the object definition as a guide on what is strictly required. For eg. I can upload a ContactType without the name field even though it's required

mainly focused on things that would crash the app

Any required fields missing from ContactType will crash the app at some point. The app functionality i.e all generated forms, heavily depends on ContactType.

i have tested this the app will only misbehave but wont crush unless there is a particular scenario im missing
if the app is misbehaving config can be update ideally it should be tested before deploying
at least for now
lets handle these other cases in a new pr trying to get this in asap its blocking work

The app won't work correctly if a ContactType doesn't have a valid name. The data clean-up you'd set yourself up for just from this one field missing is worth having the checks in place now. This is just one example.

i dont think cht would accept uploading data if the name is invalid/wrong
and for this is unlikely to happen given config has to go thru testing by the dev before deployment
again this wont break the app lets handle all these scenarios in another pr

freddieptf · 2025-03-14T13:31:11Z

src/server.ts

  checkRedisConnection();

  fastify.addHook('preValidation', async (req: FastifyRequest, reply: FastifyReply) => {
+    if (req.routeOptions.url?.startsWith('/api') && req.method === 'POST') {


Shouldn't this be for all request methods?

this is checking for credentials not all requests have credentials/authheader
only api call come with authheader

Shouldn't the check you implemented apply for all api calls? Why limit it to POST calls only?

so you can call this endpoint in a browser to view config

if this is specific to a single endpoing, why isn't it in the route file. why does it have to awkwardly be in this hook?

kennsippell · 2025-03-27T15:28:48Z

src/config/config-factory.ts

+    'CHIS-TG': togoConfig,
+    'CHIS-CIV': civConfig,
+    'CHIS-ML': maliConfig,
+    DEFAULT: defaultConfig


can you just add the config for uganda back into the repo? it should can a flag like can upload to true to ensure configs on other instances are not replaceable

kennsippell · 2025-03-27T15:32:21Z

src/config/config-factory.ts

+    this.filePath = path.join(this.getConfigUploadDirectory(), 'config.json');
+  }
+
+  static getConfigFactory(): ConfigFactory {


if this is a singleton, why not just have ConfigFactory be a static class?

kennsippell · 2025-03-27T15:34:02Z

src/config/config-factory.ts

+  };
+
+  constructor() {
+    this.filePath = path.join(this.getConfigUploadDirectory(), 'config.json');


filePath should be explicit about what file it is pointing at. maybe uploadedConfigPath

kennsippell · 2025-03-27T15:36:24Z

src/config/config-factory.ts

+
+    const usingKey = key.toUpperCase();
+    console.log(`Using configuration: ${key}`);
+    const result = this.DEFAULT_CONFIG_MAP[usingKey];


Suggested change

const result = this.DEFAULT_CONFIG_MAP[usingKey];

this.config = this.DEFAULT_CONFIG_MAP[usingKey];

This would be cleaner without the intermittent variable result imo

kennsippell · 2025-03-27T15:37:55Z

src/config/config-factory.ts

+
+  private getConfigUploadDirectory(): string {
+    const configDir = path.join(__dirname, '..', 'config_uploads');
+    if (!fs.existsSync(configDir)) {


better to have this existsSync check in writeConfig?

kennsippell · 2025-03-27T15:50:39Z

src/config/config-factory.ts

+  }
+
+  private getConfigUploadDirectory(): string {
+    const configDir = path.join(__dirname, '..', 'config_uploads');


what sort of scenarios have you tested to ensure proper persistence of this file in docker?

kennsippell · 2025-03-27T15:51:44Z

src/routes/api.ts

+    try {
+      const config = await req.body as ConfigSystem;
+      Config.assertValid({ config });
+      ConfigFactory.getConfigFactory().writeConfig(config);


seems writeConfig should call assertValid? that feels like the right flow to ensure invalid configs cannot be written

kennsippell · 2025-03-27T15:52:56Z

src/server.ts

  checkRedisConnection();

  fastify.addHook('preValidation', async (req: FastifyRequest, reply: FastifyReply) => {
+    if (req.routeOptions.url?.startsWith('/api') && req.method === 'POST') {


if this is specific to a single endpoing, why isn't it in the route file. why does it have to awkwardly be in this hook?

kennsippell · 2025-03-27T15:56:21Z

src/server.ts

+        return;
+      }
+
+      const { domain } = req.query as { [key: string]: string };


is basic auth really what we want to use? why not just have an authenticated API? imo we should require users of the API to use /login (one way to authenticate to UMT), get issued a cookie, and then include it in requests to /api. that seems straightforward and not terribly burdensome to users of the API (atm just jonathan)

kennsippell · 2025-03-27T15:57:09Z

test/config.spec.ts

 import { Config, PartnerConfig } from '../src/config';
-import { CONFIG_MAP } from '../src/config/config-factory';
+import DEFAULT_CONFIG_MAP from '../src/config/config-factory';
 import { mockSimpleContactType } from './mocks';


add tests for config-factory?

jonathanbataire added 2 commits January 13, 2025 17:45

v1

09971b7

working version

8c4f3c8

jonathanbataire requested a review from kennsippell January 14, 2025 07:10

jonathanbataire marked this pull request as draft January 14, 2025 07:16

fix tests

b4496f6

jonathanbataire marked this pull request as ready for review January 14, 2025 08:04

jonathanbataire added 3 commits January 14, 2025 11:05

lint fix

049792a

clean up html

7816ae2

clean up

25dea73

jonathanbataire commented Jan 16, 2025

View reviewed changes

jonathanbataire added 4 commits February 17, 2025 11:47

Merge branch 'main' into configurable-app

99bcddc

Merge branch 'main' into configurable-app

cd48fc9

/config api

abbd43f

clean up

03a913e

jonathanbataire changed the title ~~Makes configuration uploadable thru UI~~ Makes configuration uploadable thru API Feb 20, 2025

jonathanbataire added 2 commits February 20, 2025 18:12

clean up

cdfa31a

clean up - serverjs

1cb1943

jonathanbataire requested a review from freddieptf February 21, 2025 07:26

await get template

25359bc

freddieptf reviewed Feb 24, 2025

View reviewed changes

src/server.ts Show resolved Hide resolved

jonathanbataire added 2 commits March 3, 2025 15:10

feedback

8ec80d4

Merge branch 'main' into configurable-app

d335eb3

jonathanbataire requested a review from freddieptf March 7, 2025 06:48

freddieptf reviewed Mar 11, 2025

View reviewed changes

Merge branch 'main' into configurable-app

ec8d8f3

jonathanbataire requested a review from freddieptf March 12, 2025 06:29

jonathanbataire added 5 commits March 12, 2025 16:24

config factory class

777eb61

revert tests

afe40dc

Merge branch 'configurable-app' of https://github.com/medic/cht-user-…

47366f2

…management into configurable-app

clean up config index

ca25ce2

clean up

742492d

freddieptf requested changes Mar 12, 2025

View reviewed changes

feedback

967aa7e

jonathanbataire requested a review from freddieptf March 14, 2025 10:51

jonathanbataire added 2 commits March 14, 2025 13:59

lint errors

22c4658

Merge branch 'main' into configurable-app

176d775

freddieptf requested changes Mar 14, 2025

View reviewed changes

feedback 02

603fa62

jonathanbataire requested a review from freddieptf March 17, 2025 06:59

Merge branch 'main' into configurable-app

bd4ce57

kennsippell requested changes Mar 27, 2025

View reviewed changes

remove can_configure

214772b


		});

		fastify.post('/api/config', async (req: FastifyRequest, reply: FastifyReply) => {

	const result = this.DEFAULT_CONFIG_MAP[usingKey];
	this.config = this.DEFAULT_CONFIG_MAP[usingKey];

Makes configuration uploadable thru API #240

Are you sure you want to change the base?

Makes configuration uploadable thru API #240

Uh oh!

Conversation

jonathanbataire commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

changes

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!

jonathanbataire Mar 3, 2025 • 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!

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!

jonathanbataire Mar 12, 2025 • 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!

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!

freddieptf Mar 14, 2025 • 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!

jonathanbataire Mar 14, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

freddieptf left a comment

jonathanbataire commented Jan 14, 2025 •

edited

Loading

jonathanbataire Mar 3, 2025 •

edited

Loading

jonathanbataire Mar 12, 2025 •

edited

Loading

freddieptf Mar 14, 2025 •

edited

Loading

jonathanbataire Mar 14, 2025 •

edited

Loading

jonathanbataire Mar 17, 2025 •

edited

Loading

jonathanbataire Mar 17, 2025 •

edited

Loading