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(4157): adding option to hide UI button to disable apply commands #4158

Merged
merged 2 commits into from
Jan 24, 2024

Conversation

igaskin
Copy link
Contributor

@igaskin igaskin commented Jan 20, 2024

what

Adds a flag for --disable-global-apply-lock, to the server command. The flag can be used to optional hid the "disable apply commands" button in the UI.

why

In shared environments it may not be desirable to have this button available to all users. In leu of RBAC, hiding the button can be an acceptable option.
before
Screenshot 2024-01-19 at 4 17 01 PM
after
Screenshot 2024-01-19 at 4 17 33 PM

tests

references

#4157

@igaskin igaskin marked this pull request as ready for review January 22, 2024 05:05
@igaskin igaskin requested review from a team as code owners January 22, 2024 05:05
@igaskin igaskin requested review from jamengual, lukemassa and nitrocode and removed request for a team January 22, 2024 05:05
@github-actions github-actions bot added the go Pull requests that update Go code label Jan 22, 2024
@lukemassa
Copy link
Contributor

Hi @igaskin thanks for the contribution!

A few notes:

  • I'm not sure about the name --disable-global-lock-feature, I wonder if --disable-global-apply-lock would be clearer? There are a number of locks in atlantis and I don't want it to be confusing
  • Unless I'm misreading the code, it looks like this only removes the button from the GUI, it doesn't actually prevent someone from hitting /apply/lock and triggering the lock:
    s.Router.HandleFunc("/apply/lock", s.LocksController.LockApply).Methods("POST").Queries()
    . If we're disabling it for security reasons we might as well not leave that backdoor

@igaskin
Copy link
Contributor Author

igaskin commented Jan 23, 2024

Hi @igaskin thanks for the contribution!

A few notes:

  • I'm not sure about the name --disable-global-lock-feature, I wonder if --disable-global-apply-lock would be clearer? There are a number of locks in atlantis and I don't want it to be confusing
  • Unless I'm misreading the code, it looks like this only removes the button from the GUI, it doesn't actually prevent someone from hitting /apply/lock and triggering the lock:
    s.Router.HandleFunc("/apply/lock", s.LocksController.LockApply).Methods("POST").Queries()

    . If we're disabling it for security reasons we might as well not leave that backdoor

sure, I can make that name change! And agreed on the API still being available, I close that off as well. Honestly I was being lazy, but your comment was the encouragement I needed.

@igaskin
Copy link
Contributor Author

igaskin commented Jan 23, 2024

removed adding the /lock and /unlock routes if the --disable-global-apply-lock is enabled.

$ curl -XPOST localhost:4141/apply/lock
404 page not found
$ curl -XDELETE localhost:4141/apply/unlock
404 page not found

@DanielCastronovo
Copy link

When the --disable-global-apply-lock is enabled, as an admin is it possible to bypass it and use the API ? or maybe a specific route for secure specificaly with an OIDC provider like Okta.

@igaskin
Copy link
Contributor Author

igaskin commented Jan 23, 2024

When the --disable-global-apply-lock is enabled, as an admin is it possible to bypass it and use the API ? or maybe a specific route for secure specificaly with an OIDC provider like Okta.

Agreed, it would be a great to have robust RBAC with permissions that are granular to the individual user. This PR is meant to be a easy option for Atlantis admins to restrict global locks. A proper RBAC/OIDC integration would be best fit in a separate PR.

@igaskin
Copy link
Contributor Author

igaskin commented Jan 23, 2024

@lukemassa addressed comments. Please let me know if there are other changes you would like to see as part of this PR.

Copy link
Contributor

@lukemassa lukemassa left a comment

Choose a reason for hiding this comment

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

Looks good to me! I'll let other maintainers weigh in if they have any concerns.

Copy link
Member

@GenPage GenPage left a comment

Choose a reason for hiding this comment

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

LGTM

@GenPage GenPage added this to the v0.28.0 milestone Jan 24, 2024
@GenPage GenPage added feature New functionality/enhancement ui labels Jan 24, 2024
@GenPage GenPage merged commit fe32f86 into runatlantis:main Jan 24, 2024
24 checks passed
kvanzuijlen pushed a commit to kvanzuijlen/atlantis that referenced this pull request Jan 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality/enhancement go Pull requests that update Go code ui
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

4 participants