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

OpenID Connect - Validate Discovery Endpoint response #1981

Merged
merged 17 commits into from
Aug 15, 2023

Conversation

cieciurm
Copy link
Contributor

@cieciurm cieciurm commented Aug 4, 2023

What this PR does / why we need it:

  • This PR adds validation of OpenID Connect Discover response
  • Validation follows rules from the spec

Which issue(s) this PR fixes:
closes #701

Special notes for your reviewer:

  • This is a very early PR
  • I want to ask for feedback on the approach
  • Tests will be added when we agree on solution

Does this PR introduce a user-facing change?:

  • No

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Unit tests passing
  • End-to-end tests passing
  • Extended the documentation
  • Provided sample for the feature

@cieciurm
Copy link
Contributor Author

cieciurm commented Aug 4, 2023

Thanks for any suggestions and feedback 😄
cc @sungam3r

@sungam3r
Copy link
Collaborator

sungam3r commented Aug 6, 2023

I want to ask for feedback on the approach
Tests will be added when we agree on solution

Looks good thought I have not read the spec.

@sungam3r sungam3r added the enhancement New feature or request label Aug 6, 2023
@cieciurm
Copy link
Contributor Author

cieciurm commented Aug 7, 2023

I would like to add tests for validation.

@sungam3r
Copy link
Collaborator

sungam3r commented Aug 7, 2023

Reminder - bump version.

@cieciurm cieciurm changed the title OpenID Connect - Validate discover endpoint response OpenID Connect - Validate Discovery Endpoint response Aug 8, 2023
@cieciurm
Copy link
Contributor Author

cieciurm commented Aug 8, 2023

I've added applied code suggestions, added unit tests and bumped the version.

I decided to write more sophisticated check for response_types_supported. The spec mentiones a combined response type token id_token, but some identity providers (f.e. Identity Server and Azure AD) return id_token token. So I decided to validate separately the single response types (code, id_token) and separately combined response type. This should prevent false negatives.

cc @sungam3r

@sungam3r sungam3r added this to the V 7.1 milestone Aug 8, 2023
@cieciurm
Copy link
Contributor Author

cieciurm commented Aug 9, 2023

Thanks, <InternalsVisibleTo /> has been removed.

cc @sungam3r

@codecov-commenter
Copy link

Codecov Report

Merging #1981 (2d7c340) into master (136d8dc) will increase coverage by 0.30%.
The diff coverage is 87.23%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #1981      +/-   ##
==========================================
+ Coverage   66.19%   66.49%   +0.30%     
==========================================
  Files         253      253              
  Lines        8726     8728       +2     
  Branches      627      626       -1     
==========================================
+ Hits         5776     5804      +28     
+ Misses       2785     2760      -25     
+ Partials      165      164       -1     
Flag Coverage Δ
Dapr ?
OpenIdConnectServer 39.28% <87.23%> (+14.28%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...althChecks.OpenIdConnectServer/IdSvrHealthCheck.cs 73.07% <60.00%> (-5.88%) ⬇️
...s.OpenIdConnectServer/DiscoveryEndpointResponse.cs 93.93% <93.93%> (ø)
.../HealthChecks.OpenIdConnectServer/OidcConstants.cs 100.00% <100.00%> (ø)

... and 7 files with indirect coverage changes

@sungam3r sungam3r merged commit 20d236d into Xabaril:master Aug 15, 2023
2 checks passed
@cieciurm cieciurm deleted the openidc_validate branch August 21, 2023 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request openidconnect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenID Connect server looks only at HTTP response code
3 participants