Skip to content

Avoid removing TrustedClient tokens after a password reset - #20836

Closed
OmarIthawi wants to merge 3 commits into
openedx:masterfrom
appsembler:omar/tursted-oauth-clients
Closed

Avoid removing TrustedClient tokens after a password reset#20836
OmarIthawi wants to merge 3 commits into
openedx:masterfrom
appsembler:omar/tursted-oauth-clients

Conversation

@OmarIthawi

@OmarIthawi OmarIthawi commented Jun 17, 2019

Copy link
Copy Markdown
Member

This is a chery-pick from Hawthorn (appsembler#398). It needs rebasing, but I'll do that after product review.

What's the Issue?

We found out that both of the Access and Refresh tokens of a TrustedClient are removed after a password reset by the destroy_oauth_tokens method.

These tokens should only be removed for untrusted clients such as mobile apps or third party apps. A trusted client such as the edX Marketing site (for example) should retain those tokens and only be removed in the case of a marketing site security compromise.

This pull request fixes the issue by avoid removing the tokens for TrustedClients.

See: https://courses.edx.org/admin/edx_oauth2_provider/trustedclient/

@OmarIthawi
OmarIthawi requested a review from a team June 17, 2019 13:32
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @OmarIthawi! I've created OSPR-3665 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Jun 17, 2019
@feanil

feanil commented Jun 18, 2019

Copy link
Copy Markdown
Contributor

@OmarIthawi I'll put this in the product review queue .

FYI @marcotuts

@openedx-webhooks openedx-webhooks added product review PR requires product review before merging and removed needs triage labels Jun 18, 2019
@marcotuts

Copy link
Copy Markdown
Contributor

Ok to move this to engineering review next, I don't have any specific product feedback. Will need to make sure we address security needs as part of that.

@openedx-webhooks openedx-webhooks added awaiting prioritization and removed product review PR requires product review before merging labels Jul 15, 2019
@OmarIthawi

Copy link
Copy Markdown
Member Author

Thanks @marcotuts and @feanil!

@OmarIthawi OmarIthawi changed the title Avoid removing AMC tokens after a password reset Avoid removing TurstedClient tokens after a password reset Jul 18, 2019
@OmarIthawi OmarIthawi changed the title Avoid removing TurstedClient tokens after a password reset Avoid removing TrustedClient tokens after a password reset Sep 2, 2019
@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests. The following contexts failed:

  • jenkins/quality
  • jenkins/python
  • jenkins/bokchoy
  • jenkins/a11y

@OmarIthawi

Copy link
Copy Markdown
Member Author

Thanks @edx-status-bot, I'm aware of that. I need the code to be reviewed first, and I'll fix those issues afterward.

Your PR has finished running tests. The following contexts failed:

  • jenkins/quality
  • jenkins/python
  • jenkins/bokchoy
  • jenkins/a11y

@natabene

natabene commented Sep 3, 2019

Copy link
Copy Markdown
Contributor

@ormsbee This is ready for your review when you have a chance.

@natabene

natabene commented Oct 7, 2019

Copy link
Copy Markdown
Contributor

jenkins run quality

@OmarIthawi

Copy link
Copy Markdown
Member Author

Thanks @natabene! I was just testing if those commands still work.

@OmarIthawi OmarIthawi closed this Nov 21, 2019
@OmarIthawi
OmarIthawi deleted the omar/tursted-oauth-clients branch November 21, 2019 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U rejected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants