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: Copy ACS URL for SAML configurations to clipboard. Disable editing SAML configuration names #4494

Merged
merged 7 commits into from
Aug 20, 2024

Conversation

rolodato
Copy link
Member

@rolodato rolodato commented Aug 13, 2024

Thanks for submitting a PR! Please check the boxes below:

  • I have added information to docs/ if required so people know about the feature!
  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have used a Conventional Commit title for this Pull Request

Changes

Disable editing SAML configuration name

Changing a SAML configuration name has the following effects:

  • "Remember this SAML organisation" no longer works for users that previously logged in with this configuration, locking users out until they can figure this out
  • ACS URL changes, breaking any previously-configured IdPs which have whitelisted this URL
  • SP metadata XML changes, which can cause confusion if it was already downloaded before the name changed

Preventing changes to the configuration name from the UI prevents these footguns, while still leaving an escape hatch open to change names from the API or Django Admin. A better solution would be to use fixed SAML configuration IDs (e.g. numbers) in the ACS URLs instead of the configuration name, but this is a good stopgap until we can implement that. For example:

https://flagsmith.example.com/api/v1/auth/saml/1234/response/

instead of

https://flagsmith.example.com/api/v1/auth/saml/my_config_name/response/

Add button to copy ACS URL

The ACS URL is only currently available in documentation and requires figuring out the API URL and manually building the URL, remembering to include trailing slashes, etc. This PR adds a button to copy the correct URL.

I decided to add a button in the list of configurations instead of showing the full URL inside the edit modal because it's easier to implement.

Unfinished: The API URL used is just whatever URL is configured for Flagsmith-on-Flagsmith. For example, in my dev environment this resolves to https://api.flagsmith.com/api/v1/ but should be http://localhost:8000/api/v1/. I haven't figured out how to get the correct URL yet.

This still looks bad but works:

image

How did you test this code?

Manually - create a SAML configuration and look at it from the frontend.

Copy link

vercel bot commented Aug 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 19, 2024 7:08pm
flagsmith-frontend-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 19, 2024 7:08pm
flagsmith-frontend-staging ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 19, 2024 7:08pm

@github-actions github-actions bot added front-end Issue related to the React Front End Dashboard api Issue related to the REST API docs Documentation updates labels Aug 13, 2024
@rolodato rolodato added feature New feature or request and removed api Issue related to the REST API docs Documentation updates labels Aug 13, 2024
@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels Aug 13, 2024
@rolodato rolodato changed the title feat: Copy ACS URL for SAML configurations feat: Copy ACS URL for SAML configurations to clipboard Aug 13, 2024
@github-actions github-actions bot added docs Documentation updates feature New feature or request and removed feature New feature or request docs Documentation updates labels Aug 13, 2024
Copy link
Contributor

github-actions bot commented Aug 13, 2024

Uffizzi Preview deployment-55232 was deleted.

@rolodato rolodato changed the title feat: Copy ACS URL for SAML configurations to clipboard feat: Copy ACS URL for SAML configurations to clipboard. Disable editing SAML configuration names Aug 13, 2024
@kyle-ssg kyle-ssg self-requested a review August 14, 2024 07:51
@rolodato
Copy link
Member Author

Moved the ACS URL into the edit modal - not too happy with the styling but it's an improvement over what we have now in prod for sure

saml.new.mov

@rolodato rolodato marked this pull request as ready for review August 19, 2024 19:15
@rolodato rolodato requested review from a team as code owners August 19, 2024 19:15
@rolodato rolodato requested review from zachaysan and removed request for a team August 19, 2024 19:15
@github-actions github-actions bot added feature New feature or request and removed feature New feature or request labels Aug 19, 2024
Copy link
Contributor

github-actions bot commented Aug 19, 2024

Docker builds report

Image Build Status Security report
ghcr.io/flagsmith/flagsmith-api-test:pr-4494 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-e2e:pr-4494 Finished ✅ Skipped
ghcr.io/flagsmith/flagsmith-api:pr-4494 Finished ✅ Results
ghcr.io/flagsmith/flagsmith:pr-4494 Finished ✅ Results
ghcr.io/flagsmith/flagsmith-frontend:pr-4494 Finished ✅ Results
ghcr.io/flagsmith/flagsmith-private-cloud:pr-4494 Finished ✅ Results

@rolodato
Copy link
Member Author

@kyle-ssg could I bother you again with a quick review on this? 🙏

@rolodato rolodato added this pull request to the merge queue Aug 20, 2024
Merged via the queue into main with commit 3f561ee Aug 20, 2024
51 checks passed
@rolodato rolodato deleted the feat/copy-acs-url branch August 20, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request front-end Issue related to the React Front End Dashboard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants