Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add dex auth #2793

Merged
merged 10 commits into from
Feb 13, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -167,5 +167,9 @@ jobs:
KC_ADMIN_CLIENT_SECRET: so_secret_000
ETL_MIRROR_URL: https://geo-datasets-mirror.s3.fr-par.scw.cloud
ETL_ARCHIVES_URL: https://geo-datasets-archives.s3.fr-par.scw.cloud
PRO_CONNECT_CLIENT_ID: ${{ secrets.PRO_CONNECT_CLIENT_ID }}
PRO_CONNECT_CLIENT_SECRET: ${{ secrets.PRO_CONNECT_CLIENT_SECRET }}
APP_AUTH_CLIENT_ID: secret
APP_AUTH_CLIENT_SECRET: secret
run: just ci_test_integration
working-directory: api
19 changes: 19 additions & 0 deletions api/justfile
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,25 @@ api *command:
--unstable-fs \
src/main.ts {{ command }}

debug:
deno repl \
-A \
--unstable-fs \
--eval 'import { getKernel } from "./src/debug.ts"; const kernel = await getKernel();'

Comment on lines +163 to +168
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Verify the use of unstable-fs flag and add error handling.

The debug command uses the --unstable-fs flag which is experimental and may change in future Deno releases. Additionally, consider adding error handling for failed imports.


🌐 Web query:

What is the current status of Deno's unstable-fs flag and are there any stable alternatives?

💡 Result:

The --unstable-fs flag has been deprecated as of Deno 1.44 (May 2024) and is no longer required for core file system operations[1][4]. Key file system APIs previously behind this flag have been stabilized:

Stabilized replacements include:

  • Deno.FsFile.syncData()/syncDataSync() for explicit file data synchronization[1]
  • Deno.FsFile.sync()/syncSync() for metadata synchronization[1]
  • Deno.FsFile.lock()/lockSync() and unlock()/unlockSync() for file locking[1]

These changes eliminate previous compatibility issues with frameworks like Next.js while maintaining Deno's security model[1]. The stabilization means developers can now use these file system features without special flags:

// Example using stabilized APIs
const file = await Deno.open("data.txt", { read: true, write: true });
await file.lock(); // Now stable
await Deno.write(file.rid, new TextEncoder().encode("updated"));
await file.syncData(); // Now stable
file.close();

For advanced filesystem access patterns beyond core operations, Deno recommends:

  1. Web File System Access API: For browser-like file handling (specification)[3]
  2. Third-party libraries: Such as deno.land/x/fs for extended utilities
  3. Node.js compatibility layer: Via --compat flag for npm package interoperability[2]

While most filesystem features are now stable, some edge-case operations remain behind newer granular unstable flags like --unstable-write for experimental write optimizations[9]. The broader --unstable flag itself is being phased out in favor of these targeted feature flags[9].

Citations:


Action Required: Update debug command in api/justfile

  • Remove the deprecated --unstable-fs flag as it is no longer required (core file system operations are now stable since Deno 1.44).
  • Update the inline evaluation to include error handling—for example, wrap the dynamic import in a try-catch block to log or manage failures.

watch:
deno run \
--allow-net \
--allow-env \
--allow-read \
--allow-write \
--allow-ffi \
--allow-sys \
--allow-run \
--watch \
--unstable-fs \
src/main.ts http \$PORT

Comment on lines +169 to +181
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add PORT validation and review unstable-fs usage.

The watch command has a few potential issues:

  1. The $PORT environment variable is used without validation.
  2. The --unstable-fs flag is experimental.

Consider adding PORT validation before starting the server:

 watch:
+  #!/usr/bin/env bash
+  if [[ -z "${PORT}" ]]; then
+    echo "Error: PORT environment variable is not set"
+    exit 1
+  fi
   deno run \
   --allow-net \
   --allow-env \
   --allow-read \
   --allow-write \
   --allow-ffi \
   --allow-sys \
   --allow-run \
   --watch \
   --unstable-fs \
   src/main.ts http \$PORT
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
watch:
deno run \
--allow-net \
--allow-env \
--allow-read \
--allow-write \
--allow-ffi \
--allow-sys \
--allow-run \
--watch \
--unstable-fs \
src/main.ts http \$PORT
watch:
#!/usr/bin/env bash
if [[ -z "${PORT}" ]]; then
echo "Error: PORT environment variable is not set"
exit 1
fi
deno run \
--allow-net \
--allow-env \
--allow-read \
--allow-write \
--allow-ffi \
--allow-sys \
--allow-run \
--watch \
--unstable-fs \
src/main.ts http \$PORT

# Start the HTTP API server on port 8080
@serve:
just api http \$PORT
Expand Down
6 changes: 6 additions & 0 deletions api/src/db/migrations/20250210000000-alter-users.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
ALTER TABLE auth.users
ALTER COLUMN firstname DROP NOT NULL,
ALTER COLUMN lastname DROP NOT NULL;

ALTER TABLE territory.territories
ADD COLUMN siret VARCHAR;
4 changes: 4 additions & 0 deletions api/src/ilos/common/Decorators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,3 +130,7 @@ export function extension(config: ExtensionConfigurationType) {
}

export { inject, injectable } from "@/deps.ts";

export const proxy = Symbol.for("PROXY");
export const router = Symbol.for("ROUTER");
export const children = Symbol.for("CHILDREN");
10 changes: 4 additions & 6 deletions api/src/ilos/connection-redis/RedisConnection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,11 @@ import {
* @fixme
*/
type WindowProcess = Window & typeof globalThis & { process: NodeJS.Process };
(window as WindowProcess).process = process;
if (typeof window !== "undefined") {
(window as WindowProcess).process = process;
}

export class RedisConnection
implements
ConnectionInterface<Redis>,
DestroyHookInterface,
InitHookInterface {
export class RedisConnection implements ConnectionInterface<Redis>, DestroyHookInterface, InitHookInterface {
protected client: Redis | null = null;

get connected(): boolean {
Expand Down
3 changes: 2 additions & 1 deletion api/src/ilos/core/foundation/ServiceContainer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
RegisterHookInterface,
ServiceContainerInterface,
ServiceContainerInterfaceResolver,
children as childrenSymbol,
} from "@/ilos/common/index.ts";
import { ExtensionRegistry } from "../container/ExtensionRegistry.ts";
import { Container, HookRegistry } from "../container/index.ts";
Expand Down Expand Up @@ -107,7 +108,7 @@ export abstract class ServiceContainer
for (const child of children) {
const childInstance = new child(this.getContainer());
this.getContainer().bind(child).toConstantValue(childInstance);
this.getContainer().bind("children").toConstantValue(child);
this.getContainer().bind(childrenSymbol).toConstantValue(childInstance);
this.registerHooks(childInstance);
}
}
Expand Down
18 changes: 18 additions & 0 deletions api/src/pdc/proxy/HttpTransport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,21 @@ import {
Response,
} from "@/deps.ts";
import {
children,
ConfigInterface,
ConfigInterfaceResolver,
ContextType,
InvalidRequestException,
KernelInterface,
proxy,
RegisterHookInterface,
router,
RPCResponseType,
RPCSingleCallType,
TransportInterface,
UnauthorizedException,
} from "@/ilos/common/index.ts";
import { ServiceProvider } from "@/ilos/core/index.ts";
import { env_or_fail, env_or_false } from "@/lib/env/index.ts";
import { logger } from "@/lib/logger/index.ts";
import { get } from "@/lib/object/index.ts";
Expand Down Expand Up @@ -105,6 +110,7 @@ export class HttpTransport implements TransportInterface {
this.registerMetrics();
this.registerGlobalMiddlewares();
this.registerCache();
this.registerNestedRoutes();
this.registerAuthRoutes();
this.registerApplicationRoutes();
this.registerCertificateRoutes();
Expand All @@ -126,6 +132,18 @@ export class HttpTransport implements TransportInterface {
return this.app;
}

private registerNestedRoutes() {
this.kernel.getContainer().bind(proxy).toConstantValue(this.app);
const serviceProviders = this.kernel.getContainer().getAll<ServiceProvider>(children);
for (const serviceProvider of serviceProviders) {
const container = serviceProvider.getContainer();
if (container.isBound(router)) {
const routerInstance = container.resolve<RegisterHookInterface>(container.get(router));
routerInstance.register();
}
}
}
Comment on lines +135 to +145
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for route registration.

The registerNestedRoutes method should handle potential errors during route registration to prevent silent failures.

Apply this diff to add error handling:

 private registerNestedRoutes() {
   this.kernel.getContainer().bind(proxy).toConstantValue(this.app);
   const serviceProviders = this.kernel.getContainer().getAll<ServiceProvider>(children);
   for (const serviceProvider of serviceProviders) {
     const container = serviceProvider.getContainer();
     if (container.isBound(router)) {
-      const routerInstance = container.resolve<RegisterHookInterface>(container.get(router));
-      routerInstance.register();
+      try {
+        const routerInstance = container.resolve<RegisterHookInterface>(container.get(router));
+        routerInstance.register();
+      } catch (error) {
+        logger.error(`Failed to register routes for service provider: ${error.message}`);
+      }
     }
   }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private registerNestedRoutes() {
this.kernel.getContainer().bind(proxy).toConstantValue(this.app);
const serviceProviders = this.kernel.getContainer().getAll<ServiceProvider>(children);
for (const serviceProvider of serviceProviders) {
const container = serviceProvider.getContainer();
if (container.isBound(router)) {
const routerInstance = container.resolve<RegisterHookInterface>(container.get(router));
routerInstance.register();
}
}
}
private registerNestedRoutes() {
this.kernel.getContainer().bind(proxy).toConstantValue(this.app);
const serviceProviders = this.kernel.getContainer().getAll<ServiceProvider>(children);
for (const serviceProvider of serviceProviders) {
const container = serviceProvider.getContainer();
if (container.isBound(router)) {
try {
const routerInstance = container.resolve<RegisterHookInterface>(container.get(router));
routerInstance.register();
} catch (error) {
logger.error(`Failed to register routes for service provider: ${error.message}`);
}
}
}
}


private async getProviders(): Promise<void> {
this.config = this.kernel.getContainer().get(ConfigInterfaceResolver);
this.tokenProvider = this.kernel.getContainer().get(
Expand Down
2 changes: 2 additions & 0 deletions api/src/pdc/proxy/Kernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { Kernel as BaseKernel } from "@/ilos/framework/index.ts";
import { SentryProvider } from "@/pdc/providers/sentry/index.ts";
import { TokenProvider } from "@/pdc/providers/token/index.ts";
import { ListCommand } from "@/pdc/proxy/commands/ListCommand.ts";
import { AuthServiceProvider } from "@/pdc/services/auth/AuthServiceProvider.ts";
import { MonitoringServiceProvider } from "@/pdc/services/monitoring/MonitoringServiceProvider.ts";
import { PostgresConnection } from "../../ilos/connection-postgres/index.ts";
import { AcquisitionServiceProvider } from "../services/acquisition/AcquisitionServiceProvider.ts";
Expand All @@ -26,6 +27,7 @@ import { config } from "./config/index.ts";
@kernel({
config,
children: [
AuthServiceProvider,
AcquisitionServiceProvider,
APDFServiceProvider,
ApplicationServiceProvider,
Expand Down
33 changes: 33 additions & 0 deletions api/src/pdc/services/auth/AuthRouter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { express } from "@/deps.ts";
import { ConfigInterfaceResolver, inject, injectable, proxy } from "@/ilos/common/index.ts";
import { asyncHandler } from "@/pdc/proxy/helpers/asyncHandler.ts";
import { OidcCallbackAction } from "@/pdc/services/auth/actions/OidcCallbackAction.ts";
import { OidcProvider } from "@/pdc/services/auth/providers/OidcProvider.ts";

@injectable()
export class AuthRouter {
constructor(
@inject(proxy) private app: express.Express,
private oidcProvider: OidcProvider,
private oidcCallbackAction: OidcCallbackAction,
private config: ConfigInterfaceResolver,
) {}

register() {
this.app.get("/auth/login", (req, res, next) => {
return res.redirect(this.oidcProvider.getLoginUrl());
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling and CSRF protection to the login route.

  1. Add error handling to catch potential exceptions from getLoginUrl.
  2. Remove the unused next parameter.
  3. Add CSRF protection to prevent request forgery.
-    this.app.get("/auth/login", (req, res, next) => {
+    this.app.get("/auth/login", csrfProtection, (req, res) => {
+      try {
         return res.redirect(this.oidcProvider.getLoginUrl());
+      } catch (error) {
+        console.error('Login error:', error);
+        return res.status(500).json({ error: 'Login failed' });
+      }
     });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
this.app.get("/auth/login", (req, res, next) => {
return res.redirect(this.oidcProvider.getLoginUrl());
});
this.app.get("/auth/login", csrfProtection, (req, res) => {
try {
return res.redirect(this.oidcProvider.getLoginUrl());
} catch (error) {
console.error('Login error:', error);
return res.status(500).json({ error: 'Login failed' });
}
});


this.app.get(
"/auth/callback",
asyncHandler(async (req: express.Request, res: express.Response) => {
const { code } = req.query;
if (typeof code === "string") {
const user = await this.oidcCallbackAction.handle({ code });
req.session.user = user;
}
return res.redirect(this.config.get("oidc.app_url"));
}),
);
}
}
23 changes: 23 additions & 0 deletions api/src/pdc/services/auth/AuthServiceProvider.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { router, serviceProvider } from "@/ilos/common/index.ts";
import { ServiceProvider as AbstractServiceProvider } from "@/ilos/core/index.ts";
import { ValidatorMiddleware } from "@/pdc/providers/superstruct/ValidatorMiddleware.ts";

import { OidcProvider } from "@/pdc/services/auth/providers/OidcProvider.ts";
import { OidcCallbackAction } from "./actions/OidcCallbackAction.ts";
import { AuthRouter } from "./AuthRouter.ts";
import { config } from "./config/index.ts";
import { UserRepository } from "./providers/UserRepository.ts";

@serviceProvider({
config,
providers: [
UserRepository,
OidcProvider,
[router, AuthRouter],
],
middlewares: [
["validate", ValidatorMiddleware],
],
handlers: [OidcCallbackAction],
})
export class AuthServiceProvider extends AbstractServiceProvider {}
47 changes: 47 additions & 0 deletions api/src/pdc/services/auth/actions/OidcCallbackAction.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { ConfigInterfaceResolver, handler, UnauthorizedException } from "@/ilos/common/index.ts";
import { Action as AbstractAction } from "@/ilos/core/index.ts";
import { OidcCallback } from "../dto/OidcCallback.ts";
import { OidcProvider } from "../providers/OidcProvider.ts";
import { UserRepository } from "../providers/UserRepository.ts";

export type ResultInterface = {
email: string;
role: string;
permissions: Array<string>;
operator_id?: number;
territory_id?: number;
};

@handler({
service: "auth",
method: "oidcCallback",
middlewares: [
["validate", OidcCallback],
],
})
export class OidcCallbackAction extends AbstractAction {
constructor(
private oidcProvider: OidcProvider,
private userRepository: UserRepository,
protected config: ConfigInterfaceResolver,
) {
super();
}

public async handle(params: OidcCallback): Promise<ResultInterface> {
const token = await this.oidcProvider.getTokenFromCode(params.code);
const info = await this.oidcProvider.getUserInfoFromToken(token);
const user = await this.userRepository.findUserByEmail(info.email);
if (!user || user.siret !== info.siret) {
throw new UnauthorizedException("User not found");
}
return {
...user,
permissions: this.getPermissionsFromRole(user.role),
};
}

private getPermissionsFromRole(role: string): string[] {
return this.config.get(`permissions.${role}.permissions`, []);
}
}
7 changes: 7 additions & 0 deletions api/src/pdc/services/auth/config/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import * as oidc from "./oidc.ts";
import * as permissions from "./permissions.ts";

export const config = {
permissions,
oidc,
};
6 changes: 6 additions & 0 deletions api/src/pdc/services/auth/config/oidc.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { env_or_fail } from "@/lib/env/index.ts";
export const client_id = env_or_fail("OIDC_CLIENT_ID");
export const client_secret = env_or_fail("OIDC_CLIENT_SECRET");
export const base_url = env_or_fail("OIDC_BASE_URL");
export const redirect_url = env_or_fail("OIDC_REDIRECT_URL");
export const app_url = env_or_fail("APP_APP_URL");
Loading
Loading