Details
- Reviewers
csteipp 20after4 btrahan - Maniphest Tasks
- T4251: Auth provider for Bitbucket
T5096: Create a phabricator authentication provider that uses mediawiki oauth - Commits
- Restricted Diffusion Commit
rP2d36afeaab7c: Manage OAuth1 request token secrets in core OAuth1 workflow
- OAuthed with Twitter.
- OAuthed with JIRA.
- OAuthed with some Bitbucket code I had partially laying around in a partial state, which works after this change.
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
This looks really good. So it looks like we can drop the cookies and use this instead?
Yeah, this should just automatically take care of storing the request secret and then providing it to the Adapter before it finishes the handshake, so you can remove the cookie stuff. Let me know if it doesn't work with the Mediawiki stuff, I still need to do a local install so I haven't been able to actually test in live code yet.
| src/applications/auth/provider/PhabricatorAuthProviderOAuth1.php | ||
|---|---|---|
| 79 | @epriestley I can't get it to work for me unless I override verifyAuthCSRFCode in my subclass with an empty method. Isn't the CSRF check redundant now that we save and check the token secret? | |
I think it's not redundant for JIRA, which uses RSA-SHA1 without the secret being part of the signature.
Let's add a method like shouldAddCSRFTokenToCallbackURI() to the base OAuth1 adapter in libphutil, default it to return true;, have it return false; for MW, and then do the CSRF token parts here conditionally (marked inline)?
| src/applications/auth/provider/PhabricatorAuthProviderOAuth1.php | ||
|---|---|---|
| 55–57 | This, and.. | |
| 79 | ...this. | |
Well, I guess it's sort of redundant since we always require the token to load from the DB, but I still think we should retain it for providers which support it. I think it raises enough barriers that it's worth the modest increase in complexity.