Skip to content

Conversation

apognu
Copy link
Contributor

@apognu apognu commented Sep 24, 2025

DO NOT MERGE

See checkmarble/marble-backend#1204.

This is not mergeable, for several reasons, it will need an actual frontend engineer to clean up, handle edge cases, and support refresh.

Also, note that there is no error handling. If the IDP returns an error, it will be displayed as plain text in the browser. :D

@apognu apognu self-assigned this Sep 24, 2025
@apognu apognu added enhancement New feature or request do-not-merge labels Sep 24, 2025
@apognu apognu force-pushed the feat/poc/oidc-authentication branch 3 times, most recently from fa7b163 to 3d717db Compare September 25, 2025 18:19
@apognu apognu force-pushed the feat/poc/oidc-authentication branch from 3d717db to 3284e11 Compare October 3, 2025 13:50
@ChibiBlasphem ChibiBlasphem force-pushed the feat/poc/oidc-authentication branch 2 times, most recently from 09ea524 to 46d0611 Compare October 16, 2025 13:24
@ChibiBlasphem ChibiBlasphem force-pushed the feat/poc/oidc-authentication branch 5 times, most recently from 09d94d6 to 8829924 Compare October 20, 2025 12:54
@ChibiBlasphem ChibiBlasphem marked this pull request as ready for review October 20, 2025 12:58
@ChibiBlasphem ChibiBlasphem force-pushed the feat/poc/oidc-authentication branch from 8829924 to 353b542 Compare October 20, 2025 13:34
@ChibiBlasphem ChibiBlasphem requested a review from a team October 20, 2025 14:37
Copy link
Contributor

@Pascal-Delange Pascal-Delange left a comment

Choose a reason for hiding this comment

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

looks ok to me, I didn't read the middleware functions part in great detail however.

const response = await basicFetch(input, { ...init, headers });

if (response.status === 401) {
// TODO: here, we use the new tokens for the current request, but it is
Copy link
Contributor

Choose a reason for hiding this comment

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

comment still relevant ?

failureRedirect: getRoute('/sign-in'),
});

const res = await next({ context: { authInfo } });
Copy link
Contributor

Choose a reason for hiding this comment

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

just testing my understanding of your middleware function: should we not call next with the received context to which we add authInfo ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The merge on the context sent to next and the previous one is automatically merge. You only need to send the new context properties you want to set.

@ChibiBlasphem ChibiBlasphem force-pushed the feat/poc/oidc-authentication branch 3 times, most recently from f5bed9e to 3b5984d Compare October 21, 2025 11:01
@ChibiBlasphem ChibiBlasphem force-pushed the feat/poc/oidc-authentication branch from 3b5984d to f813572 Compare October 21, 2025 11:03
@ChibiBlasphem ChibiBlasphem merged commit 66ed9f2 into main Oct 21, 2025
5 of 7 checks passed
@ChibiBlasphem ChibiBlasphem deleted the feat/poc/oidc-authentication branch October 21, 2025 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants