Skip to content

clean up console_api endpoints #1757

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Oct 3, 2022
Merged

clean up console_api endpoints #1757

merged 10 commits into from
Oct 3, 2022

Conversation

@davepacheco
Copy link
Collaborator

@davepacheco davepacheco commented Sep 29, 2022 •
edited
Loading

This change cleans up a few things in console_api.rs:

  • Renames the endpoint functions with a consistent naming scheme. I found "login", "spoof_login", "consume_credentials", "login_redirect", etc. really confusing. Now, we have:
    • login_spoof_begin (gets the "spoof" login page) / login_spoof (accepts "spoof" creds and logs you in)
    • login_saml_begin (gets the SAML login page) / login_saml (accepts SAML creds and logs you in)
    • a future PR will add login_password_begin / login_password
    • login_redirect became login_begin. (All of these endpoints provide login-related redirects.)
  • Most of the places where we were returning Response<Body>, we're now returning a more specific dropshot response type. We had to use Response<Body> previously because dropshot didn't have response types that supported setting headers or any of the 300-level status codes. Modern dropshot does have header support and add response types for various 300-level response codes dropshot#451 adds support for 300-level status codes. This in turn fixes a bunch of things:
    • The OpenAPI spec says what these endpoints return on success.
    • I was able to remove some TODOs and uncomment some of the oximeter metric stuff that had been commented out because it didn't work with returning Response<Body>.
    • We were previously returning 401 responses constructed by hand rather than automatically. This meant they could be generated differently than other 401s -- and they were. We currently expect (and verify in the test suite) that all 400-level and 500-level responses return a JSON error object with a known schema. These errors weren't doing that. That broke the test suite if you tried to call them.
  • login_spoof now has the same success return code as login_saml (was 200 with no body, now 303 "See Other" redirecting to "/").
  • logout used to return a 200 with no content. That's now a 204.
  • We were using "302 Found" to respond to a POST when we want the client to GET the redirect URL. That's not right -- "303 See Other" is for this. (TIL)
  • I added oximeter metrics to a bunch of endpoints that didn't have them before.

Some notes:

  • In a few places, we were previously clearing the cookie in an error case (returning a 401) and now aren't. I talked this over with @david-crespo and we couldn't think of a reason this should be necessary. (This change is currently necessary because dropshot doesn't support setting headers on error responses.)
  • The 302s that remain here should probably be 307s, per the same doc linked above. This didn't seem worth breaking the console or other clients.

Heads up @karencfv @david-crespo @zephraph because of the breaking changes here:

  • OpenAPI operation names have changed.
  • Successful POST to the login endpoints now returns 307, not 302 or 200.

Open tasks:

@davepacheco davepacheco mentioned this pull request Sep 29, 2022
Merged
@davepacheco
Copy link
Collaborator Author

davepacheco commented Sep 29, 2022

The dropshot change is ready to land, so this is now ready for review. (Leaving it "draft" so I don't accidentally merge it.)

@ahl
Copy link
Contributor

ahl commented Sep 29, 2022

This is great; thanks for doing this.

@davepacheco
Copy link
Collaborator Author

davepacheco commented Sep 30, 2022

CC @david-crespo

@davepacheco davepacheco mentioned this pull request Sep 30, 2022
Merged
david-crespo
david-crespo approved these changes Oct 2, 2022
View reviewed changes
Copy link
Contributor

@david-crespo david-crespo 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

@davepacheco davepacheco marked this pull request as ready for review October 3, 2022 17:29
@davepacheco davepacheco enabled auto-merge (squash) October 3, 2022 17:31
@davepacheco davepacheco merged commit acdae06 into main Oct 3, 2022
@davepacheco davepacheco deleted the dropshot-redirects branch October 3, 2022 19:00
david-crespo added a commit to oxidecomputer/console that referenced this pull request Oct 4, 2022
@david-crespo
spoof_login -> login_spoof_begin
886d75c 
see oxidecomputer/omicron#1757
@david-crespo david-crespo mentioned this pull request Oct 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@david-crespo david-crespo david-crespo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@davepacheco @ahl @david-crespo