I think we don't need the storage, and can always default to minimal permissions (just whoami). Is there a reason that's no good?

If you want offline_access, you just ship the user to /path/to/oauth/?scope=offline_access on your third-party website, right?

The scope stuff is mainly valuable for clients using the PhabricatorPhabricatorAuthProvider, where there is no 3rd party site and Phabricator itself ends up asking for scope. I don't think its possible to differentiate between such clients that want perm access versus not without some storage here.

In theory, in some future where we have more scopes we can keep this sort of dialogue nice and lean with this infrastructure that has been built. It seems nice for clients using other providers but not necessary unlike in the PhabricatorPhabricatorAuthProvider case.

Also, we could someday make it so if the client is only supposed to ask for scope x, y, z as registered but then asks for some other scope a big exception.

The scope stuff is mainly valuable for clients using the PhabricatorPhabricatorAuthProvider

Hmm, when would they use the option? Don't they always only need whoami?

In theory, in some future where we have more scopes we can keep this sort of dialogue nice and lean with this infrastructure that has been built.

I don't quite understand how this makes the dialog leaner. Don't we show the same number of checkboxes whether the third-party asks for "X + Y + Z + whoami" or the client is set to always ask for them?

Also, we could someday make it so if the client is only supposed to ask for scope x, y, z as registered but then asks for some other scope a big exception.

This makes some degree of sense, but in this case the list should be a list of possible scopes the third-party is allowed to ask for, not the default set?

Another concern is that if we change the client from needing "whoami" to needing "whoami + X", all installs will break instead of just prompting users to grant the extra permission (which is the behavior I'd expect).

The biggest thing for me is just that no other OAuth sites work this way that I know of, so it feels weird. It's not necessarily bad, but it seems to interact oddly with the case where we need more permissions later, and the case where we log in with low permissions (just whoami) and ask for extra capabilities later (e..g, to publish to feed). This is straightforward and always works the same way with scope=required,perms but with customizable defaults it ends up sometimes using defaults and sometimes using scope=required,perms.

Perhaps most relevant is that we ask for scope=whoami in the request in the AuthProvider anyway, so we should never hit this code?

Taking a step back, this diff is supposed to solve the base problem of "we have N scopes, but 1 should really always be defaulted, and if the client doesn't want the other N-1 anyway don't ask the user."

So this diff solves that for all values of N, although it does require the client to register as to which of the N-1 things it wants. This is mainly because one of the clients is completely under our control and I have no idea how to programmatically ask for 1 + M scopes if I don't have the M scopes saved somehow...?

Hmm, when would they use the option? Don't they always only need whoami?

Today this is true. In the future, I have absolutely no idea, and it seems like something more than whoami is inevitable and coming soon. An example could be using OAuth + scope to control which bot accounts can access which repositories? Maybe some central phaclity bot has the scope to get stats from your instance? I also thought long-term our scope was DEFINITELY going to grow to cover say the conduit API set?

In theory, in some future where we have more scopes we can keep this sort of dialogue nice and lean with this infrastructure that has been built.

What I mean by this is...
Client A wants every scope ever. The dialogue to a user to authorize client A presents every scope ever.
Client B wants no scope beyond whoami. The dialogue to a user to authorize client B presents no scope (like how its going to be working in the average case here.)
This should work like this as part of this diff if I got it right.

This makes some degree of sense, but in this case the list should be a list of possible scopes the third-party is allowed to ask for, not the default set?

The list of all possible scopes is possible at client registration / edit. However, when the user is being asked about authorizing a given client, this is now showing the subset of scopes the client specified at registration.

This is how e.g. Facebook works and as far as I know once you get bigger and are concerned about application abuse, having the client register for scopes becomes quite powerful. Facebook for example drops you into some manual review process if you ask for aggressive scopes. Unfortunately, its pretty easy to make a fancy dialogue signing your life away on some 3rd party website, and we know how e.g. Facebook gets blamed on these hyjinx...

Note also this diff doesn't actually do this enforcement or anything... all it does is "be nice" and if there is no scope parameter set then it asks for exactly the set of scope the client was configured to ask for. Maybe that should change now?

Given all that, I think if I make one of the two options in my latest inline comment we'll be good here?

Otherwise, please do request explicit changes and I'll make 'em happen. :D

I have no idea how to programmatically ask for 1 + M scopes if I don't have the M scopes saved somehow...?

You'd change this from whoami to whoami,M to request permission M.

https://secure.phabricator.com/diffusion/PHU/browse/master/src/auth/PhutilPhabricatorAuthAdapter.php;2b23ac46a8473304b68afa16c353539e18e2e9f4$65-67

This makes the third-party (epriestley.phacility.com) send the user to admin.phacility.com/oauth/?scope=whoami,M, and then we read the requested scopes, ignore whoami since we'll always grant it, and show a checkbox for whatever permission M is.

This is how all other providers work -- for example, with Google, we ask for multiple permissions: both email and profile:

https://secure.phabricator.com/diffusion/PHU/browse/master/src/auth/PhutilGoogleAuthAdapter.php;2b23ac46a8473304b68afa16c353539e18e2e9f4$75-82

Client A wants every scope ever. The dialogue to a user to authorize client A presents every scope ever.
Client B wants no scope beyond whoami. The dialogue to a user to authorize client B presents no scope (like how its going to be working in the average case here.)

This is what we want, but this should work without storage. Client A's third-party site will request ?scope=X,Y,Z, so we'll show checkboxes for X, Y and Z. Client B's third party site will request ?scope=X, so we'll only show a checkbox for X. We can figure out which checkboxes to show purely by examining the scope parameter in the request, without caring about settings on the Client object.

This is how e.g. Facebook works

It is, but Facebook and all other OAuth providers I'm aware of make this work by reading the scope parameter in the request only, not any settings stored in the Client.

I think just making scope mandatory is the cleanest way forward here, even if the spec technically says it's optional. All reasonable clients should provide it, it's easy to fix if we throw a clear error, and Google requires it, for example:

(I wasn't easily able to check if Facebook or other providers require it since I don't have enough test apps / test accounts configured locally at the moment.)

I just want to clarify that I know you can ask for new scope via get / post data, particularly in the "I want X" case. I was thinking more of the "I may want X, Y, Z.. and the dear user isn't going to be directly involved moving forward" case, which at least to me seems like something Phabricator will have in spades. I guess this scope list could be programmatically derived via other configuration - e.g. maybe all enabled applications can ask for some scope for their conduit APIs needed for offline data scraping - but its also nice if you can use the concept of a "client" to make that access more granular from an administration perspective. ("The maniphest scrape is killing the db? kill the maniphest oauth client then and we'll figure it out later.") This is all hypothetical and probably the wrong architecture given I think you think I just don't understand URL construction.

Also, Facebook really does require you to specify which scopes you are using as part of making it accessible to users, during the submission for the approval process. https://developers.facebook.com/docs/facebook-login/permissions/v2.2#review

/notcrazy :D