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

feat: make rafiki admin auth optional #2883

Merged
merged 8 commits into from
Aug 26, 2024
Merged

Conversation

JoblersTune
Copy link
Collaborator

@JoblersTune JoblersTune commented Aug 22, 2024

Changes proposed in this pull request

  • While auth is enabled by deafult, you can now run Rafiki Admin without auth.
  • Included another docker-compose to include auth changes and provided a new command in package.json.

Context

Fixes #2806

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Documentation added
  • Make sure that all checks pass
  • Bruno collection updated

@github-actions github-actions bot added pkg: frontend Changes in the frontend package. type: localenv Local playground labels Aug 22, 2024
Copy link

netlify bot commented Aug 22, 2024

Deploy Preview for brilliant-pasca-3e80ec canceled.

Name Link
🔨 Latest commit 715fe11
🔍 Latest deploy log https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/66c71a5f02c83000088751c7

@JoblersTune
Copy link
Collaborator Author

JoblersTune commented Aug 22, 2024

I assume I should remove all auth stuff from the localenv README then? No mention of Kratos, or mailslurper etc. Change the architecture diagram etc.?

I guess my hesitation here is because to lose that information here makes me feel like we need it elsewhere then and this becomes a bit of a bigger docs discussion. Perhaps it's needed in the bigger architecture diagram instead of just in localenv? This may be true regardless but it also means that we don't have visual access to the ports for accessing e.g. mailslurper anymore.

Perhaps we need to break this out into a docs issue?

@JoblersTune JoblersTune marked this pull request as ready for review August 22, 2024 09:31
@JoblersTune
Copy link
Collaborator Author

@melissahenderson and @brad-dow please just have a look at the README updates. Is it still okay to publish these while we're on a docs hold?

Also note, we'll need these changes reflected in the Local Playground, and Rafiki Admin docs when they're updated as well.

@JoblersTune
Copy link
Collaborator Author

A lot of the changes are repeated because I consolidated all the new logic from the redirect functions into a single checkAuthAndRedirect function in packages/frontend/app/lib/kratos_checks.server.ts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You'll see some custom logic here and in the sidebar component because the logout button and account settings menu item should only appear if auth is enabled and a user is logged in.

@JoblersTune JoblersTune force-pushed the 2806/sj/make-kratos-optional branch from 959a1c0 to fc1c526 Compare August 22, 2024 10:33
@JoblersTune JoblersTune force-pushed the 2806/sj/make-kratos-optional branch from fc1c526 to adbb734 Compare August 22, 2024 10:44
@melissahenderson
Copy link
Contributor

@JoblersTune (and @brad-dow) I think it's OK to push the changes to the README since it's outside of /documentation, but it's probably best for @huijing to chime in. I made the doc-freeze announcement yesterday on her recommendation. The writers have an action item to discuss all the READMEs during the work week. I'll make a note to ensure we cover the local playground and Rafiki Admin as well.

@JoblersTune JoblersTune self-assigned this Aug 23, 2024
@huijing
Copy link
Contributor

huijing commented Aug 26, 2024

I say go ahead and merge this. When the README discussion happens, we make sure all agree which bits we remove (if at all, because I don't really foresee major changes to the READMEs, maybe just tweaking of wording here and there)

@JoblersTune JoblersTune merged commit 94c9140 into main Aug 26, 2024
42 checks passed
@JoblersTune JoblersTune deleted the 2806/sj/make-kratos-optional branch August 26, 2024 10:39
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@melissahenderson the local playground changes would be something along these lines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@melissahenderson and these kinds of changes in the Rafiki Admin docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: frontend Changes in the frontend package. type: localenv Local playground
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Kratos optional for the Admin UI
4 participants