Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicacate client_ids and missing client_ids #348

Open
gene1wood opened this issue Dec 11, 2019 · 4 comments
Open

Duplicacate client_ids and missing client_ids #348

gene1wood opened this issue Dec 11, 2019 · 4 comments
Labels

Comments

@gene1wood
Copy link
Contributor

When there are mutiple records in apps.yml with the same client_id, the Auth0 rule logic (in AccessRules.js) will

  • permit access to the RP for users that are members of the intersection of permitted groups in every apps.yml record with that client_id
  • deny access based on expire_access_when_unused_after if any of the records indicate expiration

This applies to two clients

  • smKTjsVVxUJDEkjIftOsP0bop2NWjysa Google
  • o2e391VjmnPk0115UedNTmRL8x2nySOa people.mozilla.org

I recommend we remove the client ID for the duplicate entries

There are 12 RPs which exist in apps.yml but have no client ID

  • One is Account Portal which is not an RP.
  • MDN uses GitHub for auth
  • 6 are Egencia which use user groups to control what is displayed in SSO but Egencia handles auth
  • Expensify uses user groups to control what is displayed in SSO but Expensify handles auth
  • Netflify which is a duplicate of a similar entry with a client ID
    • The duplicated record which was added in Add configuration for Netlify #298 likely intended to restrict access to just team_sre but because it has no client_id everyone at Mozilla can use the RP and is added automatically as a collaborator granting them rights to create sites
    • We should unify these and re-constrain access to team_sre
  • New Relic
    • IT SRE which is a duplicate of a similar entry with a client ID
      • This duplicate was added in Add NR logo to it-sre team members #304 and explicity asks for different groups to be able to access the RP and be able to see the sso tile
      • This use case may highlight a shortcoming in the apps.yml schema
    • New Relic EIS
    • Other New Relic entried do have client IDs
    • If New Relic takes care of authentication shouldn't these all not have client IDs?

How can we annotate entries in apps.yml which have no client ID because the RP handles authentication?

@gene1wood
Copy link
Contributor Author

@gdestuynder @gozer Was the intent of #298 to grant only team_sre rights to Netlify?

@gdestuynder
Copy link
Contributor

  1. "duplicate entries"
    This is on purpose. The dashboard code does not handle displaying multiple icons for different search terms, or matching the same icon for different search terms. Eg if you search people or phonebook, it is the same RP. If you would like a prettier "fix" this has to be fixed in the dashboard code to support such a feature.

  2. "RPs without client_id"
    Most of these clients are also shown on the dashboard but are not behind SSO, and therefore also have no client_id. They're there for convenience. This is not a bug.

Note that because it's 2 stage auth, clients like New Relic should have a first check (for "team_moco" for example) even if New Relic itself does additional checks.

Because of the amount of text I'll make another comment about what could be actually improved

@gdestuynder
Copy link
Contributor

What could be improved:

  • netlify (first entry) could-should have a client_id set - i suppose that is historical (we used not to list the client_id and only what we now call "stage 2 authentication" was supported)
  • new relic IT client could-should have a client_id set - same thing

cc @jdow

@sciurus
Copy link
Contributor

sciurus commented Feb 4, 2021

cc @the-smooth-operator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants