Expire USERTOKEN on logout, and after reasonable time

REOPENED
Assigned to

Status

--
enhancement
REOPENED
8 years ago
7 years ago

People

(Reporter: carljm, Assigned: vadimk)

Tracking

unspecified

Details

(Reporter)

Description

8 years ago
The /login call sends a USERTOKEN cookie which can be used in place of Basic Auth to authenticate further calls. For security, this token should be invalidated when /logout is called, or after a set expiry time.
(Assignee)

Updated

8 years ago
Assignee: nobody → vadimk
(Assignee)

Comment 1

8 years ago
Added expiration time 1 hour and remove login token when logout method is called.
(Reporter)

Comment 2

8 years ago
(In reply to comment #1)
> Added expiration time 1 hour and remove login token when logout method is
> called.

Great. I should have been more specific on expiration timeframe - I think one hour might be a bit too aggressive. It's a bit of a tradeoff between convenience for users and security, but the main thing is that it should expire eventually so if an attacker gets ahold of a token they can't save it and use it whenever. I'd been thinking more like several days or a week, rather than an hour.

I'd say let's change it to a week, unless Aakash has a different opinion.
(Assignee)

Comment 3

8 years ago
It's 1 hour of inactivity. While user keeps sending requests, it should not expire at all. In any case, this parameter should be made configurable via maven settings.xml file or something like that....
(Reporter)

Comment 4

8 years ago
(In reply to comment #3)
> It's 1 hour of inactivity. While user keeps sending requests, it should not
> expire at all. In any case, this parameter should be made configurable via
> maven settings.xml file or something like that....

Ah yes, one hour of inactivity is certainly better, though it still means a user will have to login again on any repeat visit, pretty much. 

Configurable would be great.
(Assignee)

Comment 5

8 years ago
Added new variable to settings.xml. Make sure to update your settings file before building new version.

<!-- Web Services -->
<login.expiration.seconds>3600</login.expiration.seconds>
(Reporter)

Comment 6

8 years ago
(In reply to comment #5)
> Added new variable to settings.xml. Make sure to update your settings file
> before building new version.
> 
> <!-- Web Services -->
> <login.expiration.seconds>3600</login.expiration.seconds>

Is this setting there but not functional yet? I tried changing it to 5 seconds, built a new version, but it wasn't expiring my token after 5 seconds of inactivity...
(Assignee)

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 7

8 years ago
I don't think the fix here achieves the goal. If I'm not mistaken, you're setting an expiration time on the USERTOKEN cookie. But cookie expiration times are only advisory. This doesn't help with security at all - if the database is compromised and someone gets hold of a user token, the platform continues to accept it forever. (According to my test here, even if the user logs out the platform continues to accept the token).

Setting expiration time on the cookie, or deleting the cookie, is not relevant here, because the client is not a browser, it's the UI app. The only change that will serve the security purpose is for the platform itself to invalidate the token internally so it will no longer be accepted as a valid credential after the expiration time or the user logout.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

8 years ago
yes, we need eventually to implement more robust solution, maybe persist token in db...
I wouldn't consider it a very serious problem now, since the site will be open to community anyway...
Severity: normal → minor
(Reporter)

Comment 9

8 years ago
(In reply to comment #8)
> yes, we need eventually to implement more robust solution, maybe persist token
> in db...
> I wouldn't consider it a very serious problem now, since the site will be open
> to community anyway...

Well, it's open to the community, but with registration. Obviously some one who is able to get ahold of a user token for a privileged user could potentially do a lot of damage. I would say being open to the community makes it a higher priority to implement secure systems, because the profile is higher for possible attacks.
A test for this bug is automated in my Lettuce tests.  Currently failing.
Failing test scenario:

  Scenario: Log in and out as user: (Bug 636586)                               # features/bugs/users.feature:7
    Given I create the seed company and product with these names:              # features/steps.py:184
      | company name    | product name |
      | Massive Dynamic | Cortexiphan  |
    And I create a new user with name "Walter Bishop"                          # features/steps_users.py:63
    When I log in user with that name                                          # features/steps_users.py:150
    Then I am logged in as the user with that name                             # features/steps_users.py:167
    And when I log out the user with that name                                 # features/steps_users.py:140
    Then that user is not logged in                                            # features/steps_users.py:182
    Traceback (most recent call last):
      File "/Library/Python/2.6/site-packages/lettuce-0.1.20-py2.6.egg/lettuce/core.py", line 81, in __call__
        ret = self.function(self.step, *args, **kw)
      File "/Users/camerondawson/gitspace/tcm/tests/bdd/features/steps_users.py", line 187, in user_not_logged_in
        headers = headers, exp_status = 401)
      File "/Users/camerondawson/gitspace/tcm/tests/bdd/features/tcm_request_helper.py", line 90, in do_get
        return verify_status(exp_status, response, str(uri))
      File "/Users/camerondawson/gitspace/tcm/tests/bdd/features/tcm_request_helper.py", line 50, in verify_status
        eq_(response.status, exp_status, msg + ": " + str(data))
      File "/Users/camerondawson/gitspace/tcm/tests/bdd/features/tcm_data_helper.py", line 47, in eq_
        assert exp == act, "\n\tExp:%r\n\tAct:%r\n%r" % (exp, act, msg)
    AssertionError: 
    	Exp:401
    	Act:200
    '/tcm/services/v2/rest/users/current: {"ns1.user":[{"@xsi.type":"ns1:user","ns1.resourceIdentity":{"@id":"2","@url":"http:\\/\\/localhost:8080\\/tcm\\/services\\/v2\\/rest\\/users\\/2","@version":"0","@xsi.type":"ns1:ResourceIdentity"},"ns1.timeline":{"@createDate":"2011-03-11T09:44:52-08:00","@createdBy":"1","@lastChangeDate":"2011-03-11T09:44:52-08:00","@lastChangedBy":"1","@xsi.type":"ns1:Timeline"},"ns1.companyId":21,"ns1.companyLocator":{"@id":"21","@url":"http:\\/\\/localhost:8080\\/tcm\\/services\\/v2\\/rest\\/companies\\/21","@xsi.type":"ns1:ResourceLocator"},"ns1.email":"WalterBishop@mozilla.com","ns1.firstName":"Walter","ns1.lastName":"Bishop","ns1.password":{"@xsi.nil":"true"},"ns1.screenName":"WalterBishop","ns1.userStatusId":2}]}'
(Reporter)

Updated

7 years ago
Severity: minor → enhancement
Target Milestone: --- → 1.0
You need to log in before you can comment on or make changes to this bug.