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

Implementation of PKCE authentification #191

Merged
merged 5 commits into from
Jun 21, 2024

Conversation

eik-dahms
Copy link
Contributor

In the current version of ARCitect client ids and secrets have to be stored in code. It would be nice to avoid storing the secrets in code. This can be avoided by using PKCE flow. Here I implemented this in DataHubService.ts.

The PKCE will be used if a datahub entry in Credentials does not contain a "secret" field.:

  • on authorization the existence of secret field is checked
  • if not existing the strings: state, code_verifier and code_challenge are generated
  • code request is done with code_challenge instead of client secret
  • subsequently token request is done with code_verifier

The previous authentification method is still possible

pkce will only work if confidential is turned off - and I assume that then client_id/ client_secret method will not work anymore.

image


Not thouroughly tested. But from receving the token/refresh token everything should be the same as before.

@JonasLukasczyk
Copy link
Collaborator

JonasLukasczyk commented Jun 20, 2024

Thank you for this PR. If we would merge this in right now would everything still work or do we need to change something on the datahubs? @eik-dahms

So the question is how we migrate to this authentication method.

@eik-dahms
Copy link
Contributor Author

If you would merge this everything works as before. (I have tested that) Meaning the existing hosts with secret entry will still use the client_id and secret method.

The new method will only be used if a host is missing the secret entry.

If you want to migrate existing hosts I would register a new App in GitLab with the confidential setting turned of (as described above) because when I tried toggling the confidential setting with an existing App registration it did not work.

@JonasLukasczyk
Copy link
Collaborator

Thank you! Then this is good to go.

@JonasLukasczyk JonasLukasczyk merged commit 4464766 into nfdi4plants:main Jun 21, 2024
@eik-dahms eik-dahms deleted the pkce_oauth branch June 26, 2024 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants