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

Add oauth2 authorization #1626

Merged
merged 1 commit into from
Jun 5, 2023

Conversation

joachimmathes
Copy link

Hi there,

the oauth2 provider is missing a basic authorization concept. That's why I propose this PR. The implementation is based on the saml provider:

// check authorization if needed

  1. I added an attribute to specify the claim which is supposed to hold an array of strings of role names: CMD_OAUTH2_ROLES_CLAIM.
  2. I added an attribute to specify a dedicated role which has to be included in the role claim of the ID token to become authorized.
  3. Besides that I decided to allow another optional user profile attribute as id, which overrides userProfileUsernameAttr, if set: userProfileIdAttr.
    I introduced that attribute because a username might be unique, but nevertheless it is also prone to change. Imagine usernames which are built from parts of your first and lastname, either automatically or due to company guidelines. If people want to change their name, because of marriage or whatever reason else, the username no longer provides a stable ID. Thus, it's not only uniqueness but also immutability, which makes a good ID, e.g. a uuid provided by another claim

Example configuration

CMD_OAUTH2_ROLES_CLAIM = roles
CMD_OAUTH2_ACCESS_ROLE = role/codimd
CMD_OAUTH2_USER_PROFILE_ID_ATTR = user_uuid

Please note: I am no JavaScript developer. So, the code style of my modifications is merely an educated guess. 🙂

Signed-off-by: Joachim Mathes <[email protected]>
@jackycute
Copy link
Member

Thanks @joachimmathes

@jackycute jackycute merged commit be1f4cf into hackmdio:develop Jun 5, 2023
@stanley2058 stanley2058 mentioned this pull request Dec 26, 2023
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.

2 participants