#2055 Implement user API token (ie project-less tokens)
Merged by pingou. Opened by pingou.
instance_token  into  master

Download 2055.patch

This PR implement project-less API token allowing to generate token that aren't
specific to a single project. This make senses for actions such as:

  • Create a project
  • Fork a project
  • Flag a PR
  • Comment on a ticket or a PR

Fixes https://pagure.io/pagure/issue/1500

The tests are actually passing, jenkins is failing somewhere later and that break the test results...

rebased

After giving it more thoughts, I removed the possibility for any user to create API token allowed to flag or comment any PR/ticket as this would become way to easy to abuse.

So regular user can only now create project-less API token to create new project or to fork existing project.

Admins on the other hand will be allowed to create more powerfull project-less API token and it will be their responsability to give these API token to responsible 3rd party.

This however, will rely on the pagure-admin CLI implemented in : https://pagure.io/pagure/pull-request/2061

So this PR can be reviewed and merged as is, however a part is missing that will be added once #2061 is merged. This can be in another PR if we want to review and merge this first :)

Why were we checking for the fullname and not anymore ?

I was just trying to be consistent with other parts of the API that are directly comparing the objects. I am not sure which is best (object vs fullpath) but I just wanted to be consistent :)

Maybe if restrict is not None would be better here.

Well, in this case if restrict is None, '' or False the query won't work :)

We are using the same code in add_user_token maybe we could have a function with the redirection url has parameter ?

Yeah, i ve seen just after that restrict could be also a string :)

:thumbsup: consistency

Why do we need to set it to None here ?

we are testing for 'false', 'False', should we have similar test for 'true' and 'True' ?

Can you create a repo within a namespace using the API ? if so we might want to test that case

A few comments, other than that looks good.

The tests part are quiet repetitive, I don't know if we could find a way to refactor them, but this could be done in another PR.

Not sure to follow you entirely, this is to revoke a token while add_user_token is meant to create one

I guess I'm just making sure it's defined before the loop but I think it could be safely removed, I'll try w/o it

Sure we can :)

I'll add one :)

My comment was not clear, I meant that the code that check for the admin_session_timeout is the same in both add_user_token and revoke_api_user_token except for the url used in the redirect.

I was just thinking that making a generic function that is checking admin_session_timeout and redirects to a given url might be useful

We could consider renaming add_user_token to add_api_user_token so it follows the same pattern than revoke_api_user_token

I was just thinking that making a generic function that is checking admin_session_timeout and redirects to a given url might be useful

We could do that, but there will be quite a few more places to check :)

We could consider renaming add_user_token to add_api_user_token so it follows the same pattern than revoke_api_user_token

Ok :)

rebased

Adjusted and rebased :)

?

It increases the output a little bit (also to be a little consistent with the other test file)

LGTM :beers:

Pull-Request has been merged by pingou

Metadata