TimothyJones
Add mechanism to easily restrict access to users that meet given conditions
Hello, thank you very much for this great piece of work!
I have a question: I want to allow access only for members of a particular GH Team. Is it possible? How? Thank you!!!
Yes, this is possible with some extra development effort. It depends a bit on your use case.
In another project, I added the github organisation / team metadata to the userinfo response, which the app then read to determine whether access was appropriate.
Another approach could be to add a callback to the wrapper, which would be called before the cognito callback. This callback would check the teams, and either redirect to a "login failed" page on failure, or redirect to the cognito callback on success. If you go this route, a PR would be welcome (the functionality could be generally useful).
Thank you!
It turns out we have Azure AD which we need to use so I will not go this way after all.
On Fri, Jan 3, 2020 at 3:44 AM Timothy Jones [email protected] wrote:
Yes, this is possible with some extra development effort. It depends a bit on your use case.
In another project, I added the github organisation / team metadata to the userinfo response, which the app then read to determine whether access was appropriate.
Another approach could be to add a callback to the wrapper, which would be called before the cognito callback. This callback would check the teams, and either redirect to a "login failed" page on failure, or redirect to the cognito callback on success. If you go this route, a PR would be welcome (the functionality could be generally useful).
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/TimothyJones/github-cognito-openid-wrapper/issues/26?email_source=notifications&email_token=AAEYSPVZ34SHJCF6LDX4SBDQ32Q7LA5CNFSM4J7KEX52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIAD3HY#issuecomment-570441119, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEYSPXO3XZTPTBDTYPR6FDQ32Q7LANCNFSM4J7KEX5Q .
No problem. I'll leave the ticket open, because adding a way to easily add this functionality would be useful.
just for info Timothy could you possibly merge that functionality with team/organization into this project ? i am looking into this for some time and since you already did it i would like not to reinvent a wheel. or just point me to some code where this is already done (if it is public). thank you so much.
So, it depends where you do the restriction.
If you control the server(s) where you want to use your user pool tokens, then extending the userinfo to include custom claims (such as organisation) is fairly straightforward - you would do so by adding another call to the promise array here. Then, you could map the custom claim to an attribute in your user pool, and read that claim on your target server to determine whether or not to allow access. With this pattern, github users outside the organisation(s) can successfully use github to authenticate, but are unable to use their tokens to do anything. This is what I did when I implemented this feature, but because most of the code is on the server side (and out of the scope of this repository), it doesn't make sense to include it here.
If you want to prevent users outside the organisation from authenticating to the userpool at all, then you would need to add a callback URL to intercept the callback from GitHub (the wrapper currently doesn't have a callback URL, it passes the callback straight to Cognito). This callback URL would need to exchange tokens with github, call the organisations endpoint in github's API, and then determine whether to redirect back to Cognito's callback, or to a page that communicates the failure. If you go this route, I think you would also have to implement your own tokens, since cognito will want to complete the code flow. You'd also need to do something like store the github token on the token you generate, so that you can unpack it and use it when Cognito calls the userinfo endpoint.
The second option is considerably more fiddly, which is why I went with the first option when I was faced with this problem.
And of course, if you (or anyone else) sees an easier way of implementing this within this wrapper, I'd welcome suggestions and/or pull requests.
Thanks for the explanation and this super useful project. I am facing the same problem and was going to implement the first option above. Your comment gives me confidence that this approach can work.
You’re very welcome- I’m glad it helps!
If there’s anything I can do in the main repo to make your life easier, do let me know (and of course PRs are welcome if you end up making improvements that might be generally useful)