Skip to content

Conversation

@melsawy
Copy link
Contributor

@melsawy melsawy commented Oct 27, 2025

Description

Check existing API keys and limit the usage of global keys.

  • Log ApiKey last activity date
  • Remove global API key permissions
  • Verify that API key belongs to team
  • Add rake task to set team_id for existing keys
  • Add rake task to list global keys

References: CV2-6604

How to test?

  • Re-run automated tests
  • Using UI
  • A) Generate API using workspace settings page(settings => Integrations => API Access)
  • B) Generate API key using this guide
    For both cases I used graphql UI to run different quires

Checklist

  • I have performed a self-review of my code and ensured that it is safe and runnable, that code coverage has not decreased, and that there are no new Code Climate issues. I have also followed Meedan's internal coding guidelines.

@melsawy melsawy changed the title CV2-6604: remove global keys permission CV2-6604: Remove global keys permission Nov 26, 2025
@melsawy melsawy marked this pull request as ready for review December 1, 2025 12:20
Copy link
Contributor

@caiosba caiosba left a comment

Choose a reason for hiding this comment

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

Sawy, to be extra safe, I think we should make the team_id column on ApiKey mandatory. This should simplify the implementation since you won’t need to filter by non-null team IDs, and it also avoids the risk of forgetting to update that logic somewhere. Of course, you’ll need to run the rake task before deploying the code change; otherwise, the database migration will fail. Also, please rebase the PR since you already handled the last activity part in another PR.

@melsawy
Copy link
Contributor Author

melsawy commented Dec 3, 2025

I think we should make the team_id column on ApiKey mandatory.

@caiosba for now I added a validation inside the model ApiKey so I can deploy and run the rake task and after deploying this PR I'll make the team_id column mandatory

@melsawy melsawy requested a review from caiosba December 3, 2025 15:11
Copy link
Contributor

@caiosba caiosba left a comment

Choose a reason for hiding this comment

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

@melsawy , why not the opposite? A first PR with just the rake task, and then another PR that makes team_id mandatory?

@melsawy
Copy link
Contributor Author

melsawy commented Dec 3, 2025

@melsawy , why not the opposite? A first PR with just the rake task, and then another PR that makes team_id mandatory?
it's same just a suggestion but it's OK to deploy the rake task first

@caiosba
Copy link
Contributor

caiosba commented Dec 3, 2025

@melsawy , why not the opposite? A first PR with just the rake task, and then another PR that makes team_id mandatory?
it's same just a suggestion but it's OK to deploy the rake task first

Thanks, I think it's better.

@melsawy
Copy link
Contributor Author

melsawy commented Dec 3, 2025

@caiosba I created a PR with rake task only so I can deploy it
please review it

@melsawy melsawy requested a review from caiosba December 3, 2025 18:29
@caiosba
Copy link
Contributor

caiosba commented Dec 3, 2025

@caiosba I created a PR with rake task only so I can deploy it please review it

Thanks, done!

Copy link
Contributor

@caiosba caiosba left a comment

Choose a reason for hiding this comment

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

Sawy, so now that the other PR contains only the rake task, you still need to update this one to remove the rake task and to make team_id mandatory, right?

@melsawy
Copy link
Contributor Author

melsawy commented Dec 4, 2025

Sawy, so now that the other PR contains only the rake task, you still need to update this one to remove the rake task and to make team_id mandatory, right?

Right

@melsawy melsawy requested a review from caiosba December 16, 2025 16:40
Comment on lines +31 to +32
bot = BotUser.find_by_login(login) || BotUser.create!(team_author_id: team.id, login: login, name: login.capitalize, settings: { approved: true })
team_user = bot.team_users.first
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you run Check Web integration tests after this change?

private

def api_key_perms
can :read, :all
Copy link
Contributor

@caiosba caiosba Dec 16, 2025

Choose a reason for hiding this comment

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

Blocker: This means any API key can read anything in any team. Other than trying to fix this rule, I think it's safer to just delete this line.

Comment on lines 13 to 14
# @b.api_key = @a
# @b.save!
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't you just delete these lines?

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