Github organization membership check
Closed this issue · 7 comments
With the current implementation, the github authentication plugin only allow to check that the user has an account in github. It would be useful to be able to check additionnaly that the user belong to an organization (provided as an additional parameter).
I would prefer it if the GitHub addition in this project has minimal functionality, as I believe it's a better idea to actually make a new package for it. That way the functionality of the GitHub oauth endpoints is not tied to updates from this project.
That's a good idea.
I think this project could use a little refactoring to expose the pieces to more easily build on top of it in a separate package. Last I looked, I think there was some duplication between the current plugins that could be removed by slicing the functions along different lines.
Maybe there's a way to capture the minimal case such that you can still reuse that and just plug in the endpoint URL (to provide scopes) and part of the fetch function (to check org, etc).
@freiric would you be interested in moving your code to a separate project, one dependent on this, and opening a new PR here with any refactorings that could make that process easier / trivial (while keeping things mostly backward compatible here)?
I am not sure where is the limit between minimal and additional functionalities. For github, I think that most user wanting to authenticate a user through github, will not want to authenticate/authorize all user of github. So they will mostly want to either authenticate against a particular org or else get information about the org through the authentication request, to be able later to authorize only user of (a) particular org(s). I think it is indeed essential to the github authentication.
For the refactoring I do agree with @pbrisbin, and I have made an attempt to have a function constructing the auth plugin taking a variable number of pair of an url and a function filling the session map form the result of the query at the url (so an rgument would be of type (Text, FromJSON a => a -> Map Text Text). This is possible but then I realized that subsequent query may need result of previous too and this make the situation much more complex.
So instead, I have defined several utilities functions to construct the request function Manager -> AccessToken -> IO (Creds m) (as used in Auth.authOAuth2). I think that these utilities functions can be moved in a central place and used in the other providers. But I have the feeling that these utilities functions are internal in nature (so not exposed to third party project).
Anyway, I understand that as project owners you prefer to avoid adding complexity, but creating a project to provide only the org-checking functionality seems overkill to me.
I am not sure where is the limit between minimal and additional functionalities. For github, I think that most user wanting to authenticate a user through github, will not want to authenticate/authorize all user of github. So they will mostly want to either authenticate against a particular org or else get information about the org through the authentication request, to be able later to authorize only user of (a) particular org(s). I think it is indeed essential to the github authentication.
Hmm, this gets into a tricky authentication vs authorization discussion.
IIUIC, we use OAuth to authenticate users via 3rd party services. Authentication is an identity check -- are you who you say you are? Being in a organization or not is irrelevant. If your site needs to authorize you, meaning provide access (or not) to something based on who you are, that's a separate concern and may or may not require accessing GitHub to check an organization. For this reason, I don't think that logic should live in this package.
As an optimization, we should provide some mechanism (if we don't already) for our end users to get the token from when we authenticated. That way, at least they don't have to request their own for the purposes of their own API calls.
We already provide the token. I still find it a bit unfortunate to force the user to implement a further call outside this api to implement such a common worflow. That said I understand you want to keep OAuth focused on simple authentication. So please, feel free close the ticket and the corresponding PR.
So, closing as out of scope.