Skip to content

Upgrade to OmniAuth CAS 3.x#20

Open
awilfox wants to merge 1 commit intomainfrom
awilfox/AP-270-cas
Open

Upgrade to OmniAuth CAS 3.x#20
awilfox wants to merge 1 commit intomainfrom
awilfox/AP-270-cas

Conversation

@awilfox
Copy link
Member

@awilfox awilfox commented Feb 13, 2026

  • Display button on UnauthorizedError instead of redirecting to login.

  • Use ForbiddenError instead of redirecting to login when a user that is not an admin tries to access an admin page in the section for ref cards/stack passes.

  • Update specs.


Potential TODOs:

  • How do we want the button to look? Right now it's a plain button, and could probably use a bit of style.
  • Is my change of using Forbidden when a non-admin user tries to access admin pages for the ref card/stack pass the correct way to go? It feels weird to ask them to log in again when they're already logged in, just not as an admin user. (Is this for shared computers, perhaps? That would … almost make sense.)

* Display button on `UnauthorizedError` instead of redirecting to login.

* Use `ForbiddenError` instead of redirecting to login when a user
  that is not an admin tries to access an admin page in the section
  for ref cards/stack passes.

* Update specs.
@awilfox
Copy link
Member Author

awilfox commented Feb 13, 2026

Example of what the login button looks like right now:

Screenshot 2026-02-12 at 19 20 08

Copy link
Contributor

@davezuckerman davezuckerman 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 as far as the code goes. As for the questions.

I think the current button looks fine (maybe a bit boring) but I don't have a great artistic eye for that kind of stuff.

For the question about a user being logged in but not being an admin. It seems correct to raise the forbidden error. Even if it's a shared computer they should still be logging in as themself right?

Copy link
Contributor

@yzhoubk yzhoubk left a comment

Choose a reason for hiding this comment

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

It looks good on my local and the workflow makes sense. Admin page looks solid, would be nice to get Jesse's thoughts too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants