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

Allow for easier extension of OAuth2 grants and add support for OpenID Connect Authorization Code Flow #39

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

cmd-johnson
Copy link
Owner

@cmd-johnson cmd-johnson commented Sep 20, 2023

There have been a few requests to add support for OpenID Connect (OIDC) related extensions to the OAuth2 protocol (#28, #29, #32).
Instead of adding these extensions to the OAuth2 client (making it more complex than it needs to be), I've decided to add a separate OIDC Client, living in its own subdirectory. That way, if you only need the OAuth2 client you can just continue to import it as is. If you do want to use OIDC specific features and APIs you'll be able to import those from https://deno.land/x/oauth2_client/oidc.ts.
The OAuth2 and OIDC clients mostly share the same interface, with the OIDC client marking a few fields of the OAuth2 config required and adding a few extra options (like the URI of the userinfo endpoint).

This PR isn't quite ready to be merged yet, but we're getting there!

To Do

  • Fix OAuth2 tests (they're not yet adapted to the refactored implementation of OAuth2 grant classes)
  • Add Tests for the OIDC implementation

Future Work

This is a pretty basic implementation of an OIDC client so far.
Apart from the userinfo endpoint (#29), there's additional APIs that OIDC specifies, like

And then there's APIs that are often used with OIDC, but also apply to OAuth2, like

When this PR is merged I'll create a separate issue to track all these.

@cmd-johnson cmd-johnson self-assigned this Sep 20, 2023
@cmd-johnson cmd-johnson changed the title Add support for OpenID Connect Authorization Code Flow Allow for easier extension of OAuth2 grants and add support for OpenID Connect Authorization Code Flow Sep 20, 2023
@cmd-johnson cmd-johnson marked this pull request as draft September 20, 2023 06:18
@christian-hackyourshack
Copy link

christian-hackyourshack commented Oct 7, 2023

I tried your OIDCClient and found out, that it throws an error if I reduce the scope to "openid", because it tries to match the at_hash with a non-existing access_token, because the OpenID provider (at least Google) then just provides the id_token (which is completely fine, if you are only interested in authentication):

EDIT: Debugged a bit more... There still is an access_token and there is an at_hash, but there are special characters in there, that do not match: a - in the at_hash is a + in the jose.base64EncodedHash, rest is equal. Special character = is already replaced with an empty string. Besides that I found other solutions (https://stackoverflow.com/a/52298549) that try to make the base64 encoding url-safe by also replacing the following "+" -> "-" and "/" -> "_".

An error occurred during route handling or page rendering. Error: Invalid token response: id_token at_hash claim does not match access_token hash
at AuthorizationCodeFlow.validateAccessToken (https://raw.githubusercontent.com/cmd-johnson/deno-oauth2-client/feature/oidc/src/oidc/authorization_code_flow.ts:246:13)
at eventLoopTick (ext:core/01_core.js:183:11)
at async AuthorizationCodeFlow.getToken (https://raw.githubusercontent.com/cmd-johnson/deno-oauth2-client/feature/oidc/src/oidc/authorization_code_flow.ts:206:7)

Should I open a seperate issue for this?
I could provide a PR, if you like, although you might want to change it yourself:

Add a function to authorization_code_flow.ts

function makeUrlSafe(hash: string) {
  return hash.replaceAll("+", "-").replaceAll("/", "_").replaceAll("=", "");
}

Use that in line 245 instead of manipulating the hash in line 243

    const base64EncodedHash = base64Encode(leftHalf);

    if (makeUrlSafe(base64EncodedHash) !== makeUrlSafe(atHash)) {

@christian-hackyourshack
Copy link

Another finding: When using OpenID with Facebook, the id_token comes back with an empty string as nonce property, even when nonce option was not used, causing the authorization_code_flow to throw in line 290. You should check for a non-empty string there instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants