Instead of using ints here, you should use enums so you can refer to the error codes by name, instead of a cryptic number.

What does this do here?

It might be nice to use an enum for the ACLs as well, so you don't accidentally typo an acl name somewhere.

This is nice!

Basically the idea is to allow token authentication but not require it.

Private tickets are restricted, so if you are not authenticated, you will be blocked but if you are authenticated, it will check who you are and if you are allowed to see the ticket and behave accordingly :)

I take it back. the ACLs dict in the config is sufficient.

The whole check_api_acls function should probably be rewritten to accomodate returning either a 401 or a 403 depending on the circumstances (authn failure or authz failure). Right now it just returns a 401 for both situations (I think).

Makes sense!

I think I did this in purpose to avoid leaking if the token is valid but the user can't access/do the action vs the token is plain invalid.

Could you explain what you mean with enums? I only know this term in relation with databases.

I think this is ready to be reviewed if someone has the courage for it.

There are still changes I want to make but I will propose them separately

Question: Should the args be in a separate dict or top level like this?

I think you're missing the word "to" between %s and get

Sorry, what does this error mean?

Maybe be a bit more verbose, like "Issue tracker disabled for this project" and have it called ETRACKERDISABLED?

It's the error code used when the error message actually corresponds to the exception thrown by the backend.

So when this error code is used, use the message provided.

Indeed yes

I think you should just make sure that the error codes defined cover all possible error cases (maybe a general EUNKNOWN or something for unspecified errors).

Wouldn't a general ENOPERMISSION be better, and sue that for all types of permission issues?

I'm just not sure wat "EISSUEREST" stands for.

Same as with tracker, maybe EPULLREQUESTSDISABLED?

Maybe a more general ENOTFOUND?

This is also a permission issue.

EPERMISSION?

This would crash if I'd set a header like Authorization: token

Maybe a bit more careful handling of this?

Note that in the optional case, this means the code cannot depend on flask.g.fas_user and flask.g.token exist. Maybe set them to None explicitly if there's no (valid) token provided?

You're not checking the result here.

Maybe it's an idea to still provide an error if a token was provided but invalid?
Otherwise this can lead to very hard to debug issues where the token was expired or sent invalid, but the only thing they'll get is a permission issue, instead of EINVALIDTOK.

Perhaps use the constant instead of [3]?

I have no idea what "3" is, and I think it would change depending on the order in which stuff is in the enum.

As commented earlier, I don't think it's a good idea to send the exact Python error to clients: that's going to be able to leak more info than you imagine.

Maybe start at version 1? I know most stuff in IT is 0-indexed, but having version 0 sounds like it's not ready for production yet.

I personally think this isn't the best idea: you should just document it somewhere public.

The whole reason I suggested using error codes rather than returning error strings was so clients could "translate" (either to another language or some UI change) the error, but if the list of error codes is bound to change within the same API version, that still wouldn't work.

Nitpicky: it's "Red Hat, Inc." :-)

I think you should include a licensing header, regardless of how big it is.

This so that people can see just any file and know that it's FOSS code, even if people pull single files from the source repo.

I'm missing either of the api_login_ wrappers here.

I could imagine maybe adding private projects some time to Pagure, in which case you'd need to be authenticated to see project info.

I think they don't need to be in the response explicitly.

But if you really want to, I think a "request": {"assignee": ".....} would probably be best.

Maybe rename to pull-requests?

Please note that having this as a wrapper means it's going to be impossible to have ACLs per project for example, which would be useful.

Maybe you should consider making the wrapper just do the getting/checking of the token and setting a flask.g.token_acls, and then have a function check_acls() that checks if the ACLs the user has are enough for the current request.

Sorry, what is "user" here? I think it'd be cleared to name this "Owner".

Maybe name this "username"?

Maybe call this "requestor"? Too many "user"s to understand which is which.

Why do you need this? Just the HTTP status code should be enough.

Also, I don't see any code after the httpcode = 200 line that would ever change this?

I think "date_created" is a weird name for a timestamp. Also, wouldn't it be better to encode the timestamp as int?

Why are you providing all this info? I said I was interrested in PRs for a specific repo, I don't need all settings for the project or repo_from with every single PR I get returned. This would bulk the API result a LOT.

Another string-encoded integer

Sorry, but what does "status: true" mean?

This is a REALLY confusing error, as the token was valid.

Didn't you call this ENOPR?

No because this is used specifically for issues

Didn't you switch the two here? I think ENOTASSIG was "current user is not assignee" and ENOASSIG was "No assignee on this PR".

Maybe make the error codes differ by more than a single character?

But worth a different error message imho

Same here, giving a permission error does not explicit what permission you are missing.

Do not forget that these errors are set per project, not all project activate these features

Seems to work:

>>> h = 'Authorization: token'
>>> h.split('token', 1)
['Authorization: ', '']
>>> h.split('token', 1)[1].strip()
''

The code should depend on the authenticated() that handles this

I don't quite follow you here.

tokens are restricted to a certain project, so ACLs are per project by definition.

This sentence is a bit.. weird: it's "Pull request", and you define the API endpoint as "closing" the PR, not "cancelling". Oh, and then there's that Canceled is the US English version :)

Yup, that's a left over from when I was using numbers instead of words

It's not the python error that's thrown, it's Pagure specific exceptions whose message was manually set

We have per-project tokens instead of per-user? That's just confusing.

which would be true :)

It's still alpha-stage for now

Don't you mean to rollback and log the error?

The list of errors is bound to increase, not to change, as I see it

This is a 500-series error.

I used the same headers for all files, so this is (eventually) a separate issue

Currently it's not missing the wrapper as the list of pull-requests is public.

For the use-case you present (and that isn't supported at this point) it would use the optional login wrapper :)

Could be adjusted true

It means it's open

The UID is stored as a string

But not for this repo

That's a different error

Sure, but I would think you'd keep the definition of the API itself stable, even if the implementation changes?

Well, you're EUNKNOWN is my ENOCODE

agreed

Well, within an API version, I think it would be bad to add new possible errors, at least for existing endpoints, since that would make implementing a client very difficult.

Not sure anymore for EISSUEREST either ^^

I do not check the result because I do not care about the result.

The idea is that the user may or may not provide a token and if there is one and it is valid, great, authenticated() will work and return the user, otherwise no problem either

I want them in as they explain how the data was obtained/filtered

It's relying on: date_created.strftime('%s')

ENOTASSIG is issue is not assigneed while ENOASSIG is current user is not the assignee

ENOASSIG = 'Only the assignee can merge this review'
ENOTASSIG = 'This request must be assigned to be merged'

I tried to keep the error code short but maybe I shouldn't

Well, closing a pull-request without merging it is basically canceling it :)

(Note: my firefox does not like cancelled while it accepts canceled)

It's per user per project :)

Yup thanks

I've been trying to avoid a plain 500 to distinguish between error we send and internal server error happening because of a bug in the application.

Any suggestions then?

I'll do the switch when we'll consider the API working and complete enough to be publicly used and built upon, atm this is really early stages I think

agreed, but I also expect that the API will grow and eventually get new error codes.

agreed

No error in case an invalid token was provided.

This message is kind of wrong: 1. It's a "Pull request", not a "Request pull", 2. The API is called "pull-request/../close", not "pull-request/../cancel"

No app.logger call?

I think you should wait with committing until the very end of the request, when everything passed. Otherwise you might still get in a partial correct state, which is exactly what transactions try to avoid.

I still don't really agree to the request params being in the response body.

Doesn't it error out at another time?

What do you mean?

Let's make a ticket of this so that we can gather more feedbacks

ok will fix

Thanks, will fix

Ok moved down the stack, but I replaced it with calls to SESSION.flush() to find error early

I will change this, I was wondering the same

@puiterwijk has approved this PR on irc and will comment on it later.

Let's merge this and iterate further on it. +1 on v0.1.

:confetti_ball: 正しい

Commit 2d403c3d fixes this pull-request