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

Long-lived access token #16453

Merged
merged 11 commits into from
Sep 11, 2018
Merged

Conversation

awarecan
Copy link
Contributor

@awarecan awarecan commented Sep 6, 2018

Description:

Implement long-lived access token back end logic

  • Allow create refresh_token with different access_token_expiration,
  • Add client_name, client_icon fields
  • Add token_type, normal, system or long_lived_access_token
  • Each such client_id can only have one long_lived_access_token type refresh_token, each such refresh_token can only have one active access_token.

Send websocket command auth/long_lived_access_token will create or refresh a long-lived access token for current user. Access token will not be saved in Home Assistant. User need to record the token in secure place.

{
    "id": 11,
    "type": "auth/long_lived_access_token",
    "client_id": "gps_logger",
    "client_name": "GPS Logger",
    "client_icon": null,
    "lifespan": 365
}

Result will be a long-lived access token:

{
    "id": 11,
    "type": "result",
    "success": true,
    "result": "ABCDEFGH"
}

To use access token, put it in HTTP Authorization Header, for example

curl -H "Authorization: Bearer ABCDEFGH" http://hassbian.local:8123/api

Related issue (if applicable): fixes back-end part #15195

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/developers.home-assistant#82

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

'refresh tokens')

if (token_type == models.TOKEN_TYPE_LONG_LIVED_ACCESS_TOKEN and
(client_id is None or client_id.lower().startswith(
Copy link
Member

Choose a reason for hiding this comment

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

I think instead we should just enforce client_id is None

raise ValueError('Client_id is not allowed start with http:// or '
'https:// for long-lived access token')

if token_type == models.TOKEN_TYPE_LONG_LIVED_ACCESS_TOKEN:
Copy link
Member

Choose a reason for hiding this comment

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

I think that this is a weird check. We shouldn't enforce this but instead leave this up to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current is
1 client_id : 1 refresh_token : 1 long-lived access_token

Did you want to change to
1 client_id : m refresh_token : m long-lived access_token

client_name: Optional[str] = None,
client_icon: Optional[str] = None,
token_type: str = models.TOKEN_TYPE_NORMAL,
access_token_expiration: Optional[timedelta] = None) \
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this one always passed in and so doesn't have to be optional? I think it's a security concern if expiration defaults are applied whenever we load a refresh token.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current workflow, the normal login flow, did not pass in access_token_expiration

Default value will be set in the lowest level of access_token creation. if user pass in None, this field would not be set in **kwargs, therefore the access_token will carry on default expiration value, 30 minutes

Copy link
Member

Choose a reason for hiding this comment

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

This should not live in auth_store but in auth_manager, as that's where the business logic should live.

@@ -178,6 +195,31 @@ def __init__(self, hass: HomeAssistant) -> None:

return found

@callback
def async_mutate_refresh_token(
Copy link
Member

@balloob balloob Sep 6, 2018

Choose a reason for hiding this comment

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

I am thinking now if all this hassle is even necessary.

If we just never tell the user the refresh token, they can never get a second access token. If our API endpoint just returns the access token, this is no longer our concern.

And if it's not our concern, we don't have to enforce single access token, we don't have to re-generate JWT tokens. It makes everything a lot simpler. We can still keep type, as it helps showing the user what's going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only use refresh_token as a container to save client information in this case. It never returns back to user. But I am imaging there is "refresh" or "regenerate" button in UI to allow user get a new access token, at the same time revoke previous issued

Back to the above comment in, if we have 1:m for client_id and refresh_token, then this enforce is unnecessary, otherwise, we still need a method to either recreate refresh_token or mutate jwt_key

Copy link
Member

Choose a reason for hiding this comment

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

I think it's easier to solve the regenerate case by removing old refresh token and generate a new refresh and access token with same client id/icon. auth_store is already growing a lot and this should be kept as simple as possible.

Copy link
Member

Choose a reason for hiding this comment

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

So I suggest we do without this method.

from homeassistant.core import HomeAssistant, callback
from homeassistant.util import dt as dt_util

from . import models
from . import models, util

Choose a reason for hiding this comment

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

'.util' imported but unused

'refresh tokens')

if (token_type == models.TOKEN_TYPE_LONG_LIVED_ACCESS_TOKEN and
(client_id is None or client_name is None)):
Copy link
Member

Choose a reason for hiding this comment

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

client_id should be None. Client ID is what is used when a user generates a token via an external client. When creating a long lived token, they use the UI and no external client is involved.

for token in list(user.refresh_tokens.values()):
if (token.client_id == client_id and token.token_type ==
models.TOKEN_TYPE_LONG_LIVED_ACCESS_TOKEN):
# Each client_id can only have one long_lived_access_token
Copy link
Member

Choose a reason for hiding this comment

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

Client ID should always be None. I think that this check can be removed.

'System generated users can only have system type '
'refresh tokens')

if not user.system_generated and \
Copy link
Member

Choose a reason for hiding this comment

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

You can collapse this if-statement in the previous one:

if user.system_generated != (token_type == models.TOKEN_TYPE_SYSTEM):
    raise ValueError(…)

@ghost ghost assigned balloob Sep 11, 2018
@balloob balloob merged commit 9583947 into home-assistant:dev Sep 11, 2018
@ghost ghost removed the in progress label Sep 11, 2018
@balloob balloob mentioned this pull request Sep 11, 2018
@balloob balloob added this to the 0.78.0 milestone Sep 11, 2018
balloob pushed a commit that referenced this pull request Sep 11, 2018
* Allow create refresh_token with specific access_token_expiration

* Add token_type, client_name and client_icon

* Add unit test

* Add websocket API to create long-lived access token

* Allow URL use as client_id for long-lived access token

* Remove mutate_refresh_token method

* Use client name as id for long_lived_access_token type refresh token

* Minor change

* Do not allow duplicate client name

* Update docstring

* Remove unnecessary `list`
@balloob balloob mentioned this pull request Sep 17, 2018
@qingdujun
Copy link

qingdujun commented Sep 26, 2018

Hi, Help Me! Is the content sent incorrectly?

I have successfully authenticated, but I want a long-term token.

...
As you can see.
...

{"type": "auth_required", "ha_version": "0.77.3"}

var v = {type: 'auth', access_token: token};

ws.send(JSON.stringify(v));

{"type": "auth_ok", "ha_version": "0.77.3"}

Ok, It was all successful before here.
...

THEN (error)
...

SEND

var long_lived = {id:'11', type: 'auth/long_lived_access_token', 
client_name: 'GPS Logger', client_icon:null, lifespan:365};

ws.send(JSON.stringify(long_lived));

RECV

{"id": 11, "type": "result", "success": false, "error": {"code": 4, "message": "Unknown command."}}

@MartinHjelmare
Copy link
Member

Please open an issue. Closed PRs should not be used for support or bug reports.

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Sep 26, 2018
@awarecan awarecan deleted the long-lived-access-token branch March 12, 2019 06:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants