feedback on the authentication implementation

overview

I have been working @slajax to implement authentication using slack + github + jwt and we've run into a number of points of friction that I think could be improved to help users integrate authentication more easily

problems

when implementing our slack strategy it was not clear that changing the number of arguments in side your verifier's verify method would expose params, we needed this for slack web hook urls but it is also commonly needed in other circumstances where profile data doesn't include certain things
when implementing slack, we were forced to implement two different auth endpoint flows because slack does not allow us to mix scopes for app and user identity. This was hard because each endpoint is app.configured statically and it would be a lot easier if we could combine them to one endpoint that could over-ride scopes, or redirect urls based on a parameter. Currently only certain config options are included in "INCLUDE_KEYS" and therefore aren't even configurable at startup.
when implementing the custom verifier for our slack strategy we used the jwt strategy with a cookie to persist our session between the flows. That said, we ran into a number of issues with how the example verifier made assumptions for mapping the user (ie:this.service.id vs options.idField) which made the serialization and deserialization logic of our user entity inconsistent based on which user flow (existing or new) was selected. I think there could be some improvements for how the entity id is configured, we ended up hardcoding everything to always useoptions.idField which allowed us to carry the convention we configured at startup, the whole way through the process and we found that easier to manage because it allowed to have one find / update / create statement within our verifier, so the execution flow was more consistent to debug. At very least it'l lower the ramp needed for users to configure custom logic in their verifiers perhaps.

We'll take a look at how we can implement our ideas in pull requests, but we just wanted to get this issue open for the consideration of @daffl @ekryski and @marshallswain's conversation about auth. xoxo fayez + kyle

Hey @fayezosaadi! Thanks for reporting! @slajax was mentioning you guys were having some trouble and that you had a couple different use cases that may not have been considered.

I have a couple follow up questions/clarifications:

when implementing our slack strategy it was not clear that changing the number of arguments in side your verifier's verify method would expose params

With that do you mean this line? Or do you mean the actual verify method signature?

That said, we ran into a number of issues with how the example verifier made assumptions for mapping the user (ie:this.service.id vs options.idField) which made the serialization and deserialization logic of our user entity inconsistent based on which user flow (existing or new) was selected

Could you clarify a bit more? What were you using as ids?

when implementing slack, we were forced to implement two different auth endpoint flows because slack does not allow us to mix scopes for app and user identity. This was hard because each endpoint is app.configured statically and it would be a lot easier if we could combine them to one endpoint that could over-ride scopes, or redirect urls based on a parameter....

Yeah that's the main one that @slajax mentioned that I don't think we considered when designing this. Definitely could be an oversight. However, I'm also not totally following why having 2 endpoints is necessarily a bad thing. Having dynamic routes and redirects seems like it would be harder to test and debug.

With slack if you were, say, having one scope for enabling webhooks and another for enabling a bot could you not have:

/auth/slack/webhooks
/auth/slack/bots

You can set the urls to be whatever you like. You would configure the Slack passport strategy twice, each time with your routes and the appropriate configs with the differing scopes. I think your callbackUrl would need to be the same for both configs though.

Likely you'd then have 2 separate custom verifiers that behave the way you'd want each one to based on the scopes you specified and the data that Slack is returning.

💯 we completely appreciate this feedback. Just trying to get a better sense of what you gus have been trying to do, what you ran into, and whether this is an architecture issue/oversight or a documentation issue. 🍻

I'll chime in with my understanding of our experience - keep in mind, much of this was probably driven by the fact that we didn't explicitly understand how different auth settings impacted the underlying logic, rather then the code itself and to that point, I think much of what is needed is simply some documentation and examples of more complex auth setups perhaps.

With that do you mean this line? Or do you mean the actual verify method signature?

Actually we mean this line. I forget where we ended up, but there is a magic method down the chain in either passport-slack or passport itself (I think the later) which counts the numbers of arguments defined in this method (kind of like express does for error handlers) and returns back data that matches the number of arguments you define. I'll try to find this but essentially if you define 5 arguments instead of 6, it omits "params" which is where slack specifically stores all the info you need to integrate with them via a webhook url. Without it, all you get is profile info.

Could you clarify a bit more? What were you using as ids?

this logic didn't hold up real well for our use case for some reason. Specifically this, this and this - perhaps due to our approach of trying to combine multiple instances of the same strategy with the same profile keys, while also serializing our jwt to a cookie. What we found was that the logic that resulted from our configuration attempts was serializing {userId: 1} into the payload, but then when it went to deserialize in subsequent requests, it would query for { slackId: 1} which didn't exist. We went through and hardcoded everything to always serialize / deserialize based on the logic of { "${options.idField}": profile.id } which meant that the logic started consistently looking up the remote id on the remote id field. Again, it's entirely possible we put ourselves in this situation, but the reason we got there was that there are a few different ways to define the ID mappings here and not all of them we had control over without over riding the verifier. I'm suggesting, it may be a bit more simplistic for people to understand if we always followed the idField (or something else) which can be controlled from app.configure. There are also two, separate, blocks of logic that handle the user update / creation and once we followed a consistent method for mapping these keys, we were able to combine them into one (the lower one) and simplify our flow in a way that was consistent for every login. I think the second block does everything needed to create or update the user, but in our case the earlier return'd block was being executed when we didn't expect it to. Lastly, we also had to write custom code within the verifier to deserialize the JWT from a cookie and populate the user. Based on the session: true parameter, we were surprised we had to handle this logic so that's definitely a dot where we can connect a line for our feathers users.

With slack if you were, say, having one scope for enabling webhooks and another for enabling a bot could you not have:

Yes, we did do this in the end, however debugging everything needed to get there was complicated by the above issue with relation to mapping foreign IDs as both these auth flows were mapping to the same user, but if you follow the name / path convention explicitly, the logic downt he line diverges them so that they result in different property mappings: firstAuthId, secondAuthId etc, IRC, somewhere down the line this is based on a mix of options.name and the other considerations discussed above. Ultimately It wasn't very clear how setting the different variables in the app.configure would impact the combined effect of these two auth flows so we ran into a lot of issues trying to get them to both share the same slackId. Further complicating the issue is that when you use different scopes you get different data back and therefore one auth flow would clobber the data of the other. We managed to reconcile it all by moving the data that was different to a new service via a user hook, but it took a few days of messing with example code, debugging deep down the dependency tree (feathers-authentication-oauth2, feathers-authentication-jwt, passport-slack, passport etc) to really get a solid understanding of how the abstracted properties & custom verifier all cascade down into passport to get the desired outcome. When we started simplifying the verifier logic it in the ways we mentioned above did get much easier to manage as there weren't different abstracted logic paths for new / existing users.

Sorry this is such a terrible brain dump with literally no code or tangible value, but we can chat on slack to help clarify. I just wanted to try to capture as much as I can recall while it is fresh.

One other small nuance that is a bit slack related. When you first auth a slack user, you cannot auth them on the application scopes, until you have auth'd them on the identity scopes. That said however, it seems you can't combine these scopes, so we find ourselves needing to chain both of these auth flows and as a result we're desperate for a ?next=/auth/slack/webhook type of interface to the auth endpoint so we can chain them ideally. Obviously this is an edge case, but it got us thinking about how it would be nice to just be able to do something like; /auth/slack?scopes=profile&next=uriencode("/auth/slack/webhook?scopes=bot,commands") ...and boom - override the default settings, on the fly like a boss.

Yeah I think I follow @slajax. Thanks for all that. I think a lot of it definitely has to do with lack of documentation and best practices. Also, reading up on how Slack does OAuth has helped me understand the pain. The way they do things is a bit different than a lot of other providers.

Did you guys happen to take a look at this module? https://github.com/aoberoi/passport-slack. Looks like it has better support for dynamic scopes.

I also started to brain dump some stuff into a couple issues:

We're going to turn them into blog posts and/or guides. Maybe they will give you guys a hand in the interim. Overall we're not really doing much outside of passport (especially in the auth plugins like feathers-authentication-oauth2) so it's important to remember that you don't really need to use them if your flow is going to be really custom. You can just use the Passport Strategy like you would a normal Express app and write the user look up/consolidation code yourself.

?next=/auth/slack/webhook

I feel like that would get pretty messy. Likely you'd still want the server to validate that those scopes can be requested by that user.

@ekryski yes we're using passport-slack, we also tried the off brand one that is out there. We can definitely use the passport stuff directly, but we wanted to make sure we follow the "feathers way TM". Falling back to the traditional express way isn't ideal, but definitely an option to move things forward. Luckily we've got to a place where things are working now and we don't need to.

I believe most of these points have been addressed with Feathers v4 authentication. You can more easily customize all oAuth strategies and with no longer relying on Passport it has many less moving parts. The new guide shows how to set up basic oAuth and the oAuth API docs explain the strategy methods and how to do account linking and cross-domain redirects.

feathersjs / feathers

feedback on the authentication implementation #1341

overview

problems