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 role auth #32

Merged
merged 24 commits into from
Nov 19, 2024
Merged

Add role auth #32

merged 24 commits into from
Nov 19, 2024

Conversation

therealmarv
Copy link
Contributor

@therealmarv therealmarv commented Nov 13, 2024

https://openstax.atlassian.net/browse/OTTER-36

It's in regards to the 403 pages a MVP. But the focus is more that members cannot be researchers and vice versa (protect routes). Also non logged in users should not access any member or researcher pages (which before this PR was possible). The 403 page could definitely look "nice" or "styled". If that's needed on top I would prefer to built on top of this PR.

@therealmarv therealmarv changed the title wip: Add role auth Add role auth Nov 13, 2024
@therealmarv therealmarv requested a review from a team November 13, 2024 22:36
@therealmarv
Copy link
Contributor Author

therealmarv commented Nov 13, 2024

It's ready for review after many refactoring and boiled down to the agreed essentials only.

Things which should be improved for future (after pilot?):

  • Tests for middleware because it's such a critical piece but excluding&mocking Clerk (I'm currently trying on how this can be achieved).

Things which could be improved:

  • Nice looking 403 page

Open question for after pilot:

  • UI and UX flow of user organisation (=role) selection.

Copy link
Member

@nathanstitt nathanstitt left a comment

Choose a reason for hiding this comment

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

Looking great Marvin, see what you think about the logs and using org slugs

// Define user roles
const userRoles = {
isAdmin: orgId === SAFEINSIGHTS_ORG_ID,
isOpenStaxMember: orgId === OPENSTAX_ORG_ID,
Copy link
Member

Choose a reason for hiding this comment

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

I think we should use org slugs vs ids. The ids will change across clerk instances and if we ever accidentally destroy an org and recreate it later, but we can control the slugs so they'd stay the same.

},
}

export default logger
Copy link
Member

Choose a reason for hiding this comment

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

I like this, but maybe we pull in a "real" logging library that allows filtering and prefixes? I've used https://www.npmjs.com/package/debug before and liked it's functionality

Copy link
Member

@nathanstitt nathanstitt left a comment

Choose a reason for hiding this comment

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

looks great, thanks @therealmarv !

@therealmarv therealmarv merged commit 82de4fe into main Nov 19, 2024
1 check passed
@therealmarv therealmarv deleted the add-role-auth branch November 19, 2024 15:46
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