[API] Cannot revoke an authorized application

[zauberstuhl]

If I hit "Revoke" in /api/openid_connect/user_applications:

ActiveRecord::StatementInvalid (Mysql2::Error: Cannot delete or update a parent row: a foreign key constraint fails (`diaspora_development`.`id_tokens`, CONSTRAINT `fk_rails_a461603753` FOREIGN KEY (`authorization_id`) REFERENCES `authorizations` (`id`)): DELETE FROM `authorizations` WHERE `authorizations`.`id` = 1):
  app/controllers/api/openid_connect/authorizations_controller.rb:44:in `destroy'
  lib/rack/internet_explorer_version.rb:34:in `call'

[zauberstuhl]

ok after I removed the entry from id_tokens I can add and revoke apps without any problem.

I created a lot of clients without finishing the handshake. Maybe it is my dev-setup. I will test it tomorrow with a clean environment.

Please let me know if someone can or cannot reproduce it.

[denschub]

It works in my setup, but I guess we should try to reproduce this by abusing it. If we can break diaspora* by aborting the handshake, this would be a rather nasty bug.

[cmrd-senya]

Ok, here is what happens.

By default, IdToken is set to expire in 30 minutes after creation. At the same time we have the default_scope defined for IdToken. When we delete an Authorization its IdTokens are deleted by chain, but when searching them ActiveRecord applies the default_scope, so it ignores the expired IdTokens this way! Therefore the deletion of the Authorization fails.

Here is a log excerpt that shows the issue:

[2016-06-17T16:40:16] INFO  PID-4005 TID-12155240 Rails: Started DELETE "/api/openid_connect/authorizations/5" for 127.0.0.1 at 2016-06-17 16:40:16 +0000
[2016-06-17T16:40:16] INFO  PID-4005 TID-12155240 ActionController::Base: Processing by Api::OpenidConnect::AuthorizationsController#destroy as HTML
[2016-06-17T16:40:16] INFO  PID-4005 TID-12155240 ActionController::Base:   Parameters: {"utf8"=>"✓", "authenticity_token"=>"eoqlbkNFuSqZlHVkl4rdYr0t5qgAAjqGsJWrd02RopeFKg26E18VBJamaZ9H1yn/GBNBEq15N9ZnFfcnwaUxJg==", "commit"=>"Revoke", "id"=>"5"}
[2016-06-17T16:40:16] DEBUG PID-4005 TID-12155240 ActiveRecord::Base:   User Load (0.4ms)  SELECT  `users`.* FROM `users` WHERE `users`.`id` = 9  ORDER BY `users`.`id` ASC LIMIT 1
[2016-06-17T16:40:16] DEBUG PID-4005 TID-12155240 ActiveRecord::Base:   Api::OpenidConnect::Authorization Load (0.6ms)  SELECT  `authorizations`.* FROM `authorizations` WHERE `authorizations`.`id` = 5 LIMIT 1
[2016-06-17T16:40:16] DEBUG PID-4005 TID-12155240 ActiveRecord::Base:    (0.3ms)  BEGIN
[2016-06-17T16:40:16] DEBUG PID-4005 TID-12155240 ActiveRecord::Base:   Api::OpenidConnect::OAuthAccessToken Load (0.6ms)  SELECT `o_auth_access_tokens`.* FROM `o_auth_access_tokens` WHERE `o_auth_access_tokens`.`authorization_id` = 5
[2016-06-17T16:40:16] DEBUG PID-4005 TID-12155240 ActiveRecord::Base:   SQL (0.5ms)  DELETE FROM `o_auth_access_tokens` WHERE `o_auth_access_tokens`.`id` = 5
[2016-06-17T16:40:16] DEBUG PID-4005 TID-12155240 ActiveRecord::Base:   Api::OpenidConnect::IdToken Load (0.8ms)  SELECT `id_tokens`.* FROM `id_tokens` WHERE (expires_at >= '2016-06-17 16:40:16.398824') AND `id_tokens`.`authorization_id` = 5
[2016-06-17T16:40:16] DEBUG PID-4005 TID-12155240 ActiveRecord::Base:   SQL (2.1ms)  DELETE FROM `authorizations` WHERE `authorizations`.`id` = 5
[2016-06-17T16:40:16] DEBUG PID-4005 TID-12155240 ActiveRecord::Base:    (3.6ms)  ROLLBACK
[2016-06-17T16:40:16] INFO  PID-4005 TID-12155240 ActionController::Base: Completed 500 Internal Server Error in 61ms (ActiveRecord: 12.0ms)

You may see WHERE (expires_at >= '2016-06-17 16:40:16.398824') in the SELECT statement for the id_tokens table. And here is what we actually have in the id_tokens table:

INSERT INTO `id_tokens` VALUES (5,5,'2016-06-17 11:10:59','hi','2016-06-17 10:40:59','2016-06-17 10:40:59');

Commenting out the default_scope definition makes the deletion pass with no issues.

[cmrd-senya]

Yes, and about the handshake abortion, I have a feeling that it can't break anything itself.

[denschub]

Thanks for analyzing it. We should put the deletion in an unscoped block, I guess. Adding the quickfix label if someone wants to make their first contribution before the release. 🎁