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.

Sign up for GitHub

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

Jump to bottom

Issue 740 OIDC login redirect #742

Merged
merged 2 commits into from
Nov 15, 2023
Merged

Issue 740 OIDC login redirect #742

merged 2 commits into from
Nov 15, 2023

Conversation

fflorent
Copy link
Collaborator

@fflorent fflorent commented Nov 13, 2023 •
edited
Loading

Context

Resolves #740

  • After being logged in, the user should be redirected to the URL s/he was consulting before the authentication process;

Technical considerations

  • Store the targetUrl along with the state and the codeVerifier in the session;
  • Let the openid-client compare the state as stored in the session and as stored in the parameters (otherwise it will be useless as pointed out by Dmitry here;
  • Continue using the state, which I believe offers protection against CSRF that PKCE does not (cf Daniel Fett this summary in the blog article);
  • Add more logs;
  • Also don't print error the full error message in the response, which could give attackers information;

Florent FAYOLLE added 2 commits November 13, 2023 12:20
Fix OIDC redirects from team site to personal page after login gristl…
e887f95 
…abs#740

Also:
 - compare state in session and state passed through parameters
 (otherwise the state won't have any effect regarding the security).
 - delete the session even after an authentication failure
More logs for OIDC gristlabs#740
aa40dc7
@fflorent fflorent requested a review from dsagal November 13, 2023 11:44
dsagal
dsagal reviewed Nov 14, 2023
View reviewed changes
Copy link
Member

@dsagal dsagal left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you! Will ask for a second reviewer because security-related.

@georgegevoian georgegevoian self-requested a review November 14, 2023 23:52
georgegevoian
georgegevoian approved these changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@georgegevoian georgegevoian left a comment

Choose a reason for hiding this comment

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

Thanks @fflorent.

@dsagal dsagal merged commit e8789e6 into gristlabs:main Nov 15, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@georgegevoian georgegevoian georgegevoian approved these changes

@dsagal dsagal dsagal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OIDC redirects from team site to personal page after login
3 participants
@fflorent @dsagal @georgegevoian