Page MenuHomePhabricator

Allow bot users to see email addresses of users
Closed, WontfixPublic

Description

Some bots need to associate accounts between two different services, and occasionally usernames differ for the same user between those two services. It would be cool if the user.query conduit method would go back to returning a primaryEmail key when responding to bot users so bots can associate users by email address.

Event Timeline

kemurphy created this task.Feb 5 2016, 12:39 AM
chad added a subscriber: chad.Feb 5 2016, 1:16 AM

We don't take feature requests without understanding the root problem. Please see Describing Root Problems. We have many systems planned for integrations with other products, but without knowing what you want specifically to accomplish, we're unlikely to be able to actually help.

I believe I described the root problem already, but just to be extra super clear:

My company's bot listens to the phabricator feed and forwards certain stories to all subscribers in slack via direct message. We just upgraded phabricator from a version right before primaryEmail was removed from the user query. Before the upgrade it was easy to assosciate phab users with slack users because we could match email addresses, but now we have to rely on usernames and some of our slack usernames don't match phabricator.

chad added a comment.Feb 5 2016, 5:26 PM

I believe I described the root problem already, but just to be extra super clear:

Yeah, I understand people think that "feature request" typically means "let me tell you what to build into your product" but we don't work that way, as we try to have a long term plan for Phabricator, and that means understanding root problems and not just the button/widget/code you'd like to see.

We'd still like to know the root problem of what information you're passing to Slack and why it's helpful.

chad added a comment.Feb 5 2016, 5:30 PM

For example a root problem might be:

We want to ping developers in Slack when a build fails in Harbormaster.

chad added a comment.Feb 5 2016, 5:38 PM

I'd recommend reading up on feed-hooks and Doorkeeper as well (see T5462 for an overview).

chad added a subscriber: epriestley.Feb 5 2016, 9:49 PM

I don't know the history of that field, but reading the previous diffs makes it sound like this was a mistakenly added "feature" in D11791 and "fixed" in D12848. @epriestley could answer those specifics, but my guess is that field won't be re-considered for Conduit. I imagine the solution would be Doorkeeper / Oauth for Slack. In the meantime you can crib notes from those diffs to patch in the functionality you need locally.

epriestley closed this task as Wontfix.Feb 5 2016, 9:55 PM
epriestley claimed this task.

Yeah, this feature was a mistake and the data should never have been disclosed. We consider user email addresses private. I don't plan to expose this information again.

You can write a kemurphy.gimmie-all-dem-emails Conduit API method by dumping a file in src/extensions/ (see Adding New Classes) or fork locally and restore the field, or query the database directly.

If you want first-class upstream support without an extension or fork, file a task describing your root problem (probably something in the vein of "Slack integration"). To set expectations, I'd be surprised if we pursue Slack integration this year. Upstream support would involve connecting accounts via OAuth, not via email address similarity.

The plugin approach will probably work fine for us. Thanks @epriestley.

@chad, I was not trying to tell you what to build into the app, but I find it hard to describe a cause more root than "there's no way for a bot to associate user accounts across two different services". My particular application uses Slack but this is not a Slack-specific feature request.

There's no problem getting the data we need out of Phabricator to pass to Slack. The problem is for corporate deployments Phabricator no longer exposes what could be the only attribute unique to employees when querying users.

The OAuth solution is silly because it requires each user to manually associate their two logins. A metadata field in Phabricator, or Slack, or $SERVICE, or a command that the bot listens for are equally silly for the same reasons. A reasonable upstream solution might be only enabling this functionality for ldap logins and exposing a configurable set of ldap keys in the user.query method. Or letting the server administrator pick what they think is a better default for their deployment (e.g. expose emails always, expose emails only to bots, never expose emails).

chad added a comment.Feb 6 2016, 1:55 AM

I'm genuinely curious what you use the integration for. I am interested in more integrations down the road and knowing what installs are really doing with it is helpful.

It's a very simple hook to listen to the Phabricator feed, figure out who is subscribed to the object that is the subject of the feed, and ping them in Slack. The goal was to make it stateless so it could be used as an AWS lambda. It's done wonders for our review response time.

avivey added a subscriber: avivey.Feb 6 2016, 2:53 AM

Basically, an alternative Notification implementation?

ofbeaton added a subscriber: ofbeaton.EditedFeb 21 2017, 9:30 PM

I realise this is a wontfix, but you kept asking for root problems.

I ran into this old feature request when looking for a way to email a custom maniphest summary report to users. I have a weekly summary of activity generated by a CRON'd CLI command using the api token.

First I looked for a way to email users through conduit, but didn't find anything (maybe I'm blind), then I looked to see if I could get the emails myself from conduit and do it myself, but no dice.

I'll likely be querying the db directly for now, and maybe doing a conduit extension in the future.

There's probably also a way to make it a CLI command of phabricator as some kind of extension, but as we're not intimately familiar with the internals of phabricator, this made us nervous. It would also make us coupled to phabricator's CLI framework in a way a 3rd party tool does not.