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

Documentation for Sigle Sign-On feature on commercial #7212

Merged
merged 15 commits into from Jul 14, 2020

Conversation

humitos
Copy link
Member

@humitos humitos commented Jun 22, 2020

Initial documentation for SSO on Read the Docs for Business.

My goals here are:

  • mention different types of SSO:
    • authentication & authorization (GitHub)
    • authentication (verified email address)
  • make it clear where permissions are managed
    • social accounts (GitHub, etc)
    • normal Read the Docs auth (Admins teams)
  • mention that this is still on Beta and enabled by request only
  • avoid confusions mentioning differences with "regular organizations": users only needs to know how SSO works

Rendered version: https://docs--7212.org.readthedocs.build/en/7212/commercial/single-sign-on.html

@humitos humitos requested a review from a team Jun 22, 2020
Copy link
Member

@ericholscher ericholscher left a comment

This document definitely feels half-finished. I think it shows how confusing this is with the current setup, and we likely need to think more to explain it better. There's a large number of changes I'd suggest, but in general we definitely need to do more work here.

docs/commercial/single-sign-on.rst Outdated Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Jul 6, 2020

@eric I did another iteration on this document. I tried to focus on the distinction of the two different types of SSO we currently support: Social Account and @company.com email address. Besides, I made the normal actions (e.g. grant admin access) as sections so it's easier to see and find them.

I think it's too much better than the previous one and makes things clearer. I'd like to have "better making names" for the two different types, but at least, I think it communicates clearly what the differences are. I suppose we can grow from here...

@humitos
Copy link
Member Author

humitos commented Jul 6, 2020

Note with this we will have 3 different types of Auth for organizations:

  1. Read the Docs Auth:
    • Owners
      • admin of the organization's settings
      • admin of all projects
      • can import projects
    • Admins
      • admins of projects under "Admin Team"
      • can import projects
      • every user must have to be invited to the team
    • Members
      • read access of projects under "Read Team"
      • every user must have to be invited to the organization
  2. Verified email:
    • Owners
      • admin of the organization's settings
      • admin of all the projects
      • can import projects
    • Admins
      • admins of projects under "Admin Team"
      • can import projects
      • every user must have to be invited to the team
    • Members
      • any user with @company.com verified email
      • read access to all the projects on the organization
      • there is no need to invite/add this users manually to any Team
  3. GitHub:
    • Owners
      • admin of the organization's settings
      • no admin of any projects under the organization
    • Admins
      • users with admin access in the GitHub repository associated
    • Members
      • users with read (or more) permissions in the GitHub repository associated
    • There is no concept of teams at all

@ericholscher
Copy link
Member

ericholscher commented Jul 6, 2020

@humitos I think #1 and #2 should be exactly the same, except for the membership with email.

@humitos
Copy link
Member Author

humitos commented Jul 6, 2020

@ericholscher what do you mean exactly with "membership with email"? What access a user would have after signup with a @company.com email if the organization already have 3 projects imported?

NOTE, I added "can import project" on Verified Email -> Admins, that was missing

@humitos
Copy link
Member Author

humitos commented Jul 6, 2020

The main important difference that make this feature useful (between #1 and #2) is that "users with @company.com email will have read access to all the projects of the organization" --the rest is exactly the same.

Copy link
Member

@ericholscher ericholscher left a comment

This is a much better doc, very clear 👍

In case you want a user to have access to your documentation project under Read the Docs,
that user just needs to be granted permissions in the GitHub, Bitbucket or GitLab repository associated with it.

Note the users created under Read the Docs must have their GitHub, Bitbucket or GitLab
Copy link
Member

@ericholscher ericholscher Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think GitHub, Bitbucket or GitLab is a bit verbose, we should probably just say VCS provider or something, and explain that above:

GitHub, Bitbucket or GitLab ("VCS provider")

Maybe it's better to be explicit here though?

Copy link
Member Author

@humitos humitos Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine using "VCS provider" as a general concept here I think. We use "VCS account" already in other documents, https://docs.readthedocs.io/en/stable/connected-accounts.html. Probably better to stick with that term.

I think it makes sense to mention them explicitly at the beginning maybe and use the general term in the rest of the document. It's good to avoid confusions with projects imported from outside these three providers. However, they have to be imported manually which is not discoverable in .com

Copy link
Member Author

@humitos humitos Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some small changes for this. I left it explicit where I thought it makes sense to make it explicit and used "VCS social provider" or "VCS provider" as well.

docs/commercial/single-sign-on.rst Outdated Show resolved Hide resolved
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Making the user member of any "Admin Team" under your organization (as mentioned in the previous section),
they will be granted access to import a project.
Copy link
Member

@ericholscher ericholscher Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can only import projects into that team, right?

Copy link
Member Author

@humitos humitos Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. In fact, if you are an admin, you can only import projects under the Admins teams you belong to. You cannot import projects on "Read" teams, even if you belong to --which is a bug, I'd say. See readthedocs/readthedocs-corporate#929

I'm not sure if this has to be clarified here, since it's not related to SSO. It probably fits better on https://docs.readthedocs.io/en/stable/commercial/organizations.html

Making the user member of any "Admin Team" under your organization (as mentioned in the previous section),
they will be granted access to import a project.

Note that to be able to import a project, that user must have at least **read** permissions in the GitHub, Bitbucket or GitLab repository associated,
Copy link
Member

@ericholscher ericholscher Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong, no? We can't set webhooks on these repos, and I think it isn't how it works currently right?

Copy link
Member Author

@humitos humitos Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. This is a mistake.

Copy link
Member Author

@humitos humitos Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait... This is confusing.

Currently, without SSO, if the project is public and you have read permissions on it, you can imported it, see

self.is_locked = ko.computed(function () {
if (view.has_sso_enabled) {
return !self.admin();
}
return (self.private() && !self.admin());
});
. In that case, you will be able to import the project, but the webhook won't be connected, ssh key not setup and we will show these notifications:

Screenshot_2020-07-07_19-32-25

I changed that behavior to only make is_locked = !self.admin() only when SSO is enabled because it doesn't make sense to import a project that you are not able to modify later (GitHub SSO won't give you access to Admin tab if you don't have admin permissions in the GH repo).

Summarizing,

  • current behavior with RTD Auth, allows you to import public projects with only read permissions
  • GitHub SSO, allows you to import projects only if you admin on the GH repository
  • Verified Email, same as RTD Auth

I think, for corporate, it makes sense to always force to be admin on the GH repository to be able to import a project; because the reasons that you mentioned. After imported, you will end up with a half configured project. This would change the current behavior, tho.

Copy link
Member Author

@humitos humitos Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this behavior to allow import projects only with admin permissions in the repository for Email SSO and VCS SSO.

I think, we should eventually do the same for RTD Auth and make all behave the same under corporate: "admin permissions on RemoteRepository are required to import a project".

humitos and others added 7 commits Jul 7, 2020
Co-authored-by: Eric Holscher <25510+ericholscher@users.noreply.github.com>
`autosectionlabel` creates a unique label per section. Since we have exactly the
same section name multiple times in the same document, Sphinx raises a warning.
We could `autosectionlabel_maxdepth=1` to avoid this, but that will make other
subsections to stop being labeled and we may be using them.

Renaming the section with a very similar title is a quick fix for now.

https://www.sphinx-doc.org/es/master/usage/extensions/autosectionlabel.html
Copy link
Member

@ericholscher ericholscher left a comment

We are using VCS, VCS provider, VCS repository and VCS social provider all in this single document. We need to standardize on terms, not introduce even more! I think VCS provider or VCS repository is probably best.

@@ -25,7 +25,7 @@ The best way to think about this relationship is:
.. warning::

Owners, Members and Teams behave differently if you are using
:ref:`SSO with GitHub, Bitbucket or GitLab <SSO with GitHub, Bitbucket or GitLab>`.
:ref:`SSO with VCS social provider (GitHub, Bitbucket or GitLab) <commercial/single-sign-on:SSO with VCS social provider (GitHub, Bitbucket or GitLab)>`
Copy link
Member

@ericholscher ericholscher Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VCS social provider feels even more verbose and isn't a term I've ever heard before, and we're still including the full set of 3. I'm not convinced this is better :)

@humitos
Copy link
Member Author

humitos commented Jul 14, 2020

OK. I finally used "VCS provider" when the document refers to GitHub/GitLab/Bitbucket and "VCS repository" when it refers to a specific repository associated to a Read the Docs' project. I'm not 100% convinced, but seems a little better at least :)

@ericholscher
Copy link
Member

ericholscher commented Jul 14, 2020

Looks good 👍

@ericholscher ericholscher merged commit 0c96ab8 into master Jul 14, 2020
2 checks passed
@ericholscher ericholscher deleted the humitos/docs-for-sso branch Jul 14, 2020
@ericholscher
Copy link
Member

ericholscher commented Jul 14, 2020

Excited to have this public 🎉

@humitos humitos mentioned this pull request Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants