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

use StoreTTL in place of Store for managing OAuth2 state #83

Merged
merged 9 commits into from
May 7, 2020

Conversation

avddvd
Copy link
Contributor

@avddvd avddvd commented Apr 14, 2020

Summary

  • Use StoreTTL method of KVStore to manage the OAuth2 state.

Ticket Link

Fixes #71

@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Apr 14, 2020
@codecov-io
Copy link

codecov-io commented Apr 14, 2020

Codecov Report

Merging #83 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #83   +/-   ##
=======================================
  Coverage   25.31%   25.31%           
=======================================
  Files          61       61           
  Lines        2303     2303           
=======================================
  Hits          583      583           
  Misses       1654     1654           
  Partials       66       66           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4cd92a...03cd79a. Read the comment docs.

Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @avddvd!

@levb Is 5 minutes a practical time duration for this in your opinion? I suggested this duration in the HW ticket. I'm not sure if there is user intervention within that window. If it is all server-to-server talking, then we can go much lower with the duration.

Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks! 🎉

@larkox
Copy link
Contributor

larkox commented Apr 16, 2020

@mickmister The state of the OAuth has to live from the moment the user hits the link, until the login process is finished. Therefore, you have to take in account the time the user takes to put their credentials. I think 5 minutes is a reasonable time.

Copy link
Contributor

@levb levb left a comment

Choose a reason for hiding this comment

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

I'd choose 15m myself, but 5 seems alright, too.

@levb levb added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels Apr 16, 2020
@levb levb requested a review from DHaussermann April 16, 2020 12:56
Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

LGTM thanks @avddvd!

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

  • Regression tested authentication with multiple users
  • Ensure authentication records are not accruing in the KV store
  • Ensured time to live is respected
    LGTM!
    Thanks @avddvd for this improvement. Sorry for the delay on getting this tested.

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels May 6, 2020
@mickmister
Copy link
Contributor

/update-branch

@mickmister
Copy link
Contributor

Thanks @avddvd!

@mickmister mickmister merged commit aeefae9 into mattermost:master May 7, 2020
@mickmister
Copy link
Contributor

@avddvd Make sure to join the community server to work more closely with the team! We have a channel for this project on the server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete OAuth state from KV store after OAuth process is complete
8 participants