Skip to content

DCOS-1521 Fix OAuth wording #192

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.

Sign up for GitHub

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

Merged
jsancio merged 1 commit into from
Jun 2, 2015
+33 −24
Merged

DCOS-1521 Fix OAuth wording #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions cli/tests/integrations/cli/test_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,8 @@ def test_when_authenticated():


def test_anonymous_login():
with patch('six.moves.input',
return_value=''), patch('uuid.uuid1',
return_value='anonymous@email'):
with patch('sys.stdin.readline', return_value='\n'), \
patch('uuid.uuid1', return_value='anonymous@email'):

assert _mock_dcos_run([util.which('dcos'),
'help'], False) == 0
Expand All @@ -35,17 +34,24 @@ def test_anonymous_login():


def _mock_dcos_run(args, authenticated=True):
env = _config_with_credentials() if authenticated else \
_config_without_credentials()
if authenticated:
env = _config_with_credentials()
else:
env = _config_without_credentials()

with patch('sys.argv', args), patch.dict(os.environ, env):
return main()


def _config_with_credentials():
return {constants.DCOS_CONFIG_ENV: os.path.join(
'tests', 'data', 'auth', 'dcos_with_credentials.toml')}
return {
constants.DCOS_CONFIG_ENV: os.path.join(
'tests', 'data', 'auth', 'dcos_with_credentials.toml')
}


def _config_without_credentials():
return {constants.DCOS_CONFIG_ENV: os.path.join(
'tests', 'data', 'auth', 'dcos_without_credentials.toml')}
return {
constants.DCOS_CONFIG_ENV: os.path.join(
'tests', 'data', 'auth', 'dcos_without_credentials.toml')
}
33 changes: 18 additions & 15 deletions dcos/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import json
import os
import sys
import uuid

import pkg_resources
import toml
from dcos import config, constants, emitting, errors, http, jsonitem, util
from dcos.errors import DCOSException
Copy link
Copy Markdown
Contributor

@mgummelt mgummelt Jun 2, 2015

Choose a reason for hiding this comment

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

what was wrong with moves?

Copy link
Copy Markdown
Contributor Author

@jsancio jsancio Jun 2, 2015

Choose a reason for hiding this comment

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

We were using for six.moves.input. That function takes a prompt string that gets printed to stdout. This is not correct because the user could be calling dcos marathon app show which prints a JSON object. A principal that we try to keep in the dcos cli is that stdout should have a predictable output that is pipe-able to a program. E.g. grep, jq, etc.

from six import iteritems, moves
from six import iteritems

from oauth2client import client

Expand Down Expand Up @@ -69,29 +70,31 @@ def _run(flow):
:rtype: dict
"""

auth_url = flow.step1_get_authorize_url()
message = """Thank you for installing the Mesosphere DCOS CLI.
Please log in with your Mesosphere Account by pasting
the following URL into your browser to continue."""
emitter.publish(errors.DefaultError(
'{message}\n\n {url}\n\n'.format(message=message,
url=auth_url,)))
emitter.publish(
Copy link
Copy Markdown
Contributor

@mgummelt mgummelt Jun 2, 2015

Choose a reason for hiding this comment

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

we haven't been using named parameters in string interpolation. I know you didn't write this, but can you change it?

Copy link
Copy Markdown
Contributor

@mgummelt mgummelt Jun 2, 2015

Choose a reason for hiding this comment

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

also, why are we printing this to stderr?

Copy link
Copy Markdown
Contributor Author

@jsancio jsancio Jun 2, 2015

Choose a reason for hiding this comment

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

also, why are we printing this to stderr?

A principal that we try to keep in the dcos cli is that stdout should have a predictable output that is pipe-able to a program. E.g. grep, jq, etc.

Let say that you want to: dcos marathon app list | ... if we the above message is written to stdout then the pipe is probably incorrect. In those cases we try to print those messages to stderr.

Copy link
Copy Markdown
Contributor Author

@jsancio jsancio Jun 2, 2015

Choose a reason for hiding this comment

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

haven't been using named parameters in string interpolation. I know you didn't write this, but can you change it?

Done.

errors.DefaultError(
'\n\n\n{}\n\n {}\n\n'.format(
'Go to the following link in your browser:',
flow.step1_get_authorize_url())))

code = moves.input('Please enter Mesosphere verification code: ').strip()

This comment was marked as off-topic.

Sign in to view

sys.stderr.write('Enter verification code: ')
code = sys.stdin.readline().strip()
if not code:
email = moves.input('Skipping authentication.'
' Please enter email address:').strip()
sys.stderr.write('Skipping authentication.\nEnter email address: ')

email = sys.stdin.readline().strip()
if not email:
emitter.publish(errors.DefaultError('Skipping email input,'
' using anonymous id:'))
emitter.publish(
errors.DefaultError(
'Skipping email input.'))
email = str(uuid.uuid1())

return {CORE_EMAIL_KEY: email}

return make_oauth_request(code, flow)


def check_if_user_authenticated():
""" check if user is authenticated already
"""Check if user is authenticated already

:returns user auth status
:rtype: boolean
Expand All @@ -102,7 +105,7 @@ def check_if_user_authenticated():


def force_auth():
""" Make user authentication process
"""Make user authentication process

:returns authentication process status
:rtype: boolean
Expand Down