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

adding cookie secret for tornado app #1801

Closed
wants to merge 28 commits into from
Closed

Conversation

lukh
Copy link
Contributor

@lukh lukh commented Sep 16, 2019

adding a cookie_secret made from bcrypt.gensalt for cookie_secure feature in tornado app (for auth. process on the web app)

@codecov
Copy link

@codecov codecov bot commented Sep 16, 2019

Codecov Report

Merging #1801 into develop will increase coverage by 0.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1801      +/-   ##
===========================================
+ Coverage    82.17%   82.19%   +0.01%     
===========================================
  Files           77       77              
  Lines         6318     6328      +10     
===========================================
+ Hits          5192     5201       +9     
- Misses        1126     1127       +1
Impacted Files Coverage Δ
mopidy/http/actor.py 63.7% <83.33%> (+2.3%) ⬆️

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 ff991b6...95a88fd. Read the comment docs.

@adamcik
Copy link
Member

@adamcik adamcik commented Sep 16, 2019

I take it this is so that web app can optionally use cookies with a reasonable value set.

setup.py Outdated Show resolved Hide resolved
mopidy/http/actor.py Outdated Show resolved Hide resolved
kingosticks
Copy link
Member

@kingosticks kingosticks commented on b5f2f64 Sep 18, 2019

Choose a reason for hiding this comment

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

@lukh, you can run flake8 locally, as well as the other CI tests if you want. Details are at https://docs.mopidy.com/en/latest/devenv/

@lukh
Copy link
Contributor Author

@lukh lukh commented Sep 18, 2019

Oh. Sorry. Thank you !

mopidy/http/actor.py Outdated Show resolved Hide resolved
@lukh lukh changed the title adding cookie secret for tornado app (w/ bcrypt) adding cookie secret for tornado app Sep 19, 2019
@lukh
Copy link
Contributor Author

@lukh lukh commented Nov 17, 2019

sorry, I messed up, had to force push to clear the work related to my others PR...
(http_redirection_op namely)

@lukh
Copy link
Contributor Author

@lukh lukh commented Nov 18, 2019

I must say, I have absolutely no idea about how to test the added code.
Do you have any idea ? thank you !

@lukh lukh requested a review from adamcik Nov 28, 2019
jodal
jodal approved these changes Nov 30, 2019
mopidy/http/actor.py Show resolved Hide resolved
@jodal jodal added this to the v3.0 milestone Nov 30, 2019
@jodal jodal added A-http C-enhancement labels Nov 30, 2019
@jodal
Copy link
Member

@jodal jodal commented Nov 30, 2019

Bonus points if you add a changelog entry referring back to this PR :-)

jodal
jodal approved these changes Dec 1, 2019
docs/changelog.rst Outdated Show resolved Hide resolved
docs/changelog.rst Outdated Show resolved Hide resolved
mopidy/http/actor.py Outdated Show resolved Hide resolved
tests/http/test_server.py Outdated Show resolved Hide resolved
tests/http/test_server.py Outdated Show resolved Hide resolved
tests/http/test_server.py Outdated Show resolved Hide resolved

class HttpServerTestCookieSecret(TestCase):
def test_get_secure_cookie(self):
self._dirpath = tempfile.mkdtemp()
Copy link
Member

@jodal jodal Dec 1, 2019

Choose a reason for hiding this comment

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

Since we use pytest, you can have pytest create and clean up a temporary dir for each test:

def test_get_secure_cookie(tmp_path):
    tmp_path.mkdir()
    ...
    # You can rmeove the shutil.rmtree() at the end

lukh and others added 7 commits Dec 2, 2019
@jodal jodal closed this in cd25198 Dec 2, 2019
@jodal
Copy link
Member

@jodal jodal commented Dec 2, 2019

Again, thank you for your patience!

@lukh
Copy link
Contributor Author

@lukh lukh commented Dec 2, 2019

Well ! thank you for you help ! happy to help !

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

Successfully merging this pull request may close these issues.

None yet

4 participants