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

Review the public settings API #4074

Open
deviantony opened this issue Jul 21, 2020 · 4 comments
Open

Review the public settings API #4074

deviantony opened this issue Jul 21, 2020 · 4 comments

Comments

@deviantony
Copy link
Member

At the moment, the api/settings/public endpoint exposes the following details for non-authenticated users:

type publicSettingsResponse struct {
	LogoURL                            string                         `json:"LogoURL"`
	AuthenticationMethod               portainer.AuthenticationMethod `json:"AuthenticationMethod"`
	AllowBindMountsForRegularUsers     bool                           `json:"AllowBindMountsForRegularUsers"`
	AllowPrivilegedModeForRegularUsers bool                           `json:"AllowPrivilegedModeForRegularUsers"`
	AllowVolumeBrowserForRegularUsers  bool                           `json:"AllowVolumeBrowserForRegularUsers"`
	EnableHostManagementFeatures       bool                           `json:"EnableHostManagementFeatures"`
	EnableEdgeComputeFeatures          bool                           `json:"EnableEdgeComputeFeatures"`
	OAuthLoginURI                      string                         `json:"OAuthLoginURI"`
}

Only the following properties should be exposed publicly:

  • LogoURL (to display it in the authentication view)
  • AuthenticationMethod (to display proper OAuth/LDAP controls)
  • OAuthLoginURI (to redirect to OAuth provider)

The rest of the API parameters should be available behind authentication.

This will introduce an API breaking change.

@deviantony deviantony added this to the 2.0 milestone Jul 21, 2020
@chiptus
Copy link
Contributor

chiptus commented Jul 21, 2020

These views are calling publicSettings:

  • createContainer - needs AllowBindMountsForRegularUsers && AllowPrivilegedModeForRegularUsers
  • createService - AllowBindMountsForRegularUsers
  • service - AllowBindMountsForRegularUsers
  • stateManager - needs everything from settings
  • account - AuthenticationMethod
  • auth - AuthenticationMethod && OAuthLoginURI (&& logo, but for some reason it takes it from the state manager)
  • templates - AllowBindMountsForRegularUsers
  • users - AuthenticationMethod
  • user - AuthenticationMethod

so in this breaking change we need to refactor createContainer, createService, service and templates to received this flag from the state manager.

stateManager should be refactored to call the authenticated settings method, and only when authenticated.

account, users, user - can probably also take their values from the state manager (reducing calls to the server)
auth - take logo from publicSettings

@chiptus chiptus self-assigned this Jul 21, 2020
@chiptus
Copy link
Contributor

chiptus commented Jul 21, 2020

main problem is this:

StateManager should be refactored to call the authenticated settings method, and only when authenticated.

one way to call init only when authenticated is calling it twice:

  • in the root resolver if user is authenticated (calling initAuthentication first and then checking if user is already authenticated)
  • in the authController after the user successfully logs in.
    another way is to introduce another route that will wrap all the routes that require authentication, and move state initialization there.

StateManager.initalize should be called anyway, because otherwise the app doesn't work. But the call to the authenticated settings should be moved to another method, so we can either call it inside initalize if the user is authenticated, or call it from authController after authentication

@chiptus
Copy link
Contributor

chiptus commented Jul 21, 2020

just found another needed change. GET api/settings is admin restricted. Is there a reason for this? should we create another route for regular users?

@deviantony
Copy link
Member Author

@chiptus we're gonna postpone this piece of work.

@deviantony deviantony modified the milestones: 2.0, 2.1 Jul 22, 2020
@deviantony deviantony removed the priority/normal Core team priority label Jul 22, 2020
@deviantony deviantony modified the milestones: 2.1, backlog Oct 28, 2020
@deviantony deviantony modified the milestones: backlog, 2.2 Nov 25, 2020
chiptus pushed a commit to chiptus/portainer that referenced this issue Dec 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants