MemberJunction / MemberJunction/MJ

Explorer: validateEnvironment() blocks catalog-only auth deployments — compiled environment.ts is treated as the only source

Open
#3,836 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TSQL
Stars
29
Forks
6
Avg merge
2d 1h
Merged PRs (30d)
323

Description

Follow-up to #2985 (metadata-driven pluggable authentication providers), agreed as a fast-follow rather than folded into that PR. Original analysis by @MattC-BC in https://github.com/MemberJunction/MJ/pull/2985#issuecomment-5297150046 — recorded here so it isn't lost when the PR merges.

## The gap

`validateEnvironment()` in `packages/Angular/Explorer/explorer-app/src/lib/explorer-app.component.ts` (line ~992) still treats the compiled `environment.ts` as the **only** source of authentication configuration. It hard-requires `AUTH_TYPE`, and when `AUTH_TYPE === 'msal'` it additionally requires compiled `CLIENT_ID` and `TENANT_ID` via `validateMsalFields()`.

It runs early in `ngOnInit` and short-circuits the whole app on failure:

```ts
const envValid = this.validateEnvironment();
if (!envValid) {
this.HasError = true;
this.ErrorMessage = 'Environment configuration is incomplete. See errors above.';
return; // setupAuth() never runs
}
```

So a deployment that configures authentication **purely from the new provider catalog** — blank compiled auth values, everything coming from `MJ: Authentication Providers` — is stopped at the "Environment configuration is incomplete" screen before catalog resolution ever gets a chance to matter.

Two details worth noting:

- **`TENANT_ID` has no runtime consumer.** It is read only by the validator itself (`validateMsalFields`) and declared in `bootstrap.types.ts`. The browser driver reads `CLIENT_AUTHORITY`, and #2985 added `MJMSALProvider.EnvironmentFromCatalog` which derives that authority from the row's `Issuer`. So the validator can block a deployment on a variable nothing actually uses.
- **The validator is checking the wrong layer.** It validates the compiled environment, but the value the drivers ultimately consume is the *merged* result of the compiled environment plus the catalog overlay (`mergeCatalogEnvironment`), which the auth module computes privately inside `AuthServicesModule.forRoot()`.

## Why this was scoped out of #2985

Every existing deployment has a populated `environment.ts`, so behavior is unchanged for all of them — catalog values still win where provided, and that is covered by tests. The blocked scenario is the catalog-only end state that nothing runs yet. This was a deliberate scoping decision, not an oversight.

## Proposed direction

**Preferred — merge the catalog into the environment once, at the composition root.** `packages/MJExplorer/src/main.ts` already awaits `AuthProviderCatalog.Preload()` before the app module is imported, so a single merged environment object could feed the validator, bootstrap, and drivers alike, instead of `AuthServicesModule.forRoot()` keeping a private merged copy. That removes the class of bug rather than one instance of it, and gives the validator something meaningful to validate.

**Lighter alternative — relax the validator when a catalog resolution is present.** `MJExplorerAppComponent` already injects `MJ_AUTH_PROVIDER_RESOLUTION` (`@Optional()`), so the component can see whether a provider resolved from the catalog and skip the compiled-value requirements accordingly. Cheaper, but leaves two sources of truth in place.

Either way, `TENANT_ID` should stop being a hard requirement — nothing reads it.

This touches the boot sequence, so it deserves its own review rather than riding along on #2985.

## Acceptance criteria

- [ ] An Explorer deployment with blank compiled auth values and an Active, client-visible catalog row reaches the login screen and can sign in.
- [ ] A deployment with a populated `environment.ts` and no catalog behaves exactly as it does today, including the existing validation error messages when values really are missing.
- [ ] A deployment with **neither** a catalog nor compiled auth config still fails loudly and legibly — this must not become a silent path to a broken login screen.
- [ ] `TENANT_ID` is no longer required by the validator (or the requirement is justified by an actual runtime consumer).
- [ ] Unit coverage for all three states above.

## References

- PR #2985 — metadata-driven pluggable authentication providers
- `packages/Angular/Explorer/explorer-app/src/lib/explorer-app.component.ts` — `validateEnvironment()`, `validateMsalFields()`
- `packages/Angular/Explorer/auth-services/src/lib/catalog-environment.ts` — `mergeCatalogEnvironment()`
- `packages/MJExplorer/src/main.ts` — the preload/composition root

Contributor guide

Open the contributing guide

Research direction

Start with validateEnvironment() and validateMsalFields() in packages/Angular/Explorer/explorer-app/src/lib/explorer-app.component.ts, then trace AuthProviderCatalog.Preload() in packages/MJExplorer/src/main.ts and mergeCatalogEnvironment() in packages/Angular/Explorer/auth-services/src/lib/catalog-environment.ts. Verify the three acceptance states with unit coverage: catalog-only authentication reaches login, existing compiled configuration is unchanged, and neither source still fails clearly without requiring TENANT_ID.

Written by the indexing model from the issue text.

Assessment

Tech stack
angular, typescript
Domain
authentication, frontend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.