debtcollective / parent

1 stars 0 forks source link

Move authenticated links logic to handled by the Header #210

Closed orlando closed 6 years ago

orlando commented 6 years ago

Right now we are handling which links should be shown given the session in the tools (by checking the user.admin flag). The header has already the functionality to handle this, and works by passing a roles:['required_role'] attribute to the links configuration

var headerLinks = [
      { text: 'Admin', href: '/admin/disputes', roles: [!{JSON.stringify(adminRole)}] },
];

We will leave the session in the tools as a way to update the user data when the user tries to access an authenticated route.

Tasks

Acceptance criteria

sarayourfriend commented 6 years ago

How will the user parameter work with in the power report and landing pages? The logout might also be unnecessary... The header already has the tools-endpoint parameter which the header uses to hit both the discourse logout and dispute-tools log out endpoints: https://github.com/debtcollective/dc-vue-header/blob/master/src/services/ProfileService.js#L36

sarayourfriend commented 6 years ago

@orlando @shoshber Can y'all give some input here? I'm fairly certain the "user" part of this ticket is technically impossible given how the power report and landing pages are implemented.

orlando commented 6 years ago

@marcondag yeah, for this we can leave it like it's working now. The user attribute should be used if it's present. I don't want to remove that functionality from the header as it allows us to work with backend-less apps

sarayourfriend commented 6 years ago

Will we move it out of the community release milestone then?

orlando commented 6 years ago

no, the tools needs this, I'll work on this today

sarayourfriend commented 6 years ago

Why do the tools need this?

orlando commented 6 years ago

because the tools handle their own session

berleant commented 6 years ago

Let me see if I understand this right with an example:.

  1. User logs in on discourse.
  2. User enters tools site. They are automatically logged in.
  3. User logs out of tools site.
  4. User is still logged in to discourse.

Is that what you mean, @orlando ?

sarayourfriend commented 6 years ago

Yes, except the "automatic" logging in will only happen after they have hit an authenticated route. Which is why we let the header handle whether to show authenticated links, so that the tools do not have to log you in. Otherwise how will the admins be able to get an authenticated session in the tools?

I am fighting with this constantly locally. If you log into discourse and then go to the tools without hitting a route that requires you to be logged in, the admin header links will not show. The only routes you can hit without being authenticated in the dispute tools that will then force authentication is to create a new dispute.

Or, we could use the utilities that the header already has, not have to make any changes whatsoever to the header and have the admin routes display in the header.

Otherwise we will also need to add an extra step to have people manually log into the tools even though they already logged into discourse. And then they'll be redirected back to discourse where they then can log in or create an account.

If they have to create an account, by the way, then they will not be routed back to the tools after completing that process and have to go back to the tools and do it all over again to be able to log in.

None of those problems are true if we simply remove the logic in header.pug in the tools to exclude links based on whether the tools have themselves authenticated you. Just pass all the links and the group names like the header is designed for.

Unless you can answer how you're planning to solve the UX nightmare I described above (like, seriously, use the app locally and tell me it's not a horrible experience if you try to a) create an account starting from the tools and b) log in as an admin to discourse without having a session in the tools and get to the admin backoffice) then I really do not think we should spend time making the changes proposed in this ticket.

berleant commented 6 years ago

Thanks for explaining that user experience so clearly.

orlando commented 6 years ago

So after discussing with @marcondag, we can solve this by changing the logic in header.pug to have a roles attribute for every link that needs to check roles to be displayed (ex: Admin link or My Disputes), and the header will use filterLinks to only the ones that the user has access (based on the roles the user has in Discourse).

We will still keep the session in the tools, but only for updating user data since it will go through the SSO flow when the user tries to access an authenticated route.

sarayourfriend commented 6 years ago

These are the two links we need to change and move into the regular headerLinks array.

{ text: 'Admin', href: '/admin/disputes', roles: ['dispute_pro'] }

{ text: 'My Disputes', href: '/disputes/my/', roles: ['trust_level_0'] }