Skip to content
This repository has been archived by the owner on Jan 23, 2024. It is now read-only.

FI-563: Omit invalid client ID token refresh test for public clients #434

Merged
merged 8 commits into from
Feb 13, 2020

Conversation

Jammjammjamm
Copy link
Contributor

This branch omits the invalid client ID token refresh test for public clients because that test is only valid for confidential clients. There is no client ID based authentication for public client during a token refresh.

Submitter:

  • This pull request describes why these changes were made
  • Internal ticket for this PR: https://oncprojectracking.healthit.gov/support/browse/FI-563
  • Internal ticket links to this PR
  • Internal ticket is properly labeled (Community/Program)
  • Internal ticket has a justification for its Community/Program label
  • Code diff has been reviewed for extraneous/missing code
  • Tests are included and test edge cases
  • Tests/code quality metrics have been run locally and pass

Reviewer 1:

Name:

  • Code is maintainable and reusable, reuses existing code and infrastructure
    where appropriate, and accomplishes the task's purpose
  • The tests appropriately test the new code, including edge cases
  • You have tried to break the code

Reviewer 2:

Name:

  • Code is maintainable and reusable, reuses existing code and infrastructure
    where appropriate, and accomplishes the task's purpose
  • The tests appropriately test the new code, including edge cases
  • You have tried to break the code

@Jammjammjamm Jammjammjamm self-assigned this Feb 4, 2020
@arscan
Copy link
Contributor

arscan commented Feb 9, 2020

Why are we passing client_id in the refresh token grant type for non-confidential clients? This doesn't seem right.

https://github.com/onc-healthit/inferno/blob/426e8af6aa9f3d8acd528e84638f973cfc02e400/lib/modules/smart/token_refresh_sequence.rb#L152-L156

@Jammjammjamm
Copy link
Contributor Author

I agree, that doesn't seem right.

Copy link
Contributor

@radamson radamson left a comment

Choose a reason for hiding this comment

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

LGTM

@arscan arscan merged commit f34521c into development Feb 13, 2020
@arscan arscan deleted the fi-563-omit-tr02-public-clients branch February 13, 2020 14:31
@radamson radamson mentioned this pull request Aug 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants