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

Prevent device tokens with same name #24778

Closed
ChristophWurst opened this issue May 23, 2016 · 7 comments
Closed

Prevent device tokens with same name #24778

ChristophWurst opened this issue May 23, 2016 · 7 comments

Comments

@ChristophWurst
Copy link
Contributor

ChristophWurst commented May 23, 2016

From #24703 (comment):

bug? I can create several devices with the same name

Maybe we should prevent it or at least warn the user as they then see

Steps to reproduce

  1. Go to personal settings once Personal settings auth tokens #24703 is merged
  2. Create device token 'my phone'
  3. Create device token 'my phone'

Expected behaviour

See a warning

Actual behaviour

I can create multiple tokens with the same name

cc @PVince81

@ChristophWurst ChristophWurst added this to the 9.2-next milestone May 23, 2016
@PVince81 PVince81 modified the milestones: backlog, 10.0 Dec 8, 2016
@viraj96
Copy link
Contributor

viraj96 commented Mar 6, 2017

Firstly how can one create a new device? Second, do we need to check before creating whether a device with the same name already exists in AuthSettingsController.php? Or in DefaultTokenProvider.php where tokens are generated. Also, where are all active device tokens stored? Are they stored in the database? If yes then all we need to do is to query the database for all tokens of this "name" and if it exists then throw an exception. Is it the right approach?

@PVince81
Copy link
Contributor

PVince81 commented Mar 6, 2017

The best would be to adjust DefaultTokenProvider::generateToken to throw an exception if the $name is already used. Then catch that exception on the higher layers to display a proper error message in the UI.

@viraj96
Copy link
Contributor

viraj96 commented Mar 7, 2017

Okay, and how do we generate the device tokens. I dont get any option to do so in the personal settings tab.

@PVince81
Copy link
Contributor

PVince81 commented Mar 7, 2017

It's called "App passwords". On master it's in the "Security" section on the left navigation bar.

@viraj96
Copy link
Contributor

viraj96 commented Mar 9, 2017

Is this the correct way to getTokensByName ->

$qb = $this->db->getQueryBuilder(); $result = $qb->select('id', 'uid', 'login_name', 'password', 'name', 'type', 'token', 'last_activity', 'last_check') 
->from('authtoken')
->where($qb->expr()->eq('name', $qb->createParameter($name))) 
->setParameter('name', $name) 
->execute();  	
$data = $result->fetch();
$result->closeCursor();

The problem with this one is, that it gets called sometimes and sometimes it just does not get called. I can say this because I was cheking mysql.log file.
Logically this seems to be correct to me but suppose if I called the following,

$qb = $this->db->getQueryBuilder(); $result = $qb->select('id', 'uid', 'login_name', 'password', 'name', 'type', 'token', 'last_activity', 'last_check') 
->from('authtoken')
->where($qb->expr()->eq('name',$name))  
->execute();  	
$data = $result->fetch();
$result->closeCursor();

This one gets called everytime.

@PVince81
Copy link
Contributor

@phisch can you help here ?

@viraj96
Copy link
Contributor

viraj96 commented Mar 17, 2017

Sorry for being late. Was busy in college work. Well, I have been able to rectify the previous errors with some help from Phillip.
As for as error capturing is concerned, I guess they are captured in
authtoken_view.js -> _addAppPassword.
But all it does is on the event of fail, it just shows a notification without checking what kind of failure it is. I think that "Error while creating device token" is sufficient. Do we need another error message because the handler does not seem to bother about it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants